Split iowait into two states

In 3 of the 4 spots where we modify rq->nr_iowait we already hold the

rq lock, and hence don't need atomics to modify the current per-rq iowait count. In the 4th case, where we are scheduling in on a different CPU than the task was previously on, we do not hold the previous rq lock, and hence still need to use an atomic to increment the iowait count.

Rename the existing nr_iowait to nr_iowait_remote, and use that for the 4th case. The other three cases can simply inc/dec in a non-atomic fashion under the held rq lock.

The per-rq iowait now becomes the difference between the two, the local count minus the remote count.

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

Quoted 1 line · Jens Axboe

In 3 of the 4 spots where we modify rq->nr_iowait we already hold the

We modify something and hold locks? It's documented that changelogs should not impersonate code. It simply does not make any sense.

Other than that:

Reviewed-by Thomas Gleixner <[email protected]>

Quoted 5 lines · Thomas Gleixner

Quoted 1 line · Jens Axboe

In 3 of the 4 spots where we modify rq->nr_iowait we already hold the

We modify something and hold locks? It's documented that changelogs should not impersonate code. It simply does not make any sense.

Agree it doesn't read that well... It's meant to say that we already hold the rq lock in 3 of the 4 spots, hence using atomic_inc/dec is pointless for those cases.

Quoted 7 lines · Jens Axboe

Quoted 5 lines · Thomas Gleixner

Quoted 1 line · Jens Axboe

In 3 of the 4 spots where we modify rq->nr_iowait we already hold the

We modify something and hold locks? It's documented that changelogs should not impersonate code. It simply does not make any sense.

Quoted 3 lines

Agree it doesn't read that well... It's meant to say that we already hold the rq lock in 3 of the 4 spots, hence using atomic_inc/dec is pointless for those cases.

That and the 'we'. Write it neutral.

The accounting of rq::nr_iowait is using an atomic_t but 3 out of 4 places hold runqueue lock already. ....

So but I just noticed that there is actually an issue with this:

kernel/sched/core.c unsigned long long nr_context_switches(void) · line 5470

Quoted 6 lines

unsigned int nr_iowait_cpu(int cpu)
{

kernel/sched/core.c +3 -1

-	return atomic_read(&cpu_rq(cpu)->nr_iowait);
+	struct rq *rq = cpu_rq(cpu);
+
+	return rq->nr_iowait - atomic_read(&rq->nr_iowait_remote);

The access to rq->nr_iowait is not protected by the runqueue lock and therefore a data race when @cpu is not the current CPU.

This needs to be properly annotated and explained why it does not matter.

So s/Reviewed-by/Un-Reviewed-by/

Though thinking about it some more. Is this split a real benefit over always using the atomic? Do you have numbers to show?

Quoted 12 lines · Thomas Gleixner

So but I just noticed that there is actually an issue with this:

kernel/sched/core.c unsigned long long nr_context_switches(void) · line 5470

Quoted 6 lines

unsigned int nr_iowait_cpu(int cpu)
{

kernel/sched/core.c +3 -1

-	return atomic_read(&cpu_rq(cpu)->nr_iowait);
+	struct rq *rq = cpu_rq(cpu);
+
+	return rq->nr_iowait - atomic_read(&rq->nr_iowait_remote);

The access to rq->nr_iowait is not protected by the runqueue lock and therefore a data race when @cpu is not the current CPU.

Quoted 2 lines

This needs to be properly annotated and explained why it does not matter.

But that was always racy before as well, if someone else is inc/dec'ing ->nr_iowait while it's being read, you could get either the before or after value. This doesn't really change that. I could've sworn I mentioned that in the commit message, but I did not.

Quoted 4 lines

So s/Reviewed-by/Un-Reviewed-by/

Though thinking about it some more. Is this split a real benefit over always using the atomic? Do you have numbers to show?

It was more on Peter's complaint that now we're trading a single atomic for two, hence I got to thinking about nr_iowait in general. I don't have numbers showing it matters, as mentioned in another email the most costly part about this seems to be fetching task->in_iowait and not the actual atomic.

Quoted 13 lines · Jens Axboe

Quoted 12 lines · Thomas Gleixner

So but I just noticed that there is actually an issue with this:

kernel/sched/core.c unsigned long long nr_context_switches(void) · line 5470

Quoted 6 lines

unsigned int nr_iowait_cpu(int cpu)
{

kernel/sched/core.c +3 -1

-	return atomic_read(&cpu_rq(cpu)->nr_iowait);
+	struct rq *rq = cpu_rq(cpu);
+
+	return rq->nr_iowait - atomic_read(&rq->nr_iowait_remote);

The access to rq->nr_iowait is not protected by the runqueue lock and therefore a data race when @cpu is not the current CPU.

Quoted 7 lines

Quoted 2 lines

This needs to be properly annotated and explained why it does not matter.

But that was always racy before as well, if someone else is inc/dec'ing ->nr_iowait while it's being read, you could get either the before or after value. This doesn't really change that. I could've sworn I mentioned that in the commit message, but I did not.

There are actually two issues here:

1) atomic_read() vs. atomic_inc/dec() guarantees that the read value is consistent in itself.

Non-atomic inc/dec is not guaranteeing that the concurrent read is a consistent value as the compiler is free to do store/load tearing. Unlikely but not guaranteed to never happen.

KCSAN will complain about it sooner than later and then someone has to go and do the analysis and the annotation. I rather let you do the reasoning now than chasing you down later :)

2) What's worse is that the result can be completely bogus:

   i.e.

   CPU0                                 CPU1                    CPU2
   a = rq(CPU1)->nr_iowait; // 0
                                        rq->nr_iowait++;
                                                                rq(CPU1)->nr_iowait_remote++;
   b = rq(CPU1)->nr_iowait_remote; // 1

   r = a - b; // -1
   return (unsigned int) r; // UINT_MAX

   The consumers of this interface might be upset. :)

   While with a single atomic_t it's guaranteed that the result is
   always greater or equal zero.

Quoted 10 lines

Quoted 4 lines

So s/Reviewed-by/Un-Reviewed-by/

Though thinking about it some more. Is this split a real benefit over always using the atomic? Do you have numbers to show?

It was more on Peter's complaint that now we're trading a single atomic for two, hence I got to thinking about nr_iowait in general. I don't have numbers showing it matters, as mentioned in another email the most costly part about this seems to be fetching task->in_iowait and not the actual atomic.

On the write side (except for the remote case) the cache line is already dirty on the current CPU and I doubt that the atomic will be noticable. If there is concurrent remote access to the runqueue then the cache line is bouncing no matter what.

On the read side there is always an atomic operation required, so it's not really different.

I assume Peter's complaint was about the extra nr_iowait_acct part. I think that's solvable without the extra atomic_t member and with a single atomic_add()/sub(). atomic_t is 32bit wide, so what about splitting the thing and adding/subtracting both in one go?

While sketching this I noticed that prepare/finish can be written w/o any conditionals.

int io_schedule_prepare(void)

{
	int flags = current->in_iowait + current->in_iowait_acct << 16;

	current->in_iowait = 1;
	current->in_iowait_acct = 1;
	blk_flush_plug(current->plug, true);
	return flags;
}

void io_schedule_finish(int old_wait_flags)

{
	current->in_iowait = flags & 0x01;
        current->in_iowait_acct = flags >> 16;
}

Now __schedule():

	if (prev->in_iowait) {
           	int x = 1 + current->in_iowait_acct << 16;

		atomic_add(x, rq->nr_iowait);
		delayacct_blkio_start();
	}

and ttwu_do_activate():

	if (p->in_iowait) {
        	int x = 1 + current->in_iowait_acct << 16;

                delayacct_blkio_end(p);
                atomic_sub(x, task_rq(p)->nr_iowait);
	}

and try_to_wake_up():

	delayacct_blkio_end(p);

	int x = 1 + current->in_iowait_acct << 16;

	atomic_add(x, task_rq(p)->nr_iowait);

nr_iowait_acct_cpu() becomes:

        return atomic_read(&cpu_rq(cpu)->nr_iowait) >> 16;

and nr_iowait_cpu():

        return atomic_read(&cpu_rq(cpu)->nr_iowait) & ((1 << 16) - 1);

Obviously written with proper inline wrappers and defines, but you get the idea.

Quoted 4 lines · Thomas Gleixner

        return atomic_read(&cpu_rq(cpu)->nr_iowait) & ((1 << 16) - 1);

Obviously written with proper inline wrappers and defines, but you get the idea.

I'll play with this a bit, but do we want to switch to an atomic_long_t for this? 2^16 in iowait seems extreme, but it definitely seems possible to overflow it.

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 we would've 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 we need boosting for short waits, and one that says we need to account the task

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.

kernel/sched/core.c +13 -2

+/*
+ * Returns a token which is comprised of the two bits of iowait wait state -
+ * one is whether we're making ourselves as in iowait for cpufreq reasons,
+ * and the other is if the task should be accounted as such.
+ */
 int io_schedule_prepare(void)
 {
-	int old_iowait = current->in_iowait;
+	int old_wait_flags = 0;
+
+	if (current->in_iowait)
+		old_wait_flags |= TASK_IOWAIT;
+	if (current->in_iowait_acct)
+		old_wait_flags |= TASK_IOWAIT_ACCT;
 
 	current->in_iowait = 1;
+	current->in_iowait_acct = 1;
 	blk_flush_plug(current->plug, true);
-	return old_iowait;
+	return old_wait_flags;

Quoted 7 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 we would've 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 3 lines

Split the iowait part into two parts - one that tracks whether we need boosting for short waits, and one that says we need to account the task

We :)

Quoted 22 lines

kernel/sched/core.c +13 -2

+/*
+ * Returns a token which is comprised of the two bits of iowait wait state -
+ * one is whether we're making ourselves as in iowait for cpufreq reasons,
+ * and the other is if the task should be accounted as such.
+ */
 int io_schedule_prepare(void)
 {
-	int old_iowait = current->in_iowait;
+	int old_wait_flags = 0;
+
+	if (current->in_iowait)
+		old_wait_flags |= TASK_IOWAIT;
+	if (current->in_iowait_acct)
+		old_wait_flags |= TASK_IOWAIT_ACCT;
 
 	current->in_iowait = 1;
+	current->in_iowait_acct = 1;
 	blk_flush_plug(current->plug, true);
-	return old_iowait;
+	return old_wait_flags;
 }

kernel/sched/core.c int __sched yield_to(struct task_struct *p, bool preempt) · line 9029

Quoted 8 lines

kernel/sched/core.c +5 -2

-void io_schedule_finish(int token)
+void io_schedule_finish(int old_wait_flags)
 {
-	current->in_iowait = token;
+	if (!(old_wait_flags & TASK_IOWAIT))
+		current->in_iowait = 0;
+	if (!(old_wait_flags & TASK_IOWAIT_ACCT))
+		current->in_iowait_acct = 0;

Why? TASK_IOWAIT_ACCT requires TASK_IOWAIT, right? So if TASK_IOWAIT was not set then TASK_IOWAIT_ACCT must have been clear too, no?