| Message ID | 172644394073.17050.16376953609629336068@noble.neil.brown.name (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [nfsd-next] SQUASH sunrpc: allow svc threads to fail initialisation cleanly | expand |
On Mon, 2024-09-16 at 09:45 +1000, NeilBrown wrote: > The memory barriers in this patch were all wrong. > smp_store_release / smp_load_acquire ensures that all changes written > before the store_release are equally visible after the acquire. > These are not needed here as the *only* value that > svc_thread_init_status() or its caller sets that is of any interest to > svc_start_kthread() is ->rq_err. The barrier wold effect writes before > ->rq_err is written and reads after ->rq_err is read. > > However we DO need a full memory barrier between setting ->rq_err and > before the the waitqueue_active() read in wake_up_var(). This is a > barrier between a write and a read, hence a full barrier: smb_mb(). > > This addresses a race if wait_var_event() adds itself to the wait queue > and tests ->rq_err such that the read in waitqueue_active() happens > earlier and doesn't see that the task has been added, and the ->rq_err > write is delayed so that the waiting task doesn't see it. In this case > the wake_up never happens. > > Signed-off-by: NeilBrown <neilb@suse.de> > --- > include/linux/sunrpc/svc.h | 7 +++++-- > net/sunrpc/svc.c | 3 +-- > 2 files changed, 6 insertions(+), 4 deletions(-) > > diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h > index 437672bcaa22..558e5ae03103 100644 > --- a/include/linux/sunrpc/svc.h > +++ b/include/linux/sunrpc/svc.h > @@ -326,8 +326,11 @@ static inline bool svc_thread_should_stop(struct svc_rqst *rqstp) > */ > static inline void svc_thread_init_status(struct svc_rqst *rqstp, int err) > { > - /* store_release ensures svc_start_kthreads() sees the error */ > - smp_store_release(&rqstp->rq_err, err); > + rqstp->rq_err = err; > + /* memory barrier ensures assignment to error above is visible before > + * waitqueue_active() test below completes. > + */ > + smb_mb(); > wake_up_var(&rqstp->rq_err); > if (err) > kthread_exit(1); > diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c > index ff6f3e35b36d..9aff845196ce 100644 > --- a/net/sunrpc/svc.c > +++ b/net/sunrpc/svc.c > @@ -818,8 +818,7 @@ svc_start_kthreads(struct svc_serv *serv, struct svc_pool *pool, int nrservs) > svc_sock_update_bufs(serv); > wake_up_process(task); > > - /* load_acquire ensures we get value stored in svc_thread_init_status() */ > - wait_var_event(&rqstp->rq_err, smp_load_acquire(&rqstp->rq_err) != -EAGAIN); > + wait_var_event(&rqstp->rq_err, rqstp->rq_err != -EAGAIN); > err = rqstp->rq_err; > if (err) { > svc_exit_thread(rqstp); Makes sense. Reviewed-by: Jeff Layton <jlayton@kernel.org>
On Mon, Sep 16, 2024 at 09:45:40AM +1000, NeilBrown wrote: > > The memory barriers in this patch were all wrong. > smp_store_release / smp_load_acquire ensures that all changes written > before the store_release are equally visible after the acquire. > These are not needed here as the *only* value that > svc_thread_init_status() or its caller sets that is of any interest to > svc_start_kthread() is ->rq_err. The barrier wold effect writes before > ->rq_err is written and reads after ->rq_err is read. > > However we DO need a full memory barrier between setting ->rq_err and > before the the waitqueue_active() read in wake_up_var(). This is a > barrier between a write and a read, hence a full barrier: smb_mb(). > > This addresses a race if wait_var_event() adds itself to the wait queue > and tests ->rq_err such that the read in waitqueue_active() happens > earlier and doesn't see that the task has been added, and the ->rq_err > write is delayed so that the waiting task doesn't see it. In this case > the wake_up never happens. > > Signed-off-by: NeilBrown <neilb@suse.de> > --- > include/linux/sunrpc/svc.h | 7 +++++-- > net/sunrpc/svc.c | 3 +-- > 2 files changed, 6 insertions(+), 4 deletions(-) > > diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h > index 437672bcaa22..558e5ae03103 100644 > --- a/include/linux/sunrpc/svc.h > +++ b/include/linux/sunrpc/svc.h > @@ -326,8 +326,11 @@ static inline bool svc_thread_should_stop(struct svc_rqst *rqstp) > */ > static inline void svc_thread_init_status(struct svc_rqst *rqstp, int err) > { > - /* store_release ensures svc_start_kthreads() sees the error */ > - smp_store_release(&rqstp->rq_err, err); > + rqstp->rq_err = err; > + /* memory barrier ensures assignment to error above is visible before > + * waitqueue_active() test below completes. > + */ > + smb_mb(); This should have been "smp_mb();". I fixed it up before applying. > wake_up_var(&rqstp->rq_err); > if (err) > kthread_exit(1); > diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c > index ff6f3e35b36d..9aff845196ce 100644 > --- a/net/sunrpc/svc.c > +++ b/net/sunrpc/svc.c > @@ -818,8 +818,7 @@ svc_start_kthreads(struct svc_serv *serv, struct svc_pool *pool, int nrservs) > svc_sock_update_bufs(serv); > wake_up_process(task); > > - /* load_acquire ensures we get value stored in svc_thread_init_status() */ > - wait_var_event(&rqstp->rq_err, smp_load_acquire(&rqstp->rq_err) != -EAGAIN); > + wait_var_event(&rqstp->rq_err, rqstp->rq_err != -EAGAIN); > err = rqstp->rq_err; > if (err) { > svc_exit_thread(rqstp); > -- > 2.46.0 > I've squashed this into the already-applied patch in nfsd-next. Thanks!
diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h index 437672bcaa22..558e5ae03103 100644 --- a/include/linux/sunrpc/svc.h +++ b/include/linux/sunrpc/svc.h @@ -326,8 +326,11 @@ static inline bool svc_thread_should_stop(struct svc_rqst *rqstp) */ static inline void svc_thread_init_status(struct svc_rqst *rqstp, int err) { - /* store_release ensures svc_start_kthreads() sees the error */ - smp_store_release(&rqstp->rq_err, err); + rqstp->rq_err = err; + /* memory barrier ensures assignment to error above is visible before + * waitqueue_active() test below completes. + */ + smb_mb(); wake_up_var(&rqstp->rq_err); if (err) kthread_exit(1); diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c index ff6f3e35b36d..9aff845196ce 100644 --- a/net/sunrpc/svc.c +++ b/net/sunrpc/svc.c @@ -818,8 +818,7 @@ svc_start_kthreads(struct svc_serv *serv, struct svc_pool *pool, int nrservs) svc_sock_update_bufs(serv); wake_up_process(task); - /* load_acquire ensures we get value stored in svc_thread_init_status() */ - wait_var_event(&rqstp->rq_err, smp_load_acquire(&rqstp->rq_err) != -EAGAIN); + wait_var_event(&rqstp->rq_err, rqstp->rq_err != -EAGAIN); err = rqstp->rq_err; if (err) { svc_exit_thread(rqstp);
The memory barriers in this patch were all wrong. smp_store_release / smp_load_acquire ensures that all changes written before the store_release are equally visible after the acquire. These are not needed here as the *only* value that svc_thread_init_status() or its caller sets that is of any interest to svc_start_kthread() is ->rq_err. The barrier wold effect writes before ->rq_err is written and reads after ->rq_err is read. However we DO need a full memory barrier between setting ->rq_err and before the the waitqueue_active() read in wake_up_var(). This is a barrier between a write and a read, hence a full barrier: smb_mb(). This addresses a race if wait_var_event() adds itself to the wait queue and tests ->rq_err such that the read in waitqueue_active() happens earlier and doesn't see that the task has been added, and the ->rq_err write is delayed so that the waiting task doesn't see it. In this case the wake_up never happens. Signed-off-by: NeilBrown <neilb@suse.de> --- include/linux/sunrpc/svc.h | 7 +++++-- net/sunrpc/svc.c | 3 +-- 2 files changed, 6 insertions(+), 4 deletions(-)