lkml.org 
[lkml]   [2014]   [Jan]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v2 1/1] mm: fix the theoretical compound_lock() vs prep_new_page() race
On 01/09, Mel Gorman wrote:
>
> On Wed, Jan 08, 2014 at 08:04:43PM +0100, Oleg Nesterov wrote:
> >
> > Yes.
> >
> > But suppose that CPU B completes split_huge_page_to_list/munmap/etc
> > and frees this head page.
> >
>
> Where did the reference taken by get_huge_page_multiple go?

In __split_huge_page_refcount(), see below.

> CPU A
> static noinline int gup_huge_pmd(pmd_t pmd, unsigned long addr,
> unsigned long end, int write, struct page **pages, int *nr)
> {
> ....
> do {
> ...
> if (PageTail(page))
> /* Increment page->_mapcount */
> get_huge_page_tail(page);
> ...
> refs++;
> } while (...)
> get_head_page_multiple(head, refs);
> }
>
> CPU A in get_futex_key has taken multiple references to the head page,
> one for every base page on the huge page

Not sure I understand "multiple references to the head page" above...
I mean, in this particular case case nr == 1.

IOW, If gup returns a tail page, this page_tail has 1 in ->_mapcount
(to simplify) and its ->first_page gets the additional 1 in ->_count.

> static void __split_huge_page_refcount(struct page *page,
> struct list_head *list)
> {
> ...
> spin_lock_irq(&zone->lru_lock);
> compound_lock(page);
>
> for_every_tail_page() {
> /* This picks up refcounts from GUP get_huge_page_tail */
> tail_count += page_mapcount(page_tail);
>
> /* Propogate all mapcounts to the "real" refcount in the tail page */
> atomic_add(page_mapcount(head) + page_mapcount(tail), tail->_count)
>
> .... flag reinits with barriers ...
> }
> atomic_sub(tail_count, headpage->_count);
> ...
> unlock stuff
> }
>
> The refcounts on page->_mapcount taken while the page was huge is
> propogated to the tail pages so it's still pinned in place.

Yes. But at the same time atomic_sub(tail_count, headpage->_count)
above reverts the result of get_head_page_multiple(head) done by
gup() above.

IOW, after __split_huge_page_refcount() page_tail no longer pins its
former page_head.

> > > takes reference unless zero
> >
> > suppose this page_head was reallocated and get_page_unless_zero()
> > succeds right after set_page_refcounted(),
> >
>
> You're right. The head page can still be freed and reallocated as a *smaller*
> compound page but futex.c is doing the reference count on the tail page
> that should have an elevated count even after the split

Yes, page_tail can't go away, the reference was moved to page_tail->_count.

> #ifdef CONFIG_TRANSPARENT_HUGEPAGE
> page_head = page;
> if (unlikely(PageTail(page))) {
> put_page(page);
>
>
> so I'm still not seeing how a tail page racing with a split ends up with
> mayhem.

But get/put(page_tail) plays with page_head which can be freed/reallocated,
it does compound_lock(page_head).

> I could also still be stuck in a "la la la, everything is fine" mode.

More likely it is me who tries to deny the fact I missed something ;)



But let me try again. Lets ignore PageSlab/PageHeadHuge. put_compound_page()
is complicated, but roughly it does:

CPU 0 CPU 1

if (!PageTail(page_tail))
return;

page_head = page_tail->first_page;

unmap this vma, free everything.
page_tail can't go away, its
->_count was incremented by
__split_huge_page_refcount().


alloc_pages(GFP_COMP, 1) reallocates
page_head, prep_new_page() does
set_page_refcounted(page_head),


// succeeds after set_page_refcounted()
get_page_unless_zero(page_head);

compound_lock(page_head) prep_compound_page(page_head);

Now, both compound_lock() and prep_compound_page() play with the same
page_head->flags, but __SetPageHead(page_head) is non-atomic.

OK. Even if I am right, we can probably make another fix.
put_compound_page() and __get_page_tail() can do yet another PageTail()
check _before_ compound_lock().

Although personally I'd prefer this patch. And if we change get/put
I think it would be better to do this on top of

"[PATCH -mm 6/7] mm: thp: introduce get_lock_thp_head()"
http://marc.info/?l=linux-kernel&m=138739438800899

Oleg.



\
 
 \ /
  Last update: 2014-01-09 16:01    [W:0.131 / U:0.100 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site