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

[Suren Baghdasaryan]

On Wed, Sep 9, 2026 at 10:16 AM David Hildenbrand (Arm)
<david@xxxxxxxxxx> wrote:
>
> On 9/7/26 08:39, Suren Baghdasaryan wrote:
> > smap_gather_stats() interprets its start parameter to mean vma->vm_start
> > when it's set to 0. Eliminate this special interpretation and pass
> > vma->vm_start explicitly when needed.
> >
> > Since smap_gather_stats() operates within a single VMA, we can replace
> > walk_page_vma()/walk_page_range() calls with walk_page_range_vma()
> > which is simpler and also can be called while holding per-VMA lock.
> >
> > No functional change intended.
> >
> > Suggested by: Lorenzo Stoakes <ljs@xxxxxxxxxx>
> > Signed-off-by: Suren Baghdasaryan <surenb@xxxxxxxxxx>
> > ---
> >  fs/proc/task_mmu.c | 40 ++++++++++++++++++++++------------------
> >  1 file changed, 22 insertions(+), 18 deletions(-)
> >
> > diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> > index 9908ba32f180..3351decd1172 100644
> > --- a/fs/proc/task_mmu.c
> > +++ b/fs/proc/task_mmu.c
> > @@ -1246,20 +1246,27 @@ get_smaps_shmem_walk_ops(struct proc_maps_private *priv)
> >       return &smaps_shmem_walk_vma_lock_ops;
> >  }
> >
> > -/*
> > - * Gather mem stats from @vma with the indicated beginning
> > - * address @start, and keep them in @mss.
> > +/**
> > + * smap_gather_stats() - Gather mem stats from @vma.
> > + * @priv: proc maps private state.
> > + * @vma: The VMA to gather stats for.
> > + * @mss: The accumulated stats.
> > + * @start: The address from which to start.
> >   *
> > - * Use vm_start of @vma as the beginning address if @start is 0.
> > + * This gathers stats for the whole of the VMA unless the lock was dropped
> > + * and VMA grew or got merged and we found it again, in which case we only
> > + * gather stats for the remainder of the VMA range.
> >   */
> >  static void smap_gather_stats(struct proc_maps_private *priv,
> >                             struct vm_area_struct *vma,
> > -                           struct mem_size_stats *mss, unsigned long start)
> > +                           struct mem_size_stats *mss,
> > +                           unsigned long start)
> >  {
> >       const struct mm_walk_ops *ops = get_smaps_walk_ops(priv);
> > +     const bool is_partial = start > vma->vm_start;
> >
> >       /* Invalid start */
> > -     if (start >= vma->vm_end)
> > +     if (start < vma->vm_start || start >= vma->vm_end)
> >               return;
> >
> >       if (vma == get_gate_vma(priv->lock_ctx.mm))
> > @@ -1279,20 +1286,17 @@ static void smap_gather_stats(struct proc_maps_private *priv,
> >                * Unless we know that the shmem object (or the part mapped by
> >                * our VMA) has no swapped out pages at all.
> >                */
> > -             unsigned long shmem_swapped = shmem_swap_usage(vma);
> > +             const unsigned long shmem_swapped = shmem_swap_usage(vma);
> > +             const bool shared_or_ro = vma_test(vma, VMA_SHARED_BIT) ||
> > +                                       !vma_test(vma, VMA_WRITE_BIT);
> >
> > -             if (!start && (!shmem_swapped || (vma->vm_flags & VM_SHARED) ||
> > -                                     !(vma->vm_flags & VM_WRITE))) {
> > +             if (!is_partial && (!shmem_swapped || shared_or_ro))
> >                       mss->swap += shmem_swapped;
> > -             } else {
> > +             else
> >                       ops = get_smaps_shmem_walk_ops(priv);
> > -             }
>
> Horrible, horrible code, really. But not your fault :)
>
> I think we can just make the shared_or_ro less odd by just checking for cow
> mappings (as described in the comment).
>
>         const bool is_cow = vma_is_cow_mapping(vma);
>
> ...
>
>         if (is_partial || (shmem_swapped && is_cow))
>                 ops = get_smaps_shmem_walk_ops(priv);
>         else
>                 mss->swap += shmem_swapped;
>
> That's almost in a form that I could understand what's happening.

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?

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

>
> --
> Cheers,
>
> David
>