* Re: [PATCH] blk-iocost: publish hweights as one atomic value
2026-10-15 13:59 [PATCH] blk-iocost: publish hweights as one atomic value Kunhai Dai
@ 2026-10-16 11:13 ` Tao Cui
2026-10-16 19:45 ` Tejun Heo
1 sibling, 0 replies; 3+ messages in thread
From: Tao Cui @ 2026-10-16 11:13 UTC (permalink / raw)
To: Kunhai Dai, tj
Cc: cui.tao, axboe, josef, yukuai, cgroups, linux-block, linux-kernel,
rostedt, mhiramat, mathieu.desnoyers, linux-trace-kernel
Hello，Kunhai
在 2026/10/15 21:59, Kunhai Dai 写道:
> 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().
>
> Pack the generation and both weights into a single atomic64_t. Hierarchical
> weights are in [1, 65536], so storing each weight minus one preserves its
> full range in 16 bits and leaves 32 bits for the generation. Readers copy
> one complete entry, and refreshers publish one complete entry. Return the
> local snapshot instead of rereading fields another refresher can modify.
>
> Keep the existing lockless refresh and cache reuse behavior. A late
> refresh may replace a newer entry, but carries its own generation so the
> next reader can detect the mismatch. Preserve the existing barrier paired
> with commit_weights() and the allowance for approximate hierarchy reads.
>
> Keep the old weight fields solely as donation workspace. Update cache
> initialization, activation invalidation, tracepoints and the drgn monitor
> accordingly. Tracepoints decode one cached snapshot without recalculating
> or refreshing weights. Stop invalid donation calculations before applying
> leaf adjustments, and check donating before subtracting one.
>
> Validated against commit 24a19b953e2a ("Linux 7.3-rc7"):
> - clean application, whitespace and strict checkpatch checks;
> - W=1 compilation of blk-iocost.o with the existing x86-64 configuration
> and PREEMPT_RT plus PROVE_LOCKING;
> - extracted production-function checks covering all weight values,
> generation high bits, publication interleavings, donation cleanup and
> 400000 concurrent refresh calls using C11 atomic test stubs;
> - Python syntax and actual monitor-decoder checks;
> - x86-64 disassembly confirms a single 64-bit cache load and store.
>
> Runtime kprobe injection on an x86-64 VM with 32 vCPUs completed three
> rounds without WARN or panic. With a 30 ms publication delay, the probe
> reported 23102 publication-point holds, 3234 cache-change observations
> within those windows and 72 matching-generation reader observations. The
> unpatched baseline panicked under the corresponding reproduction load.
>
> The runtime build predates this revision's tracepoint-only simplification;
> the packed-cache and donation logic are unchanged. The current tracepoint
> change has passed normal and PREEMPT_RT compilation, but has not been
> rerun in the runtime injection test. Performance regression testing has
> not been performed.
>
Thanks for the thorough patch. We tried your injection protocol on our side
(linux-next 20261014, clang 17, qemu/KVM, nullb0, the rt/s1..C topology with
1 <-> 10000 weight alternation, a 30ms kprobe hold at the publication
stores). The patched kernel came through cleanly -- no WARN or oops even
with a hold on every refresh, and the tracepoint decode of the final
revision worked in all runs, which nicely covers the gap you disclosed in
the commit message. We could not reproduce the divide error on the baseline,
likely because our clang build orders the stores gen-last, so the direct
window from your step 4 does not exist in our binary -- your repro's
precondition is indeed toolchain-dependent.
Two small observations:
1. The goto out in transfer_surpluses() skips the donation tail for all
donors, so while a degenerate weight configuration persists, donation is
given up every period (and inuse_transfer tracepoints go quiet). Strictly
better than the old panic, but it might be worth a sentence in the commit
message.
2. after == 0 may not be race-only: a boundary rounding in the over_hwa
rescale can also floor a small over-donor's after value to zero. So the
guard has some value even independent of the race.
Along the same lines, would you consider splitting the patch? A small
[1/2] with just the div0 guard (Fixes: e08d02aa5fc9, Cc: stable) would
backport easily, while the packed hweight_cache plus tracepoint/monitor
changes could go as [2/2] (Fixes: 7caa47151ab2). Also, the patch is
currently missing Fixes: entirely.
Thanks，
Tao
> Developed with assistance from OpenAI Codex.
>
> Assisted-by: LLM
> Signed-off-by: Kunhai Dai <daikunhai@didiglobal.com>
>
> ---
> Reproduction outline (x86-64, cgroup v2):
> 1. Enable BLK_CGROUP_IOCOST and KPROBES. Enable the io controller at each
> parent, and configure io.cost.qos and io.cost.model for the test device.
> 2. Create four leaves: rt/s1, rt/K/s2, rt/K/P/s3 and rt/K/P/C. Run continuous
> direct 1 MiB writes in the first three; issue four direct 4 KiB writes
> every 200 ms in C, keeping it active with low utilization.
> 3. Every 300 ms, alternate the K/P/C chain weights between 1 and 10000,
> with the sibling leaves s1/s2/s3 taking the opposite weights. This
> repeatedly makes C's hierarchical share fall to the minimum of 1.
> 4. On the unpatched build, confirm that the compiler placed the cached
> generation store before the weight stores. Insert a kprobe delay of
> 30 ms after that generation store and before the weight stores.
> Derive the instruction offset from the binary being tested.
> 5. On the packed-cache build, delay just before the atomic cache store
> instead: there is no separate generation-publication window. This
> stresses concurrent refresh completion and late cache overwrites.
>
> Unpatched baseline, abbreviated crash and stack excerpt:
>
> iocg: invalid donation weights in /rt/K/P:
> active=1 donating=1 after=0
> Oops: divide error: 0000 [#1] SMP PTI
> RIP: 0010:transfer_surpluses+0x43e/0x6b0
> RAX: ffffffffffffffff RCX: 0000000000000000
> <IRQ>
> ioc_timer_fn+0x1158/0x13b0
> call_timer_fn+0x1e/0x100
> __run_timer_base.part.36+0x1f0/0x220
> [...]
> handle_softirqs+0xa9/0x2a0
> irq_exit_rcu+0x48/0xe0
> sysvec_apic_timer_interrupt+0x6b/0x80
> </IRQ>
> [...]
> win_pre+0x44/0x70 [repro_iocost]
> [...]
> ? current_hweight+0xc1/0x110
> ? adjust_inuse_and_calc_cost+0x44/0x290
> ? ioc_rqos_throttle+0x13e/0x590
> Kernel panic - not syncing: Fatal exception in interrupt
>
> block/blk-iocost.c | 65 ++++++++++++++++++++++++++++--------------
> include/trace/events/iocost.h | 6 ++--
> tools/cgroup/iocost_monitor.py | 7 +++--
> 3 files changed, 53 insertions(+), 25 deletions(-)
>
> diff --git a/block/blk-iocost.c b/block/blk-iocost.c
> index 2745bffcd..fcc7ae866 100644
> --- a/block/blk-iocost.c
> +++ b/block/blk-iocost.c
> @@ -472,8 +472,8 @@ struct ioc_gq {
> *
> * When an idle cgroup becomes active its `active` goes from 0 to
> * `weight`. `inuse` is the surplus adjusted active weight.
> - * `active` and `inuse` are used to calculate `hweight_active` and
> - * `hweight_inuse`.
> + * `active` and `inuse` determine the hierarchical weights cached in
> + * `hweight_cache`.
> *
> * `last_inuse` remembers `inuse` while an iocg is idle to persist
> * surplus adjustments.
> @@ -520,7 +520,9 @@ struct ioc_gq {
> u64 child_active_sum;
> u64 child_inuse_sum;
> u64 child_adjusted_sum;
> - int hweight_gen;
> + /* Generation in bits 63:32; active - 1 and inuse - 1 in 31:16/15:0. */
> + atomic64_t hweight_cache;
> + /* Donation workspace, protected by ioc->lock. */
> u32 hweight_active;
> u32 hweight_inuse;
> u32 hweight_donating;
> @@ -1154,13 +1156,17 @@ static void current_hweight(struct ioc_gq *iocg, u32 *hw_activep, u32 *hw_inusep
> {
> struct ioc *ioc = iocg->ioc;
> int lvl;
> - u32 hwa, hwi;
> - int ioc_gen;
> -
> - /* hot path - if uptodate, use cached */
> - ioc_gen = atomic_read(&ioc->hweight_gen);
> - if (ioc_gen == iocg->hweight_gen)
> + u32 hwa, hwi, ioc_gen;
> + u64 cache;
> +
> + /* Read the generation and both weights as one atomic snapshot. */
> + ioc_gen = (u32)atomic_read(&ioc->hweight_gen);
> + cache = (u64)atomic64_read(&iocg->hweight_cache);
> + if ((u32)(cache >> 32) == ioc_gen) {
> + hwa = ((cache >> 16) & (WEIGHT_ONE - 1)) + 1;
> + hwi = (cache & (WEIGHT_ONE - 1)) + 1;
> goto out;
> + }
>
> /*
> * Paired with wmb in commit_weights(). If we saw the updated
> @@ -1194,14 +1200,22 @@ static void current_hweight(struct ioc_gq *iocg, u32 *hw_activep, u32 *hw_inusep
> hwi = div64_u64((u64)hwi * inuse, inuse_sum);
> }
>
> - iocg->hweight_active = max_t(u32, hwa, 1);
> - iocg->hweight_inuse = max_t(u32, hwi, 1);
> - iocg->hweight_gen = ioc_gen;
> + hwa = max_t(u32, hwa, 1);
> + hwi = max_t(u32, hwi, 1);
> + /*
> + * Each weight is in [1, WEIGHT_ONE]. Subtracting one preserves its
> + * full range in 16 bits, leaving 32 bits for the generation. An
> + * interrupt or another CPU can only observe a complete snapshot.
> + * A late refresher may overwrite a newer cache, but its generation
> + * stays paired with its own weights and forces a future refresh.
> + */
> + cache = ((u64)ioc_gen << 32) | ((u64)(hwa - 1) << 16) | (hwi - 1);
> + atomic64_set(&iocg->hweight_cache, (s64)cache);
> out:
> if (hw_activep)
> - *hw_activep = iocg->hweight_active;
> + *hw_activep = hwa;
> if (hw_inusep)
> - *hw_inusep = iocg->hweight_inuse;
> + *hw_inusep = hwi;
> }
>
> /*
> @@ -1300,10 +1314,12 @@ static bool iocg_activate(struct ioc_gq *iocg, struct ioc_now *now)
>
> /*
> * Activate, propagate weight and start period timer if not
> - * running. Reset hweight_gen to avoid accidental match from
> - * wrapping.
> + * running. Invalidate the cached generation to avoid accidental match
> + * from wrapping.
> */
> - iocg->hweight_gen = atomic_read(&ioc->hweight_gen) - 1;
> + atomic64_set(&iocg->hweight_cache,
> + ((u64)(u32)(atomic_read(&ioc->hweight_gen) - 1) << 32) |
> + (u32)atomic64_read(&iocg->hweight_cache));
> list_add(&iocg->active_list, &ioc->active_iocgs);
>
> propagate_weights(iocg, iocg->weight,
> @@ -1863,7 +1879,7 @@ static void transfer_surpluses(struct list_head *surpluses, struct ioc_now *now)
> list_for_each_entry(iocg, surpluses, surplus_list) {
> u32 hwa;
>
> - current_hweight(iocg, &hwa, NULL);
> + current_hweight(iocg, &hwa, &iocg->hweight_inuse);
> after_sum += iocg->hweight_after_donation;
>
> if (iocg->hweight_after_donation > hwa) {
> @@ -1946,8 +1962,10 @@ static void transfer_surpluses(struct list_head *surpluses, struct ioc_now *now)
>
> iocg->hweight_donating = min(iocg->hweight_donating,
> iocg->hweight_active);
> - iocg->hweight_after_donation = min(iocg->hweight_after_donation,
> - iocg->hweight_donating - 1);
> + if (iocg->hweight_donating > 1)
> + iocg->hweight_after_donation =
> + min(iocg->hweight_after_donation,
> + iocg->hweight_donating - 1);
> if (WARN_ON_ONCE(iocg->hweight_active <= 1 ||
> iocg->hweight_donating <= 1 ||
> iocg->hweight_after_donation == 0)) {
> @@ -1956,6 +1974,7 @@ static void transfer_surpluses(struct list_head *surpluses, struct ioc_now *now)
> pr_cont(": active=%u donating=%u after=%u\n",
> iocg->hweight_active, iocg->hweight_donating,
> iocg->hweight_after_donation);
> + goto out;
> }
> }
>
> @@ -2054,6 +2073,7 @@ static void transfer_surpluses(struct list_head *surpluses, struct ioc_now *now)
> __propagate_weights(iocg, iocg->active, inuse, true, now);
> }
>
> +out:
> /* walk list should be dissolved after use */
> list_for_each_entry_safe(iocg, tiocg, &inner_walk, walk_list)
> list_del_init(&iocg->walk_list);
> @@ -2360,7 +2380,7 @@ static void ioc_timer_fn(struct timer_list *timer)
> */
> TRACE_IOCG_PATH(inuse_shortage, iocg, &now,
> iocg->inuse, iocg->active,
> - iocg->hweight_inuse, new_hwi);
> + old_hwi, new_hwi);
>
> __propagate_weights(iocg, iocg->active,
> iocg->active, true, &now);
> @@ -3032,6 +3052,9 @@ static void ioc_pd_init(struct blkg_policy_data *pd)
> INIT_LIST_HEAD(&iocg->active_list);
> INIT_LIST_HEAD(&iocg->walk_list);
> INIT_LIST_HEAD(&iocg->surplus_list);
> + atomic64_set(&iocg->hweight_cache,
> + ((u64)(u32)(atomic_read(&ioc->hweight_gen) - 1) << 32) |
> + U32_MAX);
> iocg->hweight_active = WEIGHT_ONE;
> iocg->hweight_inuse = WEIGHT_ONE;
>
> diff --git a/include/trace/events/iocost.h b/include/trace/events/iocost.h
> index e772b1bc6..aac25edaa 100644
> --- a/include/trace/events/iocost.h
> +++ b/include/trace/events/iocost.h
> @@ -34,6 +34,8 @@ DECLARE_EVENT_CLASS(iocost_iocg_state,
> ),
>
> TP_fast_assign(
> + u64 cache = (u64)atomic64_read(&iocg->hweight_cache);
> +
> __assign_str(devname);
> __assign_str(cgroup);
> __entry->now = now->now;
> @@ -44,8 +46,8 @@ DECLARE_EVENT_CLASS(iocost_iocg_state,
> __entry->vtime = vtime;
> __entry->weight = iocg->weight;
> __entry->inuse = iocg->inuse;
> - __entry->hweight_active = iocg->hweight_active;
> - __entry->hweight_inuse = iocg->hweight_inuse;
> + __entry->hweight_active = ((cache >> 16) & (WEIGHT_ONE - 1)) + 1;
> + __entry->hweight_inuse = (cache & (WEIGHT_ONE - 1)) + 1;
> ),
>
> TP_printk("[%s:%s] now=%llu:%llu vrate=%llu "
> diff --git a/tools/cgroup/iocost_monitor.py b/tools/cgroup/iocost_monitor.py
> index bdd78ba27..8a4f844c9 100644
> --- a/tools/cgroup/iocost_monitor.py
> +++ b/tools/cgroup/iocost_monitor.py
> @@ -149,8 +149,11 @@ class IocgStat:
> self.weight = iocg.weight.value_() / WEIGHT_ONE
> self.active = iocg.active.value_() / WEIGHT_ONE
> self.inuse = iocg.inuse.value_() / WEIGHT_ONE
> - self.hwa_pct = iocg.hweight_active.value_() * 100 / WEIGHT_ONE
> - self.hwi_pct = iocg.hweight_inuse.value_() * 100 / WEIGHT_ONE
> + hweight_cache = iocg.hweight_cache.counter.value_()
> + hwa = ((hweight_cache >> 16) & (WEIGHT_ONE - 1)) + 1
> + hwi = (hweight_cache & (WEIGHT_ONE - 1)) + 1
> + self.hwa_pct = hwa * 100 / WEIGHT_ONE
> + self.hwi_pct = hwi * 100 / WEIGHT_ONE
> self.address = iocg.value_()
>
> vdone = iocg.done_vtime.counter.value_()
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] blk-iocost: publish hweights as one atomic value
2026-10-15 13:59 [PATCH] blk-iocost: publish hweights as one atomic value Kunhai Dai
2026-10-16 11:13 ` Tao Cui
@ 2026-10-16 19:45 ` Tejun Heo
1 sibling, 0 replies; 3+ messages in thread
From: Tejun Heo @ 2026-10-16 19:45 UTC (permalink / raw)
To: Kunhai Dai
Cc: axboe, josef, yukuai, cgroups, linux-block, linux-kernel, rostedt,
mhiramat, mathieu.desnoyers, linux-trace-kernel, Tao Cui
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
^ permalink raw reply [flat|nested] 3+ messages in thread