Re: [v5 02/15] mm/huge_memory: add device-private THP support to PMD operations

[Balbir Singh]

On 9/15/25 18:10, David Hildenbrand wrote:
>>>> +
>>>
>>> Wrong indentation.
>>
>> checkpatch.pl did not complain, what I see is
>>
>> +        VM_WARN_ON(!is_pmd_migration_entry(pmd) &&
>> +                !is_pmd_device_private_entry(pmd));
>>
> 
> David complains :)
> 
>> It looks different in your reply (is your email converting tabs to space?
>> did you want me to align the conditions?
> 
> Indeed, looks wrong, not sure why. Likely I messed it up.
> 

Ack

>>
>> +        VM_WARN_ON(!is_pmd_migration_entry(pmd) &&
>> +               !is_pmd_device_private_entry(pmd));
>>
>>
> 
> Exactly.
> 
>>
>>>
>>>> +        if (is_migration_entry(entry) &&
>>>> +            !is_readable_migration_entry(entry)) {
>>>
>>> Dito.
>>>
>>
>> Same as above :)
>>
>>
>>> Wonder if we want to be more explicit.
>>>
>>> if (is_readable_migration_entry(enrty) ||
>>>      is_readable_exclusive_migration_entry)) {
>>>
>>
>> !is_readable_migration_entry => writable entry or read exclusive, did you mean is_writable_migration_entry()
>> above?
> 
> Yes, sorry, my brain was dizzy after all the review lately.
> 
> if (is_writable_migration_entry(enrty) ||
>     is_readable_exclusive_migration_entry))
> 
> [...]
> 

Ack

>>>
>>> Couldn't we do here
>>>
>>> if (!pmd_present(pmdval))
>>>      goto nomap;
>>>
>>> To replace the original pmd_none() .. check.
>>>
>>> A page table must always be present IIRC.
>>>
>>
>> I am not sure about the pmd_none(), a page table may not be present, I've not audited
>> the callers. But I think we can do
> 
> IIRC page tables must always have the present bit set. So we can just simplify to the single pmd_present() check.
> 

Not sure about that one, are you happy if we follow it up later with a separate fix

Balbir