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

[David Hildenbrand <david@redhat.com>]

>> 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