Re: [V9fs-developer] [PATCH 4/6] 9p: Remove an unnecessary memory barrier

[Matthew Wilcox]

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
	 */