Add comments to aid in safer usage of swake_up.

The behavior of swake_up() differs from that of wake_up(), and from the swake_up() that came from RT linux. A memory barrier, or some other synchronization, is needed prior to a swake_up so that the waiter sees the condition set by the waker, and so that the waker does not see an empty wait list.

Quoted 5 lines · Krister Johansen

The behavior of swake_up() differs from that of wake_up(), and from the swake_up() that came from RT linux. A memory barrier, or some other synchronization, is needed prior to a swake_up so that the waiter sees the condition set by the waker, and so that the waker does not see an empty wait list.

Urgh.. let me stare at that. But it sounds like the wrong solution since we wanted to keep the wait and swait APIs as close as possible.

Quoted 7 lines · Peter Zijlstra

Quoted 5 lines · Krister Johansen

The behavior of swake_up() differs from that of wake_up(), and from the swake_up() that came from RT linux. A memory barrier, or some other synchronization, is needed prior to a swake_up so that the waiter sees the condition set by the waker, and so that the waker does not see an empty wait list.

Quoted 2 lines

Urgh.. let me stare at that. But it sounds like the wrong solution since we wanted to keep the wait and swait APIs as close as possible.

But don't they both need some sort of ordering, be it memory barriers or locking, to handle the case where the wait/swait doesn't actually sleep?

Quoted 8 lines · On Fri, 9 Jun 2017 05:45:54 -0700 "Paul E. McKenney"…

Quoted 7 lines · Peter Zijlstra

Quoted 5 lines · Krister Johansen

The behavior of swake_up() differs from that of wake_up(), and from the swake_up() that came from RT linux. A memory barrier, or some other synchronization, is needed prior to a swake_up so that the waiter sees the condition set by the waker, and so that the waker does not see an empty wait list.

Quoted 6 lines

Quoted 2 lines

Urgh.. let me stare at that. But it sounds like the wrong solution since we wanted to keep the wait and swait APIs as close as possible.

But don't they both need some sort of ordering, be it memory barriers or locking, to handle the case where the wait/swait doesn't actually sleep?

Looking at an RCU example, and assuming that ordering can move around within a spin lock, and that changes can leak into a spin lock region from both before and after. Could we have:

(looking at __call_rcu_core() and rcu_gp_kthread()

	CPU0				CPU1
	----				----
				__call_rcu_core() {

				 spin_lock(rnp_root)
				 need_wake = __rcu_start_gp() {
				  rcu_start_gp_advanced() {
				   gp_flags = FLAG_INIT
				  }
				 }

 rcu_gp_kthread() {
   swait_event_interruptible(wq,
	gp_flags & FLAG_INIT) {
   spin_lock(q->lock)

				*fetch wq->task_list here! *

   list_add(wq->task_list, q->task_list)
   spin_unlock(q->lock);

   *fetch old value of gp_flags here *

				 spin_unlock(rnp_root)

				 rcu_gp_kthread_wake() {
				  swake_up(wq) {
				   swait_active(wq) {
				    list_empty(wq->task_list)

				   } * return false *

  if (condition) * false *
    schedule();

Looks like a memory barrier is missing. Perhaps we should slap on into swait_active()? I don't think it is wise to let users add there own, as I think we currently have bugs now.

Quoted 45 lines · Steven Rostedt

Quoted 8 lines · On Fri, 9 Jun 2017 05:45:54 -0700 "Paul E. McKenney"…

Quoted 7 lines · Peter Zijlstra

Quoted 5 lines · Krister Johansen

The behavior of swake_up() differs from that of wake_up(), and from the swake_up() that came from RT linux. A memory barrier, or some other synchronization, is needed prior to a swake_up so that the waiter sees the condition set by the waker, and so that the waker does not see an empty wait list.

Quoted 6 lines

Quoted 2 lines

Urgh.. let me stare at that. But it sounds like the wrong solution since we wanted to keep the wait and swait APIs as close as possible.

But don't they both need some sort of ordering, be it memory barriers or locking, to handle the case where the wait/swait doesn't actually sleep?

Looking at an RCU example, and assuming that ordering can move around within a spin lock, and that changes can leak into a spin lock region from both before and after. Could we have:

(looking at __call_rcu_core() and rcu_gp_kthread()

	CPU0				CPU1
	----				----
				__call_rcu_core() {

				 spin_lock(rnp_root)
				 need_wake = __rcu_start_gp() {
				  rcu_start_gp_advanced() {
				   gp_flags = FLAG_INIT
				  }
				 }

 rcu_gp_kthread() {
   swait_event_interruptible(wq,
	gp_flags & FLAG_INIT) {
   spin_lock(q->lock)

				*fetch wq->task_list here! *

   list_add(wq->task_list, q->task_list)
   spin_unlock(q->lock);

Quoted 1 line

*fetch old value of gp_flags here *

Both reads of ->gp_flags are READ_ONCE(), so having seen the new value in swait_event_interruptible(), this task/CPU cannot see the old value from some later access. You have to have accesses to two different variables to require a memory barrier (at least assuming consistent use of READ_ONCE(), WRITE_ONCE(), or equivalent).

From my understandings of reading memory-barrier.txt, there's no

Quoted 10 lines

				 spin_unlock(rnp_root)

				 rcu_gp_kthread_wake() {
				  swake_up(wq) {
				   swait_active(wq) {
				    list_empty(wq->task_list)

				   } * return false *

Quoted 6 lines

  if (condition) * false *
    schedule();

Looks like a memory barrier is missing. Perhaps we should slap on into swait_active()? I don't think it is wise to let users add there own, as I think we currently have bugs now.

I -know- I have bugs now. ;-)

But I don't believe this is one of them. Or am I getting confused?

Thanx, Paul

Quoted 46 lines · On Tue, 13 Jun 2017 16:42:05 -0700 "Paul E. McKenney"…

Quoted 45 lines · Steven Rostedt

Quoted 8 lines · On Fri, 9 Jun 2017 05:45:54 -0700 "Paul E. McKenney"…

Quoted 7 lines · Peter Zijlstra

Quoted 5 lines · Krister Johansen

The behavior of swake_up() differs from that of wake_up(), and from the swake_up() that came from RT linux. A memory barrier, or some other synchronization, is needed prior to a swake_up so that the waiter sees the condition set by the waker, and so that the waker does not see an empty wait list.

Quoted 6 lines

Quoted 2 lines

Urgh.. let me stare at that. But it sounds like the wrong solution since we wanted to keep the wait and swait APIs as close as possible.

But don't they both need some sort of ordering, be it memory barriers or locking, to handle the case where the wait/swait doesn't actually sleep?

Looking at an RCU example, and assuming that ordering can move around within a spin lock, and that changes can leak into a spin lock region from both before and after. Could we have:

(looking at __call_rcu_core() and rcu_gp_kthread()

	CPU0				CPU1
	----				----
				__call_rcu_core() {

				 spin_lock(rnp_root)
				 need_wake = __rcu_start_gp() {
				  rcu_start_gp_advanced() {
				   gp_flags = FLAG_INIT
				  }
				 }

 rcu_gp_kthread() {
   swait_event_interruptible(wq,
	gp_flags & FLAG_INIT) {
   spin_lock(q->lock)

				*fetch wq->task_list here! *

   list_add(wq->task_list, q->task_list)
   spin_unlock(q->lock);

Quoted 7 lines

Quoted 1 line

*fetch old value of gp_flags here *

Both reads of ->gp_flags are READ_ONCE(), so having seen the new value in swait_event_interruptible(), this task/CPU cannot see the old value from some later access. You have to have accesses to two different variables to require a memory barrier (at least assuming consistent use of READ_ONCE(), WRITE_ONCE(), or equivalent).

If I'm not mistaken, READ_ONCE() and WRITE_ONCE() is just volatiles added. The compiler may not leak or move the the fetches, but what about the hardware?

A spin_lock() only needs to make sure what is after it does not leak before it.

A spin_unlock() only needs to make sure what is before it must not leak after it.

From my understandings of reading memory-barrier.txt, there's no guarantees that the hardware doesn't let reads or writes that happen before a spin_lock() happen after it. Nor does it guarantee that reads or writes that happen after a spin_unlock() doesn't happen before it.

The spin_locks only need to protect the inside of the critical section, not the outside of it leaking in.

I'm looking at this in particular:

====
(1) ACQUIRE operation implication:

Memory operations issued after the ACQUIRE will be completed after the ACQUIRE operation has completed.

Memory operations issued before the ACQUIRE may be completed after the ACQUIRE operation has completed. An smp_mb__before_spinlock(), combined with a following ACQUIRE, orders prior stores against subsequent loads and stores. Note that this is weaker than smp_mb()! The smp_mb__before_spinlock() primitive is free on many architectures.

(2) RELEASE operation implication:

Memory operations issued before the RELEASE will be completed before the RELEASE operation has completed.

Memory operations issued after the RELEASE may be completed before the RELEASE operation has completed.
====

-- Steve

Quoted 22 lines

Quoted 10 lines

				 spin_unlock(rnp_root)

				 rcu_gp_kthread_wake() {
				  swake_up(wq) {
				   swait_active(wq) {
				    list_empty(wq->task_list)

				   } * return false *

Quoted 6 lines

  if (condition) * false *
    schedule();

Looks like a memory barrier is missing. Perhaps we should slap on into swait_active()? I don't think it is wise to let users add there own, as I think we currently have bugs now.

I -know- I have bugs now. ;-)

But I don't believe this is one of them. Or am I getting confused?

Thanx, Paul

And here is the part you also need to look at:

====

(*) Overlapping loads and stores within a particular CPU will appear to be
ordered within that CPU. This means that for:

a = READ_ONCE(*X); WRITE_ONCE(*X, b);

the CPU will only issue the following sequence of memory operations:

a = LOAD *X, STORE *X = b

And for:

WRITE_ONCE(*X, c); d = READ_ONCE(*X);

the CPU will only issue:

STORE *X = c, d = LOAD *X

(Loads and stores overlap if they are targeted at overlapping pieces of memory).

====

This section needs some help -- the actual guarantee is stronger, that all CPUs will agree on the order of volatile same-sized aligned accesses to a given single location. So if a previous READ_ONCE() sees the new value, any subsequent READ_ONCE() from that same variable is guaranteed to also see the new value (or some later value).

Does that help, or am I missing something here?

Now let's make it simpler. I'll even add the READ_ONCE and WRITE_ONCE where applicable.

	CPU0				CPU1
	----				----
				LOCK(A)

LOCK(B)
WRITE_ONCE(X, INIT)

(the cpu may postpone writing X)

(the cpu can fetch wq list here)
list_add(wq, q)

UNLOCK(B)

(the cpu may fetch old value of X)

(write of X happens here)

if (READ_ONCE(X) != init)
schedule();

				UNLOCK(A)

				 if (list_empty(wq))
				   return;

Tell me again how the READ_ONCE() and WRITE_ONCE() helps in this scenario?

Because we are using spinlocks, this wont be an issue for most architectures. The bug happens if the fetching of the list_empty() leaks into before the UNLOCK(A).

If the reading/writing of the list and the reading/writing of gp_flags gets reversed in either direction by the CPU, then we have a problem.

Quoted 33 lines · On Wed, 14 Jun 2017 09:10:15 -0400 Steven Rostedt <[email protected]> wrote:

Now let's make it simpler. I'll even add the READ_ONCE and WRITE_ONCE where applicable.

	CPU0				CPU1
	----				----
				LOCK(A)

LOCK(B)
WRITE_ONCE(X, INIT)

(the cpu may postpone writing X)

(the cpu can fetch wq list here)
list_add(wq, q)

UNLOCK(B)

(the cpu may fetch old value of X)

(write of X happens here)

if (READ_ONCE(X) != init)
schedule();

				UNLOCK(A)

				 if (list_empty(wq))
				   return;

Tell me again how the READ_ONCE() and WRITE_ONCE() helps in this scenario?

Quoted 6 lines

Because we are using spinlocks, this wont be an issue for most architectures. The bug happens if the fetching of the list_empty() leaks into before the UNLOCK(A).

If the reading/writing of the list and the reading/writing of gp_flags gets reversed in either direction by the CPU, then we have a problem.

FYI..

Both sides need a memory barrier. Otherwise, even with a memory barrier on CPU1 we can still have:

	CPU0				CPU1
	----				----

				LOCK(A)

LOCK(B)

list_add(wq, q)

(cpu waits to write wq list)

(cpu fetches X)

				 WRITE_ONCE(X, INIT)

				UNLOCK(A)

				smp_mb();

				if (list_empty(wq))
				   return;

(cpu writes wq list)

UNLOCK(B)

if (READ_ONCE(X) != INIT)
schedule()

Luckily for us, there is a memory barrier on CPU0. In prepare_to_swait() we have:

	raw_spin_lock_irqsave(&q->lock, flags);
	__prepare_to_swait(q, wait);
	set_current_state(state);
	raw_spin_unlock_irqrestore(&q->lock, flags);

And that set_current_state() call includes a memory barrier, which will prevent the above from happening, as the addition to the wq list must be flushed before fetching X.

I still strongly believe that the swait_active() requires a memory barrier.

Quoted 86 lines · Steven Rostedt

Quoted 33 lines · On Wed, 14 Jun 2017 09:10:15 -0400 Steven Rostedt <[email protected]> wrote:

Now let's make it simpler. I'll even add the READ_ONCE and WRITE_ONCE where applicable.

	CPU0				CPU1
	----				----
				LOCK(A)

LOCK(B)
WRITE_ONCE(X, INIT)

(the cpu may postpone writing X)

(the cpu can fetch wq list here)
list_add(wq, q)

UNLOCK(B)

(the cpu may fetch old value of X)

(write of X happens here)

if (READ_ONCE(X) != init)
schedule();

				UNLOCK(A)

				 if (list_empty(wq))
				   return;

Tell me again how the READ_ONCE() and WRITE_ONCE() helps in this scenario?

Quoted 6 lines

Because we are using spinlocks, this wont be an issue for most architectures. The bug happens if the fetching of the list_empty() leaks into before the UNLOCK(A).

If the reading/writing of the list and the reading/writing of gp_flags gets reversed in either direction by the CPU, then we have a problem.

FYI..

Both sides need a memory barrier. Otherwise, even with a memory barrier on CPU1 we can still have:

	CPU0				CPU1
	----				----

				LOCK(A)

LOCK(B)

list_add(wq, q)

(cpu waits to write wq list)

(cpu fetches X)

				 WRITE_ONCE(X, INIT)

				UNLOCK(A)

				smp_mb();

				if (list_empty(wq))
				   return;

(cpu writes wq list)

UNLOCK(B)

if (READ_ONCE(X) != INIT)
schedule()

Luckily for us, there is a memory barrier on CPU0. In prepare_to_swait() we have:

	raw_spin_lock_irqsave(&q->lock, flags);
	__prepare_to_swait(q, wait);
	set_current_state(state);
	raw_spin_unlock_irqrestore(&q->lock, flags);

Quoted 6 lines

And that set_current_state() call includes a memory barrier, which will prevent the above from happening, as the addition to the wq list must be flushed before fetching X.

I still strongly believe that the swait_active() requires a memory barrier.

FWLIW, I agree. There was a smb_mb() in RT-linux's equivalent of swait_activate().

https://www.spinics.net/lists/linux-rt-users/msg10340.html

If the barrier goes in swait_active() then we don't have to require all of the callers of swait_active and swake_up to issue the barrier instead. Handling this in swait_active is likely to be less error prone. Though, we could also do something like wq_has_sleeper() and use that preferentially in swake_up and its variants.

Quoted 97 lines · Krister Johansen

Quoted 86 lines · Steven Rostedt

Quoted 33 lines · On Wed, 14 Jun 2017 09:10:15 -0400 Steven Rostedt <[email protected]> wrote:

Now let's make it simpler. I'll even add the READ_ONCE and WRITE_ONCE where applicable.

	CPU0				CPU1
	----				----
				LOCK(A)

LOCK(B)
WRITE_ONCE(X, INIT)

(the cpu may postpone writing X)

(the cpu can fetch wq list here)
list_add(wq, q)

UNLOCK(B)

(the cpu may fetch old value of X)

(write of X happens here)

if (READ_ONCE(X) != init)
schedule();

				UNLOCK(A)

				 if (list_empty(wq))
				   return;

Tell me again how the READ_ONCE() and WRITE_ONCE() helps in this scenario?

Quoted 6 lines

Because we are using spinlocks, this wont be an issue for most architectures. The bug happens if the fetching of the list_empty() leaks into before the UNLOCK(A).

If the reading/writing of the list and the reading/writing of gp_flags gets reversed in either direction by the CPU, then we have a problem.

FYI..

Both sides need a memory barrier. Otherwise, even with a memory barrier on CPU1 we can still have:

	CPU0				CPU1
	----				----

				LOCK(A)

LOCK(B)

list_add(wq, q)

(cpu waits to write wq list)

(cpu fetches X)

				 WRITE_ONCE(X, INIT)

				UNLOCK(A)

				smp_mb();

				if (list_empty(wq))
				   return;

(cpu writes wq list)

UNLOCK(B)

if (READ_ONCE(X) != INIT)
schedule()

Luckily for us, there is a memory barrier on CPU0. In prepare_to_swait() we have:

	raw_spin_lock_irqsave(&q->lock, flags);
	__prepare_to_swait(q, wait);
	set_current_state(state);
	raw_spin_unlock_irqrestore(&q->lock, flags);

Quoted 6 lines

And that set_current_state() call includes a memory barrier, which will prevent the above from happening, as the addition to the wq list must be flushed before fetching X.

I still strongly believe that the swait_active() requires a memory barrier.

FWLIW, I agree. There was a smb_mb() in RT-linux's equivalent of swait_activate().

Quoted 8 lines

https://www.spinics.net/lists/linux-rt-users/msg10340.html

If the barrier goes in swait_active() then we don't have to require all of the callers of swait_active and swake_up to issue the barrier instead. Handling this in swait_active is likely to be less error prone. Though, we could also do something like wq_has_sleeper() and use that preferentially in swake_up and its variants.

I think it makes more sense that we delete the swait_active() in swake_up()? Because we seems to encourage users to do the quick check on wait queue on their own, so why do the check again in swake_up()? Besides, wake_up() doesn't call waitqueue_activie() outside the lock critical section either.

So how about the patch below(Testing is in progress)? Peter?

Regards,
Boqun

--------------------->8
Subject: [PATCH] swait: Remove the lockless swait_active() check in
swake_up*()

Steven Rostedt reported a potential race in RCU core because of swake_up():

        CPU0                            CPU1
        ----                            ----
                                __call_rcu_core() {

                                 spin_lock(rnp_root)
                                 need_wake = __rcu_start_gp() {
                                  rcu_start_gp_advanced() {
                                   gp_flags = FLAG_INIT
                                  }
                                 }

 rcu_gp_kthread() {
   swait_event_interruptible(wq,
        gp_flags & FLAG_INIT) {
   spin_lock(q->lock)

                                *fetch wq->task_list here! *

   list_add(wq->task_list, q->task_list)
   spin_unlock(q->lock);

   *fetch old value of gp_flags here *

                                 spin_unlock(rnp_root)

                                 rcu_gp_kthread_wake() {
                                  swake_up(wq) {
                                   swait_active(wq) {
                                    list_empty(wq->task_list)

                                   } * return false *

  if (condition) * false *
    schedule();

In this case, a wakeup is missed, which could cause the rcu_gp_kthread waits for a long time.

The reason of this is that we do a lockless swait_active() check in swake_up(). To fix this, we can either 1) add a smp_mb() in swake_up() before swait_active() to provide the proper order or 2) simply remove the swait_active() in swake_up().

The solution 2 not only fixes this problem but also keeps the swait and wait API as close as possible, as wake_up() doesn't provide a full barrier and doesn't do a lockless check of the wait queue either. Moreover, there are users already using swait_active() to do their quick checks for the wait queues, so it make less sense that swake_up() and swake_up_all() do this on their own.

This patch then removes the lockless swait_active() check in swake_up() and swake_up_all().

Reported-by Steven Rostedt <[email protected]>

Signed-off-by Boqun Feng <[email protected]>

---
 kernel/sched/swait.c | 6 ------
 1 file changed, 6 deletions(-)

kernel/sched/swait.c +0 -6

index 3d5610dcce11..2227e183e202 100644
@@ -33,9 +33,6 @@ void swake_up(struct swait_queue_head *q)
 {
 	unsigned long flags;
 
-	if (!swait_active(q))
-		return;
-
 	raw_spin_lock_irqsave(&q->lock, flags);
 	swake_up_locked(q);
 	raw_spin_unlock_irqrestore(&q->lock, flags);

@@ -51,9 +48,6 @@ void swake_up_all(struct swait_queue_head *q)
 	struct swait_queue *curr;
 	LIST_HEAD(tmp);
 
-	if (!swait_active(q))
-		return;
-
 	raw_spin_lock_irq(&q->lock);
 	list_splice_init(&q->task_list, &tmp);
 	while (!list_empty(&tmp)) {

Signature · 1 line

2.13.0

Quoted 13 lines

Quoted 4 lines

FWLIW, I agree. There was a smb_mb() in RT-linux's equivalent of swait_activate().

Quoted 8 lines

https://www.spinics.net/lists/linux-rt-users/msg10340.html

If the barrier goes in swait_active() then we don't have to require all of the callers of swait_active and swake_up to issue the barrier instead. Handling this in swait_active is likely to be less error prone. Though, we could also do something like wq_has_sleeper() and use that preferentially in swake_up and its variants.

Quoted 7 lines

I think it makes more sense that we delete the swait_active() in swake_up()? Because we seems to encourage users to do the quick check on wait queue on their own, so why do the check again in swake_up()? Besides, wake_up() doesn't call waitqueue_activie() outside the lock critical section either.

So how about the patch below(Testing is in progress)? Peter?

It is quite possible that a problem I am seeing is caused by this, but there are reasons to believe otherwise. And in any case, the problem is quite rare, taking tens or perhaps even hundreds of hours of rcutorture to reproduce.

So, would you be willing to create a dedicated swait torture test to check this out? The usual approach would be to create a circle of kthreads, with each waiting on the previous kthread and waking up the next one. Each kthread, after being awakened, checks a variable that its waker sets just before the wakeup. Have another kthread check for hangs.

Possibly introduce timeouts and random delays to stir things up a bit.

But maybe such a test already exists. Does anyone know of one? I don't see anything obvious.

Interested?

Thanx, Paul

On Thu, Jun 15, 2017 at 10:56:29AM -0700, Paul E. McKenney wrote:
[...]

Quoted 32 lines

Quoted 13 lines

Quoted 4 lines

FWLIW, I agree. There was a smb_mb() in RT-linux's equivalent of swait_activate().

Quoted 8 lines

https://www.spinics.net/lists/linux-rt-users/msg10340.html

If the barrier goes in swait_active() then we don't have to require all of the callers of swait_active and swake_up to issue the barrier instead. Handling this in swait_active is likely to be less error prone. Though, we could also do something like wq_has_sleeper() and use that preferentially in swake_up and its variants.

Quoted 7 lines

I think it makes more sense that we delete the swait_active() in swake_up()? Because we seems to encourage users to do the quick check on wait queue on their own, so why do the check again in swake_up()? Besides, wake_up() doesn't call waitqueue_activie() outside the lock critical section either.

So how about the patch below(Testing is in progress)? Peter?

It is quite possible that a problem I am seeing is caused by this, but there are reasons to believe otherwise. And in any case, the problem is quite rare, taking tens or perhaps even hundreds of hours of rcutorture to reproduce.

So, would you be willing to create a dedicated swait torture test to check this out? The usual approach would be to create a circle of kthreads, with each waiting on the previous kthread and waking up the next one. Each kthread, after being awakened, checks a variable that its waker sets just before the wakeup. Have another kthread check for hangs.

Quoted 5 lines

Possibly introduce timeouts and random delays to stir things up a bit.

But maybe such a test already exists. Does anyone know of one? I don't see anything obvious.

Your waketorture patchset[1] seems to be something similar, at least a good start ;-)

As we don't know which kind of scenario will trigger the problem easily, I will play around with different ones, and hopefully we can find a way.

Regards,
Boqun

[1]: https://marc.info/?l=linux-kernel&m=146602969518960

Quoted 4 lines

Interested?

Thanx, Paul

[...]

Quoted 1 line · On Tue, 13 Jun 2017 20:58:43 -0700 "Paul E. McKenney"…

And here is the part you also need to look at:

Why? We are talking about two different, unrelated variables modified on two different CPUs. I don't see where the overlap is.

Quoted 24 lines

====

(*) Overlapping loads and stores within a particular CPU will appear to be
ordered within that CPU. This means that for:

a = READ_ONCE(*X); WRITE_ONCE(*X, b);

the CPU will only issue the following sequence of memory operations:

a = LOAD *X, STORE *X = b

And for:

WRITE_ONCE(*X, c); d = READ_ONCE(*X);

the CPU will only issue:

STORE *X = c, d = LOAD *X

(Loads and stores overlap if they are targeted at overlapping pieces of memory).

====

Quoted 7 lines

This section needs some help -- the actual guarantee is stronger, that all CPUs will agree on the order of volatile same-sized aligned accesses to a given single location. So if a previous READ_ONCE() sees the new value, any subsequent READ_ONCE() from that same variable is guaranteed to also see the new value (or some later value).

Does that help, or am I missing something here?

Maybe I'm missing something. Let me rewrite what I first wrote, and then abstract it into a simpler version:

Here's what I first wrote:

(looking at __call_rcu_core() and rcu_gp_kthread()

	CPU0				CPU1
	----				----
				__call_rcu_core() {

				 spin_lock(rnp_root)
				 need_wake = __rcu_start_gp() {
				  rcu_start_gp_advanced() {
				   gp_flags = FLAG_INIT
				  }
				 }

 rcu_gp_kthread() {
   swait_event_interruptible(wq,
	gp_flags & FLAG_INIT) {

   spin_lock(q->lock)

				*fetch wq->task_list here! *

   list_add(wq->task_list, q->task_list)
   spin_unlock(q->lock);

   *fetch old value of gp_flags here *

				 spin_unlock(rnp_root)

				 rcu_gp_kthread_wake() {
				  swake_up(wq) {
				   swait_active(wq) {
				    list_empty(wq->task_list)

				   } * return false *

  if (condition) * false *
    schedule();

Now let's make it simpler. I'll even add the READ_ONCE and WRITE_ONCE where applicable.

	CPU0				CPU1
	----				----
				LOCK(A)

LOCK(B)
WRITE_ONCE(X, INIT)

(the cpu may postpone writing X)

(the cpu can fetch wq list here)
list_add(wq, q)

UNLOCK(B)

(the cpu may fetch old value of X)

(write of X happens here)

if (READ_ONCE(X) != init)
schedule();

				UNLOCK(A)

				 if (list_empty(wq))
				   return;

Tell me again how the READ_ONCE() and WRITE_ONCE() helps in this scenario?

Because we are using spinlocks, this wont be an issue for most architectures. The bug happens if the fetching of the list_empty() leaks into before the UNLOCK(A).

If the reading/writing of the list and the reading/writing of gp_flags gets reversed in either direction by the CPU, then we have a problem.

-- Steve