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

[Ridong Chen <ridong.chen@linux.dev>]

On 9/11/2026 3:42 PM, sashiko-bot@kernel.org wrote:
> 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.
> 

Thanks, will pass lruvec instead in v2.

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

-- 
Best regards
Ridong