Date: Wed, 14 Jun 2017 09:25:58 -0700
From: Krister Johansen <kjlx@templeofstupid.com>
To: Steven Rostedt <rostedt@goodmis.org>
Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>,
        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: <20170614162558.GA2368@templeofstupid.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>
 <20170614091015.01d7dc89@gandalf.local.home>
 <20170614110240.10abe2ed@gandalf.local.home>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20170614110240.10abe2ed@gandalf.local.home>
User-Agent: Mutt/1.5.24 (2015-08-30)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2584
Lines: 106

On Wed, Jun 14, 2017 at 11:02:40AM -0400, Steven Rostedt wrote:
> On Wed, 14 Jun 2017 09:10:15 -0400
> Steven Rostedt <rostedt@goodmis.org> 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?
> > 
> > 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.

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.

-K