Re: [PATCH 2/3] mm/mglru: add tracepoint for scan_folios()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Evaluating pointer dereferences at the tracepoint call site adds unnecessary overhead when tracing is disabled.
--

commit 3d7b5caa2f51b1f8f623f8379f3a020ee141f537
Author: Ridong Chen <chenridong@xiaomi.com>

mm/mglru: add tracepoint for scan_folios()

This commit introduces a new MGLRU-specific tracepoint,
mm_mglru_scan_folios, to report scan and isolate counts along with
MGLRU-specific context like memcg ID, generation, and tier. The tracepoint
is placed alongside the classic-LRU tracepoint in scan_folios().

> diff --git a/include/trace/events/vmscan.h b/include/trace/events/vmscan.h
> --- a/include/trace/events/vmscan.h
> +++ b/include/trace/events/vmscan.h
[ ... ]
> +TRACE_EVENT(mm_mglru_scan_folios,
> +
> +	TP_PROTO(u64 memcg_id,
> +		int highest_zoneidx,

[Severity: Medium]
Does this prototype incorrectly accept an evaluated u64 instead of a pointer?

Accepting the evaluated value here forces evaluation at the call site.
Could we pass the struct lruvec pointer in the prototype instead, so the
expensive pointer dereferences can happen safely inside TP_fast_assign
where they only execute when tracing is enabled?

> +		int order,
[ ... ]
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
[ ... ]
> @@ -4927,6 +4927,12 @@ static int scan_folios(unsigned long nr_to_scan, struct lruvec *lruvec,
>  	trace_mm_vmscan_lru_isolate(sc->reclaim_idx, sc->order, nr_to_scan,
>  				scanned, skipped, isolated,
>  				type ? LRU_INACTIVE_FILE : LRU_INACTIVE_ANON);
> +	trace_mm_mglru_scan_folios(mem_cgroup_id(lruvec_memcg(lruvec)),
> +				   sc->reclaim_idx, sc->order, nr_to_scan,

[Severity: Medium]
Can this introduce unnecessary overhead on the memory reclaim hot path?

Because trace_mm_mglru_scan_folios evaluates its arguments before checking
the tracepoint's static key, calling mem_cgroup_id(lruvec_memcg(lruvec))
here forces multiple pointer dereferences to happen unconditionally.

The compiler cannot optimize them into the inactive branch because it cannot
prove the arbitrary pointer accesses won't trap. Placing the dereferences
inside TP_fast_assign avoids this execution when tracing is disabled.

> +				   scanned, sorted, skipped, isolated,
> +				   type ? LRU_INACTIVE_FILE : LRU_INACTIVE_ANON,
> +				   lrugen->max_seq, tier,
> +				   lrugen->min_seq[type]);
>  
>  	*isolatedp = isolated;
>  	return scanned;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260911072848.2346073-1-ridong.chen@linux.dev?part=2