Split iowait into two states

In preparation for storing two separate iowait states in there, bump the size from a 32-bit to a 64-bit size, for 64-bit kernels.

Note that on 32-bit, the number of tasks are limited to 0x8000, which fits just fine in even half of the existiing 32-bit atomic_t. For 64-bit, no such limit exists, hence play it safe and make it a 64-bit atomic.

Signed-off-by Jens Axboe <[email protected]>

---
 kernel/sched/core.c  | 14 +++++++++++++-
 kernel/sched/sched.h |  4 ++++
 2 files changed, 17 insertions(+), 1 deletion(-)

kernel/sched/core.c +12 -0

index 977bb08a33d2..6a6c985220b1 100644
@@ -3723,17 +3723,29 @@ static inline cpumask_t *alloc_user_cpus_ptr(int node)
 
 static void task_iowait_inc(struct task_struct *p)
 {
+#ifdef CONFIG_64BIT
+	atomic_long_inc(&task_rq(p)->nr_iowait);
+#else
 	atomic_inc(&task_rq(p)->nr_iowait);
+#endif
 }
 
 static void task_iowait_dec(struct task_struct *p)
 {
+#ifdef CONFIG_64BIT
+	atomic_long_dec(&task_rq(p)->nr_iowait);
+#else
 	atomic_dec(&task_rq(p)->nr_iowait);
+#endif
 }
 
 int rq_iowait(struct rq *rq)
 {
+#ifdef CONFIG_64BIT
+	return atomic_long_read(&rq->nr_iowait);
+#else
 	return atomic_read(&rq->nr_iowait);
+#endif
 }
 
 static void

kernel/sched/core.c +0 -0

 #ifdef CONFIG_SCHED_CORE
 		rq->core = rq;

kernel/sched/sched.h +4 -0

index 387f67ddf18a..c2802d066615 100644
@@ -1049,7 +1049,11 @@ struct rq {
 	u64			clock_idle_copy;
 #endif
 
+#ifdef CONFIG_64BIT
+	atomic_long_t		nr_iowait;
+#else
 	atomic_t		nr_iowait;
+#endif
 
 #ifdef CONFIG_SCHED_DEBUG
 	u64 last_seen_need_resched_ns;

Signature · 1 line

2.43.0

iowait is a bogus metric, but it's helpful in the sense that it allows short waits to not enter sleep states that have a higher exit latency than would've otherwise have been picked for iowait'ing tasks. However, it's harmless in that lots of applications and monitoring assumes that

iowait is busy time, or otherwise use it as a health metric. Particularly for async IO it's entirely nonsensical.

Split the iowait part into two parts - one that tracks whether the task needs boosting for short waits, and one that marks whether the task needs to be accounted as such. ->in_iowait_acct nests inside of ->in_iowait, both for efficiency reasons, but also so that the relationship between the two is clear. A waiter may set ->in_wait alone

and not care about the accounting.

Existing users of nr_iowait() for accounting purposes are switched to use nr_iowait_acct(), which leaves the governor using nr_iowait() as it only cares about iowaiters, not the accounting side.

Utilize that there's enough space in rq->nr_iowait to store both values in there, shifting the accounting side by half the size of the type. Thank you to Thomas Gleixner for that [1] suggestion.

[1] https://lore.kernel.org/lkml/87sf1b6o9w.ffs@tglx/

Signed-off-by Jens Axboe <[email protected]>

---
 arch/s390/appldata/appldata_base.c |  2 +-
 arch/s390/appldata/appldata_os.c   |  2 +-
 fs/proc/stat.c                     |  2 +-
 include/linux/sched.h              |  6 +++
 include/linux/sched/stat.h         |  5 ++-
 kernel/sched/core.c                | 62 +++++++++++++++++++++++-------
 kernel/sched/sched.h               |  1 +
 kernel/time/tick-sched.c           |  6 +--
 8 files changed, 65 insertions(+), 21 deletions(-)

arch/s390/appldata/appldata_base.c +1 -1

index c2978cb03b36..6844b5294a8b 100644
@@ -423,4 +423,4 @@ EXPORT_SYMBOL_GPL(si_swapinfo);
 #endif
 EXPORT_SYMBOL_GPL(nr_threads);
 EXPORT_SYMBOL_GPL(nr_running);
-EXPORT_SYMBOL_GPL(nr_iowait);
+EXPORT_SYMBOL_GPL(nr_iowait_acct);

arch/s390/appldata/appldata_os.c +1 -1

index a363d30ce739..fa4b278aca6c 100644
@@ -100,7 +100,7 @@ static void appldata_get_os_data(void *data)
 
 	os_data->nr_threads = nr_threads;
 	os_data->nr_running = nr_running();
-	os_data->nr_iowait  = nr_iowait();
+	os_data->nr_iowait  = nr_iowait_acct();
 	os_data->avenrun[0] = avenrun[0] + (FIXED_1/200);
 	os_data->avenrun[1] = avenrun[1] + (FIXED_1/200);
 	os_data->avenrun[2] = avenrun[2] + (FIXED_1/200);

fs/proc/stat.c +1 -1

index da60956b2915..149be7a884fb 100644
@@ -180,7 +180,7 @@ static int show_stat(struct seq_file *p, void *v)
 		(unsigned long long)boottime.tv_sec,
 		total_forks,
 		nr_running(),
-		nr_iowait());
+		nr_iowait_acct());
 
 	seq_put_decimal_ull(p, "softirq ", (unsigned long long)sum_softirq);
 

include/linux/sched.h +6 -0

index dcfc2830ed8e..26c69db5484b 100644
@@ -924,7 +924,13 @@ struct task_struct {
 
 	/* Bit to tell TOMOYO we're in execve(): */
 	unsigned			in_execve:1;
+	/* task is in iowait */
 	unsigned			in_iowait:1;
+	/*
+	 * task is in iowait and should be accounted as such. can only be set
+	 * if ->in_iowait is also set.
+	 */
+	unsigned			in_iowait_acct:1;
 #ifndef TIF_RESTORE_SIGMASK
 	unsigned			restore_sigmask:1;
 #endif

include/linux/sched/stat.h +3 -2

index 0108a38bb64d..31e8a44b3d71 100644
@@ -19,8 +19,9 @@ DECLARE_PER_CPU(unsigned long, process_counts);
 extern int nr_processes(void);
 extern unsigned int nr_running(void);
 extern bool single_task_running(void);
-extern unsigned int nr_iowait(void);
-extern unsigned int nr_iowait_cpu(int cpu);
+unsigned int nr_iowait_acct(void);
+unsigned int nr_iowait_acct_cpu(int cpu);
+unsigned int nr_iowait_cpu(int cpu);
 
 static inline int sched_info_on(void)
 {

kernel/sched/core.c +0 -0

index 63f6d44f460c..d52d3118dd73 100644
@@ -3721,30 +3721,47 @@ static inline cpumask_t *alloc_user_cpus_ptr(int node)
 
 #endif /* !CONFIG_SMP */

kernel/sched/core.c +18 -5

+	atomic_long_add(val, &task_rq(p)->nr_iowait);
 #else
-	atomic_inc(&task_rq(p)->nr_iowait);
+	int val = 1 + ((int) p->in_iowait_acct << 16);
+	atomic_add(val, &task_rq(p)->nr_iowait);
 #endif
 }
 
 static void task_iowait_dec(struct task_struct *p)
 {
 #ifdef CONFIG_64BIT
-	atomic_long_dec(&task_rq(p)->nr_iowait);
+	long val = 1 + ((long) p->in_iowait_acct << 32);
+	atomic_long_sub(val, &task_rq(p)->nr_iowait);
 #else
-	atomic_dec(&task_rq(p)->nr_iowait);
+	int val = 1 + ((int) p->in_iowait_acct << 16);
+	atomic_sub(val, &task_rq(p)->nr_iowait);
 #endif
 }
 
 int rq_iowait(struct rq *rq)
 {
 #ifdef CONFIG_64BIT
-	return atomic_long_read(&rq->nr_iowait);
+	return atomic_long_read(&rq->nr_iowait) & ((1UL << 32) - 1);
+#else
+	return atomic_read(&rq->nr_iowait) & ((1U << 16) - 1);
+#endif
+}
+
+int rq_iowait_acct(struct rq *rq)
+{
+#ifdef CONFIG_64BIT
+	return atomic_long_read(&rq->nr_iowait) >> 32;
 #else
-	return atomic_read(&rq->nr_iowait);
+	return atomic_read(&rq->nr_iowait) >> 16;
 #endif
 }
 

kernel/sched/core.c +2 -2

 {
 	return rq_iowait(cpu_rq(cpu));
 }

@@ -5532,12 +5554,12 @@ unsigned int nr_iowait_cpu(int cpu)
  * Task CPU affinities can make all that even more 'interesting'.
  */
 
-unsigned int nr_iowait(void)
+unsigned int nr_iowait_acct(void)
 {
 	unsigned int i, sum = 0;
 
 	for_each_possible_cpu(i)
-		sum += nr_iowait_cpu(i);
+		sum += nr_iowait_acct_cpu(i);
 
 	return sum;
 }

@@ -9032,18 +9054,32 @@ int __sched yield_to(struct task_struct *p, bool preempt)
 }
 EXPORT_SYMBOL_GPL(yield_to);
[snip]

Quoted 46 lines

[snip]

(quoted diff) +18 -5

+	atomic_long_add(val, &task_rq(p)->nr_iowait);
 #else
-	atomic_inc(&task_rq(p)->nr_iowait);
+	int val = 1 + ((int) p->in_iowait_acct << 16);
+	atomic_add(val, &task_rq(p)->nr_iowait);
 #endif
 }
 
 static void task_iowait_dec(struct task_struct *p)
 {
 #ifdef CONFIG_64BIT
-	atomic_long_dec(&task_rq(p)->nr_iowait);
+	long val = 1 + ((long) p->in_iowait_acct << 32);
+	atomic_long_sub(val, &task_rq(p)->nr_iowait);
 #else
-	atomic_dec(&task_rq(p)->nr_iowait);
+	int val = 1 + ((int) p->in_iowait_acct << 16);
+	atomic_sub(val, &task_rq(p)->nr_iowait);
 #endif
 }
 
 int rq_iowait(struct rq *rq)
 {
 #ifdef CONFIG_64BIT
-	return atomic_long_read(&rq->nr_iowait);
+	return atomic_long_read(&rq->nr_iowait) & ((1UL << 32) - 1);
+#else
+	return atomic_read(&rq->nr_iowait) & ((1U << 16) - 1);
+#endif
+}
+
+int rq_iowait_acct(struct rq *rq)
+{
+#ifdef CONFIG_64BIT
+	return atomic_long_read(&rq->nr_iowait) >> 32;
 #else
-	return atomic_read(&rq->nr_iowait);
+	return atomic_read(&rq->nr_iowait) >> 16;
 #endif
 }

@@ -5497,7 +5514,12 @@ unsigned long long nr_context_switches(void)
  * it does become runnable.
  */
 

Review unsigned long long nr_context_switches(void) · line 5517

Quoted 2 lines

(quoted diff) +1 -1

-unsigned int nr_iowait_cpu(int cpu)
+unsigned int nr_iowait_acct_cpu(int cpu)

There is a comment above here by Daniel referring to "these two
interfaces [...]", originally referring to nr_iowait_cpu() and
get_iowait_load().
get_iowait_load() was removed in commit a7fe5190c03f ("cpuidle: menu: Remove get_loadavg() from the performance multiplier") but nr_iowait_cpu() remains, so the comment should remain above it.
Rewording is also way overdue, although that's hardly on your
patch ;)

iowait is a bogus metric, but it's helpful in the sense that it allows short waits to not enter sleep states that have a higher exit latency than would've otherwise have been picked for iowait'ing tasks. However, it's harmless in that lots of applications and monitoring assumes that iowait is busy time, or otherwise use it as a health metric. Particularly for async IO it's entirely nonsensical.

Quoted 6 lines · Jens Axboe

iowait is a bogus metric, but it's helpful in the sense that it allows short waits to not enter sleep states that have a higher exit latency than would've otherwise have been picked for iowait'ing tasks. However, it's harmless in that lots of applications and monitoring assumes that iowait is busy time, or otherwise use it as a health metric. Particularly for async IO it's entirely nonsensical.

Let me get this straight, all of this is about working around cpuidle menu governor insaity?

Rafael, how far along are we with fully deprecating that thing? Yes it still exists, but should people really be using it still?

Quoted 11 lines · Peter Zijlstra

Quoted 6 lines · Jens Axboe

iowait is a bogus metric, but it's helpful in the sense that it allows short waits to not enter sleep states that have a higher exit latency than would've otherwise have been picked for iowait'ing tasks. However, it's harmless in that lots of applications and monitoring assumes that iowait is busy time, or otherwise use it as a health metric. Particularly for async IO it's entirely nonsensical.

Let me get this straight, all of this is about working around cpuidle menu governor insaity?

Quoted 3 lines

Rafael, how far along are we with fully deprecating that thing? Yes it still exists, but should people really be using it still?

Well there is also the iowait boost handling in schedutil and intel_pstate which, at least in synthetic benchmarks, does have an effect [1].

io_uring (the only user of iowait but not iowait_acct) works around both.

See commit ("8a796565cec3 io_uring: Use io_schedule* in cqring wait")

[1]
https://lore.kernel.org/lkml/[email protected]/#t

Quoted 16 lines · Christian Loehle

Quoted 14 lines · Peter Zijlstra

Quoted 6 lines · Jens Axboe

iowait is a bogus metric, but it's helpful in the sense that it allows short waits to not enter sleep states that have a higher exit latency than would've otherwise have been picked for iowait'ing tasks. However, it's harmless in that lots of applications and monitoring assumes that iowait is busy time, or otherwise use it as a health metric. Particularly for async IO it's entirely nonsensical.

Let me get this straight, all of this is about working around cpuidle menu governor insaity?

Rafael, how far along are we with fully deprecating that thing? Yes it still exists, but should people really be using it still?

Quoted 2 lines

Well there is also the iowait boost handling in schedutil and intel_pstate which, at least in synthetic benchmarks, does have an effect [1].

Those are cpufreq not cpuidle and at least they don't use nr_iowait. The original Changelog mentioned idle states, and I hate on menu for using nr_iowait.

Quoted 7 lines

io_uring (the only user of iowait but not iowait_acct) works around both.

See commit ("8a796565cec3 io_uring: Use io_schedule* in cqring wait")

[1]
https://lore.kernel.org/lkml/[email protected]/#t

So while I agree with most of the short-commings listed in that set, however that patch is quite terrifying.

I would prefer to start with something a *lot* simpler. How about a tick driven decay of iops count per task. And that whole step array *shudder*.

Quoted 6 lines · Jens Axboe

iowait is a bogus metric, but it's helpful in the sense that it allows short waits to not enter sleep states that have a higher exit latency than would've otherwise have been picked for iowait'ing tasks. However, it's harmless in that lots of applications and monitoring assumes that iowait is busy time, or otherwise use it as a health metric. Particularly for async IO it's entirely nonsensical.

Let me get this straight, all of this is about working around cpuidle menu governor insaity?

Rafael, how far along are we with fully deprecating that thing? Yes it still exists, but should people really be using it still?