Hi John,
On Tue, May 10, 2022 at 03:56:37PM -0700, John Hubbard wrote:
> On 5/10/22 14:17, 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 pinning CMA pages as longterm at pin_user_pages
> > APIs so CMA allocation keep failed until the pin is released.
> >
> >
> > CPU 0 CPU 1 - Task B
> >
> > cma_alloc
> > alloc_contig_range
> > pin_user_pages_fast(FOLL_LONGTERM)
> > change pageblock as MIGRATE_ISOLATE
> > internal_get_user_pages_fast
> > lockless_pages_from_mm
> > gup_pte_range
> > try_grab_folio
> > is_pinnable_page
> > return true;
> > So, pinned the page successfully.
> > page migration failure with pinned page
> > ..
> > .. After 30 sec
> > unpin_user_page(page)
> >
> > CMA allocation succeeded after 30 sec.
>
> Hi Minchan,
>
> Thanks for spelling out how this works, that really speeds up the
> review and helps others quickly learn what is going on with the code.
>
> For my own information, mainly: where is CMA blocking, so that
> it waits (apparently) for the during of the pin, before proceeding?
> (Or is the caller retrying?)
It would fail the cma_alloc in the first place since it couldn't
migrate page out due to the elevated refcount and cma_allc would
proceed next pageblocks to keep pages migrated out but it ends up
failing the cma allocation because the user tries to allocate
entire CMA pageblocks, not part of them so one of the pinned page
make the cma allocation failure. Since then, user(e.g., dmabuf)
could retry a few more times but it keeps failed until Task B
release the refcount of the page.
>
> I noticed a few minor points but was too slow to reply, notes below:
>
> >
> > 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 migration type.
> > Thus, we don't need zone->lock but just checks migration type in
> > either of (MIGRATE_ISOLATE and MIGRATE_CMA).
> >
> > Adding the MIGRATE_ISOLATE check in is_pinnable_page could cause
> > rejecting of pinning pages on MIGRATE_ISOLATE pageblocks even
> > though it's neither CMA nor movable zone if the page is temporarily
> > unmovable. However, such a migration failure by unexpected temporal
> > refcount holding is general issue, not only come from MIGRATE_ISOLATE
> > and the MIGRATE_ISOLATE is also transient state like other temporal
> > elevated refcount problem.
> >
> > Cc: David Hildenbrand <[email protected]>
> > Signed-off-by: Minchan Kim <[email protected]>
> > ---
> > * from v3 - https://lore.kernel.org/all/[email protected]/
> > * Fix typo and adding more description - akpm
> >
> > * from v2 - https://lore.kernel.org/all/[email protected]/
> > * Use __READ_ONCE instead of volatile - akpm
> >
> > * from v1 - https://lore.kernel.org/all/[email protected]/
> > * fix build warning - lkp
> > * fix refetching issue of migration type
> > * add side effect on !ZONE_MOVABLE and !MIGRATE_CMA in description - david
> >
> > include/linux/mm.h | 15 +++++++++++++--
> > 1 file changed, 13 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/linux/mm.h b/include/linux/mm.h
> > index 6acca5cecbc5..cbf79eb790e0 100644
> > --- a/include/linux/mm.h
> > +++ b/include/linux/mm.h
> > @@ -1625,8 +1625,19 @@ 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));
> > +#ifdef CONFIG_CMA
> > + /*
> > + * use volatile to use local variable mt instead of
> > + * refetching mt value.
> > + */
>
> This comment is stale and should therefore be deleted.
Yeah.
>
> > + int __mt = get_pageblock_migratetype(page);
> > + int mt = __READ_ONCE(__mt);
>
> Although I saw the email discussion about this in v2, that discussion
> didn't go far enough. It started with "don't use volatile", and went
> on to "try __READ_ONCE() instead", but it should have continued on
> to "you don't need this at all".
That's really what I want to hear from experts so wanted to learn
"Why". How could we prevent refetching of the mt if we don't use
__READ_ONCE or volatile there?
>
> Because you don't. There is nothing you are racing with, and adding
> __READ_ONCE() in order to avoid a completely not-going-to-happen
> compiler re-invocation of a significant code block is just very wrong.
>
> So let's just let it go entirely. :)
Yeah, once it's clear for everyone, I am happy to remove the
unnecessary lines.
>
> > +
> > + if (mt == MIGRATE_CMA || mt == MIGRATE_ISOLATE)
>
> MIGRATE_ISOLATE is not always defined, and must therefore be protected
> with a check on CONFIG_MEMORY_ISOLATION...oh never mind, I see in
> mm/Kconfig:
>
> config CMA
> ...
> select MEMORY_ISOLATION
>
> ...so that's OK. What a tangled web, I wonder if enum migratetype
> really needs to be sliced up like that, but that's a separate topic.
>
> > + return false;
> > +#endif
> > +
> > + return !(is_zone_movable_page(page) || is_zero_pfn(page_to_pfn(page)));
>
> And actually this area is getting rather nested with the various ifdefs,
> and it is probably time to refactor them a bit, considering the above
> point about MIGRATE_ISOLATE. I had something in mind (which is why I
> delayed my feedback), along the lines of merging _ISOLATE and _CMA and
> the ifdefs. But it's just a fine point and not critical of course, just
> a thought.
Glad to hear someone is looking that.
On 5/10/22 4:31 PM, Minchan Kim wrote:
>>> + int __mt = get_pageblock_migratetype(page);
>>> + int mt = __READ_ONCE(__mt);
>>
>> Although I saw the email discussion about this in v2, that discussion
>> didn't go far enough. It started with "don't use volatile", and went
>> on to "try __READ_ONCE() instead", but it should have continued on
>> to "you don't need this at all".
>
> That's really what I want to hear from experts so wanted to learn
> "Why". How could we prevent refetching of the mt if we don't use
> __READ_ONCE or volatile there?
>
>>
>> Because you don't. There is nothing you are racing with, and adding
>> __READ_ONCE() in order to avoid a completely not-going-to-happen
>> compiler re-invocation of a significant code block is just very wrong.
>>
>> So let's just let it go entirely. :)
>
> Yeah, once it's clear for everyone, I am happy to remove the
> unnecessary lines.
>
>>
>>> +
>>> + if (mt == MIGRATE_CMA || mt == MIGRATE_ISOLATE)
>>
With or without __READ_ONCE() or volatile or anything else,
this code will do what you want. Which is: loosely check
for either of the above.
What functional problem do you think you are preventing
with __READ_ONCE()? Because I don't see one.
thanks,
--
John Hubbard
NVIDIA
On Tue, May 10, 2022 at 04:58:13PM -0700, John Hubbard wrote:
> On 5/10/22 4:31 PM, Minchan Kim wrote:
> > > > + int __mt = get_pageblock_migratetype(page);
> > > > + int mt = __READ_ONCE(__mt);
> > >
> > > Although I saw the email discussion about this in v2, that discussion
> > > didn't go far enough. It started with "don't use volatile", and went
> > > on to "try __READ_ONCE() instead", but it should have continued on
> > > to "you don't need this at all".
> >
> > That's really what I want to hear from experts so wanted to learn
> > "Why". How could we prevent refetching of the mt if we don't use
> > __READ_ONCE or volatile there?
> >
> > >
> > > Because you don't. There is nothing you are racing with, and adding
> > > __READ_ONCE() in order to avoid a completely not-going-to-happen
> > > compiler re-invocation of a significant code block is just very wrong.
> > >
> > > So let's just let it go entirely. :)
> >
> > Yeah, once it's clear for everyone, I am happy to remove the
> > unnecessary lines.
> >
> > >
> > > > +
> > > > + if (mt == MIGRATE_CMA || mt == MIGRATE_ISOLATE)
> > >
>
> With or without __READ_ONCE() or volatile or anything else,
> this code will do what you want. Which is: loosely check
> for either of the above.
>
> What functional problem do you think you are preventing
> with __READ_ONCE()? Because I don't see one.
I discussed the issue at v1 so please take a look.
https://lore.kernel.org/all/[email protected]/