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

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

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.

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

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

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

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