Re: [PATCH tip/sched/core] Add comments to aid in safer usage of swake_up.

[Steven Rostedt <rostedt@goodmis.org>]

On Tue, 13 Jun 2017 20:58:43 -0700
"Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:

> 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.

> 
> ====
> 
>  (*) 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?

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