Re: [PATCH] mm/migrate_device: Do not access pgmap for non zone device pages

[David Hildenbrand <david@redhat.com>]

On 20.02.25 23:45, Alistair Popple wrote:
> On Thu, Feb 20, 2025 at 11:38:10PM +1100, Balbir Singh wrote:
>> On 2/20/25 23:05, David Hildenbrand wrote:
>>> On 20.02.25 12:58, Balbir Singh wrote:
>>>> On 2/20/25 22:48, David Hildenbrand wrote:
>>>>> On 20.02.25 00:13, Balbir Singh wrote:
>>>>>> page_pgmap() is referenced before checking if the page is a zone
>>>>>> device page and this triggers the warning in page_pgmap(). Refactor
>>>>>> the code to use the helper function after relevant checks.
>>>>>>
>>>>>> Cc: Alistair Popple <apopple@nvidia.com>
>>>>>> Cc: Jason Gunthorpe <jgg@ziepe.ca>
>>>>>> Cc: David Hildenbrand <david@redhat.com>
>>>>>> Cc: Dan Williams <dan.j.williams@intel.com>
>>>>>>
>>>>>> Signed-off-by: Balbir Singh <balbirs@nvidia.com>
>>>>>> ---
>>>>>>
>>>>>> Fixes: 7f1cfd71153b ("mm: allow compound zone device pages") on
>>>>>> mm-unstable
>>>>>
>>>>> Is there actually something broken? At least for now, reading folio->pgmap should just work, although it might be garbage.
>>>>>
>>>>
>>>> It triggers the VM_WARN_ON_ONCE_PAGE
>>>>
>>>> static inline struct dev_pagemap *page_pgmap(const struct page *page)
>>>> {
>>>>      VM_WARN_ON_ONCE_PAGE(!is_zone_device_page(page), page);
>>>>      return page_folio(page)->pgmap;
>>>> }
>>>>
>>>> Nothing is broken, because the code below has checks for is_device_coherent_page(),
>>>> but in general I think the WARN_ON is correct because it warns us against garbage
>>>> and it's propagation if the correct checks are not in place.
>>>
>>> Ah! Now I read your "triggers the warning in page_pgmap()" in the description.
>>>
>>> It's usually a good idea to just include the splat you observed, if you did, and call it "triggers the VM_WARN_ON_ONCE_PAGE".
>>
>> Sure
>>
>>>
>>> The "Fixes:" should go above the "---" in that case.
>>
>> I was not sure if the commit-ids in mm-unstable are persistent, so I put it
>> below the --- to avoid confusion
> 
> They are not, but I'm unsure what the process is here. I guess either I roll
> this fix into my series and repost or maybe Andrew takes it as is a separate fix
> on top of my series?

Ah, I completely ignored that this is not in mm/mm-stable yet.

Yes, that should best just be squashed into the original commit as long 
as it is not in mm/mm-stable.

-- 
Cheers,

David / dhildenb