Re: [PATCH] blk-iocost: publish hweights as one atomic value

[Tejun Heo <tj@kernel.org>]

Hello, Kunhai.

The following is a Claude-generated review.

On Thu, Oct 15, 2026 at 09:59:19AM -0400, Kunhai Dai wrote:
> current_hweight() uses separate plain stores for the cache generation and
> both weights. The compiler can publish the generation before the weights,
> letting an interrupt accept stale weights as current. Multiple refreshers
> can also interleave their stores. Invalid donation budgets may then lead
> to a divide error in transfer_surpluses().

The second one is the main problem and doesn't need the compiler's help.
The issue path refreshes the cache without ioc->lock, so CPUs issuing
for the same iocg refresh it concurrently after each commit_weights().
When one of them computed for the previous generation, its plain stores
can interleave with the other's in program order and leave the current
generation with a stale weight, which then stays until the next commit.
The rereads at out: can also return another refresher's stores, even to
the timer under ioc->lock. So the race isn't specific to a toolchain.
Maybe lead the description with it and its effect: stale hweights used
for the issue cost and the donation decisions, and the divide error?

The packed word fixes both, given that every weight change commits the
generation before ioc->lock is released, which holds for all
__propagate_weights() callers.

> Validated against commit 24a19b953e2a ("Linux 7.3-rc7"):

The description could be a lot shorter: the bug, its effect and the fix.
The validation details and the revision notes fit better below the ---
line.

> +	cache = ((u64)ioc_gen << 32) | ((u64)(hwa - 1) << 16) | (hwi - 1);
> +	atomic64_set(&iocg->hweight_cache, (s64)cache);

The encoding is open-coded in the store, the load, ioc_pd_init(),
iocg_activate(), the tracepoint and the monitor. Maybe pack and unpack
helpers with GENMASK_U64() and FIELD_PREP()/FIELD_GET() above the trace
include? The tracepoint header already calls ioc_name() from
blk-iocost.c, so it can use them too. Using WEIGHT_ONE - 1 as the field
mask also ties the weight scale to the field width.

> +	atomic64_set(&iocg->hweight_cache,
> +		     ((u64)(u32)(atomic_read(&ioc->hweight_gen) - 1) << 32) |
> +		     (u32)atomic64_read(&iocg->hweight_cache));

What are the old weight bits kept for? If it's the iocg_activate
tracepoint, a comment would help. Otherwise, writing just the stale
generation would do.

> -		current_hweight(iocg, &hwa, NULL);
> +		current_hweight(iocg, &hwa, &iocg->hweight_inuse);

This only feeds the inuse_transfer tracepoint further down. Maybe get
the value with current_hweight() there instead of writing the scratch
field from here?

> +		if (iocg->hweight_donating > 1)
> +			iocg->hweight_after_donation =
> +				min(iocg->hweight_after_donation,
> +				    iocg->hweight_donating - 1);

Every case this changes still trips the WARN's donating <= 1 test below
and takes the goto, so it only changes the after= value printed. Is it
needed?

> +			goto out;

As Tao suggested, this would be better as a separate first patch with
Fixes: e08d02aa5fc9 ("blk-iocost: implement Andy's method for donation
weight updates") and Cc: stable, followed by the packed cache with
Fixes: 7caa47151ab2 ("blkcg: implement blk-iocost").

On Tao's second point, with two or more over-donors the over_hwa rescale
can floor a small donor's after to zero without any race, and an inner
node with hwa equal to donating then gets a zero hweight_inuse and the
same divide. This comes from arithmetic on a constructed hierarchy and
hasn't been reproduced. If it's reachable, the WARN fires from a
legitimate state. Would it be better to keep the rescaled after at 1 or
more, with the goto as the safety net? The goto alone gives up donation
for the whole period.

Thanks.

-- 
tejun