Messages in this thread | | | Date | Tue, 13 Jun 2017 19:23:08 -0400 | From | Steven Rostedt <> | Subject | Re: [PATCH tip/sched/core] Add comments to aid in safer usage of swake_up. |
| |
On Fri, 9 Jun 2017 05:45:54 -0700 "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
> On Fri, Jun 09, 2017 at 09:19:57AM +0200, Peter Zijlstra wrote: > > On Thu, Jun 08, 2017 at 08:25:46PM -0700, Krister Johansen wrote: > > > 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. > > 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.
-- Steve
| |