DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 0E507219A7
Date: Wed, 14 Jun 2017 09:10:15 -0400
From: Steven Rostedt <rostedt@goodmis.org>
To: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Cc: Peter Zijlstra <peterz@infradead.org>,
        Krister Johansen <kjlx@templeofstupid.com>,
        Ingo Molnar <mingo@kernel.org>, linux-kernel@vger.kernel.org,
        Paul Gortmaker <paul.gortmaker@windriver.com>,
        Thomas Gleixner <tglx@linutronix.de>
Subject: Re: [PATCH tip/sched/core] Add comments to aid in safer usage of
 swake_up.
Message-ID: <20170614091015.01d7dc89@gandalf.local.home>
In-Reply-To: <20170614035843.GI3721@linux.vnet.ibm.com>
References: <20170609032546.GF2553@templeofstupid.com>
        <20170609071957.GJ8337@worktop.programming.kicks-ass.net>
        <20170609124554.GF3721@linux.vnet.ibm.com>
        <20170613192308.173dd86a@gandalf.local.home>
        <20170613234205.GD3721@linux.vnet.ibm.com>
        <20170613211547.49814d25@gandalf.local.home>
        <20170614035843.GI3721@linux.vnet.ibm.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2877
Lines: 126

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