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 15:40:29 +0200
From:   Dominique Martinet <asmadeus@codewreck.org>
To:     Matthew Wilcox <willy@infradead.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: <20180628134029.GA24673@nautica>
References: <20180628132629.3148-1-willy@infradead.org>
 <20180628132629.3148-5-willy@infradead.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20180628132629.3148-5-willy@infradead.org>
User-Agent: Mutt/1.5.21 (2010-09-15)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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...
-- 
Dominique Martinet | Asmadeus