Re: [PATCH RFC v2 7/7] mm/vmscan: pass scan_control to mm_vmscan_balance_pgdat_end

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

On 9/28/2026 7:58 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] mm: BPF raw tracepoint ABI breakage and commit message mismatch in mm_vmscan_balance_pgdat_end
> --
> 
> commit eb8bc6091aab714af4b68f316fecfc08d43c39dc
> Author: Ridong Chen <chenridong@xiaomi.com>
> 
> mm/vmscan: pass scan_control to mm_vmscan_balance_pgdat_end
> 
> This commit updates the mm_vmscan_balance_pgdat_end tracepoint to receive
> a struct scan_control pointer rather than passing individual struct fields.
> It updates the TP_PROTO and TP_ARGS while leaving the exported event format
> unchanged.
> 
>>      balance_pgdat() passes sc.order and sc.nr_reclaimed to
>>      mm_vmscan_balance_pgdat_end as bare arguments. Pass the enclosing
>>      struct scan_control instead and read the fields out in TP_fast_assign,
>>      mirroring mm_vmscan_balance_pgdat_begin. highest_zoneidx stays a
> 
> [Severity: Medium]
> The commit message states this change is "mirroring
> mm_vmscan_balance_pgdat_begin." Is this description accurate?
> 
> Looking at mm_vmscan_balance_pgdat_begin in mm/trace_vmscan.h, it still takes
> individual arguments and was not updated to take struct scan_control *sc:
> 
> TRACE_EVENT(mm_vmscan_balance_pgdat_begin,
>          TP_PROTO(int nid, int order, int highest_zoneidx),
> 
> Could the commit message be more precise about the rationale?
> 

You're right, that wording is misleading and I'll fix it in next version.

I just mixed it up with other xxx_begin tracepoints.

>> diff --git a/mm/trace_vmscan.h b/mm/trace_vmscan.h
>> index 04f285e776596..c27bc7cc60630 100644
>> --- a/mm/trace_vmscan.h
>> +++ b/mm/trace_vmscan.h
> [ ... ]
>> @@ -124,10 +124,9 @@ TRACE_EVENT(mm_vmscan_balance_pgdat_begin,
>>   
>>   TRACE_EVENT(mm_vmscan_balance_pgdat_end,
>>   
>> -	TP_PROTO(int nid, int order, int highest_zoneidx,
>> -		 unsigned long nr_reclaimed),
>> +	TP_PROTO(int nid, struct scan_control *sc, int highest_zoneidx),
>>   
>> -	TP_ARGS(nid, order, highest_zoneidx, nr_reclaimed),
>> +	TP_ARGS(nid, sc, highest_zoneidx),
> 
> [Severity: Medium]
> The commit message mentions there is "No functional change", but could this
> modification break existing BPF programs hooked to this tracepoint?
> 
> While standard trace events parse the formatted output, BPF raw tracepoints
> consume the TP_PROTO arguments directly via ctx->args. Changing the number
> and type of arguments alters the BPF raw tracepoint ABI for this event.
> 
> Does this inadvertently break user space tools that attach to this raw
> tracepoint and expect the 4 original arguments?
> 

Thanks, "No functional change" is inaccurate here.

Kernel behaviour and the tracefs format are unchanged (TP_STRUCT__entry /
TP_printk are untouched), so perf / trace-cmd / libbpf CO-RE consumers are
unaffected. But for BPF raw tracepoints the arg layout does change: a program
reading the old args[3] (nr_reclaimed) now has to read it out of sc. I
shouldn't have hidden that under "no functional change".

Raw tracepoints aren't a stable ABI though 
(Documentation/bpf/bpf_design_QA.rst), so the change itself is fine — I'll just 
fix the commit message in next version.

-- 
Best regards
Ridong