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

[Suren Baghdasaryan]

On Fri, Sep 11, 2026 at 4:39 PM Lorenzo Stoakes (ARM) <ljs@xxxxxxxxxx> 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@xxxxxxxxxx>
>
> 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/

>
> 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@xxxxxxxxxx>
> > Reviewed-by: Liam R. Howlett (Oracle) <liam@xxxxxxxxxxxxx>
>
> 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]

>
> >   */
> >  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.
Thanks,
Suren.

>
>
> >                               last_vma_end = vma->vm_end;
> >                               continue;
> >                       }
> > --
> > 2.55.0.1007.g17ff1f9808-goog
> >
>
> --
> Cheers, Lorenzo