Re: [PATCH] thp: tail page refcounting fix #4

[Minchan Kim]

On Sun, Aug 28, 2011 at 2:34 AM, Andrea Arcangeli <aarcange@xxxxxxxxxx> wrote:
> On Sat, Aug 27, 2011 at 02:41:52AM -0700, Michel Lespinasse wrote:
>> I understand you may have to remove the VM_BUG_ON(page_mapcount(page) <= 0)
>> that I had suggested in __get_page_tail() (sorry about that).
>
> The function doing that is snd_pcm_mmap_data_fault and it's doing what
> I described in prev email.
>
>> My only additional suggestion is about the put_page_testzero in
>> __get_page_tail(), maybe if you could just increment the tail page count
>> instead of calling __get_page_tail_foll(), then you wouldn't have to
>> release the extra head page count there. And it would even look kinda
>> natural, head page count gets acquired before compound_lock_irqsave(),
>> so we only have to acquire an extra tail page count after confirming
>> this is still a tail page.
>
> Ok, I added a param to __get_page_tail_foll, it is constant at build
> time and because it's inline the branch should be optimized away at
> build time without requiring a separate function. The bugchecks are
> the same so we can share and just skip the atomic_inc on the
> page_head in __get_page_tail_foll. Also it had to be moved into
> internal.h as a further cleanup.
>
>> Either way, the code looks OK by now.
>>
>> Reviewed-by: Michel Lespinasse <walken@xxxxxxxxxx>
>
> Thanks. Incremental diff to correct the false positive bug on for
> drivers like alsa allocating __GFP_COMP and mapping subpages with page
> faults.
>
> diff --git a/mm/swap.c b/mm/swap.c
> --- a/mm/swap.c
> +++ b/mm/swap.c
> @@ -166,12 +166,6 @@ int __get_page_tail(struct page *page)
> Â Â Â Â Â Â Â Âflags = compound_lock_irqsave(page_head);
> Â Â Â Â Â Â Â Â/* here __split_huge_page_refcount won't run anymore */
> Â Â Â Â Â Â Â Âif (likely(PageTail(page))) {
> - Â Â Â Â Â Â Â Â Â Â Â /*
> - Â Â Â Â Â Â Â Â Â Â Â Â* get_page() can only be called on tail pages
> - Â Â Â Â Â Â Â Â Â Â Â Â* after get_page_foll() taken a tail page
> - Â Â Â Â Â Â Â Â Â Â Â Â* refcount.
> - Â Â Â Â Â Â Â Â Â Â Â Â*/
> - Â Â Â Â Â Â Â Â Â Â Â VM_BUG_ON(page_mapcount(page) <= 0);
> Â Â Â Â Â Â Â Â Â Â Â Â__get_page_tail_foll(page);
> Â Â Â Â Â Â Â Â Â Â Â Âgot = 1;
>
> Â Â Â Â Â Â Â Â Â Â Â Â/*
>
> This is the optimization.
>
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -375,26 +375,6 @@ static inline int page_count(struct page
> Â Â Â Âreturn atomic_read(&compound_head(page)->_count);
> Â}
>
> -static inline void __get_page_tail_foll(struct page *page)
> -{
> - Â Â Â /*
> - Â Â Â Â* If we're getting a tail page, the elevated page->_count is
> - Â Â Â Â* required only in the head page and we will elevate the head
> - Â Â Â Â* page->_count and tail page->_mapcount.
> - Â Â Â Â*
> - Â Â Â Â* We elevate page_tail->_mapcount for tail pages to force
> - Â Â Â Â* page_tail->_count to be zero at all times to avoid getting
> - Â Â Â Â* false positives from get_page_unless_zero() with
> - Â Â Â Â* speculative page access (like in
> - Â Â Â Â* page_cache_get_speculative()) on tail pages.
> - Â Â Â Â*/
> - Â Â Â VM_BUG_ON(atomic_read(&page->first_page->_count) <= 0);
> - Â Â Â VM_BUG_ON(atomic_read(&page->_count) != 0);
> - Â Â Â VM_BUG_ON(page_mapcount(page) < 0);
> - Â Â Â atomic_inc(&page->first_page->_count);
> - Â Â Â atomic_inc(&page->_mapcount);
> -}
> -
> Âextern int __get_page_tail(struct page *page);
>
> Âstatic inline void get_page(struct page *page)
> diff --git a/mm/internal.h b/mm/internal.h
> --- a/mm/internal.h
> +++ b/mm/internal.h
> @@ -37,6 +37,28 @@ static inline void __put_page(struct pag
> Â Â Â Âatomic_dec(&page->_count);
> Â}
>
> +static inline void __get_page_tail_foll(struct page *page,
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â bool get_page_head)
> +{
> + Â Â Â /*
> + Â Â Â Â* If we're getting a tail page, the elevated page->_count is
> + Â Â Â Â* required only in the head page and we will elevate the head
> + Â Â Â Â* page->_count and tail page->_mapcount.
> + Â Â Â Â*
> + Â Â Â Â* We elevate page_tail->_mapcount for tail pages to force
> + Â Â Â Â* page_tail->_count to be zero at all times to avoid getting
> + Â Â Â Â* false positives from get_page_unless_zero() with
> + Â Â Â Â* speculative page access (like in
> + Â Â Â Â* page_cache_get_speculative()) on tail pages.
> + Â Â Â Â*/
> + Â Â Â VM_BUG_ON(atomic_read(&page->first_page->_count) <= 0);
> + Â Â Â VM_BUG_ON(atomic_read(&page->_count) != 0);
> + Â Â Â VM_BUG_ON(page_mapcount(page) < 0);
> + Â Â Â if (get_page_head)
> + Â Â Â Â Â Â Â atomic_inc(&page->first_page->_count);
> + Â Â Â atomic_inc(&page->_mapcount);
> +}
> +
> Âstatic inline void get_page_foll(struct page *page)
> Â{
> Â Â Â Âif (unlikely(PageTail(page)))
> @@ -45,7 +67,7 @@ static inline void get_page_foll(struct
> Â Â Â Â Â Â Â Â * __split_huge_page_refcount() can't run under
> Â Â Â Â Â Â Â Â * get_page_foll() because we hold the proper PT lock.
> Â Â Â Â Â Â Â Â */
> - Â Â Â Â Â Â Â __get_page_tail_foll(page);
> + Â Â Â Â Â Â Â __get_page_tail_foll(page, true);
> Â Â Â Âelse {
> Â Â Â Â Â Â Â Â/*
> Â Â Â Â Â Â Â Â * Getting a normal page or the head of a compound page
> diff --git a/mm/swap.c b/mm/swap.c
> --- a/mm/swap.c
> +++ b/mm/swap.c
> @@ -166,16 +166,8 @@ int __get_page_tail(struct page *page)
> Â Â Â Â Â Â Â Âflags = compound_lock_irqsave(page_head);
> Â Â Â Â Â Â Â Â/* here __split_huge_page_refcount won't run anymore */
> Â Â Â Â Â Â Â Âif (likely(PageTail(page))) {
> - Â Â Â Â Â Â Â Â Â Â Â __get_page_tail_foll(page);
> + Â Â Â Â Â Â Â Â Â Â Â __get_page_tail_foll(page, false);
> Â Â Â Â Â Â Â Â Â Â Â Âgot = 1;
> - Â Â Â Â Â Â Â Â Â Â Â /*
> - Â Â Â Â Â Â Â Â Â Â Â Â* We can release the refcount taken by
> - Â Â Â Â Â Â Â Â Â Â Â Â* get_page_unless_zero() now that
> - Â Â Â Â Â Â Â Â Â Â Â Â* __split_huge_page_refcount() is blocked on
> - Â Â Â Â Â Â Â Â Â Â Â Â* the compound_lock.
> - Â Â Â Â Â Â Â Â Â Â Â Â*/
> - Â Â Â Â Â Â Â Â Â Â Â if (put_page_testzero(page_head))
> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â VM_BUG_ON(1);
> Â Â Â Â Â Â Â Â}
> Â Â Â Â Â Â Â Âcompound_unlock_irqrestore(page_head, flags);
> Â Â Â Â Â Â Â Âif (unlikely(!got))
>
>
> ===
> Subject: thp: tail page refcounting fix
>
> From: Andrea Arcangeli <aarcange@xxxxxxxxxx>
>
> Michel while working on the working set estimation code, noticed that calling
> get_page_unless_zero() on a random pfn_to_page(random_pfn) wasn't safe, if the
> pfn ended up being a tail page of a transparent hugepage under splitting by
> __split_huge_page_refcount(). He then found the problem could also
> theoretically materialize with page_cache_get_speculative() during the
> speculative radix tree lookups that uses get_page_unless_zero() in SMP if the
> radix tree page is freed and reallocated and get_user_pages is called on it
> before page_cache_get_speculative has a chance to call get_page_unless_zero().
>
> So the best way to fix the problem is to keep page_tail->_count zero at all
> times. This will guarantee that get_page_unless_zero() can never succeed on any
> tail page. page_tail->_mapcount is guaranteed zero and is unused for all tail
> pages of a compound page, so we can simply account the tail page references
> there and transfer them to tail_page->_count in __split_huge_page_refcount() (in
> addition to the head_page->_mapcount).
>
> While debugging this s/_count/_mapcount/ change I also noticed get_page is
> called by direct-io.c on pages returned by get_user_pages. That wasn't entirely
> safe because the two atomic_inc in get_page weren't atomic. As opposed other
> get_user_page users like secondary-MMU page fault to establish the shadow
> pagetables would never call any superflous get_page after get_user_page
> returns. It's safer to make get_page universally safe for tail pages and to use
> get_page_foll() within follow_page (inside get_user_pages()). get_page_foll()
> is safe to do the refcounting for tail pages without taking any locks because
> it is run within PT lock protected critical sections (PT lock for pte and
> page_table_lock for pmd_trans_huge). The standard get_page() as invoked by
> direct-io instead will now take the compound_lock but still only for tail
> pages. The direct-io paths are usually I/O bound and the compound_lock is per
> THP so very finegrined, so there's no risk of scalability issues with it. A
> simple direct-io benchmarks with all lockdep prove locking and spinlock
> debugging infrastructure enabled shows identical performance and no overhead.
> So it's worth it. Ideally direct-io should stop calling get_page() on pages
> returned by get_user_pages(). The spinlock in get_page() is already optimized
> away for no-THP builds but doing get_page() on tail pages returned by GUP is
> generally a rare operation and usually only run in I/O paths.
>
> This new refcounting on page_tail->_mapcount in addition to avoiding new RCU
> critical sections will also allow the working set estimation code to work
> without any further complexity associated to the tail page refcounting
> with THP.
>
> Signed-off-by: Andrea Arcangeli <aarcange@xxxxxxxxxx>
> Reported-by: Michel Lespinasse <walken@xxxxxxxxxx>
> Reviewed-by: Michel Lespinasse <walken@xxxxxxxxxx>

There is a just nitpick at below but the code looks more clear than
old and even fixed bug I missed but Michel found.

Thanks!

Reviewed-by: Minchan Kim <minchan.kim@xxxxxxxxx>

> @@ -1156,6 +1156,7 @@ static void __split_huge_page_refcount(s
> Â Â Â Âunsigned long head_index = page->index;
> Â Â Â Âstruct zone *zone = page_zone(page);
> Â Â Â Âint zonestat;
> + Â Â Â int tail_count = 0;
>
> Â Â Â Â/* prevent PageLRU to go away from under us, and freeze lru stats */
> Â Â Â Âspin_lock_irq(&zone->lru_lock);
> @@ -1164,11 +1165,14 @@ static void __split_huge_page_refcount(s
> Â Â Â Âfor (i = 1; i < HPAGE_PMD_NR; i++) {
> Â Â Â Â Â Â Â Âstruct page *page_tail = page + i;
>
> - Â Â Â Â Â Â Â /* tail_page->_count cannot change */
> - Â Â Â Â Â Â Â atomic_sub(atomic_read(&page_tail->_count), &page->_count);
> - Â Â Â Â Â Â Â BUG_ON(page_count(page) <= 0);
> - Â Â Â Â Â Â Â atomic_add(page_mapcount(page) + 1, &page_tail->_count);
> - Â Â Â Â Â Â Â BUG_ON(atomic_read(&page_tail->_count) <= 0);
> + Â Â Â Â Â Â Â /* tail_page->_mapcount cannot change */
> + Â Â Â Â Â Â Â BUG_ON(page_mapcount(page_tail) < 0);
> + Â Â Â Â Â Â Â tail_count += page_mapcount(page_tail);
> + Â Â Â Â Â Â Â /* check for overflow */
> + Â Â Â Â Â Â Â BUG_ON(tail_count < 0);
> + Â Â Â Â Â Â Â BUG_ON(atomic_read(&page_tail->_count) != 0);
> + Â Â Â Â Â Â Â atomic_add(page_mapcount(page) + page_mapcount(page_tail) + 1,
> + Â Â Â Â Â Â Â Â Â Â Â Â Â&page_tail->_count);

I doubt someone might try to change this with atomic_set for reducing
LOCK_PREFIX overhead in future although it's not fast path. Of course,
we can prevent that patch but can't prevent his wasted time. I hope
there is a comment why we use atomic_add like the errata PPro with
OOStore.





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