On Thu, Jun 28, 2018 at 03:40:29PM +0200, Dominique Martinet wrote:
> Matthew Wilcox wrote on Thu, Jun 28, 2018:
> > --- a/net/9p/client.c
> > +++ b/net/9p/client.c
> > @@ -436,13 +436,9 @@ void p9_client_cb(struct p9_client *c, struct p9_req_t
> > *req, int status)
> > {
> > p9_debug(P9_DEBUG_MUX, " tag %d\n", req->tc->tag);
> >
> > - /*
> > - * This barrier is needed to make sure any change made to req before
> > - * the other thread wakes up will indeed be seen by the waiting side.
> > - */
> > - smp_wmb();
> > req->status = status;
> >
> > + /* wake_up is an implicit write memory barrier */
>
> Nope.
> Please note the wmb is _before_ setting status, basically it protects
> from cpu optimizations where status could be set before other fields,
> then other core opportunistically checking and finding status is good so
> other thread continuing.
>
> I could only reproduce this bug with infiniband network, but it is very
> definitely needed. Here is the commit message of when I added that barrier:
> -----
> 9P: Add memory barriers to protect request fields over cb/rpc threads handoff
>
> We need barriers to guarantee this pattern works as intended:
> [w] req->rc, 1 [r] req->status, 1
> wmb rmb
> [w] req->status, 1 [r] req->rc
>
> Where the wmb ensures that rc gets written before status,
> and the rmb ensures that if you observe status == 1, rc is the new value.
> -----
>
> It might need an update to the comment though, if you thought about
> removing it...
Ah! Yes, that situation is different from what the comment documents.
How about this?
/*
* This barrier is needed to make sure any change made to req before
- * the other thread wakes up will indeed be seen by the waiting side.
+ * the status change is visible to another thread
*/
