Question on follow_page_mask

Question on follow_page_mask

[Anshuman Khandual]

Not able to understand the first code block of follow_page_mask
function. follow_huge_addr function is expected to find the page
struct for the given address if it turns out to be a HugeTLB page
but then when it finds the page we bug on if it had been called
with FOLL_GET flag.

	page = follow_huge_addr(mm, address, flags & FOLL_WRITE);
	if (!IS_ERR(page)) {
		BUG_ON(flags & FOLL_GET);
		return page;
	}

do_move_page_to_node_array calls follow_page with FOLL_GET which
in turn calls follow_page_mask with FOLL_GET. On POWER, the
function follow_huge_addr is defined and does not return -EINVAL
like the generic one. It returns the page struct if its a HugeTLB
page. Just curious to know what is the purpose behind the BUG_ON.
Thank you.

Regards
Anshuman

Re: Question on follow_page_mask

[Kirill A. Shutemov]

On Tue, Feb 23, 2016 at 06:45:05PM +0530, Anshuman Khandual wrote:
> Not able to understand the first code block of follow_page_mask
> function. follow_huge_addr function is expected to find the page
> struct for the given address if it turns out to be a HugeTLB page
> but then when it finds the page we bug on if it had been called
> with FOLL_GET flag.
> 
> 	page = follow_huge_addr(mm, address, flags & FOLL_WRITE);
> 	if (!IS_ERR(page)) {
> 		BUG_ON(flags & FOLL_GET);
> 		return page;
> 	}
> 
> do_move_page_to_node_array calls follow_page with FOLL_GET which
> in turn calls follow_page_mask with FOLL_GET. On POWER, the
> function follow_huge_addr is defined and does not return -EINVAL
> like the generic one. It returns the page struct if its a HugeTLB
> page. Just curious to know what is the purpose behind the BUG_ON.

I would guess requesting pin on non-reclaimable page is considered
useless, meaning suspicius behavior. BUG_ON() is overkill, I think.
WARN_ON_ONCE() would make it.

Not that this follow_huge_addr() on Power is not reachable via
do_move_page_to_node_array(), because the vma is !vma_is_migratable().

-- 
 Kirill A. Shutemov

Re: Question on follow_page_mask

[Hugh Dickins]

On Tue, 23 Feb 2016, Kirill A. Shutemov wrote:
> On Tue, Feb 23, 2016 at 06:45:05PM +0530, Anshuman Khandual wrote:
> > Not able to understand the first code block of follow_page_mask
> > function. follow_huge_addr function is expected to find the page
> > struct for the given address if it turns out to be a HugeTLB page
> > but then when it finds the page we bug on if it had been called
> > with FOLL_GET flag.
> > 
> > 	page = follow_huge_addr(mm, address, flags & FOLL_WRITE);
> > 	if (!IS_ERR(page)) {
> > 		BUG_ON(flags & FOLL_GET);
> > 		return page;
> > 	}
> > 
> > do_move_page_to_node_array calls follow_page with FOLL_GET which
> > in turn calls follow_page_mask with FOLL_GET. On POWER, the
> > function follow_huge_addr is defined and does not return -EINVAL
> > like the generic one. It returns the page struct if its a HugeTLB
> > page. Just curious to know what is the purpose behind the BUG_ON.
> 
> I would guess requesting pin on non-reclaimable page is considered
> useless, meaning suspicius behavior. BUG_ON() is overkill, I think.
> WARN_ON_ONCE() would make it.

No, it's there to guard against abuse, until the correct functionality
is implemented: which has not so far been required, I think.

The problem is that a get_page() here is too late: it needs to be done
inside each arch's implementation of follow_huge_addr(), while holding
whatever is the appropriate lock, dropped by the time it returns here.

If you look through where FOLL_GET is usually implemented, such as in
follow_page_pte(), but pud and pmd cases too, I hope you'll still find
that they are careful to get the reference on the page while it's safe
in the pagetable.

But follow_huge_addr() would need some work to offer the same guarantees:
it's good for those "peep at a page without actually getting a reference"
cases, but not good enough for preventing a page for being put to some
other use completely, before we've secured it with our reference.

Unless something's changed: the last time I recall the issue coming up,
was when Naoya Horiguchi was working on hugetlbfs page migration: see
linux-kernel/linux-mm mail thread "BUG at mm/memory.c:1489!" from
28 May 2014; and the resolution there was not to support the
follow_huge_addr() case (which IIRC is peculiar to powerpc alone?).

Hugh

Re: Question on follow_page_mask

[Anshuman Khandual]

On 02/23/2016 07:33 PM, Kirill A. Shutemov wrote:
> On Tue, Feb 23, 2016 at 06:45:05PM +0530, Anshuman Khandual wrote:
>> Not able to understand the first code block of follow_page_mask
>> function. follow_huge_addr function is expected to find the page
>> struct for the given address if it turns out to be a HugeTLB page
>> but then when it finds the page we bug on if it had been called
>> with FOLL_GET flag.
>>
>> 	page = follow_huge_addr(mm, address, flags & FOLL_WRITE);
>> 	if (!IS_ERR(page)) {
>> 		BUG_ON(flags & FOLL_GET);
>> 		return page;
>> 	}
>>
>> do_move_page_to_node_array calls follow_page with FOLL_GET which
>> in turn calls follow_page_mask with FOLL_GET. On POWER, the
>> function follow_huge_addr is defined and does not return -EINVAL
>> like the generic one. It returns the page struct if its a HugeTLB
>> page. Just curious to know what is the purpose behind the BUG_ON.
> 
> I would guess requesting pin on non-reclaimable page is considered
> useless, meaning suspicius behavior. BUG_ON() is overkill, I think.
> WARN_ON_ONCE() would make it.

pin on non-reclaimable page ? I thought the page reference is obtained
for page migration purpose only. I may be missing something here.

> 
> Not that this follow_huge_addr() on Power is not reachable via
> do_move_page_to_node_array(), because the vma is !vma_is_migratable().

Was experimenting with that enabled via ARCH_ENABLE_HUGEPAGE_MIGRATION.

Re: Question on follow_page_mask

[Anshuman Khandual]

On 02/24/2016 02:37 AM, Hugh Dickins via Linuxppc-dev wrote:
> On Tue, 23 Feb 2016, Kirill A. Shutemov wrote:
>> On Tue, Feb 23, 2016 at 06:45:05PM +0530, Anshuman Khandual wrote:
>>> Not able to understand the first code block of follow_page_mask
>>> function. follow_huge_addr function is expected to find the page
>>> struct for the given address if it turns out to be a HugeTLB page
>>> but then when it finds the page we bug on if it had been called
>>> with FOLL_GET flag.
>>>
>>> 	page = follow_huge_addr(mm, address, flags & FOLL_WRITE);
>>> 	if (!IS_ERR(page)) {
>>> 		BUG_ON(flags & FOLL_GET);
>>> 		return page;
>>> 	}
>>>
>>> do_move_page_to_node_array calls follow_page with FOLL_GET which
>>> in turn calls follow_page_mask with FOLL_GET. On POWER, the
>>> function follow_huge_addr is defined and does not return -EINVAL
>>> like the generic one. It returns the page struct if its a HugeTLB
>>> page. Just curious to know what is the purpose behind the BUG_ON.
>>
>> I would guess requesting pin on non-reclaimable page is considered
>> useless, meaning suspicius behavior. BUG_ON() is overkill, I think.
>> WARN_ON_ONCE() would make it.

Hugh, thanks for such a detailed response.

> 
> No, it's there to guard against abuse, until the correct functionality
> is implemented: which has not so far been required, I think.
> 
> The problem is that a get_page() here is too late: it needs to be done
> inside each arch's implementation of follow_huge_addr(), while holding
> whatever is the appropriate lock, dropped by the time it returns here.

Got it.

> 
> If you look through where FOLL_GET is usually implemented, such as in
> follow_page_pte(), but pud and pmd cases too, I hope you'll still find
> that they are careful to get the reference on the page while it's safe
> in the pagetable.

yeah, true.

> 
> But follow_huge_addr() would need some work to offer the same guarantees:
> it's good for those "peep at a page without actually getting a reference"
> cases, but not good enough for preventing a page for being put to some
> other use completely, before we've secured it with our reference.

Yeah, I understand that now.

> 
> Unless something's changed: the last time I recall the issue coming up,
> was when Naoya Horiguchi was working on hugetlbfs page migration: see
> linux-kernel/linux-mm mail thread "BUG at mm/memory.c:1489!" from
> 28 May 2014; and the resolution there was not to support the
> follow_huge_addr() case (which IIRC is peculiar to powerpc alone?).

Yeah correct, looked into that discussion. Also tried out the discussed
small patch where we do a get_page(page) if the returned page is a head
of a HugeTLB compound page and the flag contains FOLL_GET.

That made the do_move_page_to_node_array function work before hitting a
race condition with the test case in the commit message of e66f17ff7177
("mm/hugetlb: take page table lock in follow_huge_pmd()").

I believe the test exposes the locking problem which is not being taken
care at the follow_huge_addr function level right now on powerpc and
forces it to call follow_huge_pmd (which does a BUG_ON() on powerpc)
after failing to detect a huge page at the PMD level when it checked
the first time around.