time/tick-broadcast: Fix tick_broadcast_offline() lockdep complaint

[ Hearing no objections and given no test failures in multiple weeks of
rcutorture testing, I intend to submit this to the upcoming merge window. Thoughts? ]

Some debugging code showed that the culprit was sched_cpu_dying(). It had irqs enabled after return from sched_tick_stop(). Which in turn had irqs enabled after return from cancel_delayed_work_sync(). Which is a wrapper around __cancel_work_timer(). Which can sleep in the case where something else is concurrently trying to cancel the same delayed work, and as Thomas Gleixner pointed out on IRC, sleeping is a decidedly bad idea when you are invoked from take_cpu_down(), regardless of the state you leave interrupts in upon return.

Code inspection located no reason why the delayed work absolutely needed to be canceled from sched_tick_stop(): The work is not bound to the outgoing CPU by design, given that the whole point is to collect statistics without disturbing the outgoing CPU.

This commit therefore simply drops the cancel_delayed_work_sync() from sched_tick_stop(). Instead, a new ->state field is added to the tick_work structure so that the delayed-work handler function sched_tick_remote() can avoid reposting itself. A cpu_is_offline() check is also added to sched_tick_remote() to avoid mucking with the state of an offlined CPU (though it does appear safe to do so). The sched_tick_start() and sched_tick_stop() functions also update ->state, and sched_tick_start() also schedules the delayed work if ->state indicates that it is not already in flight.

Signed-off-by Paul E. McKenney <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Peter Zijlstra <[email protected]>

Reviewed-by Frederic Weisbecker <[email protected]>

kernel/sched/core.c +0 -0

index 102dfcf0a29a..8409c83aa5fa 100644

kernel/sched/core.c +14 -0

+
+// State diagram for ->state:
+//
+//
+//      +----->OFFLINE--------------------------+
+//      |                                       |
+//      |                                       |
+//      |                                       | sched_tick_start()
+//      | sched_tick_remote()                   |
+//      |                                       |
+//      |                                       V
+//      |                        +---------->RUNNING
+//      |                        |              |
+//      |                        |              |

kernel/sched/core.c +24 -6

 	struct tick_work *twork = container_of(dwork, struct tick_work, work);
 	int cpu = twork->cpu;
 	struct rq *rq = cpu_rq(cpu);

@@ -3077,7 +3107,7 @@ static void sched_tick_remote(struct work_struct *work)
 
 	rq_lock_irq(rq, &rf);
 	curr = rq->curr;
-	if (is_idle_task(curr))
+	if (is_idle_task(curr) || cpu_is_offline(cpu))
 		goto out_unlock;
 
 	update_rq_clock(rq);

@@ -3097,13 +3127,22 @@ static void sched_tick_remote(struct work_struct *work)
 	/*
 	 * Run the remote tick once per second (1Hz). This arbitrary
 	 * frequency is large enough to avoid overload but short enough
-	 * to keep scheduler internal stats reasonably up to date.
+	 * to keep scheduler internal stats reasonably up to date.  But
+	 * first update state to reflect hotplug activity if required.
 	 */
-	queue_delayed_work(system_unbound_wq, dwork, HZ);
+	do {
+		os = READ_ONCE(twork->state);
+		WARN_ON_ONCE(os == TICK_SCHED_REMOTE_OFFLINE);
+		if (os == TICK_SCHED_REMOTE_RUNNING)
+			break;
+	} while (cmpxchg(&twork->state, os, TICK_SCHED_REMOTE_OFFLINE) != os);
+	if (os == TICK_SCHED_REMOTE_RUNNING)
+		queue_delayed_work(system_unbound_wq, dwork, HZ);
 }
 
 static void sched_tick_start(int cpu)
 {
+	int os;
 	struct tick_work *twork;
 
 	if (housekeeping_cpu(cpu, HK_FLAG_TICK))

@@ -3112,14 +3151,23 @@ static void sched_tick_start(int cpu)
 	WARN_ON_ONCE(!tick_work_cpu);
 
 	twork = per_cpu_ptr(tick_work_cpu, cpu);
-	twork->cpu = cpu;
-	INIT_DELAYED_WORK(&twork->work, sched_tick_remote);
-	queue_delayed_work(system_unbound_wq, &twork->work, HZ);
+	do {
+		os = READ_ONCE(twork->state);
+		if (WARN_ON_ONCE(os == TICK_SCHED_REMOTE_RUNNING))
+			break;
+		// Either idle or offline for a short period
+	} while (cmpxchg(&twork->state, os, TICK_SCHED_REMOTE_RUNNING) != os);
+	if (os == TICK_SCHED_REMOTE_OFFLINE) {
+		twork->cpu = cpu;
+		INIT_DELAYED_WORK(&twork->work, sched_tick_remote);
+		queue_delayed_work(system_unbound_wq, &twork->work, HZ);
+	}
 }
 
 #ifdef CONFIG_HOTPLUG_CPU
 static void sched_tick_stop(int cpu)
 {
+	int os;
 	struct tick_work *twork;
 
 	if (housekeeping_cpu(cpu, HK_FLAG_TICK))

@@ -3128,7 +3176,13 @@ static void sched_tick_stop(int cpu)
 	WARN_ON_ONCE(!tick_work_cpu);
 
 	twork = per_cpu_ptr(tick_work_cpu, cpu);

Quoted 3 lines · Paul E. McKenney

[ Hearing no objections and given no test failures in multiple weeks of
rcutorture testing, I intend to submit this to the upcoming merge window. Thoughts? ]

I can't remember seeing this before; but then, there's a ton of unread email in the inbox :-(

Quoted 35 lines

Some debugging code showed that the culprit was sched_cpu_dying(). It had irqs enabled after return from sched_tick_stop(). Which in turn had irqs enabled after return from cancel_delayed_work_sync(). Which is a wrapper around __cancel_work_timer(). Which can sleep in the case where something else is concurrently trying to cancel the same delayed work, and as Thomas Gleixner pointed out on IRC, sleeping is a decidedly bad idea when you are invoked from take_cpu_down(), regardless of the state you leave interrupts in upon return.

Code inspection located no reason why the delayed work absolutely needed to be canceled from sched_tick_stop(): The work is not bound to the outgoing CPU by design, given that the whole point is to collect statistics without disturbing the outgoing CPU.

This commit therefore simply drops the cancel_delayed_work_sync() from sched_tick_stop(). Instead, a new ->state field is added to the tick_work structure so that the delayed-work handler function sched_tick_remote() can avoid reposting itself. A cpu_is_offline() check is also added to sched_tick_remote() to avoid mucking with the state of an offlined CPU (though it does appear safe to do so). The sched_tick_start() and sched_tick_stop() functions also update ->state, and sched_tick_start() also schedules the delayed work if ->state indicates that it is not already in flight.

Signed-off-by Paul E. McKenney <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Peter Zijlstra <[email protected]>

Reviewed-by Frederic Weisbecker <[email protected]>

kernel/sched/core.c +0 -0

index 102dfcf0a29a..8409c83aa5fa 100644
@@ -3050,14 +3050,44 @@ void scheduler_tick(void)
 

kernel/sched/core.c void scheduler_tick(void) · line 3059

Quoted 9 lines

kernel/sched/core.c +5 -0

 struct tick_work {
 	int			cpu;
+	int			state;
 	struct delayed_work	work;
 };
+// Values for ->state, see diagram below.
+#define TICK_SCHED_REMOTE_OFFLINE	0
+#define TICK_SCHED_REMOTE_RUNNING	1
+#define TICK_SCHED_REMOTE_OFFLINING	2

That seems a daft set of values; consider { RUNNING, OFFLINING, OFFLINE } and see below.

Quoted 18 lines

kernel/sched/core.c +18 -0

+
+// State diagram for ->state:
+//
+//
+//      +----->OFFLINE--------------------------+
+//      |                                       |
+//      |                                       |
+//      |                                       | sched_tick_start()
+//      | sched_tick_remote()                   |
+//      |                                       |
+//      |                                       V
+//      |                        +---------->RUNNING
+//      |                        |              |
+//      |                        |              |
+//      |                        |              |
+//      |     sched_tick_start() |              | sched_tick_stop()
+//      |                        |              |
+//      |                        |              |

kernel/sched/core.c void scheduler_tick(void) · line 3083

Quoted 6 lines

kernel/sched/core.c +6 -0

+//      |                        |              |
+//      +--------------------OFFLINING<---------+
+//
+//
+// Other transitions get WARN_ON_ONCE(), except that sched_tick_remote()
+// and sched_tick_start() are happy to leave the state in RUNNING.

Can we please stick to old skool C comments?

Also, I find it harder to read that needed, maybe a little something like so:

/*
* OFFLINE

 *               |   ^
 *               |   | tick_remote()
 *               |   |
 *               +--OFFLINING
 *               |   ^
 *  tick_start() |   | tick_stop()
 *               v   |

* RUNNING
*/

kernel/sched/core.c void scheduler_tick(void) · line 3090

Quoted 6 lines

kernel/sched/core.c +1 -0

 static struct tick_work __percpu *tick_work_cpu;
 
 static void sched_tick_remote(struct work_struct *work)
 {
 	struct delayed_work *dwork = to_delayed_work(work);
+	int os;

this should go at the end, reverse xmas tree preference and all that.

Quoted 68 lines

kernel/sched/core.c +24 -6

 	struct tick_work *twork = container_of(dwork, struct tick_work, work);
 	int cpu = twork->cpu;
 	struct rq *rq = cpu_rq(cpu);

@@ -3077,7 +3107,7 @@ static void sched_tick_remote(struct work_struct *work)
 
 	rq_lock_irq(rq, &rf);
 	curr = rq->curr;
-	if (is_idle_task(curr))
+	if (is_idle_task(curr) || cpu_is_offline(cpu))
 		goto out_unlock;
 
 	update_rq_clock(rq);

@@ -3097,13 +3127,22 @@ static void sched_tick_remote(struct work_struct *work)
 	/*
 	 * Run the remote tick once per second (1Hz). This arbitrary
 	 * frequency is large enough to avoid overload but short enough
-	 * to keep scheduler internal stats reasonably up to date.
+	 * to keep scheduler internal stats reasonably up to date.  But
+	 * first update state to reflect hotplug activity if required.
 	 */
-	queue_delayed_work(system_unbound_wq, dwork, HZ);
+	do {
+		os = READ_ONCE(twork->state);
+		WARN_ON_ONCE(os == TICK_SCHED_REMOTE_OFFLINE);
+		if (os == TICK_SCHED_REMOTE_RUNNING)
+			break;
+	} while (cmpxchg(&twork->state, os, TICK_SCHED_REMOTE_OFFLINE) != os);
+	if (os == TICK_SCHED_REMOTE_RUNNING)
+		queue_delayed_work(system_unbound_wq, dwork, HZ);
 }
 
 static void sched_tick_start(int cpu)
 {
+	int os;
 	struct tick_work *twork;
 
 	if (housekeeping_cpu(cpu, HK_FLAG_TICK))

@@ -3112,14 +3151,23 @@ static void sched_tick_start(int cpu)
 	WARN_ON_ONCE(!tick_work_cpu);
 
 	twork = per_cpu_ptr(tick_work_cpu, cpu);
-	twork->cpu = cpu;
-	INIT_DELAYED_WORK(&twork->work, sched_tick_remote);
-	queue_delayed_work(system_unbound_wq, &twork->work, HZ);
+	do {
+		os = READ_ONCE(twork->state);
+		if (WARN_ON_ONCE(os == TICK_SCHED_REMOTE_RUNNING))
+			break;
+		// Either idle or offline for a short period
+	} while (cmpxchg(&twork->state, os, TICK_SCHED_REMOTE_RUNNING) != os);
+	if (os == TICK_SCHED_REMOTE_OFFLINE) {
+		twork->cpu = cpu;
+		INIT_DELAYED_WORK(&twork->work, sched_tick_remote);
+		queue_delayed_work(system_unbound_wq, &twork->work, HZ);
+	}
 }
 
 #ifdef CONFIG_HOTPLUG_CPU
 static void sched_tick_stop(int cpu)
 {
+	int os;
 	struct tick_work *twork;
 
 	if (housekeeping_cpu(cpu, HK_FLAG_TICK))

@@ -3128,7 +3176,13 @@ static void sched_tick_stop(int cpu)
 	WARN_ON_ONCE(!tick_work_cpu);
 
 	twork = per_cpu_ptr(tick_work_cpu, cpu);

kernel/sched/core.c static void sched_tick_stop(int cpu) · line 3187

Quoted 10 lines

kernel/sched/core.c +7 -1

-	cancel_delayed_work_sync(&twork->work);
+	// There cannot be competing actions, but don't rely on stop_machine.
+	do {
+		os = READ_ONCE(twork->state);
+		WARN_ON_ONCE(os != TICK_SCHED_REMOTE_RUNNING);
+		// Either idle or offline for a short period
+	} while (cmpxchg(&twork->state, os, TICK_SCHED_REMOTE_OFFLINING) != os);
+	// Don't cancel, as this would mess up the state machine.
 }
 #endif /* CONFIG_HOTPLUG_CPU */

While not wrong, it seems overly complicated; can't we do something like:

tick:

	state = atomic_read(->state);
	if (state) {

		WARN_ON_ONCE(state != OFFLINING);
		if (atomic_inc_not_zero(->state))

			return;
	}
	queue_delayed_work();

stop:

	/*
	 * This is hotplug; even without stop-machine, there cannot be
	 * concurrency on offlining specific CPUs.
	 */
	state = atomic_read(->state);

	WARN_ON_ONCE(state != RUNNING);
	atomic_set(->state, OFFLINING);

start:

	state = atomic_xchg(->state, RUNNING);
	WARN_ON_ONCE(state == RUNNING);
	if (state == OFFLINE) {
		// ...
		queue_delayed_work();
	}

Quoted 6 lines

kernel/sched/core.c void scheduler_tick(void) · line 3059

Quoted 3 lines

kernel/sched/core.c +3 -0

+#define TICK_SCHED_REMOTE_OFFLINE	0
+#define TICK_SCHED_REMOTE_RUNNING	1
+#define TICK_SCHED_REMOTE_OFFLINING	2

That seems a daft set of values; consider { RUNNING, OFFLINING, OFFLINE } and see below.

As in make it an enum? I could do that.

Quoted 37 lines

Quoted 14 lines

kernel/sched/core.c +14 -0

+
+// State diagram for ->state:
+//
+//
+//      +----->OFFLINE--------------------------+
+//      |                                       |
+//      |                                       |
+//      |                                       | sched_tick_start()
+//      | sched_tick_remote()                   |
+//      |                                       |
+//      |                                       V
+//      |                        +---------->RUNNING
+//      |                        |              |
+//      |                        |              |

kernel/sched/core.c void scheduler_tick(void) · line 3083

Quoted 10 lines

kernel/sched/core.c +10 -0

+//      |                        |              |
+//      |     sched_tick_start() |              | sched_tick_stop()
+//      |                        |              |
+//      |                        |              |
+//      |                        |              |
+//      +--------------------OFFLINING<---------+
+//
+//
+// Other transitions get WARN_ON_ONCE(), except that sched_tick_remote()
+// and sched_tick_start() are happy to leave the state in RUNNING.

Also, I find it harder to read that needed, maybe a little something like so:

/*
* OFFLINE

 *               |   ^
 *               |   | tick_remote()
 *               |   |
 *               +--OFFLINING
 *               |   ^
 *  tick_start() |   | tick_stop()
 *               v   |

Quoted 2 lines

* RUNNING
*/

As in remove the leading "sched_" from the function names? (The names were already there, so I left them be.)

Quoted 4 lines

While not wrong, it seems overly complicated; can't we do something like:

tick:

As in sched_tick_remote(), right?

Quoted 2 lines

	state = atomic_read(->state);
	if (state) {

You sure you don't want "if (state != RUNNING)"? But I guess you need to understand that RUNNING==0 to understand the atomic_inc_not_zero().

Quoted 3 lines

		WARN_ON_ONCE(state != OFFLINING);
		if (atomic_inc_not_zero(->state))

This assumes that there cannot be concurrent calls to sched_tick_remote(), otherwise, you can end up with ->state==3. Which is a situation that my version does a WARN_ON_ONCE() for, so I guess the only difference is that mine would be guaranteed to complain and yours would complain with high probability. So fair enough!

Quoted 11 lines

			return;
	}
	queue_delayed_work();

stop:

	/*
	 * This is hotplug; even without stop-machine, there cannot be
	 * concurrency on offlining specific CPUs.
	 */
	state = atomic_read(->state);

There cannot be a sched_tick_stop() or sched_tick_stop(), but there really can be a sched_tick_remote() right here in the absence of stop-machine, can't there? Or am I missing something other than stop-machine that prevents this?

Now, you could argue that concurrency is safe: Either sched_tick_remote() sees RUNNING and doesn't touch the value, or it sees offlining and sched_tick_stop() makes no further changes,

but I am not sure that this qualifies as simpler...

Quoted 2 lines

	WARN_ON_ONCE(state != RUNNING);
	atomic_set(->state, OFFLINING);

Another option would be to use atomic_xchg() as below instead of the atomic_read()/atomic_set() pair. Would that work for you?

Quoted 7 lines

start:

	state = atomic_xchg(->state, RUNNING);
	WARN_ON_ONCE(state == RUNNING);
	if (state == OFFLINE) {
		// ...
		queue_delayed_work();
	}

This one looks to be an improvement on mine regardless of the other two.

Quoted 7 lines · Paul E. McKenney

Quoted 6 lines

Review

Quoted 3 lines

(quoted diff) +3 -0

+#define TICK_SCHED_REMOTE_OFFLINE	0
+#define TICK_SCHED_REMOTE_RUNNING	1
+#define TICK_SCHED_REMOTE_OFFLINING	2

That seems a daft set of values; consider { RUNNING, OFFLINING, OFFLINE } and see below.

Quoted 1 line

As in make it an enum? I could do that.

Enum or define, I don't much care, but the 'natural' ordering of the states is either: running -> offlining -> offline, or the other way around, the given order in the patch just didn't make sense.

The one with running=0 just seems to work out nicer.

Quoted 40 lines

Quoted 39 lines

Quoted 14 lines

(quoted diff) +14 -0

+
+// State diagram for ->state:
+//
+//
+//      +----->OFFLINE--------------------------+
+//      |                                       |
+//      |                                       |
+//      |                                       | sched_tick_start()
+//      | sched_tick_remote()                   |
+//      |                                       |
+//      |                                       V
+//      |                        +---------->RUNNING
+//      |                        |              |
+//      |                        |              |

Review

Quoted 10 lines

(quoted diff) +10 -0

+//      |                        |              |
+//      |     sched_tick_start() |              | sched_tick_stop()
+//      |                        |              |
+//      |                        |              |
+//      |                        |              |
+//      +--------------------OFFLINING<---------+
+//
+//
+// Other transitions get WARN_ON_ONCE(), except that sched_tick_remote()
+// and sched_tick_start() are happy to leave the state in RUNNING.

Also, I find it harder to read that needed, maybe a little something like so:

/*
* OFFLINE

 *               |   ^
 *               |   | tick_remote()
 *               |   |
 *               +--OFFLINING
 *               |   ^
 *  tick_start() |   | tick_stop()
 *               v   |

* RUNNING
*/

Quoted 2 lines

As in remove the leading "sched_" from the function names? (The names were already there, so I left them be.)

That was just me being lazy, the main part being getting the states in a linear order, instead of spread around a 2d grid.

Quoted 5 lines

Quoted 4 lines

While not wrong, it seems overly complicated; can't we do something like:

tick:

Quoted 7 lines

As in sched_tick_remote(), right?

Quoted 2 lines

	state = atomic_read(->state);
	if (state) {

You sure you don't want "if (state != RUNNING)"? But I guess you need to understand that RUNNING==0 to understand the atomic_inc_not_zero().

Right..

Quoted 9 lines

Quoted 3 lines

		WARN_ON_ONCE(state != OFFLINING);
		if (atomic_inc_not_zero(->state))

This assumes that there cannot be concurrent calls to sched_tick_remote(), otherwise, you can end up with ->state==3. Which is a situation that my version does a WARN_ON_ONCE() for, so I guess the only difference is that mine would be guaranteed to complain and yours would complain with high probability. So fair enough!

I was assuming there was only a single work per CPU and there'd not be concurrency on this path.

Quoted 12 lines

Quoted 11 lines

			return;
	}
	queue_delayed_work();

stop:

	/*
	 * This is hotplug; even without stop-machine, there cannot be
	 * concurrency on offlining specific CPUs.
	 */
	state = atomic_read(->state);

Quoted 4 lines

There cannot be a sched_tick_stop() or sched_tick_stop(), but there really can be a sched_tick_remote() right here in the absence of stop-machine, can't there? Or am I missing something other than stop-machine that prevents this?

There can be a remote tick, indeed.

Quoted 3 lines

Now, you could argue that concurrency is safe: Either sched_tick_remote() sees RUNNING and doesn't touch the value, or it sees offlining and sched_tick_stop() makes no further changes,

That was indeed the thinking.

Quoted 1 line

but I am not sure that this qualifies as simpler...

There is that I suppose. I think my initial version was a little more complicated, but after a few passes this happened :-)

Quoted 5 lines

Quoted 2 lines

	WARN_ON_ONCE(state != RUNNING);
	atomic_set(->state, OFFLINING);

Another option would be to use atomic_xchg() as below instead of the atomic_read()/atomic_set() pair. Would that work for you?

Yes, that works I suppose. Is more expensive, but I don't think we particularly care about that here.

Quoted 9 lines

Quoted 7 lines

start:

	state = atomic_xchg(->state, RUNNING);
	WARN_ON_ONCE(state == RUNNING);
	if (state == OFFLINE) {
		// ...
		queue_delayed_work();
	}

This one looks to be an improvement on mine regardless of the other two.

So how about the following patch, which passes very light rcutorture testing but should otherwise be regarded as being under suspicion?

Quoted 2 lines · Paul E. McKenney

So how about the following patch, which passes very light rcutorture testing but should otherwise be regarded as being under suspicion?

Looks good to me,

Acked-by Peter Zijlstra (Intel) <[email protected]>

Or, if you want me to apply it, I can do that too ;-)

A pair of full hangs at boot (TASKS03 and TREE04), no console output whatsoever. Not sure how these changes could cause that, but suspicion falls on sched_tick_offload_init(). Though even that is a bit strange because if so, why didn't TREE01 and TREE07 also hang? Again, looking into it.

Quoted 5 lines · Paul E. McKenney

A pair of full hangs at boot (TASKS03 and TREE04), no console output whatsoever. Not sure how these changes could cause that, but suspicion falls on sched_tick_offload_init(). Though even that is a bit strange because if so, why didn't TREE01 and TREE07 also hang? Again, looking into it.

Pesky details ;-)

Quoted 7 lines · Peter Zijlstra

Quoted 5 lines · Paul E. McKenney

A pair of full hangs at boot (TASKS03 and TREE04), no console output whatsoever. Not sure how these changes could cause that, but suspicion falls on sched_tick_offload_init(). Though even that is a bit strange because if so, why didn't TREE01 and TREE07 also hang? Again, looking into it.

Quoted 1 line

Pesky details ;-)

And backing out to the earlier patch removes the hangs, though statistical insignificance and all that.

Ah, in answer to your earlier question, if you want it in v5.3, you will need to take it (but I do humbly request that you wait until it actually works). If you don't take it, I won't be submitting it earlier than v5.4. Either way, your choice!

Thanx, Paul

Quoted 8 lines · Paul E. McKenney

Quoted 7 lines · Peter Zijlstra

Quoted 5 lines · Paul E. McKenney

A pair of full hangs at boot (TASKS03 and TREE04), no console output whatsoever. Not sure how these changes could cause that, but suspicion falls on sched_tick_offload_init(). Though even that is a bit strange because if so, why didn't TREE01 and TREE07 also hang? Again, looking into it.

Quoted 4 lines

Quoted 1 line

Pesky details ;-)

And backing out to the earlier patch removes the hangs, though statistical insignificance and all that.

And purists might argue that four failures out of four attempts does not constitute true statistical significance, but too bad. If I interpose a twork pointer in sched_tick_offload_init()'s initialization, it seems to work fine, give or take lack of statistical significance. This is surprising, so I am rerunning with added parentheses in the atomic_set() expression.

Quoted 14 lines · Paul E. McKenney

Quoted 8 lines · Paul E. McKenney

Quoted 7 lines · Peter Zijlstra

Quoted 5 lines · Paul E. McKenney

A pair of full hangs at boot (TASKS03 and TREE04), no console output whatsoever. Not sure how these changes could cause that, but suspicion falls on sched_tick_offload_init(). Though even that is a bit strange because if so, why didn't TREE01 and TREE07 also hang? Again, looking into it.

Quoted 4 lines

Quoted 1 line

Pesky details ;-)

And backing out to the earlier patch removes the hangs, though statistical insignificance and all that.

Quoted 6 lines

And purists might argue that four failures out of four attempts does not constitute true statistical significance, but too bad. If I interpose a twork pointer in sched_tick_offload_init()'s initialization, it seems to work fine, give or take lack of statistical significance. This is surprising, so I am rerunning with added parentheses in the atomic_set() expression.

Huh. This works, albeit only once:

	int __init sched_tick_offload_init(void)
	{
		struct tick_work *twork;
		int cpu;

		tick_work_cpu = alloc_percpu(struct tick_work);
		BUG_ON(!tick_work_cpu);
		for_each_possible_cpu(cpu) {
			twork = per_cpu_ptr(tick_work_cpu, cpu);
			atomic_set(&twork->state, TICK_SCHED_REMOTE_OFFLINE);
		}

		return 0;
	}

This does not work:

	int __init sched_tick_offload_init(void)
	{
		int cpu;

		tick_work_cpu = alloc_percpu(struct tick_work);
		BUG_ON(!tick_work_cpu);
		for_each_possible_cpu(cpu)
			atomic_set(&(per_cpu(tick_work_cpu, cpu)->state), TICK_SCHED_REMOTE_OFFLINE);

		return 0;
	}

I will run more tests on the one that worked only once. In the meantime, feel free to tell me what stupid thing I did with the parentheses.

kernel/sched/core.c +2 -0

+	}
 	queue_delayed_work(system_unbound_wq, dwork, HZ);
 }
 
 static void sched_tick_start(int cpu)
 {
+	int os;
 	struct tick_work *twork;
 
 	if (housekeeping_cpu(cpu, HK_FLAG_TICK))

kernel/sched/core.c static void sched_tick_remote(struct work_struct *work) · line 3136

Quoted 13 lines · Paul E. McKenney

kernel/sched/core.c +7 -1

@@ -3097,13 +3126,21 @@ static void sched_tick_remote(struct work_struct *work)
 	/*
 	 * Run the remote tick once per second (1Hz). This arbitrary
 	 * frequency is large enough to avoid overload but short enough
-	 * to keep scheduler internal stats reasonably up to date.
+	 * to keep scheduler internal stats reasonably up to date.  But
+	 * first update state to reflect hotplug activity if required.
 	 */
+	os = atomic_read(&twork->state);
+	if (os) {
+		WARN_ON_ONCE(os != TICK_SCHED_REMOTE_OFFLINING);
+		if (atomic_inc_not_zero(&twork->state))
+			return;

Using inc makes me a bit nervous here. If we do so, we should somewhow make sure that we never exceed a value higher than TICK_SCHED_REMOTE_OFFLINE by accident.

atomic_xchg() is probably a bit costlier but also safer as it allows us to check both the old and the new value. That path shouldn't be critically fast after all.

Quoted 13 lines

kernel/sched/core.c +2 -0

+	}
 	queue_delayed_work(system_unbound_wq, dwork, HZ);
 }
 
 static void sched_tick_start(int cpu)
 {
+	int os;
 	struct tick_work *twork;
 
 	if (housekeeping_cpu(cpu, HK_FLAG_TICK))

@@ -3112,15 +3149,20 @@ static void sched_tick_start(int cpu)
 	WARN_ON_ONCE(!tick_work_cpu);
 

kernel/sched/core.c static void sched_tick_start(int cpu) · line 3153

Quoted 6 lines

kernel/sched/core.c +2 -3

 	twork = per_cpu_ptr(tick_work_cpu, cpu);
-	twork->cpu = cpu;
-	INIT_DELAYED_WORK(&twork->work, sched_tick_remote);
-	queue_delayed_work(system_unbound_wq, &twork->work, HZ);
+	os = atomic_xchg(&twork->state, TICK_SCHED_REMOTE_RUNNING);
+	WARN_ON_ONCE(os == TICK_SCHED_REMOTE_RUNNING);

See if we use atomic_inc(), we would need to also WARN(os > TICK_SCHED_REMOTE_OFFLINE).

kernel/sched/core.c static void sched_tick_start(int cpu) · line 3159

Quoted 6 lines

kernel/sched/core.c +5 -0

+	if (os == TICK_SCHED_REMOTE_OFFLINE) {
+		twork->cpu = cpu;
+		INIT_DELAYED_WORK(&twork->work, sched_tick_remote);
+		queue_delayed_work(system_unbound_wq, &twork->work, HZ);
+	}
 }

Thanks.

Quoted 19 lines · Frederic Weisbecker

kernel/sched/core.c static void sched_tick_remote(struct work_struct *work) · line 3136

Quoted 13 lines · Paul E. McKenney

kernel/sched/core.c +7 -1

@@ -3097,13 +3126,21 @@ static void sched_tick_remote(struct work_struct *work)
 	/*
 	 * Run the remote tick once per second (1Hz). This arbitrary
 	 * frequency is large enough to avoid overload but short enough
-	 * to keep scheduler internal stats reasonably up to date.
+	 * to keep scheduler internal stats reasonably up to date.  But
+	 * first update state to reflect hotplug activity if required.
 	 */
+	os = atomic_read(&twork->state);
+	if (os) {
+		WARN_ON_ONCE(os != TICK_SCHED_REMOTE_OFFLINING);
+		if (atomic_inc_not_zero(&twork->state))
+			return;

Using inc makes me a bit nervous here. If we do so, we should somewhow make sure that we never exceed a value higher than TICK_SCHED_REMOTE_OFFLINE by accident.

Quoted 3 lines

atomic_xchg() is probably a bit costlier but also safer as it allows us to check both the old and the new value. That path shouldn't be critically fast after all.

It would need to be cmpxchg() to avoid messing with the state if the state were somehow TICK_SCHED_REMOTE_RUNNING, right?

Quoted 13 lines

Quoted 13 lines

kernel/sched/core.c +2 -0

+	}
 	queue_delayed_work(system_unbound_wq, dwork, HZ);
 }
 
 static void sched_tick_start(int cpu)
 {
+	int os;
 	struct tick_work *twork;
 
 	if (housekeeping_cpu(cpu, HK_FLAG_TICK))

@@ -3112,15 +3149,20 @@ static void sched_tick_start(int cpu)
 	WARN_ON_ONCE(!tick_work_cpu);
 

Quoted 8 lines

kernel/sched/core.c static void sched_tick_start(int cpu) · line 3153

Quoted 6 lines

kernel/sched/core.c +2 -3

 	twork = per_cpu_ptr(tick_work_cpu, cpu);
-	twork->cpu = cpu;
-	INIT_DELAYED_WORK(&twork->work, sched_tick_remote);
-	queue_delayed_work(system_unbound_wq, &twork->work, HZ);
+	os = atomic_xchg(&twork->state, TICK_SCHED_REMOTE_RUNNING);
+	WARN_ON_ONCE(os == TICK_SCHED_REMOTE_RUNNING);

See if we use atomic_inc(), we would need to also WARN(os > TICK_SCHED_REMOTE_OFFLINE).

How about if I put that WARN() between the atomic_inc_not_zero() and the return, presumably also adding braces?

Quoted 20 lines · Paul E. McKenney

Quoted 19 lines · Frederic Weisbecker

kernel/sched/core.c static void sched_tick_remote(struct work_struct *work) · line 3136

Quoted 13 lines · Paul E. McKenney

kernel/sched/core.c +7 -1

@@ -3097,13 +3126,21 @@ static void sched_tick_remote(struct work_struct *work)
 	/*
 	 * Run the remote tick once per second (1Hz). This arbitrary
 	 * frequency is large enough to avoid overload but short enough
-	 * to keep scheduler internal stats reasonably up to date.
+	 * to keep scheduler internal stats reasonably up to date.  But
+	 * first update state to reflect hotplug activity if required.
 	 */
+	os = atomic_read(&twork->state);
+	if (os) {
+		WARN_ON_ONCE(os != TICK_SCHED_REMOTE_OFFLINING);
+		if (atomic_inc_not_zero(&twork->state))
+			return;

Using inc makes me a bit nervous here. If we do so, we should somewhow make sure that we never exceed a value higher than TICK_SCHED_REMOTE_OFFLINE by accident.

Quoted 6 lines

Quoted 3 lines

atomic_xchg() is probably a bit costlier but also safer as it allows us to check both the old and the new value. That path shouldn't be critically fast after all.

It would need to be cmpxchg() to avoid messing with the state if the state were somehow TICK_SCHED_REMOTE_RUNNING, right?

Ah indeed! Nevermind, let's keep things as they are then.

Quoted 22 lines

Quoted 21 lines

Quoted 11 lines

kernel/sched/core.c +2 -0

+	}
 	queue_delayed_work(system_unbound_wq, dwork, HZ);
 }
 
 static void sched_tick_start(int cpu)
 {
+	int os;
 	struct tick_work *twork;
 
 	if (housekeeping_cpu(cpu, HK_FLAG_TICK))

kernel/sched/core.c static void sched_tick_start(int cpu) · line 3153

Quoted 8 lines

kernel/sched/core.c +2 -3

@@ -3112,15 +3149,20 @@ static void sched_tick_start(int cpu)
 	WARN_ON_ONCE(!tick_work_cpu);
 
 	twork = per_cpu_ptr(tick_work_cpu, cpu);
-	twork->cpu = cpu;
-	INIT_DELAYED_WORK(&twork->work, sched_tick_remote);
-	queue_delayed_work(system_unbound_wq, &twork->work, HZ);
+	os = atomic_xchg(&twork->state, TICK_SCHED_REMOTE_RUNNING);
+	WARN_ON_ONCE(os == TICK_SCHED_REMOTE_RUNNING);

See if we use atomic_inc(), we would need to also WARN(os > TICK_SCHED_REMOTE_OFFLINE).

Quoted 2 lines

How about if I put that WARN() between the atomic_inc_not_zero() and the return, presumably also adding braces?

Yeah, unfortunately there is no atomic_add_not_zero_return().
I guess we can live with a check using atomic_read(). In the best case it returns the fresh increment, otherwise it should be REMOTE_RUNNING.

In any case the (os > TICK_SCHED_REMOTE_OFFLINE) check applies.

Yeah, unfortunately there is no atomic_add_not_zero_return().

Quoted 1 line · Frederic Weisbecker

Yeah, unfortunately there is no atomic_add_not_zero_return().

There is atomic_fetch_add_unless().

Quoted 4 lines · Peter Zijlstra

Quoted 1 line · Frederic Weisbecker

Yeah, unfortunately there is no atomic_add_not_zero_return().

There is atomic_fetch_add_unless().

Ah, that could work!

And it allows dispensing with the initialization. How about like the following?

Quoted 2 lines · Paul E. McKenney

And it allows dispensing with the initialization. How about like the following?

Looks good to me!

Quoted 5 lines · Peter Zijlstra

Quoted 2 lines · Paul E. McKenney

And it allows dispensing with the initialization. How about like the following?

Looks good to me!

Limited rcutorture testing looking good thus far. Here is hoping!

Frederic, you OK with this approach?

Quoted 5 lines · Paul E. McKenney

Quoted 4 lines · Peter Zijlstra

Quoted 2 lines · Paul E. McKenney

And it allows dispensing with the initialization. How about like the following?

Quoted 5 lines

Quoted 1 line

Looks good to me!

Limited rcutorture testing looking good thus far. Here is hoping!

Frederic, you OK with this approach?

Yep, all good!

Very well, the commit is as shown below. This is on current -rcu, but feel free to take it if you would like, Peter. Just let me know and I will mark it so that I don't push it myself. (I need to keep it in -rcu until I rebase onto a version of mainline that contains it so as to avoid spurious rcutorture failures.)

Quoted 5 lines · Paul E. McKenney

Very well, the commit is as shown below. This is on current -rcu, but feel free to take it if you would like, Peter. Just let me know and I will mark it so that I don't push it myself. (I need to keep it in -rcu until I rebase onto a version of mainline that contains it so as to avoid spurious rcutorture failures.)

Looks good. I'll pick it up and then we need to take care when all lands in tip.