* [PATCH 0/2] nfsd: use new wake_up_var interface
@ 2024-12-06 2:55 NeilBrown
2024-12-06 2:55 ` [PATCH 1/2] nfsd: use new wake_up_var interfaces NeilBrown
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: NeilBrown @ 2024-12-06 2:55 UTC (permalink / raw)
To: Chuck Lever, Jeff Layton
Cc: linux-nfs, Olga Kornievskaia, Dai Ngo, Tom Talpey
wake_up_var() in a fragile interface as it sometimes needs a memory
barrier before it. Recently new interfaces were added which include all
required barriers.
These patches update nfsd and svc parrts of sunrpc to use new interfaces
where appropriate.
NeilBrown
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/2] nfsd: use new wake_up_var interfaces.
2024-12-06 2:55 [PATCH 0/2] nfsd: use new wake_up_var interface NeilBrown
@ 2024-12-06 2:55 ` NeilBrown
2024-12-06 5:40 ` Jeff Layton
2024-12-06 2:55 ` [PATCH 2/2] sunrpc/svc: use store_release_wake_up() NeilBrown
2024-12-06 21:56 ` [PATCH 0/2] nfsd: use new wake_up_var interface cel
2 siblings, 1 reply; 8+ messages in thread
From: NeilBrown @ 2024-12-06 2:55 UTC (permalink / raw)
To: Chuck Lever, Jeff Layton
Cc: linux-nfs, Olga Kornievskaia, Dai Ngo, Tom Talpey
The wake_up_var interface is fragile as barriers are sometimes needed.
There are now new interfaces so that most wake-ups can use an interface
that is guaranteed to have all barriers needed.
This patch changes the wake up on cl_cb_inflight to use
atomic_dec_and_wake_up().
It also changes the wake up on rp_locked to use store_release_wake_up().
This involves changing rp_locked from atomic_t to int.
Signed-off-by: NeilBrown <neilb@suse.de>
---
fs/nfsd/nfs4callback.c | 3 +--
fs/nfsd/nfs4state.c | 16 ++++++----------
fs/nfsd/state.h | 2 +-
3 files changed, 8 insertions(+), 13 deletions(-)
diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
index 3877b53e429f..a8dc9de2f7fb 100644
--- a/fs/nfsd/nfs4callback.c
+++ b/fs/nfsd/nfs4callback.c
@@ -1036,8 +1036,7 @@ static void nfsd41_cb_inflight_begin(struct nfs4_client *clp)
static void nfsd41_cb_inflight_end(struct nfs4_client *clp)
{
- if (atomic_dec_and_test(&clp->cl_cb_inflight))
- wake_up_var(&clp->cl_cb_inflight);
+ atomic_dec_and_wake_up(&clp->cl_cb_inflight);
}
static void nfsd41_cb_inflight_wait_complete(struct nfs4_client *clp)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 741b9449f727..9fbf7c8f0a3e 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -4739,7 +4739,7 @@ static void init_nfs4_replay(struct nfs4_replay *rp)
rp->rp_status = nfserr_serverfault;
rp->rp_buflen = 0;
rp->rp_buf = rp->rp_ibuf;
- atomic_set(&rp->rp_locked, RP_UNLOCKED);
+ rp->rp_locked = RP_UNLOCKED;
}
static int nfsd4_cstate_assign_replay(struct nfsd4_compound_state *cstate,
@@ -4747,9 +4747,9 @@ static int nfsd4_cstate_assign_replay(struct nfsd4_compound_state *cstate,
{
if (!nfsd4_has_session(cstate)) {
wait_var_event(&so->so_replay.rp_locked,
- atomic_cmpxchg(&so->so_replay.rp_locked,
- RP_UNLOCKED, RP_LOCKED) != RP_LOCKED);
- if (atomic_read(&so->so_replay.rp_locked) == RP_UNHASHED)
+ cmpxchg(&so->so_replay.rp_locked,
+ RP_UNLOCKED, RP_LOCKED) != RP_LOCKED);
+ if (so->so_replay.rp_locked == RP_UNHASHED)
return -EAGAIN;
cstate->replay_owner = nfs4_get_stateowner(so);
}
@@ -4762,9 +4762,7 @@ void nfsd4_cstate_clear_replay(struct nfsd4_compound_state *cstate)
if (so != NULL) {
cstate->replay_owner = NULL;
- atomic_set(&so->so_replay.rp_locked, RP_UNLOCKED);
- smp_mb__after_atomic();
- wake_up_var(&so->so_replay.rp_locked);
+ store_release_wake_up(&so->so_replay.rp_locked, RP_UNLOCKED);
nfs4_put_stateowner(so);
}
}
@@ -5069,9 +5067,7 @@ move_to_close_lru(struct nfs4_ol_stateid *s, struct net *net)
* Some threads with a reference might be waiting for rp_locked,
* so tell them to stop waiting.
*/
- atomic_set(&oo->oo_owner.so_replay.rp_locked, RP_UNHASHED);
- smp_mb__after_atomic();
- wake_up_var(&oo->oo_owner.so_replay.rp_locked);
+ store_release_wake_up(&oo->oo_owner.so_replay.rp_locked, RP_UNHASHED);
wait_event(close_wq, refcount_read(&s->st_stid.sc_count) == 2);
release_all_access(s);
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index e16bb3717fb9..ba30b2335b66 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -505,7 +505,7 @@ struct nfs4_replay {
unsigned int rp_buflen;
char *rp_buf;
struct knfsd_fh rp_openfh;
- atomic_t rp_locked;
+ int rp_locked;
char rp_ibuf[NFSD4_REPLAY_ISIZE];
};
--
2.47.0
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 2/2] sunrpc/svc: use store_release_wake_up()
2024-12-06 2:55 [PATCH 0/2] nfsd: use new wake_up_var interface NeilBrown
2024-12-06 2:55 ` [PATCH 1/2] nfsd: use new wake_up_var interfaces NeilBrown
@ 2024-12-06 2:55 ` NeilBrown
2024-12-06 5:41 ` Jeff Layton
2024-12-06 21:56 ` [PATCH 0/2] nfsd: use new wake_up_var interface cel
2 siblings, 1 reply; 8+ messages in thread
From: NeilBrown @ 2024-12-06 2:55 UTC (permalink / raw)
To: Chuck Lever, Jeff Layton
Cc: linux-nfs, Olga Kornievskaia, Dai Ngo, Tom Talpey
svc_thread_init_status() contains an open-coded
store_release_wake_up(). It is cleaner to use that function directly
rather than needing to remember the barrier.
Signed-off-by: NeilBrown <neilb@suse.de>
---
include/linux/sunrpc/svc.h | 7 +------
1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
index e68fecf6eab5..e4f09f58d58c 100644
--- a/include/linux/sunrpc/svc.h
+++ b/include/linux/sunrpc/svc.h
@@ -327,12 +327,7 @@ static inline bool svc_thread_should_stop(struct svc_rqst *rqstp)
*/
static inline void svc_thread_init_status(struct svc_rqst *rqstp, int err)
{
- rqstp->rq_err = err;
- /* memory barrier ensures assignment to error above is visible before
- * waitqueue_active() test below completes.
- */
- smp_mb();
- wake_up_var(&rqstp->rq_err);
+ store_release_wake_up(&rqstp->rq_err, err);
if (err)
kthread_exit(1);
}
--
2.47.0
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] nfsd: use new wake_up_var interfaces.
2024-12-06 2:55 ` [PATCH 1/2] nfsd: use new wake_up_var interfaces NeilBrown
@ 2024-12-06 5:40 ` Jeff Layton
2024-12-06 6:10 ` NeilBrown
0 siblings, 1 reply; 8+ messages in thread
From: Jeff Layton @ 2024-12-06 5:40 UTC (permalink / raw)
To: NeilBrown, Chuck Lever; +Cc: linux-nfs, Olga Kornievskaia, Dai Ngo, Tom Talpey
On Fri, 2024-12-06 at 13:55 +1100, NeilBrown wrote:
> The wake_up_var interface is fragile as barriers are sometimes needed.
> There are now new interfaces so that most wake-ups can use an interface
> that is guaranteed to have all barriers needed.
>
> This patch changes the wake up on cl_cb_inflight to use
> atomic_dec_and_wake_up().
>
> It also changes the wake up on rp_locked to use store_release_wake_up().
> This involves changing rp_locked from atomic_t to int.
>
> Signed-off-by: NeilBrown <neilb@suse.de>
> ---
> fs/nfsd/nfs4callback.c | 3 +--
> fs/nfsd/nfs4state.c | 16 ++++++----------
> fs/nfsd/state.h | 2 +-
> 3 files changed, 8 insertions(+), 13 deletions(-)
>
> diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
> index 3877b53e429f..a8dc9de2f7fb 100644
> --- a/fs/nfsd/nfs4callback.c
> +++ b/fs/nfsd/nfs4callback.c
> @@ -1036,8 +1036,7 @@ static void nfsd41_cb_inflight_begin(struct nfs4_client *clp)
> static void nfsd41_cb_inflight_end(struct nfs4_client *clp)
> {
>
> - if (atomic_dec_and_test(&clp->cl_cb_inflight))
> - wake_up_var(&clp->cl_cb_inflight);
> + atomic_dec_and_wake_up(&clp->cl_cb_inflight);
> }
>
> static void nfsd41_cb_inflight_wait_complete(struct nfs4_client *clp)
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 741b9449f727..9fbf7c8f0a3e 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -4739,7 +4739,7 @@ static void init_nfs4_replay(struct nfs4_replay *rp)
> rp->rp_status = nfserr_serverfault;
> rp->rp_buflen = 0;
> rp->rp_buf = rp->rp_ibuf;
> - atomic_set(&rp->rp_locked, RP_UNLOCKED);
> + rp->rp_locked = RP_UNLOCKED;
> }
>
> static int nfsd4_cstate_assign_replay(struct nfsd4_compound_state *cstate,
> @@ -4747,9 +4747,9 @@ static int nfsd4_cstate_assign_replay(struct nfsd4_compound_state *cstate,
> {
> if (!nfsd4_has_session(cstate)) {
> wait_var_event(&so->so_replay.rp_locked,
> - atomic_cmpxchg(&so->so_replay.rp_locked,
> - RP_UNLOCKED, RP_LOCKED) != RP_LOCKED);
> - if (atomic_read(&so->so_replay.rp_locked) == RP_UNHASHED)
> + cmpxchg(&so->so_replay.rp_locked,
> + RP_UNLOCKED, RP_LOCKED) != RP_LOCKED);
nit: try_cmpxchg() generates more efficient assembly. Can we switch to
that here too?
> + if (so->so_replay.rp_locked == RP_UNHASHED)
> return -EAGAIN;
> cstate->replay_owner = nfs4_get_stateowner(so);
> }
> @@ -4762,9 +4762,7 @@ void nfsd4_cstate_clear_replay(struct nfsd4_compound_state *cstate)
>
> if (so != NULL) {
> cstate->replay_owner = NULL;
> - atomic_set(&so->so_replay.rp_locked, RP_UNLOCKED);
> - smp_mb__after_atomic();
> - wake_up_var(&so->so_replay.rp_locked);
> + store_release_wake_up(&so->so_replay.rp_locked, RP_UNLOCKED);
> nfs4_put_stateowner(so);
> }
> }
> @@ -5069,9 +5067,7 @@ move_to_close_lru(struct nfs4_ol_stateid *s, struct net *net)
> * Some threads with a reference might be waiting for rp_locked,
> * so tell them to stop waiting.
> */
> - atomic_set(&oo->oo_owner.so_replay.rp_locked, RP_UNHASHED);
> - smp_mb__after_atomic();
> - wake_up_var(&oo->oo_owner.so_replay.rp_locked);
> + store_release_wake_up(&oo->oo_owner.so_replay.rp_locked, RP_UNHASHED);
> wait_event(close_wq, refcount_read(&s->st_stid.sc_count) == 2);
>
> release_all_access(s);
> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
> index e16bb3717fb9..ba30b2335b66 100644
> --- a/fs/nfsd/state.h
> +++ b/fs/nfsd/state.h
> @@ -505,7 +505,7 @@ struct nfs4_replay {
> unsigned int rp_buflen;
> char *rp_buf;
> struct knfsd_fh rp_openfh;
> - atomic_t rp_locked;
> + int rp_locked;
> char rp_ibuf[NFSD4_REPLAY_ISIZE];
> };
>
Looks good otherwise.
Reviewed-by: Jeff Layton <jlayton@kernel.org>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] sunrpc/svc: use store_release_wake_up()
2024-12-06 2:55 ` [PATCH 2/2] sunrpc/svc: use store_release_wake_up() NeilBrown
@ 2024-12-06 5:41 ` Jeff Layton
0 siblings, 0 replies; 8+ messages in thread
From: Jeff Layton @ 2024-12-06 5:41 UTC (permalink / raw)
To: NeilBrown, Chuck Lever; +Cc: linux-nfs, Olga Kornievskaia, Dai Ngo, Tom Talpey
On Fri, 2024-12-06 at 13:55 +1100, NeilBrown wrote:
> svc_thread_init_status() contains an open-coded
> store_release_wake_up(). It is cleaner to use that function directly
> rather than needing to remember the barrier.
>
> Signed-off-by: NeilBrown <neilb@suse.de>
> ---
> include/linux/sunrpc/svc.h | 7 +------
> 1 file changed, 1 insertion(+), 6 deletions(-)
>
> diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
> index e68fecf6eab5..e4f09f58d58c 100644
> --- a/include/linux/sunrpc/svc.h
> +++ b/include/linux/sunrpc/svc.h
> @@ -327,12 +327,7 @@ static inline bool svc_thread_should_stop(struct svc_rqst *rqstp)
> */
> static inline void svc_thread_init_status(struct svc_rqst *rqstp, int err)
> {
> - rqstp->rq_err = err;
> - /* memory barrier ensures assignment to error above is visible before
> - * waitqueue_active() test below completes.
> - */
> - smp_mb();
> - wake_up_var(&rqstp->rq_err);
> + store_release_wake_up(&rqstp->rq_err, err);
> if (err)
> kthread_exit(1);
> }
Reviewed-by: Jeff Layton <jlayton@kernel.org>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] nfsd: use new wake_up_var interfaces.
2024-12-06 5:40 ` Jeff Layton
@ 2024-12-06 6:10 ` NeilBrown
2024-12-06 15:27 ` Jeff Layton
0 siblings, 1 reply; 8+ messages in thread
From: NeilBrown @ 2024-12-06 6:10 UTC (permalink / raw)
To: Jeff Layton
Cc: Chuck Lever, linux-nfs, Olga Kornievskaia, Dai Ngo, Tom Talpey
On Fri, 06 Dec 2024, Jeff Layton wrote:
> On Fri, 2024-12-06 at 13:55 +1100, NeilBrown wrote:
> > The wake_up_var interface is fragile as barriers are sometimes needed.
> > There are now new interfaces so that most wake-ups can use an interface
> > that is guaranteed to have all barriers needed.
> >
> > This patch changes the wake up on cl_cb_inflight to use
> > atomic_dec_and_wake_up().
> >
> > It also changes the wake up on rp_locked to use store_release_wake_up().
> > This involves changing rp_locked from atomic_t to int.
> >
> > Signed-off-by: NeilBrown <neilb@suse.de>
> > ---
> > fs/nfsd/nfs4callback.c | 3 +--
> > fs/nfsd/nfs4state.c | 16 ++++++----------
> > fs/nfsd/state.h | 2 +-
> > 3 files changed, 8 insertions(+), 13 deletions(-)
> >
> > diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
> > index 3877b53e429f..a8dc9de2f7fb 100644
> > --- a/fs/nfsd/nfs4callback.c
> > +++ b/fs/nfsd/nfs4callback.c
> > @@ -1036,8 +1036,7 @@ static void nfsd41_cb_inflight_begin(struct nfs4_client *clp)
> > static void nfsd41_cb_inflight_end(struct nfs4_client *clp)
> > {
> >
> > - if (atomic_dec_and_test(&clp->cl_cb_inflight))
> > - wake_up_var(&clp->cl_cb_inflight);
> > + atomic_dec_and_wake_up(&clp->cl_cb_inflight);
> > }
> >
> > static void nfsd41_cb_inflight_wait_complete(struct nfs4_client *clp)
> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > index 741b9449f727..9fbf7c8f0a3e 100644
> > --- a/fs/nfsd/nfs4state.c
> > +++ b/fs/nfsd/nfs4state.c
> > @@ -4739,7 +4739,7 @@ static void init_nfs4_replay(struct nfs4_replay *rp)
> > rp->rp_status = nfserr_serverfault;
> > rp->rp_buflen = 0;
> > rp->rp_buf = rp->rp_ibuf;
> > - atomic_set(&rp->rp_locked, RP_UNLOCKED);
> > + rp->rp_locked = RP_UNLOCKED;
> > }
> >
> > static int nfsd4_cstate_assign_replay(struct nfsd4_compound_state *cstate,
> > @@ -4747,9 +4747,9 @@ static int nfsd4_cstate_assign_replay(struct nfsd4_compound_state *cstate,
> > {
> > if (!nfsd4_has_session(cstate)) {
> > wait_var_event(&so->so_replay.rp_locked,
> > - atomic_cmpxchg(&so->so_replay.rp_locked,
> > - RP_UNLOCKED, RP_LOCKED) != RP_LOCKED);
> > - if (atomic_read(&so->so_replay.rp_locked) == RP_UNHASHED)
> > + cmpxchg(&so->so_replay.rp_locked,
> > + RP_UNLOCKED, RP_LOCKED) != RP_LOCKED);
>
> nit: try_cmpxchg() generates more efficient assembly. Can we switch to
> that here too?
Does it? try_cmpxchg() makes loops smaller (as described in
atomic_t.txt). I think it wins when the "old" value has to be updated
each time around the loop. In this case the "old" value is always the
same.
NeilBrown
>
> > + if (so->so_replay.rp_locked == RP_UNHASHED)
> > return -EAGAIN;
> > cstate->replay_owner = nfs4_get_stateowner(so);
> > }
> > @@ -4762,9 +4762,7 @@ void nfsd4_cstate_clear_replay(struct nfsd4_compound_state *cstate)
> >
> > if (so != NULL) {
> > cstate->replay_owner = NULL;
> > - atomic_set(&so->so_replay.rp_locked, RP_UNLOCKED);
> > - smp_mb__after_atomic();
> > - wake_up_var(&so->so_replay.rp_locked);
> > + store_release_wake_up(&so->so_replay.rp_locked, RP_UNLOCKED);
> > nfs4_put_stateowner(so);
> > }
> > }
> > @@ -5069,9 +5067,7 @@ move_to_close_lru(struct nfs4_ol_stateid *s, struct net *net)
> > * Some threads with a reference might be waiting for rp_locked,
> > * so tell them to stop waiting.
> > */
> > - atomic_set(&oo->oo_owner.so_replay.rp_locked, RP_UNHASHED);
> > - smp_mb__after_atomic();
> > - wake_up_var(&oo->oo_owner.so_replay.rp_locked);
> > + store_release_wake_up(&oo->oo_owner.so_replay.rp_locked, RP_UNHASHED);
> > wait_event(close_wq, refcount_read(&s->st_stid.sc_count) == 2);
> >
> > release_all_access(s);
> > diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
> > index e16bb3717fb9..ba30b2335b66 100644
> > --- a/fs/nfsd/state.h
> > +++ b/fs/nfsd/state.h
> > @@ -505,7 +505,7 @@ struct nfs4_replay {
> > unsigned int rp_buflen;
> > char *rp_buf;
> > struct knfsd_fh rp_openfh;
> > - atomic_t rp_locked;
> > + int rp_locked;
> > char rp_ibuf[NFSD4_REPLAY_ISIZE];
> > };
> >
>
> Looks good otherwise.
>
> Reviewed-by: Jeff Layton <jlayton@kernel.org>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] nfsd: use new wake_up_var interfaces.
2024-12-06 6:10 ` NeilBrown
@ 2024-12-06 15:27 ` Jeff Layton
0 siblings, 0 replies; 8+ messages in thread
From: Jeff Layton @ 2024-12-06 15:27 UTC (permalink / raw)
To: NeilBrown; +Cc: Chuck Lever, linux-nfs, Olga Kornievskaia, Dai Ngo, Tom Talpey
On Fri, 2024-12-06 at 17:10 +1100, NeilBrown wrote:
> On Fri, 06 Dec 2024, Jeff Layton wrote:
> > On Fri, 2024-12-06 at 13:55 +1100, NeilBrown wrote:
> > > The wake_up_var interface is fragile as barriers are sometimes needed.
> > > There are now new interfaces so that most wake-ups can use an interface
> > > that is guaranteed to have all barriers needed.
> > >
> > > This patch changes the wake up on cl_cb_inflight to use
> > > atomic_dec_and_wake_up().
> > >
> > > It also changes the wake up on rp_locked to use store_release_wake_up().
> > > This involves changing rp_locked from atomic_t to int.
> > >
> > > Signed-off-by: NeilBrown <neilb@suse.de>
> > > ---
> > > fs/nfsd/nfs4callback.c | 3 +--
> > > fs/nfsd/nfs4state.c | 16 ++++++----------
> > > fs/nfsd/state.h | 2 +-
> > > 3 files changed, 8 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
> > > index 3877b53e429f..a8dc9de2f7fb 100644
> > > --- a/fs/nfsd/nfs4callback.c
> > > +++ b/fs/nfsd/nfs4callback.c
> > > @@ -1036,8 +1036,7 @@ static void nfsd41_cb_inflight_begin(struct nfs4_client *clp)
> > > static void nfsd41_cb_inflight_end(struct nfs4_client *clp)
> > > {
> > >
> > > - if (atomic_dec_and_test(&clp->cl_cb_inflight))
> > > - wake_up_var(&clp->cl_cb_inflight);
> > > + atomic_dec_and_wake_up(&clp->cl_cb_inflight);
> > > }
> > >
> > > static void nfsd41_cb_inflight_wait_complete(struct nfs4_client *clp)
> > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > > index 741b9449f727..9fbf7c8f0a3e 100644
> > > --- a/fs/nfsd/nfs4state.c
> > > +++ b/fs/nfsd/nfs4state.c
> > > @@ -4739,7 +4739,7 @@ static void init_nfs4_replay(struct nfs4_replay *rp)
> > > rp->rp_status = nfserr_serverfault;
> > > rp->rp_buflen = 0;
> > > rp->rp_buf = rp->rp_ibuf;
> > > - atomic_set(&rp->rp_locked, RP_UNLOCKED);
> > > + rp->rp_locked = RP_UNLOCKED;
> > > }
> > >
> > > static int nfsd4_cstate_assign_replay(struct nfsd4_compound_state *cstate,
> > > @@ -4747,9 +4747,9 @@ static int nfsd4_cstate_assign_replay(struct nfsd4_compound_state *cstate,
> > > {
> > > if (!nfsd4_has_session(cstate)) {
> > > wait_var_event(&so->so_replay.rp_locked,
> > > - atomic_cmpxchg(&so->so_replay.rp_locked,
> > > - RP_UNLOCKED, RP_LOCKED) != RP_LOCKED);
> > > - if (atomic_read(&so->so_replay.rp_locked) == RP_UNHASHED)
> > > + cmpxchg(&so->so_replay.rp_locked,
> > > + RP_UNLOCKED, RP_LOCKED) != RP_LOCKED);
> >
> > nit: try_cmpxchg() generates more efficient assembly. Can we switch to
> > that here too?
>
> Does it? try_cmpxchg() makes loops smaller (as described in
> atomic_t.txt). I think it wins when the "old" value has to be updated
> each time around the loop. In this case the "old" value is always the
> same.
>
>
In most cases, it does, because we have to return "old" in the case of
the traditional cmpxchg() operation. From atomic_t.txt:
int atomic_cmpxchg(atomic_t *ptr, int old, int new)
{
(void)atomic_try_cmpxchg(ptr, &old, new);
return old;
}
That said, in this case it my not be a win. You need something to
return that value anyway, so it can properly act as a wait_var_event()
condition.
> >
> > > + if (so->so_replay.rp_locked == RP_UNHASHED)
> > > return -EAGAIN;
> > > cstate->replay_owner = nfs4_get_stateowner(so);
> > > }
> > > @@ -4762,9 +4762,7 @@ void nfsd4_cstate_clear_replay(struct nfsd4_compound_state *cstate)
> > >
> > > if (so != NULL) {
> > > cstate->replay_owner = NULL;
> > > - atomic_set(&so->so_replay.rp_locked, RP_UNLOCKED);
> > > - smp_mb__after_atomic();
> > > - wake_up_var(&so->so_replay.rp_locked);
> > > + store_release_wake_up(&so->so_replay.rp_locked, RP_UNLOCKED);
> > > nfs4_put_stateowner(so);
> > > }
> > > }
> > > @@ -5069,9 +5067,7 @@ move_to_close_lru(struct nfs4_ol_stateid *s, struct net *net)
> > > * Some threads with a reference might be waiting for rp_locked,
> > > * so tell them to stop waiting.
> > > */
> > > - atomic_set(&oo->oo_owner.so_replay.rp_locked, RP_UNHASHED);
> > > - smp_mb__after_atomic();
> > > - wake_up_var(&oo->oo_owner.so_replay.rp_locked);
> > > + store_release_wake_up(&oo->oo_owner.so_replay.rp_locked, RP_UNHASHED);
> > > wait_event(close_wq, refcount_read(&s->st_stid.sc_count) == 2);
> > >
> > > release_all_access(s);
> > > diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
> > > index e16bb3717fb9..ba30b2335b66 100644
> > > --- a/fs/nfsd/state.h
> > > +++ b/fs/nfsd/state.h
> > > @@ -505,7 +505,7 @@ struct nfs4_replay {
> > > unsigned int rp_buflen;
> > > char *rp_buf;
> > > struct knfsd_fh rp_openfh;
> > > - atomic_t rp_locked;
> > > + int rp_locked;
> > > char rp_ibuf[NFSD4_REPLAY_ISIZE];
> > > };
> > >
> >
> > Looks good otherwise.
> >
> > Reviewed-by: Jeff Layton <jlayton@kernel.org>
> >
>
--
Jeff Layton <jlayton@kernel.org>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 0/2] nfsd: use new wake_up_var interface
2024-12-06 2:55 [PATCH 0/2] nfsd: use new wake_up_var interface NeilBrown
2024-12-06 2:55 ` [PATCH 1/2] nfsd: use new wake_up_var interfaces NeilBrown
2024-12-06 2:55 ` [PATCH 2/2] sunrpc/svc: use store_release_wake_up() NeilBrown
@ 2024-12-06 21:56 ` cel
2 siblings, 0 replies; 8+ messages in thread
From: cel @ 2024-12-06 21:56 UTC (permalink / raw)
To: Jeff Layton, NeilBrown
Cc: Chuck Lever, linux-nfs, Olga Kornievskaia, Dai Ngo, Tom Talpey
From: Chuck Lever <chuck.lever@oracle.com>
On Fri, 06 Dec 2024 13:55:51 +1100, NeilBrown wrote:
> wake_up_var() in a fragile interface as it sometimes needs a memory
> barrier before it. Recently new interfaces were added which include all
> required barriers.
>
> These patches update nfsd and svc parrts of sunrpc to use new interfaces
> where appropriate.
>
> [...]
Applied to nfsd-testing for v6.14, thanks!
[1/2] nfsd: use new wake_up_var interfaces.
commit: 70b79974d849d8ff1f60295c03600f9c9db4da05
[2/2] sunrpc/svc: use store_release_wake_up()
commit: 101543f023804bc0e62a2fdb15e143c022fedc7c
--
Chuck Lever
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2024-12-06 21:56 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-12-06 2:55 [PATCH 0/2] nfsd: use new wake_up_var interface NeilBrown
2024-12-06 2:55 ` [PATCH 1/2] nfsd: use new wake_up_var interfaces NeilBrown
2024-12-06 5:40 ` Jeff Layton
2024-12-06 6:10 ` NeilBrown
2024-12-06 15:27 ` Jeff Layton
2024-12-06 2:55 ` [PATCH 2/2] sunrpc/svc: use store_release_wake_up() NeilBrown
2024-12-06 5:41 ` Jeff Layton
2024-12-06 21:56 ` [PATCH 0/2] nfsd: use new wake_up_var interface cel
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox