On Tue, 2023-07-18 at 16:38 +1000, NeilBrown wrote:
> svc threads are currently stopped using kthread_stop(). This requires
> identifying a specific thread. However we don't care which thread
> stops, just as long as one does.
>
> So instead, set a flag in the svc_pool to say that a thread needs to
> die, and have each thread check this flag instead of calling
> kthread_should_stop(). The first to find it clear the flag and moves
> towards shutting down.
>
> This removes an explicit dependency on sp_all_threads which will make a
> future patch simpler.
>
> Signed-off-by: NeilBrown <neilb@xxxxxxx>
> ---
> fs/lockd/svc.c | 4 ++--
> fs/lockd/svclock.c | 4 ++--
> fs/nfs/callback.c | 2 +-
> fs/nfsd/nfs4proc.c | 8 +++++---
> fs/nfsd/nfssvc.c | 2 +-
> include/linux/lockd/lockd.h | 2 +-
> include/linux/sunrpc/svc.h | 22 +++++++++++++++++++++-
> include/trace/events/sunrpc.h | 5 +++--
> net/sunrpc/svc.c | 39 +++++++++++++++++----------------------
> net/sunrpc/svc_xprt.c | 8 +++-----
> 10 files changed, 56 insertions(+), 40 deletions(-)
>
> diff --git a/fs/lockd/svc.c b/fs/lockd/svc.c
> index 4f55cd42c2e6..30543edd2309 100644
> --- a/fs/lockd/svc.c
> +++ b/fs/lockd/svc.c
> @@ -134,12 +134,12 @@ lockd(void *vrqstp)
> * The main request loop. We don't terminate until the last
> * NFS mount or NFS daemon has gone away.
> */
> - while (!kthread_should_stop()) {
> + while (!svc_thread_should_stop(rqstp)) {
>
> /* update sv_maxconn if it has changed */
> rqstp->rq_server->sv_maxconn = nlm_max_connections;
>
> - nlmsvc_retry_blocked();
> + nlmsvc_retry_blocked(rqstp);
>
> /*
> * Find any work to do, such as a socket with data available,
> diff --git a/fs/lockd/svclock.c b/fs/lockd/svclock.c
> index 3d7bd5c04b36..fd399c9bea5c 100644
> --- a/fs/lockd/svclock.c
> +++ b/fs/lockd/svclock.c
> @@ -1009,13 +1009,13 @@ retry_deferred_block(struct nlm_block *block)
> * be retransmitted.
> */
> void
> -nlmsvc_retry_blocked(void)
> +nlmsvc_retry_blocked(struct svc_rqst *rqstp)
> {
> unsigned long timeout = MAX_SCHEDULE_TIMEOUT;
> struct nlm_block *block;
>
> spin_lock(&nlm_blocked_lock);
> - while (!list_empty(&nlm_blocked) && !kthread_should_stop()) {
> + while (!list_empty(&nlm_blocked) && !svc_thread_should_stop(rqstp)) {
> block = list_entry(nlm_blocked.next, struct nlm_block, b_list);
>
> if (block->b_when == NLM_NEVER)
> diff --git a/fs/nfs/callback.c b/fs/nfs/callback.c
> index 660cec36c45c..c58ec2e66aaf 100644
> --- a/fs/nfs/callback.c
> +++ b/fs/nfs/callback.c
> @@ -78,7 +78,7 @@ nfs4_callback_svc(void *vrqstp)
>
> set_freezable();
>
> - while (!kthread_should_stop()) {
> + while (!svc_thread_should_stop(rqstp)) {
> /*
> * Listen for a request on the socket
> */
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index 157488290676..66024ed06181 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -1306,7 +1306,8 @@ extern void nfs_sb_deactive(struct super_block *sb);
> * setup a work entry in the ssc delayed unmount list.
> */
> static __be32 nfsd4_ssc_setup_dul(struct nfsd_net *nn, char *ipaddr,
> - struct nfsd4_ssc_umount_item **nsui)
> + struct nfsd4_ssc_umount_item **nsui,
> + struct svc_rqst *rqstp)
> {
> struct nfsd4_ssc_umount_item *ni = NULL;
> struct nfsd4_ssc_umount_item *work = NULL;
> @@ -1329,7 +1330,7 @@ static __be32 nfsd4_ssc_setup_dul(struct nfsd_net *nn, char *ipaddr,
> spin_unlock(&nn->nfsd_ssc_lock);
>
> /* allow 20secs for mount/unmount for now - revisit */
> - if (kthread_should_stop() ||
> + if (svc_thread_should_stop(rqstp) ||
> (schedule_timeout(20*HZ) == 0)) {
> finish_wait(&nn->nfsd_ssc_waitq, &wait);
> kfree(work);
> @@ -1445,7 +1446,7 @@ nfsd4_interssc_connect(struct nl4_server *nss, struct svc_rqst *rqstp,
> goto out_free_rawdata;
> snprintf(dev_name, len + 5, "%s%s%s:/", startsep, ipaddr, endsep);
>
> - status = nfsd4_ssc_setup_dul(nn, ipaddr, nsui);
> + status = nfsd4_ssc_setup_dul(nn, ipaddr, nsui, rqstp);
> if (status)
> goto out_free_devname;
> if ((*nsui)->nsui_vfsmount)
> @@ -1620,6 +1621,7 @@ static ssize_t _nfsd_copy_file_range(struct nfsd4_copy *copy,
> if (bytes_total == 0)
> bytes_total = ULLONG_MAX;
> do {
> + /* Only async copies can be stopped here */
> if (kthread_should_stop())
> break;
> bytes_copied = nfsd_copy_file_range(src, src_pos, dst, dst_pos,
> diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
> index b536b254c59e..9b282c89ea87 100644
> --- a/fs/nfsd/nfssvc.c
> +++ b/fs/nfsd/nfssvc.c
> @@ -971,7 +971,7 @@ nfsd(void *vrqstp)
> /*
> * The main request loop
> */
> - while (!kthread_should_stop()) {
> + while (!svc_thread_should_stop(rqstp)) {
> /* Update sv_maxconn if it has changed */
> rqstp->rq_server->sv_maxconn = nn->max_connections;
>
> diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/lockd.h
> index 0f016d69c996..9f565416d186 100644
> --- a/include/linux/lockd/lockd.h
> +++ b/include/linux/lockd/lockd.h
> @@ -282,7 +282,7 @@ __be32 nlmsvc_testlock(struct svc_rqst *, struct nlm_file *,
> struct nlm_host *, struct nlm_lock *,
> struct nlm_lock *, struct nlm_cookie *);
> __be32 nlmsvc_cancel_blocked(struct net *net, struct nlm_file *, struct nlm_lock *);
> -void nlmsvc_retry_blocked(void);
> +void nlmsvc_retry_blocked(struct svc_rqst *rqstp);
> void nlmsvc_traverse_blocks(struct nlm_host *, struct nlm_file *,
> nlm_host_match_fn_t match);
> void nlmsvc_grant_reply(struct nlm_cookie *, __be32);
> diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
> index 15d468d852b5..ea3ce1315416 100644
> --- a/include/linux/sunrpc/svc.h
> +++ b/include/linux/sunrpc/svc.h
> @@ -51,6 +51,8 @@ struct svc_pool {
> enum {
> SP_TASK_PENDING, /* still work to do even if no xprt is queued */
> SP_CONGESTED, /* all threads are busy, none idle */
> + SP_NEED_VICTIM, /* One thread needs to agree to exit */
> + SP_VICTIM_REMAINS, /* One thread needs to actually exit */
> };
>
> /*
> @@ -275,7 +277,7 @@ enum {
> RQ_DROPME, /* drop current reply */
> RQ_SPLICE_OK, /* turned off in gss privacy to prevent encrypting page
> * cache pages */
> - RQ_VICTIM, /* about to be shut down */
> + RQ_VICTIM, /* Have agreed to shut down */
> RQ_BUSY, /* request is busy */
> RQ_DATA, /* request has data */
> };
> @@ -315,6 +317,24 @@ static inline struct sockaddr *svc_daddr(const struct svc_rqst *rqst)
> return (struct sockaddr *) &rqst->rq_daddr;
> }
>
> +/**
> + * svc_thread_should_stop - check if this thread should stop
> + * @rqstp: the thread that might need to stop
> + *
> + * To stop an svc thread, the pool flags SP_NEED_VICTIM and SP_VICTIM_REMAINS
> + * are set. The firs thread which sees SP_NEED_VICTIM clear it becoming
> + * the victim using this function. It should then promptly call
> + * svc_exit_thread() which completes the process, clearing SP_VICTIM_REMAINS
> + * so the task waiting for a thread to exit can wake and continue.
> + */
> +static inline bool svc_thread_should_stop(struct svc_rqst *rqstp)
> +{
> + if (test_and_clear_bit(SP_NEED_VICTIM, &rqstp->rq_pool->sp_flags))
> + set_bit(RQ_VICTIM, &rqstp->rq_flags);
> +
> + return test_bit(RQ_VICTIM, &rqstp->rq_flags);
> +}
> +
> struct svc_deferred_req {
> u32 prot; /* protocol (UDP or TCP) */
> struct svc_xprt *xprt;
> diff --git a/include/trace/events/sunrpc.h b/include/trace/events/sunrpc.h
> index 60c8e03268d4..c79375e37dc2 100644
> --- a/include/trace/events/sunrpc.h
> +++ b/include/trace/events/sunrpc.h
> @@ -2041,8 +2041,9 @@ TRACE_EVENT(svc_xprt_enqueue,
> #define show_svc_pool_flags(x) \
> __print_flags(x, "|", \
> { BIT(SP_TASK_PENDING), "TASK_PENDING" }, \
> - { BIT(SP_CONGESTED), "CONGESTED" })
> -
> + { BIT(SP_CONGESTED), "CONGESTED" }, \
> + { BIT(SP_NEED_VICTIM), "NEED_VICTIM" }, \
> + { BIT(SP_VICTIM_REMAINS), "VICTIM_REMAINS" })
> DECLARE_EVENT_CLASS(svc_pool_scheduler_class,
> TP_PROTO(
> const struct svc_rqst *rqstp
> diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
> index 56b4a88776d5..b18175ef74ec 100644
> --- a/net/sunrpc/svc.c
> +++ b/net/sunrpc/svc.c
> @@ -731,19 +731,22 @@ svc_pool_next(struct svc_serv *serv, struct svc_pool *pool, unsigned int *state)
> return pool ? pool : &serv->sv_pools[(*state)++ % serv->sv_nrpools];
> }
>
> -static struct task_struct *
> +static struct svc_pool *
> svc_pool_victim(struct svc_serv *serv, struct svc_pool *pool, unsigned int *state)
> {
> unsigned int i;
> - struct task_struct *task = NULL;
>
> if (pool != NULL) {
> spin_lock_bh(&pool->sp_lock);
> + if (pool->sp_nrthreads)
> + goto found_pool;
> + spin_unlock_bh(&pool->sp_lock);
> + return NULL;
> } else {
> for (i = 0; i < serv->sv_nrpools; i++) {
> pool = &serv->sv_pools[--(*state) % serv->sv_nrpools];
> spin_lock_bh(&pool->sp_lock);
> - if (!list_empty(&pool->sp_all_threads))
> + if (pool->sp_nrthreads)
> goto found_pool;
> spin_unlock_bh(&pool->sp_lock);
> }
> @@ -751,16 +754,10 @@ svc_pool_victim(struct svc_serv *serv, struct svc_pool *pool, unsigned int *stat
> }
>
> found_pool:
> - if (!list_empty(&pool->sp_all_threads)) {
> - struct svc_rqst *rqstp;
> -
> - rqstp = list_entry(pool->sp_all_threads.next, struct svc_rqst, rq_all);
> - set_bit(RQ_VICTIM, &rqstp->rq_flags);
> - list_del_rcu(&rqstp->rq_all);
> - task = rqstp->rq_task;
> - }
> + set_bit(SP_VICTIM_REMAINS, &pool->sp_flags);
> + set_bit(SP_NEED_VICTIM, &pool->sp_flags);
> spin_unlock_bh(&pool->sp_lock);
> - return task;
> + return pool;
> }
>
> static int
> @@ -801,18 +798,16 @@ svc_start_kthreads(struct svc_serv *serv, struct svc_pool *pool, int nrservs)
> static int
> svc_stop_kthreads(struct svc_serv *serv, struct svc_pool *pool, int nrservs)
> {
> - struct svc_rqst *rqstp;
> - struct task_struct *task;
> unsigned int state = serv->sv_nrthreads-1;
> + struct svc_pool *vpool;
>
> do {
> - task = svc_pool_victim(serv, pool, &state);
> - if (task == NULL)
> + vpool = svc_pool_victim(serv, pool, &state);
> + if (!vpool)
> break;
> - rqstp = kthread_data(task);
> - /* Did we lose a race to svo_function threadfn? */
> - if (kthread_stop(task) == -EINTR)
> - svc_exit_thread(rqstp);
> + svc_pool_wake_idle_thread(serv, vpool);
> + wait_on_bit(&vpool->sp_flags, SP_VICTIM_REMAINS,
> + TASK_IDLE);
I'm not sure about this bit. Previously (AFAICT) we didn't shut down the
threads serially. With this change, we will be. Granted we only have to
wait until SP_VICTIM_REMAINS is clear. Does that happen early enough in
the shutdown process that you aren't worried about this?
> nrservs++;
> } while (nrservs < 0);
> return 0;
> @@ -932,8 +927,6 @@ svc_exit_thread(struct svc_rqst *rqstp)
>
> spin_lock_bh(&pool->sp_lock);
> pool->sp_nrthreads--;
> - if (!test_and_set_bit(RQ_VICTIM, &rqstp->rq_flags))
> - list_del_rcu(&rqstp->rq_all);
> spin_unlock_bh(&pool->sp_lock);
>
> spin_lock_bh(&serv->sv_lock);
> @@ -941,6 +934,8 @@ svc_exit_thread(struct svc_rqst *rqstp)
> spin_unlock_bh(&serv->sv_lock);
> svc_sock_update_bufs(serv);
>
> + if (svc_thread_should_stop(rqstp))
> + clear_and_wake_up_bit(SP_VICTIM_REMAINS, &pool->sp_flags);
> svc_rqst_free(rqstp);
>
> svc_put(serv);
> diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
> index 4fdf1aaa514b..948605e7043b 100644
> --- a/net/sunrpc/svc_xprt.c
> +++ b/net/sunrpc/svc_xprt.c
> @@ -595,8 +595,6 @@ void svc_wake_up(struct svc_serv *serv)
> smp_wmb();
> return;
> }
> -
> - trace_svc_wake_up(rqstp);
> }
> EXPORT_SYMBOL_GPL(svc_wake_up);
>
> @@ -688,7 +686,7 @@ static bool svc_alloc_arg(struct svc_rqst *rqstp)
> continue;
>
> set_current_state(TASK_IDLE);
> - if (kthread_should_stop()) {
> + if (svc_thread_should_stop(rqstp)) {
> set_current_state(TASK_RUNNING);
> return false;
> }
> @@ -726,7 +724,7 @@ rqst_should_sleep(struct svc_rqst *rqstp)
> return false;
>
> /* are we shutting down? */
> - if (kthread_should_stop())
> + if (svc_thread_should_stop(rqstp))
> return false;
>
> /* are we freezing? */
> @@ -787,7 +785,7 @@ static void svc_wait_for_work(struct svc_rqst *rqstp)
> }
> #endif
>
> - if (!kthread_should_stop())
> + if (!svc_thread_should_stop(rqstp))
> percpu_counter_inc(&pool->sp_threads_no_work);
> return;
> out_found:
>
>
--
Jeff Layton <jlayton@xxxxxxxxxx>
