Re: [syzbot] WARNING in follow_hugetlb_page

[Mike Kravetz <mike.kravetz@oracle.com>]

On 5/13/22 17:56, John Hubbard wrote:
> On 5/13/22 17:26, Minchan Kim wrote:
>> Anything else further can we get insight from the warning?
>>
>> For example, pin_user_pages going on against a hugetlb page
>> which are concurrently running alloc_contig_range(it's
>> exported function so anyone can call randomly) so
>> alloc_contig_range changes pageblock type as MIGRATE_ISOLATE
>> under us so the hit at the warning?
> 
> Well, yes. First of all, the comments above the warning that fired have
> gone a little bit stale: they claim that we can only hit the warning if
> the page refcount overflows. However, we almost certainly got here via:

Yes, the comment is stale.

John, you added that comment with commit 3faa52c03f44.  At that time,
the code was doing a try_grab_page(), and this routine does not check
for pinnable page in any manner.  So, the comment was accurate at that
time.

Later, that code was modified (for performance reasons) in commit  0fa5bc4023c18 to do a single try_grab_compound_page() instead of multiple
calls to try_grab_page().  At the time it was not noticed that
try_grab_compound_page had a check for 'pinnable' when try_grab_page did
not.  So, I think this commit actually changed the behavior.

> 
> try_grab_folio()
>     /*
>      * Can't do FOLL_LONGTERM + FOLL_PIN gup fast path if not in a
>      * right zone, so fail and let the caller fall back to the slow
>      * path.
>      */
>     if (unlikely((flags & FOLL_LONGTERM) &&
>              !is_pinnable_page(page))) /* which we just changed */
>         return NULL;
> 
> ...and now I'm starting to think that this warning might fire even with
> the corrected check for MIGRATE_CMA || MIGRATE_ISOLATE. Because
> try_grab_folio() didn't always have this early exit and it is starting
> to look wrong.
> 
> Simply attempting to pin a non-pinnable huge page would hit this
> warning. Adding additional reasons that a page is not pinnable (which
> the patch does) could make this more likely to fire.

Yes, that is correct.  One could easily allocate a hugetlb page from
CMA and trigger this warning.

> 
> I need to look at this a little more closely, it is making me wonder
> whether the is_pinnable_page() check is a problem in this path. The
> comment in try_grab_folio() indicates that the early return is a hack
> (it assumes that the caller is in the gup fast path), and maybe the hack
> is just wrong here--I think we're actually on the slow gup path. Not
> good.
> 
> Mike, any thoughts here?
> 

Do you know why try_grab_compound_page(now try_grab_folio) checks for
pinnable when try_grab_page does not?

Then I guess the next question is 'Should we allow pinning of hugetlb pages
in these areas?'.  My first thought would be no.  But, recall it was 'allowed'
until that commit which changed try_grab_page to try_grab_compound_page.
In the 'common' case of compaction, we do not attempt to migrate/move hugetlb
pages (last time I looked), so long term pinning should not be an issue.
However, for things like memory offline or alloc_contig_range() we want to
migrate hugetlb pages, so this would be an issue there.

At a minimum, I think the warning should go.

More thoughts?
-- 
Mike Kravetz