[PATCH 0/4] block: drain per-cpu latency stats over possible CPUs

[PATCH 0/4] block: drain per-cpu latency stats over possible CPUs

[Tao Cui]

From: Tao Cui <cuitao@kylinos.cn>

Several block-layer latency statistics accumulate samples in per-cpu
buckets and drain them periodically -- in timer callbacks, or at each
check / reporting site -- using for_each_online_cpu().  When a CPU that
holds pending samples is taken offline, that bucket is skipped during
the drain: the samples are neither accumulated into the current window
nor, where the drain also resets, cleared.  They sit in the bucket until
the CPU is brought back online, at which point they are flushed into
whatever window happens to be running.

The effect cuts both ways and is harmful in each direction:

  - while the CPU is offline, its samples are under-counted, so
    throttle / latency / scheduling decisions derived from these stats
    miss the work that actually happened ("should have throttled, but
    didn't");

  - on re-online, a burst of stale samples lands in a later window and
    can spuriously trip a throttle or skew a vrate / latency estimate
    ("shouldn't throttle, but did").

Fix: drain over for_each_possible_cpu() instead.  The per-cpu areas are
allocated for the full possible set, and the init paths already iterate
it; an offline CPU has no concurrent writer, so reading and resetting
its bucket is safe.

Cost is bounded: the extra work is over offline buckets that have no
writer and -- because each drain sums and resets -- hold a zeroed stat
after the first post-offline drain.  It amounts to O(possible - online)
trivial per-cpu reads on a timer / check / show path, alongside the
per-CPU stat work already done for online CPUs; on systems where all
possible CPUs are online it is exactly zero.

This is the same one-line mistake in four sites; each patch switches its
drain loop from for_each_online_cpu() to for_each_possible_cpu():

  - block/blk-stat.c        blk_stat_timer_fn() -- shared infrastructure
                            that also feeds wbt and blk-mq latency
  - block/blk-iolatency.c   iolatency_check_latencies() (throttle) and
                            iolatency_ssd_stat() (io.stat reporting)
  - block/blk-iocost.c      ioc_lat_stat() -- missed-ppm / rq_wait delta
  - block/kyber-iosched.c   kyber_timer_fn() -- latency histogram flush

blk-mq.c's for_each_online_cpu() at the hctx-has-online-cpu check is
correct and left untouched -- it is not a statistics drain.

Tao Cui (4):
  block/blk-stat: drain per-cpu callback stats over possible CPUs
  block/blk-iolatency: account per-cpu latency stats over possible CPUs
  block/blk-iocost: collect per-cpu latency stats over possible CPUs
  block/kyber-iosched: flush per-cpu latency buckets over possible CPUs

 block/blk-iocost.c     | 2 +-
 block/blk-iolatency.c  | 4 ++--
 block/blk-stat.c       | 2 +-
 block/kyber-iosched.c  | 2 +-
 4 files changed, 5 insertions(+), 5 deletions(-)

--
2.43.0

[PATCH 1/4] block/blk-stat: drain per-cpu callback stats over possible CPUs

[Tao Cui]

From: Tao Cui <cuitao@kylinos.cn>

blk_stat_timer_fn() sums and resets a callback's per-cpu buckets using
for_each_online_cpu().  A CPU that goes offline with pending samples is
skipped, so its samples are neither accumulated into the window nor
cleared; they sit in the bucket until the CPU comes back online, at
which point the stale values are flushed into whatever window is then
running.

This silently corrupts the latency picture that consumers (notably
writeback throttling via wbt, and blk-mq latency tracking) base
decisions on around CPU hotplug: under-counting while the CPU is
offline, then a burst of stale data on re-online.

Fixes: 34dbad5d26e2 ("blk-stat: convert to callback-based statistics reporting")
Signed-off-by: Tao Cui <cuitao@kylinos.cn>
---
 block/blk-stat.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/blk-stat.c b/block/blk-stat.c
index de126e1ea5ac..d57c2fc6bf06 100644
--- a/block/blk-stat.c
+++ b/block/blk-stat.c
@@ -83,7 +83,7 @@ static void blk_stat_timer_fn(struct timer_list *t)
 	for (bucket = 0; bucket < cb->buckets; bucket++)
 		blk_rq_stat_init(&cb->stat[bucket]);
 
-	for_each_online_cpu(cpu) {
+	for_each_possible_cpu(cpu) {
 		struct blk_rq_stat *cpu_stat;
 
 		cpu_stat = per_cpu_ptr(cb->cpu_stat, cpu);
-- 
2.43.0

[PATCH 2/4] block/blk-iolatency: account per-cpu latency stats over possible CPUs

[Tao Cui]

From: Tao Cui <cuitao@kylinos.cn>

iolatency_check_latencies() and iolatency_ssd_stat() iterate a blkg's
per-cpu latency stats with for_each_online_cpu().  When a CPU that has
accumulated io.latency samples goes offline, its bucket is skipped: the
check loop (which also resets) neither sums nor clears it, and the show
path under-reports.  On re-online the stranded samples are flushed into
a later check window, which can trigger a spurious throttle/scale
adjustment.

Fixes: d70675121546 ("block: introduce blk-iolatency io controller")
Fixes: 1fa2840e56f9 ("blk-iolatency: use a percentile approache for ssd's")
Signed-off-by: Tao Cui <cuitao@kylinos.cn>
---
 block/blk-iolatency.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/blk-iolatency.c b/block/blk-iolatency.c
index 1cc33aa0b669..d6c401d6174c 100644
--- a/block/blk-iolatency.c
+++ b/block/blk-iolatency.c
@@ -523,7 +523,7 @@ static void iolatency_check_latencies(struct iolatency_grp *iolat, u64 now)
 
 	latency_stat_init(iolat, &stat);
 	preempt_disable();
-	for_each_online_cpu(cpu) {
+	for_each_possible_cpu(cpu) {
 		struct latency_stat *s;
 		s = per_cpu_ptr(iolat->stats, cpu);
 		latency_stat_sum(iolat, &stat, s);
@@ -924,7 +924,7 @@ static void iolatency_ssd_stat(struct iolatency_grp *iolat, struct seq_file *s)
 
 	latency_stat_init(iolat, &stat);
 	preempt_disable();
-	for_each_online_cpu(cpu) {
+	for_each_possible_cpu(cpu) {
 		struct latency_stat *s;
 		s = per_cpu_ptr(iolat->stats, cpu);
 		latency_stat_sum(iolat, &stat, s);
-- 
2.43.0

[PATCH 3/4] block/blk-iocost: collect per-cpu latency stats over possible CPUs

[Tao Cui]

From: Tao Cui <cuitao@kylinos.cn>

ioc_lat_stat() walks ioc->pcpu_stat with for_each_online_cpu() to
compute missed-ppm and rq_wait deltas.  An offlined CPU is skipped, so
its delta is dropped from the period and its last_* watermark is not
advanced; on re-online the next collection sees a delta spanning the
whole offline interval, corrupting the latency/vrate picture.

Fixes: 7caa47151ab2 ("blkcg: implement blk-iocost")
Signed-off-by: Tao Cui <cuitao@kylinos.cn>
---
 block/blk-iocost.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/blk-iocost.c b/block/blk-iocost.c
index 8b2aeba2e1e3..b60625613e09 100644
--- a/block/blk-iocost.c
+++ b/block/blk-iocost.c
@@ -1592,7 +1592,7 @@ static void ioc_lat_stat(struct ioc *ioc, u32 *missed_ppm_ar, u32 *rq_wait_pct_p
 	u64 rq_wait_ns = 0;
 	int cpu, rw;
 
-	for_each_online_cpu(cpu) {
+	for_each_possible_cpu(cpu) {
 		struct ioc_pcpu_stat *stat = per_cpu_ptr(ioc->pcpu_stat, cpu);
 		u64 this_rq_wait_ns;
 
-- 
2.43.0

[PATCH 4/4] block/kyber-iosched: flush per-cpu latency buckets over possible CPUs

[Tao Cui]

From: Tao Cui <cuitao@kylinos.cn>

kyber_timer_fn() sums the per-cpu latency histograms with
for_each_online_cpu().  A CPU that goes offline mid-interval leaves its
bucket un-flushed; the samples are lost from the current decision and
re-appear (stale) when the CPU is onlined again.

Fixes: 6e25cb01ea20 ("kyber: implement improved heuristics")
Signed-off-by: Tao Cui <cuitao@kylinos.cn>
---
 block/kyber-iosched.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/kyber-iosched.c b/block/kyber-iosched.c
index 971818bcdc9d..2ee552ab8135 100644
--- a/block/kyber-iosched.c
+++ b/block/kyber-iosched.c
@@ -275,7 +275,7 @@ static void kyber_timer_fn(struct timer_list *t)
 	bool bad = false;
 
 	/* Sum all of the per-cpu latency histograms. */
-	for_each_online_cpu(cpu) {
+	for_each_possible_cpu(cpu) {
 		struct kyber_cpu_latency *cpu_latency;
 
 		cpu_latency = per_cpu_ptr(kqd->cpu_latency, cpu);
-- 
2.43.0