[PATCH] mm: fix is_pinnable_page against on cma page

[PATCH] mm: fix is_pinnable_page against on cma page

[Minchan Kim]

Pages on CMA area could have MIGRATE_ISOLATE as well as MIGRATE_CMA
so current is_pinnable_page could miss CMA pages which has MIGRATE_
ISOLATE. It ends up putting CMA pages longterm pinning possible on
pin_user_pages APIs so CMA allocation fails.

The CMA allocation path protects the migration type change race
using zone->lock but what GUP path need to know is just whether the
page is on CMA area or not rather than exact type. Thus, we don't
need zone->lock but just checks the migratype in either of
(MIGRATE_ISOLATE and MIGRATE_CMA).

Signed-off-by: Minchan Kim <minchan@kernel.org>
---
 include/linux/mm.h | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/include/linux/mm.h b/include/linux/mm.h
index 6acca5cecbc5..f59bbe3296e3 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -1625,8 +1625,10 @@ static inline bool page_needs_cow_for_dma(struct vm_area_struct *vma,
 #ifdef CONFIG_MIGRATION
 static inline bool is_pinnable_page(struct page *page)
 {
-	return !(is_zone_movable_page(page) || is_migrate_cma_page(page)) ||
-		is_zero_pfn(page_to_pfn(page));
+	int mt = get_pageblock_migratetype(page);
+
+	return !(is_zone_movable_page(page) || mt == MIGRATE_CMA ||
+		mt == MIGRATE_ISOLATE || is_zero_pfn(page_to_pfn(page)));
 }
 #else
 static inline bool is_pinnable_page(struct page *page)
-- 
2.36.0.464.gb9c8b46e94-goog

Re: [PATCH] mm: fix is_pinnable_page against on cma page

[David Hildenbrand]

On 02.05.22 19:35, Minchan Kim wrote:
> Pages on CMA area could have MIGRATE_ISOLATE as well as MIGRATE_CMA
> so current is_pinnable_page could miss CMA pages which has MIGRATE_
> ISOLATE. It ends up putting CMA pages longterm pinning possible on
> pin_user_pages APIs so CMA allocation fails.
> 
> The CMA allocation path protects the migration type change race
> using zone->lock but what GUP path need to know is just whether the
> page is on CMA area or not rather than exact type. Thus, we don't
> need zone->lock but just checks the migratype in either of
> (MIGRATE_ISOLATE and MIGRATE_CMA).
> 
> Signed-off-by: Minchan Kim <minchan@kernel.org>
> ---
>  include/linux/mm.h | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 6acca5cecbc5..f59bbe3296e3 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -1625,8 +1625,10 @@ static inline bool page_needs_cow_for_dma(struct vm_area_struct *vma,
>  #ifdef CONFIG_MIGRATION
>  static inline bool is_pinnable_page(struct page *page)
>  {
> -	return !(is_zone_movable_page(page) || is_migrate_cma_page(page)) ||
> -		is_zero_pfn(page_to_pfn(page));
> +	int mt = get_pageblock_migratetype(page);
> +
> +	return !(is_zone_movable_page(page) || mt == MIGRATE_CMA ||
> +		mt == MIGRATE_ISOLATE || is_zero_pfn(page_to_pfn(page)));
>  }
>  #else
>  static inline bool is_pinnable_page(struct page *page)

That implies that other memory ranges that are currently isolated
(memory offlining, alloc_contig_range()) cannot be pinned. That might
not be a bad thing, however, I think we could end up failing to pin
something that's temporarily unmovable (due to temporary references).

However, I assume we have the same issue right now already with
ZONE_MOVABLE and MIGRATE_CMA when trying to pin a page residing on these
there are temporarily unmovable and we fail to migrate. But it would now
apply even without ZONE_MOVABLE or MIGRATE_CMA. Hm...


-- 
Thanks,

David / dhildenb

Re: [PATCH] mm: fix is_pinnable_page against on cma page

[Minchan Kim]

On Mon, May 02, 2022 at 08:02:31PM +0200, David Hildenbrand wrote:
> On 02.05.22 19:35, Minchan Kim wrote:
> > Pages on CMA area could have MIGRATE_ISOLATE as well as MIGRATE_CMA
> > so current is_pinnable_page could miss CMA pages which has MIGRATE_
> > ISOLATE. It ends up putting CMA pages longterm pinning possible on
> > pin_user_pages APIs so CMA allocation fails.
> > 
> > The CMA allocation path protects the migration type change race
> > using zone->lock but what GUP path need to know is just whether the
> > page is on CMA area or not rather than exact type. Thus, we don't
> > need zone->lock but just checks the migratype in either of
> > (MIGRATE_ISOLATE and MIGRATE_CMA).
> > 
> > Signed-off-by: Minchan Kim <minchan@kernel.org>
> > ---
> >  include/linux/mm.h | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/include/linux/mm.h b/include/linux/mm.h
> > index 6acca5cecbc5..f59bbe3296e3 100644
> > --- a/include/linux/mm.h
> > +++ b/include/linux/mm.h
> > @@ -1625,8 +1625,10 @@ static inline bool page_needs_cow_for_dma(struct vm_area_struct *vma,
> >  #ifdef CONFIG_MIGRATION
> >  static inline bool is_pinnable_page(struct page *page)
> >  {
> > -	return !(is_zone_movable_page(page) || is_migrate_cma_page(page)) ||
> > -		is_zero_pfn(page_to_pfn(page));
> > +	int mt = get_pageblock_migratetype(page);
> > +
> > +	return !(is_zone_movable_page(page) || mt == MIGRATE_CMA ||
> > +		mt == MIGRATE_ISOLATE || is_zero_pfn(page_to_pfn(page)));
> >  }
> >  #else
> >  static inline bool is_pinnable_page(struct page *page)
> 
> That implies that other memory ranges that are currently isolated
> (memory offlining, alloc_contig_range()) cannot be pinned. That might
> not be a bad thing, however, I think we could end up failing to pin
> something that's temporarily unmovable (due to temporary references).

Sure.

> 
> However, I assume we have the same issue right now already with
> ZONE_MOVABLE and MIGRATE_CMA when trying to pin a page residing on these

ZONE_MOVALBE is also changed dynamically?

> there are temporarily unmovable and we fail to migrate. But it would now
> apply even without ZONE_MOVABLE or MIGRATE_CMA. Hm...

Didn't parse your last mention.

Re: [PATCH] mm: fix is_pinnable_page against on cma page

[David Hildenbrand]

>>>> However, I assume we have the same issue right now already with
>> ZONE_MOVABLE and MIGRATE_CMA when trying to pin a page residing on these
> 
> ZONE_MOVALBE is also changed dynamically?
> 

Sorry, with "same issue" I meant failing to pin if having to migrate and
the page is temporarily unmovable.

>> there are temporarily unmovable and we fail to migrate. But it would now
>> apply even without ZONE_MOVABLE or MIGRATE_CMA. Hm...
> 
> Didn't parse your last mention.

On a system that neither uses ZONE_MOVABLE nor MIGRATE_CMA we might have
to migrate now when pinning.

-- 
Thanks,

David / dhildenb

Re: [PATCH] mm: fix is_pinnable_page against on cma page

[Minchan Kim]

On Tue, May 03, 2022 at 03:15:24AM +0200, David Hildenbrand wrote:
> 
> >>>> However, I assume we have the same issue right now already with
> >> ZONE_MOVABLE and MIGRATE_CMA when trying to pin a page residing on these
> > 
> > ZONE_MOVALBE is also changed dynamically?
> > 
> 
> Sorry, with "same issue" I meant failing to pin if having to migrate and
> the page is temporarily unmovable.
> 
> >> there are temporarily unmovable and we fail to migrate. But it would now
> >> apply even without ZONE_MOVABLE or MIGRATE_CMA. Hm...
> > 
> > Didn't parse your last mention.
> 
> On a system that neither uses ZONE_MOVABLE nor MIGRATE_CMA we might have
> to migrate now when pinning.

I don't understand your point. My problem is pin_user_pages with
FOLL_LONGTERM. It shouldn't pin a page from ZONE_MOVABLE and cma area
without migrating page out of movable zone or CMA area.
That's why try_grab_folio checks whether target page stays in those
movable areas. However, to check CMA area, is_migrate_cma_page is
racy so the FOLL_LONGTERM flag semantic is broken right now.

Do you see any problem of the fix?

A thing to get some attention is whether we need READ_ONCE or not
for the local variable mt.

Re: [PATCH] mm: fix is_pinnable_page against on cma page

[David Hildenbrand]

On 03.05.22 17:26, Minchan Kim wrote:
> On Tue, May 03, 2022 at 03:15:24AM +0200, David Hildenbrand wrote:
>>
>>>>>> However, I assume we have the same issue right now already with
>>>> ZONE_MOVABLE and MIGRATE_CMA when trying to pin a page residing on these
>>>
>>> ZONE_MOVALBE is also changed dynamically?
>>>
>>
>> Sorry, with "same issue" I meant failing to pin if having to migrate and
>> the page is temporarily unmovable.
>>
>>>> there are temporarily unmovable and we fail to migrate. But it would now
>>>> apply even without ZONE_MOVABLE or MIGRATE_CMA. Hm...
>>>
>>> Didn't parse your last mention.
>>
>> On a system that neither uses ZONE_MOVABLE nor MIGRATE_CMA we might have
>> to migrate now when pinning.
> 
> I don't understand your point. My problem is pin_user_pages with
> FOLL_LONGTERM. It shouldn't pin a page from ZONE_MOVABLE and cma area
> without migrating page out of movable zone or CMA area.
> That's why try_grab_folio checks whether target page stays in those
> movable areas. However, to check CMA area, is_migrate_cma_page is
> racy so the FOLL_LONGTERM flag semantic is broken right now.
> 
> Do you see any problem of the fix?

My point is that you might decide to migrate a page because you stumble
over MIGRATE_ISOLATE, although there is no need to reject long-term
pinning and to trigger page migration.

Assume a system without ZONE_MOVABLE and without MIGRATE_CMA. Assume
someone reserves gigantic pages (alloc_contig_range()) and you have
concurrent long-term pinning on a page that is no MIGRATE_ISOLATE.

GUP would see MIGRATE_ISOLATE and would reject pinning. The page has to
be migrated, which can fail if the page is temporarily unmovable.

See my point? We will try migrating in cases where we don't have to
migrate. I think what we would want to do is always reject pinning a CMA
page, independent of the isolation status. but we don't have that
information available.

I raised in the past that we should look into preserving the migration
type and turning MIGRATE_ISOLATE essentially into an additional flag.


So I guess this patch is the right thing to do for now, but I wanted to
spell out the implications.

> 
> A thing to get some attention is whether we need READ_ONCE or not
> for the local variable mt.
> 

Hmm good point. Staring at __get_pfnblock_flags_mask(), I don't think
there is anything stopping the compiler from re-reading the value. But
we don't care if we're reading MIGRATE_CMA or MIGRATE_ISOLATE, not
something in between.

-- 
Thanks,

David / dhildenb

Re: [PATCH] mm: fix is_pinnable_page against on cma page

[Minchan Kim]

On Tue, May 03, 2022 at 06:02:33PM +0200, David Hildenbrand wrote:
> On 03.05.22 17:26, Minchan Kim wrote:
> > On Tue, May 03, 2022 at 03:15:24AM +0200, David Hildenbrand wrote:
> >>
> >>>>>> However, I assume we have the same issue right now already with
> >>>> ZONE_MOVABLE and MIGRATE_CMA when trying to pin a page residing on these
> >>>
> >>> ZONE_MOVALBE is also changed dynamically?
> >>>
> >>
> >> Sorry, with "same issue" I meant failing to pin if having to migrate and
> >> the page is temporarily unmovable.
> >>
> >>>> there are temporarily unmovable and we fail to migrate. But it would now
> >>>> apply even without ZONE_MOVABLE or MIGRATE_CMA. Hm...
> >>>
> >>> Didn't parse your last mention.
> >>
> >> On a system that neither uses ZONE_MOVABLE nor MIGRATE_CMA we might have
> >> to migrate now when pinning.
> > 
> > I don't understand your point. My problem is pin_user_pages with
> > FOLL_LONGTERM. It shouldn't pin a page from ZONE_MOVABLE and cma area
> > without migrating page out of movable zone or CMA area.
> > That's why try_grab_folio checks whether target page stays in those
> > movable areas. However, to check CMA area, is_migrate_cma_page is
> > racy so the FOLL_LONGTERM flag semantic is broken right now.
> > 
> > Do you see any problem of the fix?
> 
> My point is that you might decide to migrate a page because you stumble
> over MIGRATE_ISOLATE, although there is no need to reject long-term
> pinning and to trigger page migration.
> 
> Assume a system without ZONE_MOVABLE and without MIGRATE_CMA. Assume
> someone reserves gigantic pages (alloc_contig_range()) and you have
> concurrent long-term pinning on a page that is no MIGRATE_ISOLATE.
> 
> GUP would see MIGRATE_ISOLATE and would reject pinning. The page has to
> be migrated, which can fail if the page is temporarily unmovable.

Why is the page temporarily unmovable? The GUP didn't increase the
refcount in the case. If it's not migrabtable, that's not a fault
from the GUP but someone is already holding the temporal refcount.
It's not the scope this patchset would try to solve it.

> 
> See my point? We will try migrating in cases where we don't have to

Still not clear for me what you are concerning.

> migrate. I think what we would want to do is always reject pinning a CMA
> page, independent of the isolation status. but we don't have that

Always reject pinning a CMA page if it is *FOLL_LONGTERM*

> information available.

page && (MIGRATE_CMA | MIGRATE_ISOLATE) && gup_flags is not enough
for it?

> 
> I raised in the past that we should look into preserving the migration
> type and turning MIGRATE_ISOLATE essentially into an additional flag.
> 
> 
> So I guess this patch is the right thing to do for now, but I wanted to
> spell out the implications.

I want but still don't understand what you want to write further
about the implication parts. If you make more clear, I am happy to
include it.

> 
> > 
> > A thing to get some attention is whether we need READ_ONCE or not
> > for the local variable mt.
> > 
> 
> Hmm good point. Staring at __get_pfnblock_flags_mask(), I don't think
> there is anything stopping the compiler from re-reading the value. But
> we don't care if we're reading MIGRATE_CMA or MIGRATE_ISOLATE, not
> something in between.

How about this?

     CPU A                                                      CPU B

is_pinnable_page
  ..
  ..                                                set_pageblock_migratetype(MIGRATE_ISOLATE)
  mt == MIGRATE_CMA
    get_pageblock_miratetype(page)
        returns MIGRATE_ISOLATE
  mt == MIGRATE_ISOLATE                             set_pageblock_migratetype(MIGRATE_CMA)
    get_pageblock_miratetype(page)
        returns MIGRATE_CMA
 
So both conditions fails to detect it.

Re: [PATCH] mm: fix is_pinnable_page against on cma page

[David Hildenbrand]

>> GUP would see MIGRATE_ISOLATE and would reject pinning. The page has to
>> be migrated, which can fail if the page is temporarily unmovable.
> 
> Why is the page temporarily unmovable? The GUP didn't increase the
> refcount in the case. If it's not migrabtable, that's not a fault
> from the GUP but someone is already holding the temporal refcount.
> It's not the scope this patchset would try to solve it.

You can have other references on the page that turn it temporarily
unmovable, for example, via FOLL_GET, short-term FOLL_PIN.

> 
>>
>> See my point? We will try migrating in cases where we don't have to
> 
> Still not clear for me what you are concerning.
> 
>> migrate. I think what we would want to do is always reject pinning a CMA
>> page, independent of the isolation status. but we don't have that
> 
> Always reject pinning a CMA page if it is *FOLL_LONGTERM*

Yes.

> 
>> information available.
> 
> page && (MIGRATE_CMA | MIGRATE_ISOLATE) && gup_flags is not enough
> for it?
> 
>>
>> I raised in the past that we should look into preserving the migration
>> type and turning MIGRATE_ISOLATE essentially into an additional flag.
>>
>>
>> So I guess this patch is the right thing to do for now, but I wanted to
>> spell out the implications.
> 
> I want but still don't understand what you want to write further
> about the implication parts. If you make more clear, I am happy to
> include it.

What I am essentially saying is that when rejecting to long-term
FOLL_PIN something that is MIGRATE_ISOLATE now, we might now end up
having to migrate pages that are actually fine to get pinned, because
they are not actual CMA pages. And any such migration might fail when
pages are temporarily unmovable.


> 
>>
>>>
>>> A thing to get some attention is whether we need READ_ONCE or not
>>> for the local variable mt.
>>>
>>
>> Hmm good point. Staring at __get_pfnblock_flags_mask(), I don't think
>> there is anything stopping the compiler from re-reading the value. But
>> we don't care if we're reading MIGRATE_CMA or MIGRATE_ISOLATE, not
>> something in between.
> 
> How about this?
> 
>      CPU A                                                      CPU B
> 
> is_pinnable_page
>   ..
>   ..                                                set_pageblock_migratetype(MIGRATE_ISOLATE)
>   mt == MIGRATE_CMA
>     get_pageblock_miratetype(page)
>         returns MIGRATE_ISOLATE
>   mt == MIGRATE_ISOLATE                             set_pageblock_migratetype(MIGRATE_CMA)
>     get_pageblock_miratetype(page)
>         returns MIGRATE_CMA
>  
> So both conditions fails to detect it.

I think you're right. That's nasty.

-- 
Thanks,

David / dhildenb

Re: [PATCH] mm: fix is_pinnable_page against on cma page

[Minchan Kim]

On Tue, May 03, 2022 at 07:27:06PM +0200, David Hildenbrand wrote:
> >> GUP would see MIGRATE_ISOLATE and would reject pinning. The page has to
> >> be migrated, which can fail if the page is temporarily unmovable.
> > 
> > Why is the page temporarily unmovable? The GUP didn't increase the
> > refcount in the case. If it's not migrabtable, that's not a fault
> > from the GUP but someone is already holding the temporal refcount.
> > It's not the scope this patchset would try to solve it.
> 
> You can have other references on the page that turn it temporarily
> unmovable, for example, via FOLL_GET, short-term FOLL_PIN.

Sure. However, user didn't passed the FOLL_LONGTERM. In that case,
the temporal page migration failure was expected.
What we want to guarantee for successful page migration is only
FOLL_LONGTERM.

If you are talking about the general problem(any GUP API without
FOLL_LONGTERM flag which is supposed to be short-term could turn
into long-term pinning by several reasons - I had struggled with
those issues - FOLL_LONGTERM is misnormer to me), yeah, I agree
we need to fix it but it's orthgonal issue.

> 
> > 
> >>
> >> See my point? We will try migrating in cases where we don't have to
> > 
> > Still not clear for me what you are concerning.
> > 
> >> migrate. I think what we would want to do is always reject pinning a CMA
> >> page, independent of the isolation status. but we don't have that
> > 
> > Always reject pinning a CMA page if it is *FOLL_LONGTERM*
> 
> Yes.
> 
> > 
> >> information available.
> > 
> > page && (MIGRATE_CMA | MIGRATE_ISOLATE) && gup_flags is not enough
> > for it?
> > 
> >>
> >> I raised in the past that we should look into preserving the migration
> >> type and turning MIGRATE_ISOLATE essentially into an additional flag.
> >>
> >>
> >> So I guess this patch is the right thing to do for now, but I wanted to
> >> spell out the implications.
> > 
> > I want but still don't understand what you want to write further
> > about the implication parts. If you make more clear, I am happy to
> > include it.
> 
> What I am essentially saying is that when rejecting to long-term
> FOLL_PIN something that is MIGRATE_ISOLATE now, we might now end up
> having to migrate pages that are actually fine to get pinned, because
> they are not actual CMA pages. And any such migration might fail when
> pages are temporarily unmovable.

Now I understand concern. Then how about introducing cma areas list
and use it instead of migrate type in is_pinnable_page

struct cma  {
  ..
  ..
  list_head list
};

bool is_cma_page(unsigned long pfn) {
    for cma in cma_list
      if (pfn >= cma->base_pfn && pfn < cma->base_pfn + count
        return true;

    return false;
}

Do you want to fix it at this moment or just write down the
possibility in the description and then we could fix once it
really happens later?

> 
> 
> > 
> >>
> >>>
> >>> A thing to get some attention is whether we need READ_ONCE or not
> >>> for the local variable mt.
> >>>
> >>
> >> Hmm good point. Staring at __get_pfnblock_flags_mask(), I don't think
> >> there is anything stopping the compiler from re-reading the value. But
> >> we don't care if we're reading MIGRATE_CMA or MIGRATE_ISOLATE, not
> >> something in between.
> > 
> > How about this?
> > 
> >      CPU A                                                      CPU B
> > 
> > is_pinnable_page
> >   ..
> >   ..                                                set_pageblock_migratetype(MIGRATE_ISOLATE)
> >   mt == MIGRATE_CMA
> >     get_pageblock_miratetype(page)
> >         returns MIGRATE_ISOLATE
> >   mt == MIGRATE_ISOLATE                             set_pageblock_migratetype(MIGRATE_CMA)
> >     get_pageblock_miratetype(page)
> >         returns MIGRATE_CMA
> >  
> > So both conditions fails to detect it.
> 
> I think you're right. That's nasty.

Ccing Paul to borrow expertise. :)

int mt = get_pageblock_migratetype(page);

if (mt == MIGRATE_CMA)
  return true;

if (mt == MIGRATE_ISOLATE)
  return true;

I'd like to keep use the local variable mt's value in folloing
conditions checks instead of refetching the value from
get_pageblock_migratetype.

What's the right way to achieve it?

Thanks in advance!

Re: [PATCH] mm: fix is_pinnable_page against on cma page

[Minchan Kim]

On Tue, May 03, 2022 at 11:08:25AM -0700, Minchan Kim wrote:
< snip >

Ccing Paul really this time.

Attach original code for Paul.

diff --git a/include/linux/mm.h b/include/linux/mm.h
index 6acca5cecbc5..f59bbe3296e3 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -1625,8 +1625,10 @@ static inline bool page_needs_cow_for_dma(struct vm_area_struct *vma,
 #ifdef CONFIG_MIGRATION
 static inline bool is_pinnable_page(struct page *page)
 {
-	return !(is_zone_movable_page(page) || is_migrate_cma_page(page)) ||
-		is_zero_pfn(page_to_pfn(page));
+	int mt = get_pageblock_migratetype(page);
+
+	return !(is_zone_movable_page(page) || mt == MIGRATE_CMA ||
+		mt == MIGRATE_ISOLATE || is_zero_pfn(page_to_pfn(page)));
 }
 #else
 static inline bool is_pinnable_page(struct page *page)
--
2.36.0.464.gb9c8b46e94-goog


> > >>>
> > >>> A thing to get some attention is whether we need READ_ONCE or not
> > >>> for the local variable mt.
> > >>>
> > >>
> > >> Hmm good point. Staring at __get_pfnblock_flags_mask(), I don't think
> > >> there is anything stopping the compiler from re-reading the value. But
> > >> we don't care if we're reading MIGRATE_CMA or MIGRATE_ISOLATE, not
> > >> something in between.
> > > 
> > > How about this?
> > > 
> > >      CPU A                                                      CPU B
> > > 
> > > is_pinnable_page
> > >   ..
> > >   ..                                                set_pageblock_migratetype(MIGRATE_ISOLATE)
> > >   mt == MIGRATE_CMA
> > >     get_pageblock_miratetype(page)
> > >         returns MIGRATE_ISOLATE
> > >   mt == MIGRATE_ISOLATE                             set_pageblock_migratetype(MIGRATE_CMA)
> > >     get_pageblock_miratetype(page)
> > >         returns MIGRATE_CMA
> > >  
> > > So both conditions fails to detect it.
> > 
> > I think you're right. That's nasty.
> 
> Ccing Paul to borrow expertise. :)
> 
> int mt = get_pageblock_migratetype(page);
> 
> if (mt == MIGRATE_CMA)
>   return true;
> 
> if (mt == MIGRATE_ISOLATE)
>   return true;
> 
> I'd like to keep use the local variable mt's value in folloing
> conditions checks instead of refetching the value from
> get_pageblock_migratetype.
> 
> What's the right way to achieve it?
> 
> Thanks in advance!

Paul, could you give any hint?

Re: [PATCH] mm: fix is_pinnable_page against on cma page

[Minchan Kim]

On Tue, May 03, 2022 at 06:02:33PM +0200, David Hildenbrand wrote:
> On 03.05.22 17:26, Minchan Kim wrote:
> > On Tue, May 03, 2022 at 03:15:24AM +0200, David Hildenbrand wrote:
> >>
> >>>>>> However, I assume we have the same issue right now already with
> >>>> ZONE_MOVABLE and MIGRATE_CMA when trying to pin a page residing on these
> >>>
> >>> ZONE_MOVALBE is also changed dynamically?
> >>>
> >>
> >> Sorry, with "same issue" I meant failing to pin if having to migrate and
> >> the page is temporarily unmovable.
> >>
> >>>> there are temporarily unmovable and we fail to migrate. But it would now
> >>>> apply even without ZONE_MOVABLE or MIGRATE_CMA. Hm...
> >>>
> >>> Didn't parse your last mention.
> >>
> >> On a system that neither uses ZONE_MOVABLE nor MIGRATE_CMA we might have
> >> to migrate now when pinning.
> > 
> > I don't understand your point. My problem is pin_user_pages with
> > FOLL_LONGTERM. It shouldn't pin a page from ZONE_MOVABLE and cma area
> > without migrating page out of movable zone or CMA area.
> > That's why try_grab_folio checks whether target page stays in those
> > movable areas. However, to check CMA area, is_migrate_cma_page is
> > racy so the FOLL_LONGTERM flag semantic is broken right now.
> > 
> > Do you see any problem of the fix?
> 
> My point is that you might decide to migrate a page because you stumble
> over MIGRATE_ISOLATE, although there is no need to reject long-term
> pinning and to trigger page migration.
> 
> Assume a system without ZONE_MOVABLE and without MIGRATE_CMA. Assume
> someone reserves gigantic pages (alloc_contig_range()) and you have
> concurrent long-term pinning on a page that is no MIGRATE_ISOLATE.
> 
> GUP would see MIGRATE_ISOLATE and would reject pinning. The page has to
> be migrated, which can fail if the page is temporarily unmovable.

A dump question since I'm not familiar with hugetlb.

Is above reasonable scenario?

The gigantic page is about to be created using alloc_contig_range so
they has MIGRATE_ISOLATE as temporal state. It means no one uses the
page yet so I guess the page is not mapped at userspace but other is
trying to access the page using pin_user_pages?

> 
> See my point? We will try migrating in cases where we don't have to
> migrate. I think what we would want to do is always reject pinning a CMA
> page, independent of the isolation status. but we don't have that
> information available.
> 
> I raised in the past that we should look into preserving the migration
> type and turning MIGRATE_ISOLATE essentially into an additional flag.
> 
> 
> So I guess this patch is the right thing to do for now, but I wanted to
> spell out the implications.

Re: [PATCH] mm: fix is_pinnable_page against on cma page

[Minchan Kim]

On Wed, May 04, 2022 at 03:48:54PM -0700, Minchan Kim wrote:
> On Tue, May 03, 2022 at 06:02:33PM +0200, David Hildenbrand wrote:
> > On 03.05.22 17:26, Minchan Kim wrote:
> > > On Tue, May 03, 2022 at 03:15:24AM +0200, David Hildenbrand wrote:
> > >>
> > >>>>>> However, I assume we have the same issue right now already with
> > >>>> ZONE_MOVABLE and MIGRATE_CMA when trying to pin a page residing on these
> > >>>
> > >>> ZONE_MOVALBE is also changed dynamically?
> > >>>
> > >>
> > >> Sorry, with "same issue" I meant failing to pin if having to migrate and
> > >> the page is temporarily unmovable.
> > >>
> > >>>> there are temporarily unmovable and we fail to migrate. But it would now
> > >>>> apply even without ZONE_MOVABLE or MIGRATE_CMA. Hm...
> > >>>
> > >>> Didn't parse your last mention.
> > >>
> > >> On a system that neither uses ZONE_MOVABLE nor MIGRATE_CMA we might have
> > >> to migrate now when pinning.
> > > 
> > > I don't understand your point. My problem is pin_user_pages with
> > > FOLL_LONGTERM. It shouldn't pin a page from ZONE_MOVABLE and cma area
> > > without migrating page out of movable zone or CMA area.
> > > That's why try_grab_folio checks whether target page stays in those
> > > movable areas. However, to check CMA area, is_migrate_cma_page is
> > > racy so the FOLL_LONGTERM flag semantic is broken right now.
> > > 
> > > Do you see any problem of the fix?
> > 
> > My point is that you might decide to migrate a page because you stumble
> > over MIGRATE_ISOLATE, although there is no need to reject long-term
> > pinning and to trigger page migration.
> > 
> > Assume a system without ZONE_MOVABLE and without MIGRATE_CMA. Assume
> > someone reserves gigantic pages (alloc_contig_range()) and you have
> > concurrent long-term pinning on a page that is no MIGRATE_ISOLATE.
> > 
> > GUP would see MIGRATE_ISOLATE and would reject pinning. The page has to
> > be migrated, which can fail if the page is temporarily unmovable.
> 
> A dump question since I'm not familiar with hugetlb.
> 
> Is above reasonable scenario?
> 
> The gigantic page is about to be created using alloc_contig_range so
> they has MIGRATE_ISOLATE as temporal state. It means no one uses the
> page yet so I guess the page is not mapped at userspace but other is
> trying to access the page using pin_user_pages?
> 

Too dump question. Never mind.
Posted v2 - https://lore.kernel.org/all/20220505064429.2818496-1-minchan@kernel.org/T/#u

Thanks.

Re: [PATCH] mm: fix is_pinnable_page against on cma page

[Mike Kravetz]

On 5/4/22 23:48, Minchan Kim wrote:
> On Wed, May 04, 2022 at 03:48:54PM -0700, Minchan Kim wrote:
>> On Tue, May 03, 2022 at 06:02:33PM +0200, David Hildenbrand wrote:
>>> On 03.05.22 17:26, Minchan Kim wrote:
>>>> On Tue, May 03, 2022 at 03:15:24AM +0200, David Hildenbrand wrote:
>>>>>
>>>>>>>>> However, I assume we have the same issue right now already with
>>>>>>> ZONE_MOVABLE and MIGRATE_CMA when trying to pin a page residing on these
>>>>>>
>>>>>> ZONE_MOVALBE is also changed dynamically?
>>>>>>
>>>>>
>>>>> Sorry, with "same issue" I meant failing to pin if having to migrate and
>>>>> the page is temporarily unmovable.
>>>>>
>>>>>>> there are temporarily unmovable and we fail to migrate. But it would now
>>>>>>> apply even without ZONE_MOVABLE or MIGRATE_CMA. Hm...
>>>>>>
>>>>>> Didn't parse your last mention.
>>>>>
>>>>> On a system that neither uses ZONE_MOVABLE nor MIGRATE_CMA we might have
>>>>> to migrate now when pinning.
>>>>
>>>> I don't understand your point. My problem is pin_user_pages with
>>>> FOLL_LONGTERM. It shouldn't pin a page from ZONE_MOVABLE and cma area
>>>> without migrating page out of movable zone or CMA area.
>>>> That's why try_grab_folio checks whether target page stays in those
>>>> movable areas. However, to check CMA area, is_migrate_cma_page is
>>>> racy so the FOLL_LONGTERM flag semantic is broken right now.
>>>>
>>>> Do you see any problem of the fix?
>>>
>>> My point is that you might decide to migrate a page because you stumble
>>> over MIGRATE_ISOLATE, although there is no need to reject long-term
>>> pinning and to trigger page migration.
>>>
>>> Assume a system without ZONE_MOVABLE and without MIGRATE_CMA. Assume
>>> someone reserves gigantic pages (alloc_contig_range()) and you have
>>> concurrent long-term pinning on a page that is no MIGRATE_ISOLATE.
>>>
>>> GUP would see MIGRATE_ISOLATE and would reject pinning. The page has to
>>> be migrated, which can fail if the page is temporarily unmovable.
>>
>> A dump question since I'm not familiar with hugetlb.
>>
>> Is above reasonable scenario?
>>
>> The gigantic page is about to be created using alloc_contig_range so
>> they has MIGRATE_ISOLATE as temporal state. It means no one uses the
>> page yet so I guess the page is not mapped at userspace but other is
>> trying to access the page using pin_user_pages?
>>
> 
> Too dump question. Never mind.
> Posted v2 - https://lore.kernel.org/all/20220505064429.2818496-1-minchan@kernel.org/T/#u
> 

Well your question mentioned hugetlb so my mail filters caught it :)

Your question caused me to think of the following.  No need for any immediate
change: I think.  Just wanted to share.

Suppose someone has reserved CMA for gigantic hugetlb allocations.  And,
suppose FOLL_LONGTERM is attempted on such a page (it would be in use).  The
desired action would be to migrate the page out of CMA.  Correct?

Gigantic pages can only be migrated IF there is another (already allocated)
gigantic page available.  The routine to try and allocate a page 'on the fly'
for migration will fail if passed a gigantic size.  There 'might' be a free
pre-allocated gigantic page.  However, if the user set up CMA reserves for
gigantic page allocations it is likely the free gigantic page is also in CMA.
Therefore, it can not be used for this migration.  So, unless my reasoning
is wrong, FOLL_LONGTERM would almost always fail for gigantic pages in CMA.
-- 
Mike Kravetz

Re: [PATCH] mm: fix is_pinnable_page against on cma page

[Peter Xu]

On Thu, May 05, 2022 at 10:00:07AM -0700, Mike Kravetz wrote:
> Gigantic pages can only be migrated IF there is another (already allocated)
> gigantic page available.  The routine to try and allocate a page 'on the fly'
> for migration will fail if passed a gigantic size.  There 'might' be a free
> pre-allocated gigantic page.  However, if the user set up CMA reserves for
> gigantic page allocations it is likely the free gigantic page is also in CMA.
> Therefore, it can not be used for this migration.  So, unless my reasoning
> is wrong, FOLL_LONGTERM would almost always fail for gigantic pages in CMA.

I'm probably not familiar enough with CMA, but.. I just noticed that if CMA
is destined to not be able to be pinned then maybe it'll lose quite a few
scenarios where pinning is a possible use case.  It doesn't even need to be
the major use case, but as long as it's possible (e.g. hypervisors hosting
virtual machines with device assignment) it'll be a hard no to CMA, which
seems to be a pity.

-- 
Peter Xu

Re: [PATCH] mm: fix is_pinnable_page against on cma page

[David Hildenbrand]

On 05.05.22 19:25, Peter Xu wrote:
> On Thu, May 05, 2022 at 10:00:07AM -0700, Mike Kravetz wrote:
>> Gigantic pages can only be migrated IF there is another (already allocated)
>> gigantic page available.  The routine to try and allocate a page 'on the fly'
>> for migration will fail if passed a gigantic size.  There 'might' be a free
>> pre-allocated gigantic page.  However, if the user set up CMA reserves for
>> gigantic page allocations it is likely the free gigantic page is also in CMA.
>> Therefore, it can not be used for this migration.  So, unless my reasoning
>> is wrong, FOLL_LONGTERM would almost always fail for gigantic pages in CMA.
> 
> I'm probably not familiar enough with CMA, but.. I just noticed that if CMA
> is destined to not be able to be pinned then maybe it'll lose quite a few
> scenarios where pinning is a possible use case.  It doesn't even need to be
> the major use case, but as long as it's possible (e.g. hypervisors hosting
> virtual machines with device assignment) it'll be a hard no to CMA, which
> seems to be a pity.
> 

Well, the same applies to ZONE_MOVABLE as well, unfortunately.
Eventually, we might want to disable placing hugetlb pages on CMA areas
if it turns out to be a problem. In case of ZONE_MOVABLE we can already
fail "gracefully" when trying offlining (although that's really far from
beautiful).

-- 
Thanks,

David / dhildenb

Re: [PATCH] mm: fix is_pinnable_page against on cma page

[Minchan Kim]

On Thu, May 05, 2022 at 10:00:07AM -0700, Mike Kravetz wrote:
> On 5/4/22 23:48, Minchan Kim wrote:
> > On Wed, May 04, 2022 at 03:48:54PM -0700, Minchan Kim wrote:
> >> On Tue, May 03, 2022 at 06:02:33PM +0200, David Hildenbrand wrote:
> >>> On 03.05.22 17:26, Minchan Kim wrote:
> >>>> On Tue, May 03, 2022 at 03:15:24AM +0200, David Hildenbrand wrote:
> >>>>>
> >>>>>>>>> However, I assume we have the same issue right now already with
> >>>>>>> ZONE_MOVABLE and MIGRATE_CMA when trying to pin a page residing on these
> >>>>>>
> >>>>>> ZONE_MOVALBE is also changed dynamically?
> >>>>>>
> >>>>>
> >>>>> Sorry, with "same issue" I meant failing to pin if having to migrate and
> >>>>> the page is temporarily unmovable.
> >>>>>
> >>>>>>> there are temporarily unmovable and we fail to migrate. But it would now
> >>>>>>> apply even without ZONE_MOVABLE or MIGRATE_CMA. Hm...
> >>>>>>
> >>>>>> Didn't parse your last mention.
> >>>>>
> >>>>> On a system that neither uses ZONE_MOVABLE nor MIGRATE_CMA we might have
> >>>>> to migrate now when pinning.
> >>>>
> >>>> I don't understand your point. My problem is pin_user_pages with
> >>>> FOLL_LONGTERM. It shouldn't pin a page from ZONE_MOVABLE and cma area
> >>>> without migrating page out of movable zone or CMA area.
> >>>> That's why try_grab_folio checks whether target page stays in those
> >>>> movable areas. However, to check CMA area, is_migrate_cma_page is
> >>>> racy so the FOLL_LONGTERM flag semantic is broken right now.
> >>>>
> >>>> Do you see any problem of the fix?
> >>>
> >>> My point is that you might decide to migrate a page because you stumble
> >>> over MIGRATE_ISOLATE, although there is no need to reject long-term
> >>> pinning and to trigger page migration.
> >>>
> >>> Assume a system without ZONE_MOVABLE and without MIGRATE_CMA. Assume
> >>> someone reserves gigantic pages (alloc_contig_range()) and you have
> >>> concurrent long-term pinning on a page that is no MIGRATE_ISOLATE.
> >>>
> >>> GUP would see MIGRATE_ISOLATE and would reject pinning. The page has to
> >>> be migrated, which can fail if the page is temporarily unmovable.
> >>
> >> A dump question since I'm not familiar with hugetlb.
> >>
> >> Is above reasonable scenario?
> >>
> >> The gigantic page is about to be created using alloc_contig_range so
> >> they has MIGRATE_ISOLATE as temporal state. It means no one uses the
> >> page yet so I guess the page is not mapped at userspace but other is
> >> trying to access the page using pin_user_pages?
> >>
> > 
> > Too dump question. Never mind.
> > Posted v2 - https://lore.kernel.org/all/20220505064429.2818496-1-minchan@kernel.org/T/#u
> > 
> 
> Well your question mentioned hugetlb so my mail filters caught it :)
> 
> Your question caused me to think of the following.  No need for any immediate
> change: I think.  Just wanted to share.
> 
> Suppose someone has reserved CMA for gigantic hugetlb allocations.  And,
> suppose FOLL_LONGTERM is attempted on such a page (it would be in use).  The
> desired action would be to migrate the page out of CMA.  Correct?
> 
> Gigantic pages can only be migrated IF there is another (already allocated)
> gigantic page available.  The routine to try and allocate a page 'on the fly'
> for migration will fail if passed a gigantic size.  There 'might' be a free
> pre-allocated gigantic page.  However, if the user set up CMA reserves for
> gigantic page allocations it is likely the free gigantic page is also in CMA.
> Therefore, it can not be used for this migration.  So, unless my reasoning
> is wrong, FOLL_LONGTERM would almost always fail for gigantic pages in CMA.

FOLL_LONGTERM on CMA-backed gigantic page would already fail, Thanks for sharing!

Anyway, David's concern was non-CMA-backed gigantic page. The alloc_contig_range 
with MIGRATE_ISOLATE runs with concurrent FOLL_LONGTERM pinning, which could
trigger page migration we didn't have before so it might increase FOLL_LONGTERM
GUP failure rate.

Re: [PATCH] mm: fix is_pinnable_page against on cma page

[David Hildenbrand]

On 05.05.22 08:48, Minchan Kim wrote:
> On Wed, May 04, 2022 at 03:48:54PM -0700, Minchan Kim wrote:
>> On Tue, May 03, 2022 at 06:02:33PM +0200, David Hildenbrand wrote:
>>> On 03.05.22 17:26, Minchan Kim wrote:
>>>> On Tue, May 03, 2022 at 03:15:24AM +0200, David Hildenbrand wrote:
>>>>>
>>>>>>>>> However, I assume we have the same issue right now already with
>>>>>>> ZONE_MOVABLE and MIGRATE_CMA when trying to pin a page residing on these
>>>>>>
>>>>>> ZONE_MOVALBE is also changed dynamically?
>>>>>>
>>>>>
>>>>> Sorry, with "same issue" I meant failing to pin if having to migrate and
>>>>> the page is temporarily unmovable.
>>>>>
>>>>>>> there are temporarily unmovable and we fail to migrate. But it would now
>>>>>>> apply even without ZONE_MOVABLE or MIGRATE_CMA. Hm...
>>>>>>
>>>>>> Didn't parse your last mention.
>>>>>
>>>>> On a system that neither uses ZONE_MOVABLE nor MIGRATE_CMA we might have
>>>>> to migrate now when pinning.
>>>>
>>>> I don't understand your point. My problem is pin_user_pages with
>>>> FOLL_LONGTERM. It shouldn't pin a page from ZONE_MOVABLE and cma area
>>>> without migrating page out of movable zone or CMA area.
>>>> That's why try_grab_folio checks whether target page stays in those
>>>> movable areas. However, to check CMA area, is_migrate_cma_page is
>>>> racy so the FOLL_LONGTERM flag semantic is broken right now.
>>>>
>>>> Do you see any problem of the fix?
>>>
>>> My point is that you might decide to migrate a page because you stumble
>>> over MIGRATE_ISOLATE, although there is no need to reject long-term
>>> pinning and to trigger page migration.
>>>
>>> Assume a system without ZONE_MOVABLE and without MIGRATE_CMA. Assume
>>> someone reserves gigantic pages (alloc_contig_range()) and you have
>>> concurrent long-term pinning on a page that is no MIGRATE_ISOLATE.
>>>
>>> GUP would see MIGRATE_ISOLATE and would reject pinning. The page has to
>>> be migrated, which can fail if the page is temporarily unmovable.
>>
>> A dump question since I'm not familiar with hugetlb.
>>
>> Is above reasonable scenario?
>>
>> The gigantic page is about to be created using alloc_contig_range so
>> they has MIGRATE_ISOLATE as temporal state. It means no one uses the
>> page yet so I guess the page is not mapped at userspace but other is
>> trying to access the page using pin_user_pages?
>>
> 
> Too dump question. Never mind.
> Posted v2 - https://lore.kernel.org/all/20220505064429.2818496-1-minchan@kernel.org/T/#u

Sorry for the late reply, still traveling :)

Just so we're on the same page: MIGRATE_ISOLATE would be set on
pageblocks that contain either free or movable pages. In case of movable
pages, they are in uese.

Regarding your is_cma_page() proposal, I think we might want to consider
that if it really turns out to be a problem. For now, I'm fine with just
documenting it.

Thanks!

-- 
Thanks,

David / dhildenb

Re: [PATCH] mm: fix is_pinnable_page against on cma page

[kernel test robot]

Hi Minchan,

I love your patch! Yet something to improve:

[auto build test ERROR on hnaz-mm/master]

url:    https://github.com/intel-lab-lkp/linux/commits/Minchan-Kim/mm-fix-is_pinnable_page-against-on-cma-page/20220503-013733
base:   https://github.com/hnaz/linux-mm master
config: nios2-defconfig (https://download.01.org/0day-ci/archive/20220503/202205030306.in794Jr3-lkp@intel.com/config)
compiler: nios2-linux-gcc (GCC) 11.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/1b8636710c31d44310f1d344e337c207562b851d
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Minchan-Kim/mm-fix-is_pinnable_page-against-on-cma-page/20220503-013733
        git checkout 1b8636710c31d44310f1d344e337c207562b851d
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.3.0 make.cross W=1 O=build_dir ARCH=nios2 prepare

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from include/linux/pid_namespace.h:7,
                    from include/linux/ptrace.h:10,
                    from arch/nios2/kernel/asm-offsets.c:9:
   include/linux/mm.h: In function 'is_pinnable_page':
>> include/linux/mm.h:1630:54: error: 'MIGRATE_CMA' undeclared (first use in this function); did you mean 'MIGRATE_SYNC'?
    1630 |         return !(is_zone_movable_page(page) || mt == MIGRATE_CMA ||
         |                                                      ^~~~~~~~~~~
         |                                                      MIGRATE_SYNC
   include/linux/mm.h:1630:54: note: each undeclared identifier is reported only once for each function it appears in
>> include/linux/mm.h:1631:23: error: 'MIGRATE_ISOLATE' undeclared (first use in this function); did you mean 'MIGRATE_MOVABLE'?
    1631 |                 mt == MIGRATE_ISOLATE || is_zero_pfn(page_to_pfn(page)));
         |                       ^~~~~~~~~~~~~~~
         |                       MIGRATE_MOVABLE
   make[2]: *** [scripts/Makefile.build:122: arch/nios2/kernel/asm-offsets.s] Error 1
   make[2]: Target '__build' not remade because of errors.
   make[1]: *** [Makefile:1283: prepare0] Error 2
   make[1]: Target 'prepare' not remade because of errors.
   make: *** [Makefile:226: __sub-make] Error 2
   make: Target 'prepare' not remade because of errors.


vim +1630 include/linux/mm.h

  1623	
  1624	/* MIGRATE_CMA and ZONE_MOVABLE do not allow pin pages */
  1625	#ifdef CONFIG_MIGRATION
  1626	static inline bool is_pinnable_page(struct page *page)
  1627	{
  1628		int mt = get_pageblock_migratetype(page);
  1629	
> 1630		return !(is_zone_movable_page(page) || mt == MIGRATE_CMA ||
> 1631			mt == MIGRATE_ISOLATE || is_zero_pfn(page_to_pfn(page)));
  1632	}
  1633	#else
  1634	static inline bool is_pinnable_page(struct page *page)
  1635	{
  1636		return true;
  1637	}
  1638	#endif
  1639	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH] mm: fix is_pinnable_page against on cma page

[kernel test robot]

Hi Minchan,

I love your patch! Yet something to improve:

[auto build test ERROR on hnaz-mm/master]

url:    https://github.com/intel-lab-lkp/linux/commits/Minchan-Kim/mm-fix-is_pinnable_page-against-on-cma-page/20220503-013733
base:   https://github.com/hnaz/linux-mm master
config: x86_64-randconfig-a012-20220502 (https://download.01.org/0day-ci/archive/20220503/202205030525.VFo3Ds9w-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 09325d36061e42b495d1f4c7e933e260eac260ed)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/1b8636710c31d44310f1d344e337c207562b851d
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Minchan-Kim/mm-fix-is_pinnable_page-against-on-cma-page/20220503-013733
        git checkout 1b8636710c31d44310f1d344e337c207562b851d
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 prepare

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from arch/x86/kernel/asm-offsets.c:13:
   In file included from include/linux/suspend.h:5:
   In file included from include/linux/swap.h:9:
   In file included from include/linux/memcontrol.h:20:
>> include/linux/mm.h:1630:47: error: use of undeclared identifier 'MIGRATE_CMA'; did you mean 'MIGRATE_SYNC'?
           return !(is_zone_movable_page(page) || mt == MIGRATE_CMA ||
                                                        ^~~~~~~~~~~
                                                        MIGRATE_SYNC
   include/linux/migrate_mode.h:18:2: note: 'MIGRATE_SYNC' declared here
           MIGRATE_SYNC,
           ^
   In file included from arch/x86/kernel/asm-offsets.c:13:
   In file included from include/linux/suspend.h:5:
   In file included from include/linux/swap.h:9:
   In file included from include/linux/memcontrol.h:20:
>> include/linux/mm.h:1631:9: error: use of undeclared identifier 'MIGRATE_ISOLATE'; did you mean 'MIGRATE_MOVABLE'?
                   mt == MIGRATE_ISOLATE || is_zero_pfn(page_to_pfn(page)));
                         ^~~~~~~~~~~~~~~
                         MIGRATE_MOVABLE
   include/linux/mmzone.h:44:2: note: 'MIGRATE_MOVABLE' declared here
           MIGRATE_MOVABLE,
           ^
   2 errors generated.
   make[2]: *** [scripts/Makefile.build:122: arch/x86/kernel/asm-offsets.s] Error 1
   make[2]: Target '__build' not remade because of errors.
   make[1]: *** [Makefile:1283: prepare0] Error 2
   make[1]: Target 'prepare' not remade because of errors.
   make: *** [Makefile:226: __sub-make] Error 2
   make: Target 'prepare' not remade because of errors.


vim +1630 include/linux/mm.h

  1623	
  1624	/* MIGRATE_CMA and ZONE_MOVABLE do not allow pin pages */
  1625	#ifdef CONFIG_MIGRATION
  1626	static inline bool is_pinnable_page(struct page *page)
  1627	{
  1628		int mt = get_pageblock_migratetype(page);
  1629	
> 1630		return !(is_zone_movable_page(page) || mt == MIGRATE_CMA ||
> 1631			mt == MIGRATE_ISOLATE || is_zero_pfn(page_to_pfn(page)));
  1632	}
  1633	#else
  1634	static inline bool is_pinnable_page(struct page *page)
  1635	{
  1636		return true;
  1637	}
  1638	#endif
  1639	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH] mm: fix is_pinnable_page against on cma page

[kernel test robot]

Hi Minchan,

I love your patch! Perhaps something to improve:

[auto build test WARNING on hnaz-mm/master]

url:    https://github.com/intel-lab-lkp/linux/commits/Minchan-Kim/mm-fix-is_pinnable_page-against-on-cma-page/20220503-013733
base:   https://github.com/hnaz/linux-mm master
config: hexagon-randconfig-r034-20220501 (https://download.01.org/0day-ci/archive/20220503/202205031644.EtLKbqIX-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 363b3a645a1e30011cc8da624f13dac5fd915628)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/1b8636710c31d44310f1d344e337c207562b851d
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Minchan-Kim/mm-fix-is_pinnable_page-against-on-cma-page/20220503-013733
        git checkout 1b8636710c31d44310f1d344e337c207562b851d
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   In file included from fs/statfs.c:2:
   In file included from include/linux/syscalls.h:88:
   In file included from include/trace/syscall.h:7:
   In file included from include/linux/trace_events.h:6:
   In file included from include/linux/ring_buffer.h:5:
   include/linux/mm.h:1630:47: error: use of undeclared identifier 'MIGRATE_CMA'; did you mean 'MIGRATE_SYNC'?
           return !(is_zone_movable_page(page) || mt == MIGRATE_CMA ||
                                                        ^~~~~~~~~~~
                                                        MIGRATE_SYNC
   include/linux/migrate_mode.h:18:2: note: 'MIGRATE_SYNC' declared here
           MIGRATE_SYNC,
           ^
   In file included from fs/statfs.c:2:
   In file included from include/linux/syscalls.h:88:
   In file included from include/trace/syscall.h:7:
   In file included from include/linux/trace_events.h:6:
   In file included from include/linux/ring_buffer.h:5:
   include/linux/mm.h:1631:9: error: use of undeclared identifier 'MIGRATE_ISOLATE'; did you mean 'MIGRATE_MOVABLE'?
                   mt == MIGRATE_ISOLATE || is_zero_pfn(page_to_pfn(page)));
                         ^~~~~~~~~~~~~~~
                         MIGRATE_MOVABLE
   include/linux/mmzone.h:44:2: note: 'MIGRATE_MOVABLE' declared here
           MIGRATE_MOVABLE,
           ^
>> fs/statfs.c:131:3: warning: 'memcpy' will always overflow; destination buffer has size 64, but size argument is 88 [-Wfortify-source]
                   memcpy(&buf, st, sizeof(*st));
                   ^
   1 warning and 2 errors generated.


vim +/memcpy +131 fs/statfs.c

c8b91accfa1059 Al Viro 2011-03-12  125  
c8b91accfa1059 Al Viro 2011-03-12  126  static int do_statfs_native(struct kstatfs *st, struct statfs __user *p)
c8b91accfa1059 Al Viro 2011-03-12  127  {
c8b91accfa1059 Al Viro 2011-03-12  128  	struct statfs buf;
7ed1ee6118ae77 Al Viro 2010-03-23  129  
c8b91accfa1059 Al Viro 2011-03-12  130  	if (sizeof(buf) == sizeof(*st))
c8b91accfa1059 Al Viro 2011-03-12 @131  		memcpy(&buf, st, sizeof(*st));
7ed1ee6118ae77 Al Viro 2010-03-23  132  	else {
c8b91accfa1059 Al Viro 2011-03-12  133  		if (sizeof buf.f_blocks == 4) {
c8b91accfa1059 Al Viro 2011-03-12  134  			if ((st->f_blocks | st->f_bfree | st->f_bavail |
c8b91accfa1059 Al Viro 2011-03-12  135  			     st->f_bsize | st->f_frsize) &
7ed1ee6118ae77 Al Viro 2010-03-23  136  			    0xffffffff00000000ULL)
7ed1ee6118ae77 Al Viro 2010-03-23  137  				return -EOVERFLOW;
7ed1ee6118ae77 Al Viro 2010-03-23  138  			/*
7ed1ee6118ae77 Al Viro 2010-03-23  139  			 * f_files and f_ffree may be -1; it's okay to stuff
7ed1ee6118ae77 Al Viro 2010-03-23  140  			 * that into 32 bits
7ed1ee6118ae77 Al Viro 2010-03-23  141  			 */
c8b91accfa1059 Al Viro 2011-03-12  142  			if (st->f_files != -1 &&
c8b91accfa1059 Al Viro 2011-03-12  143  			    (st->f_files & 0xffffffff00000000ULL))
7ed1ee6118ae77 Al Viro 2010-03-23  144  				return -EOVERFLOW;
c8b91accfa1059 Al Viro 2011-03-12  145  			if (st->f_ffree != -1 &&
c8b91accfa1059 Al Viro 2011-03-12  146  			    (st->f_ffree & 0xffffffff00000000ULL))
7ed1ee6118ae77 Al Viro 2010-03-23  147  				return -EOVERFLOW;
7ed1ee6118ae77 Al Viro 2010-03-23  148  		}
7ed1ee6118ae77 Al Viro 2010-03-23  149  
c8b91accfa1059 Al Viro 2011-03-12  150  		buf.f_type = st->f_type;
c8b91accfa1059 Al Viro 2011-03-12  151  		buf.f_bsize = st->f_bsize;
c8b91accfa1059 Al Viro 2011-03-12  152  		buf.f_blocks = st->f_blocks;
c8b91accfa1059 Al Viro 2011-03-12  153  		buf.f_bfree = st->f_bfree;
c8b91accfa1059 Al Viro 2011-03-12  154  		buf.f_bavail = st->f_bavail;
c8b91accfa1059 Al Viro 2011-03-12  155  		buf.f_files = st->f_files;
c8b91accfa1059 Al Viro 2011-03-12  156  		buf.f_ffree = st->f_ffree;
c8b91accfa1059 Al Viro 2011-03-12  157  		buf.f_fsid = st->f_fsid;
c8b91accfa1059 Al Viro 2011-03-12  158  		buf.f_namelen = st->f_namelen;
c8b91accfa1059 Al Viro 2011-03-12  159  		buf.f_frsize = st->f_frsize;
c8b91accfa1059 Al Viro 2011-03-12  160  		buf.f_flags = st->f_flags;
c8b91accfa1059 Al Viro 2011-03-12  161  		memset(buf.f_spare, 0, sizeof(buf.f_spare));
c8b91accfa1059 Al Viro 2011-03-12  162  	}
c8b91accfa1059 Al Viro 2011-03-12  163  	if (copy_to_user(p, &buf, sizeof(buf)))
c8b91accfa1059 Al Viro 2011-03-12  164  		return -EFAULT;
7ed1ee6118ae77 Al Viro 2010-03-23  165  	return 0;
7ed1ee6118ae77 Al Viro 2010-03-23  166  }
7ed1ee6118ae77 Al Viro 2010-03-23  167  

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp