From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from bombadil.infradead.org ([198.137.202.133]:44642 "EHLO bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S934483AbeF1OEF (ORCPT ); Thu, 28 Jun 2018 10:04:05 -0400 Date: Thu, 28 Jun 2018 07:03:58 -0700 From: Matthew Wilcox To: Dominique Martinet Cc: v9fs-developer@lists.sourceforge.net, Latchesar Ionkov , Eric Van Hensbergen , linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org, Ron Minnich 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> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: 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 */