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

[bot+bpf-ci@kernel.org]

[-- Attachment #1: Type: text/plain, Size: 2341 bytes --]

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

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

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