| Message ID | 20180628132629.3148-5-willy@infradead.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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...
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 */
Matthew Wilcox wrote on Thu, Jun 28, 2018: > 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 > */ Yes, that sounds better. This code is fairly old and I was wondering if the new WRITE_ONCE and READ_ONCE macros would help but it looks like compile-time barriers only so I do not think they would be enough... Documentation/memory-barriers.txt still suggests something similar in the "SLEEP AND WAKE-UP FUNCTIONS" section so I guess this is fine. Thanks,
diff --git a/net/9p/client.c b/net/9p/client.c index 602f76de388a..2dce8d8307cc 100644 --- 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 */ wake_up(&req->wq); p9_debug(P9_DEBUG_MUX, "wakeup: %d\n", req->tc->tag); }
And add a comment about why we don't need it. Signed-off-by: Matthew Wilcox <willy@infradead.org> --- net/9p/client.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-)