linux-kernel - Re: [patch 1/6] Guest page hinting: core + volatile page cache.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <1205400288.26537.13.camel@localhost>
Date:	Thu, 13 Mar 2008 10:24:48 +0100
From:	Martin Schwidefsky <schwidefsky@...ibm.com>
To:	Rusty Russell <rusty@...tcorp.com.au>
Cc:	virtualization@...ts.linux-foundation.org,
	linux-kernel@...r.kernel.org, linux-s390@...r.kernel.org,
	virtualization@...ts.osdl.org, akpm@...l.org,
	nickpiggin@...oo.com.au, frankeh@...son.ibm.com, hugh@...itas.com
Subject: Re: [patch 1/6] Guest page hinting: core + volatile page cache.

On Thu, 2008-03-13 at 10:12 +1100, Rusty Russell wrote:
> On Thursday 13 March 2008 00:21:33 Martin Schwidefsky wrote:
> > @@ -957,6 +975,19 @@ struct page *follow_page(struct vm_area_
> >
> >  	if (flags & FOLL_GET)
> >  		get_page(page);
> > +
> > +	if (flags & FOLL_GET) {
> > +		/*
> > +		 * The page is made stable if a reference is acquired.
> > +		 * If the caller does not get a reference it implies that
> > +		 * the caller can deal with page faults in case the page
> > +		 * is swapped out. In this case the caller can deal with
> > +		 * discard faults as well.
> > +		 */
> > +		if (unlikely(!page_make_stable(page)))
> > +			goto out_discard;
> > +	}
> 
> Dumb comment: seems like this if could be folded into the one above.

In this patch yes, but a later patch adds a condition:

-       if (flags & FOLL_GET) {
+       if ((flags & FOLL_GET) || (vma->vm_flags & VM_LOCKED)) {


> > + * Attempts to change the state of a page to volatile.
> > + * If there is something preventing the state change the page stays
> > + * int its current state.
> 
> Typo "int its current state".

Fixed.

> >  		return NULL;
> >
> >  	pte = pte_offset_map(pmd, address);
> > +	ptl = pte_lockptr(mm, pmd);
> >  	/* Make a quick check before getting the lock */
> > +#ifndef CONFIG_PAGE_STATES
> > +	/*
> > +	 * If the page table lock for this pte is taken we have to
> > +	 * assume that someone might be mapping the page. To solve
> > +	 * the race of a page discard vs. mapping the page we have
> > +	 * to serialize the two operations by taking the lock,
> > +	 * otherwise we end up with a pte for a page that has been
> > +	 * removed from page cache by the discard fault handler.
> > +	 */
> > +	if (!spin_is_locked(ptl))
> > +#endif
> >  	if (!pte_present(*pte)) {
> >  		pte_unmap(pte);
> >  		return NULL;
> >  	}
> >
> > -	ptl = pte_lockptr(mm, pmd);
> >  	spin_lock(ptl);
> >  	if (pte_present(*pte) && page_to_pfn(page) == pte_pfn(*pte)) {
> >  		*ptlp = ptl;
> 
> Did you really mean ifndef here?

That is a major nit. This should be an #ifdef. In previous versions the
complete "if (!pte_present(*pte)) { }" is ifdefed, the later versions
use the !spin_is_locked condition. Only I forgot to invert the #ifndef.
Fixed.

> (BTW: I'm just reading through the code, not really understanding it, so
> this is not a real review).

I take the small review anytime. Already found one major nit.

-- 
blue skies,
  Martin.

"Reality continues to ruin my life." - Calvin.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/