Re: [RFC PATCH v12 2/4] blk-iocost: add iocost_ioc_tick tracepoint for per-period device summary

[Tao Cui <cui.tao@linux.dev>]

Hello,

在 2026/11/3 22:07, bot+bpf-ci@kernel.org 写道:
>> blk-iocost: add iocost_ioc_tick tracepoint for per-period device summary
> 
> TL;DR: In iocost_ioc_tick, the tracepoint call sits outside its enabled()
> check, so a probe attached in between may log a bogus active count of 0, which
> consumers could read as dormancy.
Confirmed. `nr_active` is only consumed by this tracepoint, so there
is no reason for `trace_iocost_ioc_tick()` to sit outside the
`trace_iocost_ioc_tick_enabled()` block. The split was simply an
oversight when the active count was made conditional on the tracepoint
being enabled.
> 
>> diff --git a/block/blk-iocost.c b/block/blk-iocost.c
>> --- a/block/blk-iocost.c
>> +++ b/block/blk-iocost.c
> 
> [ ... ]
> 
>> @@ -2478,6 +2479,22 @@ static void ioc_timer_fn(struct timer_list *timer)
>>  
>>  	ioc->busy_level = clamp(ioc->busy_level, -1000, 1000);
>>  
>> +	/*
>> +	 * busy_level, cur_period, nr_active and usage_us_sum are complete
>> +	 * here. usage is wall time over wall time, summed over iocgs, and
>> +	 * is not capped at 100. vrate and period_us reflect the current
>> +	 * controller state, which an io.cost.qos write may have changed
>> +	 * mid-period. Emit before the refresh and the period transition.
>> +	 */
>> +	if (trace_iocost_ioc_tick_enabled()) {
>> +		struct ioc_gq *active_iocg;
>> +
>> +		list_for_each_entry(active_iocg, &ioc->active_iocgs,
>> +				     active_list)
>> +			nr_active++;
>> +	}
>> +	trace_iocost_ioc_tick(ioc, &now, nr_active, usage_us_sum);
>> +
> 
> Can this record a bogus nr_active of 0?
> 
> trace_iocost_ioc_tick() is called outside the
> trace_iocost_ioc_tick_enabled() block, so the two static key tests are
> independent.  If a probe is attached on another CPU between them, the first
> test sees the key disabled and skips the count, while the tracepoint itself
> then fires with nr_active still 0 even though ioc->active_iocgs is non-empty.
> 
> Documentation/trace/tracepoints.rst says the tracepoint call should always
> be within the block of the if (trace_<tracepoint>_enabled()) check to avoid
> this race.
I'll move `trace_iocost_ioc_tick()` inside the enabled() block in the
next posting.

Thanks,
Tao
> 
> Since the commit message describes active=0 as the marker for the transition
> to dormancy, a consumer could read this spurious record as that transition,
> alongside a non-zero usage value.
> 
> Would moving the trace_iocost_ioc_tick() call inside the enabled block
> work?  Alternatively, nr_active could be counted in the existing
> list_for_each_entry() walk over ioc->active_iocgs earlier in
> ioc_timer_fn().
> 
> 
> ---
> AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
> See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md
> 
> CI run summary: https://github.com/kernel-patches/bpf/actions/runs/41646836440