Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Mon, 25 Jun 2018 16:18:30 +0200
From:   Peter Zijlstra <peterz@infradead.org>
To:     Andrea Parri <andrea.parri@amarulasolutions.com>
Cc:     linux-kernel@vger.kernel.org, linux-doc@vger.kernel.org,
        Alan Stern <stern@rowland.harvard.edu>,
        Will Deacon <will.deacon@arm.com>,
        Boqun Feng <boqun.feng@gmail.com>,
        Nicholas Piggin <npiggin@gmail.com>,
        David Howells <dhowells@redhat.com>,
        Jade Alglave <j.alglave@ucl.ac.uk>,
        Luc Maranget <luc.maranget@inria.fr>,
        "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>,
        Akira Yokosawa <akiyks@gmail.com>,
        Daniel Lustig <dlustig@nvidia.com>,
        Jonathan Corbet <corbet@lwn.net>,
        Ingo Molnar <mingo@redhat.com>,
        Randy Dunlap <rdunlap@infradead.org>
Subject: Re: [PATCH] doc: Update wake_up() & co. memory-barrier guarantees
Message-ID: <20180625141830.GC2494@hirez.programming.kicks-ass.net>
References: <1529918258-7295-1-git-send-email-andrea.parri@amarulasolutions.com>
 <20180625095031.GX2494@hirez.programming.kicks-ass.net>
 <20180625105618.GA12676@andrea>
 <20180625123121.GY2494@hirez.programming.kicks-ass.net>
 <20180625131643.GA15126@andrea>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20180625131643.GA15126@andrea>
User-Agent: Mutt/1.10.0 (2018-05-17)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Mon, Jun 25, 2018 at 03:16:43PM +0200, Andrea Parri wrote:
> > > A concrete example being the store-buffering pattern reported in [1].
> > 
> > Well, that example only needs a store->load barrier. It so happens
> > smp_mb() is the only one actually doing that, but imagine we had a
> > weaker barrier that did just that, one that did not imply the full
> > transitivity smp_mb() does.
> > 
> > Then the example from [1] could use that weaker thing.
> 
> Absolutely (and that would be "fence w,r" on RISC-V, IIUC).

Ah cute. What is the transitivity model of those "fence" instructions? I
see their smp_mb() is "fence rw,rw" and smp_mb() must be RSsc. Otoh
their smp_wmb() is "fence w,w" which is only only required to be RCpc.

So what does RISC-V do for "w,w" and "w,r" like things?

> > diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> > index a98d54cd5535..8374d01b2820 100644
> > --- a/kernel/sched/core.c
> > +++ b/kernel/sched/core.c
> > @@ -1879,7 +1879,9 @@ static void ttwu_queue(struct task_struct *p, int cpu, int wake_flags)
> >   *  C) LOCK of the rq(c1)->lock scheduling in task
> >   *
> >   * Transitivity guarantees that B happens after A and C after B.
> > - * Note: we only require RCpc transitivity.
> > + * Note: we only require RCpc transitivity for these cases,
> > + *       but see smp_mb__after_spinlock() for why rq->lock is required
> > + *       to be RCsc.
> >   * Note: the CPU doing B need not be c0 or c1
> 
> FWIW, we discussed this pattern here:
> 
>   http://lkml.kernel.org/r/20171018010748.GA4017@andrea

That's not the patter from smp_mb__after_spinlock(), right? But the
other two from this comment.

> > @@ -1966,6 +1969,10 @@ static void ttwu_queue(struct task_struct *p, int cpu, int wake_flags)
> >   * Atomic against schedule() which would dequeue a task, also see
> >   * set_current_state().
> >   *
> > + * Implies at least a RELEASE such that the waking task is guaranteed to
> > + * observe the stores to the wait-condition; see set_task_state() and the
> > + * Program-Order constraints.
> 
> [s/set_task_task/set_current_state ?]

Yes, we got rid of set_task_state(), someone forgot to tell my fingers
:-)

> I'd stick to "Implies/Executes at least a full barrier"; this is in fact
> already documented in the function body:
> 
> 	/*
> 	 * If we are going to wake up a thread waiting for CONDITION we
> 	 * need to ensure that CONDITION=1 done by the caller can not be
> 	 * reordered with p->state check below. This pairs with mb() in
> 	 * set_current_state() the waiting thread does.
> 	 */
> 
> (this is, again, that "store->load barrier"/SB).
> 
> I'll try to integrate these changes in v2, if there is no objection.

Thanks!