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
