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 10:39:01AM -0700, Suren Baghdasaryan wrote:
> On Fri, Sep 11, 2026 at 10:10 AM Lorenzo Stoakes (ARM) <ljs@kernel.org> wrote:
> >
> > 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.
>
> Ah! Now I see the problem you are pointing out. Ok, in v2 [2] this was
> done correctly and that's the way you want it!
> Okay, I agree this split was incorrect. I think I'll move is_partial
> conversion completely into the next patch and this one will only
> update the comment and use vma_is_cow_mapping() instead of open-coding
> it.

OK, it probably makes sense to have the CoW change separate.

I replied on 4/7 about how I think that should look re: wrapper functions.

>
> [2] https://lore.kernel.org/all/20260907063918.3432401-4-surenb@google.com/

--
Cheers, Lorenzo