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