Re: [PATCH v3 4/7] proc/task_mmu: remove special-casing of smap_gather_stats() start parameter

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

On Fri, Sep 11, 2026 at 11:06:28AM -0700, Suren Baghdasaryan wrote:
> On Fri, Sep 11, 2026 at 10:49 AM Lorenzo Stoakes (ARM) <ljs@kernel.org> wrote:
> >
> > On Fri, Sep 11, 2026 at 05:07:48PM +0000, Suren Baghdasaryan wrote:
> > > On Fri, Sep 11, 2026 at 4:39 PM Lorenzo Stoakes (ARM) <ljs@kernel.org> wrote:
> > > >
> > > > On Thu, Sep 10, 2026 at 04:47:34PM -0700, 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@kernel.org>
> > > >
> > > > Hmm did I? Where did I suggest this?... I guess a while ago?
> > >
> > > In [1] on June 9, 2026.
> > >
> > > [1] https://lore.kernel.org/all/aifO_rCurVhFRTcl@lucifer/
> >
> > Yup a while ago :)
> >
> > >
> > > >
> > > > I mean I also happen to suggest it in the previous patch review :) but that was
> > > > sent after you sent this...
> > > >
> > > > > Signed-off-by: Suren Baghdasaryan <surenb@google.com>
> > > > > Reviewed-by: Liam R. Howlett (Oracle) <liam@infradead.org>
> > > >
> > > > I don't love hacking a hack for a patch and then unhack it in the next in a
> > > > slightly roundabout way.
> > > >
> > > > Feels like this should be squashed. And a wrapper function for
> > > > start=vma->vm_start should be used rather than duplicating that param
> > > > constantly.
> > > >
> > > > > ---
> > > > >  fs/proc/task_mmu.c | 29 ++++++++++++++++-------------
> > > > >  1 file changed, 16 insertions(+), 13 deletions(-)
> > > > >
> > > > > diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> > > > > index 3c40c9cbb9c9..ecce7ce116cb 100644
> > > > > --- a/fs/proc/task_mmu.c
> > > > > +++ b/fs/proc/task_mmu.c
> > > > > @@ -1246,21 +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.
> > > >
> > > > This seems to be describing what callers do not what the function does unless
> > > > I'm missing something? So that's really the wrong place for it.
> > > >
> > > > I think the description of why it might be a partial walk belongs to the bit of
> > > > code that actually tries to do a partial walk.
> > > >
> > > > Anyway as per below I think separate partial/full functions make sense and there
> > > > it can simply be described as walking either the full or part of the VMA.
> > >
> > > This is verbatim of what you wrote at the end of [1]
> >
> > OK, I guess I disagree with myself of 3 months ago?
> >
> > The technical point being made here, which I think is the more constructive one
> > to engage with, is that this is a function that can be called with different
> > parameters for whatever reason.
> >
> > Somebody might decide to call it for another reason, putting something in the
> > description of the function that assumes what callers will do when that code can
> > change is asking for bit rot.
>
> Yeah, that makes sense.

Thanks.

>
> >
> > So as I suggested above:
> >
> >         I think the description of why it might be a partial walk belongs to the
> >         bit of code that actually tries to do a partial walk.
> >
> > I.e. I guess past me's description is apt, but belongs with the partial case.
>
> Ok, sounds like you want two separate functions supporting complete or
> partial walk. I don't have a strong preference here and it's easy to
> do like this:
>
> staic void smap_gather_stats_range(priv, vma, &mss, start)
> {
>         ....
> }
>
> staic void smap_gather_stats(priv, vma, &mss)
> {
>         smap_gather_stats_range(priv, vma, &mss, vma->vm_start);
> }
>
> Does that sound good?

Yeah that's the idea.

>
> >
> > >
> > > >
> > > > >   */
> > > > >  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))
> > > > > @@ -1285,10 +1291,7 @@ static void smap_gather_stats(struct proc_maps_private *priv,
> > > > >                       mss->swap += shmem_swapped;
> > > > >       }
> > > > >
> > > > > -     if (!start)
> > > > > -             walk_page_vma(vma, ops, mss);
> > > > > -     else
> > > > > -             walk_page_range(vma->vm_mm, start, vma->vm_end, ops, mss);
> > > > > +     walk_page_range_vma(vma, start, vma->vm_end, ops, mss);
> > > >
> > > > I mean obviously am in favour of this as I suggested it in the last patch :)
> > > >
> > > > >
> > > > >       reacquire_rcu(priv);
> > > > >  }
> > > > > @@ -1343,7 +1346,7 @@ static int show_smap(struct seq_file *m, void *v)
> > > > >       struct vm_area_struct *vma = v;
> > > > >       struct mem_size_stats mss = {};
> > > > >
> > > > > -     smap_gather_stats(priv, vma, &mss, 0);
> > > > > +     smap_gather_stats(priv, vma, &mss, vma->vm_start);
> > > > >
> > > > >       show_map_vma(m, vma);
> > > > >
> > > > > @@ -1396,7 +1399,7 @@ static int show_smaps_rollup(struct seq_file *m, void *v)
> > > > >
> > > > >       vma_start = vma->vm_start;
> > > > >       do {
> > > > > -             smap_gather_stats(priv, vma, &mss, 0);
> > > > > +             smap_gather_stats(priv, vma, &mss, vma->vm_start);
> > > > >               last_vma_end = vma->vm_end;
> > > > >
> > > > >               /*
> > > > > @@ -1455,7 +1458,7 @@ static int show_smaps_rollup(struct seq_file *m, void *v)
> > > > >
> > > > >                       /* Case 1 and 2 above */
> > > > >                       if (vma->vm_start >= last_vma_end) {
> > > > > -                             smap_gather_stats(priv, vma, &mss, 0);
> > > > > +                             smap_gather_stats(priv, vma, &mss, vma->vm_start);
> > > >
> > > > I mean this is all horrible, having to pass vma->vm_start explicitly.
> > > >
> > > > Although better than the hack that gets compounded in patch 3.
> > > >
> > > > There 4 invocations of smap_gather_stats(), only one of them passes a
> > > > non-vma->vm_start start.
> > > >
> > > > So it'd make more sense to just make smap_gather_stats() lose its 3rd param and
> > > > have it call smap_gather_stats_range(), then have 1 invocation of
> > > > smaps_gather_stats_range() directly, as per suggestion in last patch.
> > > >
> > > > Or something similar to that.
> > >
> > > Hmm. Ok, I'll wait for you to read your previous suggestions in [1]
> > > and after that let's discuss what the final version should look like.
> >
> > I don't really think that's hugely constructive.
>
> I wasn't trying to offend in any way. Just wanted to give you some
> time to recall previous conversation and consolidate your position.
>
> >
> > I'm sorry I'm (mildly) disagreeing with my past self, I've sent tens of
> > thousands of words of review since then so I think it can be forgiven.
>
> Definitely. Again, I wasn't trying to blame or anything like that.
> Just pointing out our previous discussion and want to make sure we are
> on the same page (while having some fun in the process).
>
> >
> > In any case, I really do think:
> >
> >         smap_gather_stats(priv, vma, &mss);
> >         smap_gather_stats(priv, vma, &mss);
> >         smap_gather_stats(priv, vma, &mss);
> >         smap_gather_stats_range(priv, vma, &mss, last_vma_end);
> >
> > Works better than:
> >
> >         smap_gather_stats(priv, vma, &mss, vma->vm_start);
> >         smap_gather_stats(priv, vma, &mss, vma->vm_start);
> >         smap_gather_stats(priv, vma, &mss, vma->vm_start);
> >         smap_gather_stats(priv, vma, &mss, last_vma_end);
> >
> > ?
> >
> > I usually come back on review very quickly so I don't think this series
> > will be held up with any such change.
> >
> > But let me know if you think it's not a good idea technically.
>
> TBH I don't have strong preference but if you like it this way, it will be done.
> I'll post an update today since I don't think there will be more
> controversial parts. The biggest blunder on my part was the way I
> split patch 3 and 4.
> Thanks for the review!

I'd quite like to have a look through the rest of the series first.

>
> >
> > > Thanks,
> > > Suren.
> >
> > --
> > Cheers, Lorenzo

--
Cheers, Lorenzo