Re: [PATCH v3 3/7] proc/task_mmu: clarify shmem mapping walk conditions in smap_gather_stats()

[Suren Baghdasaryan]

On Fri, Sep 11, 2026 at 4:28 PM Lorenzo Stoakes (ARM) <ljs@xxxxxxxxxx> wrote:
>
> On Thu, Sep 10, 2026 at 04:47:33PM -0700, Suren Baghdasaryan wrote:
> > smap_gather_stats() optimizes stats gathering by skipping the walk for
> > shmem mappings in certain conditions. Update the comment to clarify
> > these conditions and use vma_is_cow_mapping() for COW identification
> > instead of open-coding it.
> > Instead of using (start != 0) condition to identify partial walks, use
> > more semantically correct (start > vma->vm_start) check.
>
> I don't agree what you're doing is semantically correct, it's a hack really.
>
> Callers are passing start=0 to indicate that the entire VMA should be
> processed and that happens to fulfil your criteria but in a surprising way.
>
> And the start in these cases is corrupted.

Well, the "other" Lorenzo does not agree with you and suggested this
approach in [1]. Specifically, see the comment:
```
I also don't love that 0 is taken to be 'start from vma->vm_start' and I
also don't love that the code in smap_gather_stats() actually special cases
this...

How about passing last_vma_end and making smap_gather_stats() more sane? In
the other invocation of smap_gather_stats() we could pass vma->vm_start
here.
```

[1] https://lore.kernel.org/all/aifO_rCurVhFRTcl@lucifer/

>
> >
> > No functional change intended.
> >
> > Suggested by: David Hildenbrand (Arm) <david@xxxxxxxxxx>
> > Signed-off-by: Suren Baghdasaryan <surenb@xxxxxxxxxx>
> > ---
> >  fs/proc/task_mmu.c | 24 ++++++++++--------------
> >  1 file changed, 10 insertions(+), 14 deletions(-)
> >
> > diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> > index cfc7af1b551d..3c40c9cbb9c9 100644
> > --- a/fs/proc/task_mmu.c
> > +++ b/fs/proc/task_mmu.c
> > @@ -1257,6 +1257,7 @@ static void smap_gather_stats(struct proc_maps_private *priv,
> >                             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;
>
> Yeah not in love with this, without changing how it's called.

See [1]. This is exactly how you wrote it at the end of that reply.

>
> If you're reworking it all already, the actually semantically correct thing
> I think would be to do something like:
>
> static void smap_gather_stats_range(struct proc_maps_private *priv,
>                 struct vm_area_struct *vma, struct mem_size_stats *mss,
>                 unsigned long start)
> {
>         ...
> }
>
> Then to drop a parameter in smap_gather_stats() like:
>
> static void smap_gather_stats_range(struct proc_maps_private *priv,
>                 struct vm_area_struct *vma, struct mem_size_stats *mss)
> {
>         smap_gather_stats_range(priv, vma, mss, vma->vm_start);
> }
>
> And then you remove the hack and make is_partial not be accidentally true for an
> invalid start parameter.
>
> >
> >       /* Invalid start */
> >       if (start >= vma->vm_end)
> > @@ -1270,23 +1271,18 @@ static void smap_gather_stats(struct proc_maps_private *priv,
> >
> >       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.
> > -              * Unless we know that the shmem object (or the part mapped by
> > -              * our VMA) has no swapped out pages at all.
> > +              * CoW mappings might map anon folios that do not belong to
> > +              * shmem. Perform a less efficient page table walk in this
> > +              * situation, 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 is_cow = vma_is_cow_mapping(vma);
>
> Nice to see this helper naturally slot in to new stuff :)
>
> >
> > -             if (!start && (!shmem_swapped || (vma->vm_flags & VM_SHARED) ||
> > -                                     !(vma->vm_flags & VM_WRITE))) {
> > -                     mss->swap += shmem_swapped;
> > -             } else {
> > +             if (is_partial || (shmem_swapped && is_cow))
> >                       ops = get_smaps_shmem_walk_ops(priv);
> > -             }
> > +             else
> > +                     mss->swap += shmem_swapped;
> >       }
> >
> >       if (!start)
>
> Also not absolutely in love with the fact you only use is_partial above and
> leave:
>
>         if (!start)
>                 walk_page_vma(vma, ops, mss);
>         else
>                 walk_page_range(vma->vm_mm, start, vma->vm_end, ops, mss);
>
> As-is.
>
> Should be:
>
>         if (is_partial)
>                 walk_page_range(vma->vm_mm, start, vma->vm_end, ops, mss);
>         else
>                 walk_page_vma(vma, ops, mss);

True, that can be changed here too. This whole block is replaced in
the next patch though.

>
> But I also wonder whether, with start not being corrupted (!) you could
> just replace this with:
>
>         walk_page_range_vma(vma, start, vma->vm_end, ops, mss);

Yep, that's done in the very next patch.

>
> Looking at the pagewalk.c implementations I don't know why
> walk_page_range_vma() doesn't just forward [vma->vm_start, vma->vm_end) to
> walk_page_range_vma()... but that's another thing :)
>
> > --
> > 2.55.0.1007.g17ff1f9808-goog
> >
>
> --
> Cheers, Lorenzo