Re: [PATCH v2 3/5] proc/task_mmu: remove special-casing of smap_gather_stats() start parameter

["David Hildenbrand (Arm)" <david@kernel.org>]

On 9/9/26 23:51, Suren Baghdasaryan wrote:
> On Wed, Sep 9, 2026 at 12:16 PM David Hildenbrand (Arm)
> <david@kernel.org> wrote:
>>
>> On 9/9/26 20:28, Suren Baghdasaryan wrote:
>>> On Wed, Sep 9, 2026 at 10:16 AM David Hildenbrand (Arm)
>>> <david@kernel.org> wrote:
>>>
>>> Hmm. So, are you saying that !is_cow always implies shared_or_ro? Or
>>> maybe you are stating that vma_is_cow_mapping() was the actual intent
>>> here?
>>
>> So the comment says:
>>
>> "For private writable mappings, we might have COW pages that  .."
>>
>> Which translates to:
>>
>>         private writable == vma_is_cow_mapping()
> 
> Ok, just want to make sure I'm not missing something subtle.
> 
>>
>>>
>>> shared_or_ro = VMA_SHARED_BIT || !VMA_WRITE_BIT
>>>
>>> is_cow = !VMA_SHARED_BIT && VMA_MAYWRITE_BIT
>>> !is_cow = VMA_SHARED_BIT || !VMA_MAYWRITE_BIT
>>>
>>> so, !is_cow would impy shared_or_ro only if !VMA_MAYWRITE_BIT always
>>> implies !VMA_WRITE_BIT. But I think it's possible to have a VMA that
>>> has VMA_WRITE_BIT but not VMA_MAYWRITE_BIT, right?
>>
>> VMA_WRITE should imply VMA_MAYWRITE
> 
> Ah, good to know.
> 
>>
>> (in sanitize_fault_flags() we even disallow write faults entirely if VM_MAYWRITE
>> is missing)
> 
> I see.
> 
>>
>> For example, a
>>> driver can create such a VMA to allow writing to the VMA but to lock
>>> its content once mprotect(PROT_READ) gets called.
>>
>> I don't think that would be valid for a driver to do. But it wouldn't matter
>> here because
>>
>>         shmem_mapping(vma->vm_file->f_mapping)
> 
> I was obviously overthinking this :)
> 
>>
>>
>> I think we could simplify the comment as well to:
>>
>> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
>> index e671b4fd8dedd..4b7e7089cafa7 100644
>> --- a/fs/proc/task_mmu.c
>> +++ b/fs/proc/task_mmu.c
>> @@ -1303,12 +1303,10 @@ static void smap_gather_stats(struct proc_maps_private
>> *priv,if (is_partial || (shmem_swapped && is_cow))
>>
>>         if (vma->vm_file && shmem_mapping(vma->vm_file->f_mapping)) {
>>                 /*
>> -                * For shared or readonly shmem mappings we know that all
>> -                * swapped out pages belong to the shmem object, and we can
>> -                * obtain the swap value much more efficiently. For private
>> -                * writable mappings, we might have COW pages that are
>> -                * not affected by the parent swapped out pages of the shmem
>> -                * object, so we have to distinguish them during the page walk.
>> +                * In CoW mappings, we might have anon folios that are
>> +                * independent of the shmem object. So fallback to the less
>> +                * efficient mechanism in such mappings.
>> +                *
> 
> Sounds good. Will update.

Likely worth putting that into a prior cleanup patch, so there is less noise in
this patch.

-- 
Cheers,

David