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

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

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?

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.

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


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

--
Cheers, Lorenzo