lkml.org 
[lkml]   [2015]   [Jan]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v2] mm: incorporate read-only pages into transparent huge pages
Hi,

On Mon, Jan 26, 2015 at 08:36:06AM +0100, Vlastimil Babka wrote:
> > - Add fast path optimistic check to
> > __collapse_huge_page_isolate()
>
> My interpretation is that the optimistic check is in khugepaged_scan_pmd() while
> in __collapse_huge_page_isolate() it's protected by lock, as Andrea suggested?

Correct, __collapse_huge_page_isolate is the "accurate" check that was
missing in v1. The optimistic check was the one in khugepaged_scan_pmd
and it was already present.

> > @@ -2168,9 +2168,6 @@ static int __collapse_huge_page_isolate(struct vm_area_struct *vma,
> > VM_BUG_ON_PAGE(!PageAnon(page), page);
> > VM_BUG_ON_PAGE(!PageSwapBacked(page), page);
> >
> > - /* cannot use mapcount: can't collapse if there's a gup pin */
> > - if (page_count(page) != 1)
> > - goto out;
> > /*
> > * We can do it before isolate_lru_page because the
> > * page can't be freed from under us. NOTE: PG_lock
> > @@ -2179,6 +2176,31 @@ static int __collapse_huge_page_isolate(struct vm_area_struct *vma,
> > */
> > if (!trylock_page(page))
> > goto out;
> > +
> > + /*
> > + * cannot use mapcount: can't collapse if there's a gup pin.
> > + * The page must only be referenced by the scanned process
> > + * and page swap cache.
> > + */
> > + if (page_count(page) != 1 + !!PageSwapCache(page)) {
> > + unlock_page(page);
> > + goto out;
> > + }
> > + if (!pte_write(pteval)) {
> > + if (++ro > khugepaged_max_ptes_none) {
> > + unlock_page(page);
> > + goto out;
>
> So just for completeness, as I said later for v1 I think this can leave us with
> read-only VMA: consider ro == 256 and none == 256, referenced can still be >0
> (up to 256). I think that the check for referenced that follows this for loop
> should also check if (ro + none < HPAGE_PMD_NR).

The moment "ro" becomes 256 or "none" becomes 256, we immediately goto out.

if (likely(referenced))
return 1;
out:
release_pte_pages(pte, _pte);
return 0;

"out" is past the referenced check and we fail the collapse
immediately.

If ro is < 255 then "none" is also < 255 (if pte_write is true, then
pte_none cannot be true).

Overall I don't see how we could collapse in readonly vma and where
the bug is for this case, but I may be overlooking something obvious.

> > + }
> > + if (PageSwapCache(page) && !reuse_swap_page(page)) {
> > + unlock_page(page);
> > + goto out;
> > + }
> > + /*
> > + * Page is not in the swap cache, and page count is
> > + * one (see above). It can be collapsed into a THP.
> > + */
>
> I would still put the VM_BUG_ON(page_count(page) != 1) here as I suggested
> previously. Even more so that I think it would have been able to catch the
> problem that Andrea pointed out in v1.

This is subtle, but we can't do VM_BUG_ON because it's ok if the VM
comes before us in another CPU, and takes a pin on the page to isolate
the page.

In short if you changed the current upstream code like below:

/* cannot use mapcount: can't collapse if there's a gup pin */
if (page_count(page) != 1)
goto out;
VM_BUG_ON(page_count(page) != 1);
trylock...

the VM_BUG_ON could fire as a false positive. It's not even related to
the trylock_page, it's related to the LRU lock and isolate_lru_page
that the VM could run on a different CPU and it's not a bug.

The VM is free to pin the page. We only need to be sure there are no
GUP-pins after we blocked all variants of GUP and the check is enough
for that.

Thanks,
Andrea


\
 
 \ /
  Last update: 2015-01-26 16:41    [W:0.060 / U:0.980 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site