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:   Thu, 28 Jun 2018 07:03:58 -0700
From:   Matthew Wilcox <willy@infradead.org>
To:     Dominique Martinet <asmadeus@codewreck.org>
Cc:     v9fs-developer@lists.sourceforge.net,
        Latchesar Ionkov <lucho@ionkov.net>,
        Eric Van Hensbergen <ericvh@gmail.com>,
        linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org,
        Ron Minnich <rminnich@sandia.gov>
Subject: Re: [V9fs-developer] [PATCH 4/6] 9p: Remove an unnecessary memory
 barrier
Message-ID: <20180628140358.GG7646@bombadil.infradead.org>
References: <20180628132629.3148-1-willy@infradead.org>
 <20180628132629.3148-5-willy@infradead.org>
 <20180628134029.GA24673@nautica>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20180628134029.GA24673@nautica>
User-Agent: Mutt/1.9.2 (2017-12-15)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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