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

["Lorenzo Stoakes (ARM)" <ljs@kernel.org>]

On Fri, Sep 11, 2026 at 04:58:40PM +0000, Suren Baghdasaryan wrote:
> On Fri, Sep 11, 2026 at 4:28 PM Lorenzo Stoakes (ARM) <ljs@kernel.org> 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...

I'm not sure what part of this is disagreement?

It's saying passing 0 is a hack, which is one that is still in place and which
this patch makes worse, because instead of explicitly calling out the invalid
value, you're treating it as if it were valid.

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

Yup, well me of 3 months ago should have suggested what I suggested re: wrapper
(I think you cut that suggestion out of my reply).

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

>
> >
> > >
> > > No functional change intended.
> > >
> > > Suggested by: David Hildenbrand (Arm) <david@kernel.org>
> > > Signed-off-by: Suren Baghdasaryan <surenb@google.com>
> > > ---
> > >  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.

Assuming you passed vma->vm_start, not 0? Passing 0 makes it really strange.

I see that in the patch I suggested I didn't update the other callers like I
said you should in the reply, my bad from 3 months ago, but I don't think that
invalidates what's been raised here.

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

Yep I think that should be squashed into this one, as I say there.

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

As above.

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

--
Cheers, Lorenzo