| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <a5e0fbe428b85327793cac81207042b7d1d3f455.camel@kernel.org>
Date: Sun, 14 Dec 2025 06:55:53 +0900
From: Jeff Layton <jlayton@...nel.org>
To: Chuck Lever <cel@...nel.org>, Chuck Lever <chuck.lever@...cle.com>,
NeilBrown <neil@...wn.name>, Olga Kornievskaia <okorniev@...hat.com>, Dai
Ngo <Dai.Ngo@...cle.com>, Tom Talpey <tom@...pey.com>, Trond Myklebust
<trondmy@...nel.org>, Anna Schumaker <anna@...nel.org>
Cc: linux-nfs@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH RFC 1/6] sunrpc: split svc_set_num_threads() into two
functions
On Sat, 2025-12-13 at 14:29 -0500, Chuck Lever wrote:
>
> On Fri, Dec 12, 2025, at 5:39 PM, Jeff Layton wrote:
> > svc_set_num_threads() will set the number of running threads for a given
> > pool. If the pool argument is set to NULL however, it will distribute
> > the threads among all of the pools evenly.
> >
> > These divergent codepaths complicate the move to dynamic threading.
> > Simplify the API by splitting these two cases into different helpers:
> >
> > Add a new svc_set_pool_threads() function that sets the number of
> > threads in a single, given pool. Modify svc_set_num_threads() to
> > distribute the threads evenly between all of the pools and then call
> > svc_set_pool_threads() for each.
> >
> > Signed-off-by: Jeff Layton <jlayton@...nel.org>
> > ---
> > fs/lockd/svc.c | 4 ++--
> > fs/nfs/callback.c | 8 +++----
> > fs/nfsd/nfssvc.c | 21 ++++++++----------
> > include/linux/sunrpc/svc.h | 3 ++-
> > net/sunrpc/svc.c | 54 +++++++++++++++++++++++++++++++++++++---------
> > 5 files changed, 61 insertions(+), 29 deletions(-)
> >
> > diff --git a/fs/lockd/svc.c b/fs/lockd/svc.c
> > index
> > d68afa196535a8785bab2931c2b14f03a1174ef9..fbf132b4e08d11a91784c21ee0209fd7c149fd9d
> > 100644
> > --- a/fs/lockd/svc.c
> > +++ b/fs/lockd/svc.c
> > @@ -340,7 +340,7 @@ static int lockd_get(void)
> > return -ENOMEM;
> > }
> >
> > - error = svc_set_num_threads(serv, NULL, 1);
> > + error = svc_set_num_threads(serv, 1);
> > if (error < 0) {
> > svc_destroy(&serv);
> > return error;
> > @@ -368,7 +368,7 @@ static void lockd_put(void)
> > unregister_inet6addr_notifier(&lockd_inet6addr_notifier);
> > #endif
> >
> > - svc_set_num_threads(nlmsvc_serv, NULL, 0);
> > + svc_set_num_threads(nlmsvc_serv, 0);
> > timer_delete_sync(&nlmsvc_retry);
> > svc_destroy(&nlmsvc_serv);
> > dprintk("lockd_down: service destroyed\n");
> > diff --git a/fs/nfs/callback.c b/fs/nfs/callback.c
> > index
> > c8b837006bb27277ab34fe516f1b63992fee9b7f..44b35b7f8dc022f1d8c069eaf2f7d334c93f77fc
> > 100644
> > --- a/fs/nfs/callback.c
> > +++ b/fs/nfs/callback.c
> > @@ -119,9 +119,9 @@ static int nfs_callback_start_svc(int minorversion,
> > struct rpc_xprt *xprt,
> > if (serv->sv_nrthreads == nrservs)
> > return 0;
> >
> > - ret = svc_set_num_threads(serv, NULL, nrservs);
> > + ret = svc_set_num_threads(serv, nrservs);
> > if (ret) {
> > - svc_set_num_threads(serv, NULL, 0);
> > + svc_set_num_threads(serv, 0);
> > return ret;
> > }
> > dprintk("nfs_callback_up: service started\n");
> > @@ -242,7 +242,7 @@ int nfs_callback_up(u32 minorversion, struct
> > rpc_xprt *xprt)
> > cb_info->users++;
> > err_net:
> > if (!cb_info->users) {
> > - svc_set_num_threads(cb_info->serv, NULL, 0);
> > + svc_set_num_threads(cb_info->serv, 0);
> > svc_destroy(&cb_info->serv);
> > }
> > err_create:
> > @@ -268,7 +268,7 @@ void nfs_callback_down(int minorversion, struct net
> > *net)
> > nfs_callback_down_net(minorversion, serv, net);
> > cb_info->users--;
> > if (cb_info->users == 0) {
> > - svc_set_num_threads(serv, NULL, 0);
> > + svc_set_num_threads(serv, 0);
> > dprintk("nfs_callback_down: service destroyed\n");
> > svc_destroy(&cb_info->serv);
> > }
> > diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
> > index
> > 93f7435cafd2362d9ddb28815277c824067cb370..aafec8ff588b85b0e26d40b76ef00953dc6472b4
> > 100644
> > --- a/fs/nfsd/nfssvc.c
> > +++ b/fs/nfsd/nfssvc.c
> > @@ -594,7 +594,7 @@ void nfsd_shutdown_threads(struct net *net)
> > }
> >
> > /* Kill outstanding nfsd threads */
> > - svc_set_num_threads(serv, NULL, 0);
> > + svc_set_num_threads(serv, 0);
> > nfsd_destroy_serv(net);
> > mutex_unlock(&nfsd_mutex);
> > }
> > @@ -702,12 +702,9 @@ int nfsd_set_nrthreads(int n, int *nthreads,
> > struct net *net)
> > if (nn->nfsd_serv == NULL || n <= 0)
> > return 0;
> >
> > - /*
> > - * Special case: When n == 1, pass in NULL for the pool, so that the
> > - * change is distributed equally among them.
> > - */
> > + /* Special case: When n == 1, distribute threads equally among pools. */
> > if (n == 1)
> > - return svc_set_num_threads(nn->nfsd_serv, NULL, nthreads[0]);
> > + return svc_set_num_threads(nn->nfsd_serv, nthreads[0]);
> >
> > if (n > nn->nfsd_serv->sv_nrpools)
> > n = nn->nfsd_serv->sv_nrpools;
> > @@ -733,18 +730,18 @@ int nfsd_set_nrthreads(int n, int *nthreads,
> > struct net *net)
> >
> > /* apply the new numbers */
> > for (i = 0; i < n; i++) {
> > - err = svc_set_num_threads(nn->nfsd_serv,
> > - &nn->nfsd_serv->sv_pools[i],
> > - nthreads[i]);
> > + err = svc_set_pool_threads(nn->nfsd_serv,
> > + &nn->nfsd_serv->sv_pools[i],
> > + nthreads[i]);
> > if (err)
> > goto out;
> > }
> >
> > /* Anything undefined in array is considered to be 0 */
> > for (i = n; i < nn->nfsd_serv->sv_nrpools; ++i) {
> > - err = svc_set_num_threads(nn->nfsd_serv,
> > - &nn->nfsd_serv->sv_pools[i],
> > - 0);
> > + err = svc_set_pool_threads(nn->nfsd_serv,
> > + &nn->nfsd_serv->sv_pools[i],
> > + 0);
> > if (err)
> > goto out;
> > }
> > diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
> > index
> > 5506d20857c318774cd223272d4b0022cc19ffb8..dd5fbbf8b3d39df6c17a7624edf344557fffd32c
> > 100644
> > --- a/include/linux/sunrpc/svc.h
> > +++ b/include/linux/sunrpc/svc.h
> > @@ -446,7 +446,8 @@ struct svc_serv * svc_create_pooled(struct
> > svc_program *prog,
> > struct svc_stat *stats,
> > unsigned int bufsize,
> > int (*threadfn)(void *data));
> > -int svc_set_num_threads(struct svc_serv *, struct svc_pool *, int);
> > +int svc_set_pool_threads(struct svc_serv *, struct svc_pool *,
> > int);
> > +int svc_set_num_threads(struct svc_serv *, int);
> > int svc_pool_stats_open(struct svc_info *si, struct file *file);
> > void svc_process(struct svc_rqst *rqstp);
> > void svc_process_bc(struct rpc_rqst *req, struct svc_rqst *rqstp);
> > diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
> > index
> > 4704dce7284eccc9e2bc64cf22947666facfa86a..3fe5a7f8e57e3fa3837265ec06884b357d5373ff
> > 100644
> > --- a/net/sunrpc/svc.c
> > +++ b/net/sunrpc/svc.c
> > @@ -856,15 +856,12 @@ svc_stop_kthreads(struct svc_serv *serv, struct
> > svc_pool *pool, int nrservs)
> > }
> >
> > /**
> > - * svc_set_num_threads - adjust number of threads per RPC service
> > + * svc_set_pool_threads - adjust number of threads per pool
> > * @serv: RPC service to adjust
> > - * @pool: Specific pool from which to choose threads, or NULL
> > + * @pool: Specific pool from which to choose threads
> > * @nrservs: New number of threads for @serv (0 or less means kill all
> > threads)
> > *
> > - * Create or destroy threads to make the number of threads for @serv
> > the
> > - * given number. If @pool is non-NULL, change only threads in that
> > pool;
> > - * otherwise, round-robin between all pools for @serv. @serv's
> > - * sv_nrthreads is adjusted for each thread created or destroyed.
> > + * Create or destroy threads in @pool to bring it to @nrservs.
> > *
> > * Caller must ensure mutual exclusion between this and server startup
> > or
> > * shutdown.
> > @@ -873,12 +870,12 @@ svc_stop_kthreads(struct svc_serv *serv, struct
> > svc_pool *pool, int nrservs)
> > * starting a thread.
> > */
> > int
> > -svc_set_num_threads(struct svc_serv *serv, struct svc_pool *pool, int
> > nrservs)
> > +svc_set_pool_threads(struct svc_serv *serv, struct svc_pool *pool, int
> > nrservs)
> > {
> > if (!pool)
> > - nrservs -= serv->sv_nrthreads;
> > - else
> > - nrservs -= pool->sp_nrthreads;
> > + return -EINVAL;
> > +
> > + nrservs -= pool->sp_nrthreads;
> >
> > if (nrservs > 0)
> > return svc_start_kthreads(serv, pool, nrservs);
> > @@ -886,6 +883,43 @@ svc_set_num_threads(struct svc_serv *serv, struct
> > svc_pool *pool, int nrservs)
> > return svc_stop_kthreads(serv, pool, nrservs);
> > return 0;
> > }
> > +EXPORT_SYMBOL_GPL(svc_set_pool_threads);
> > +
> > +/**
> > + * svc_set_num_threads - adjust number of threads in serv
> > + * @serv: RPC service to adjust
> > + * @nrservs: New number of threads for @serv (0 or less means kill all
> > threads)
> > + *
> > + * Create or destroy threads in @serv to bring it to @nrservs. If there
> > + * are multiple pools then the new threads or victims will be
> > distributed
> > + * evenly among them.
> > + *
> > + * Caller must ensure mutual exclusion between this and server startup
> > or
> > + * shutdown.
> > + *
> > + * Returns zero on success or a negative errno if an error occurred
> > while
> > + * starting a thread.
> > + */
> > +int
> > +svc_set_num_threads(struct svc_serv *serv, int nrservs)
> > +{
> > + int base = nrservs / serv->sv_nrpools;
> > + int remain = nrservs % serv->sv_nrpools;
> > + int i, err;
> > +
> > + for (i = 0; i < serv->sv_nrpools; ++i) {
>
> If sv_nrpools happens to be zero, then the loop doesn't
> execute at all, and err is left containing stack garbage.
> Is sv_nrpools guaranteed to be non-zero? If not then err
> needs to be initialized before the loop runs. I see that
> nfsd_set_nrthreads() in fs/nfsd/nfssvc.c has "int err = 0"
> for a similar loop pattern.
>
sv_nrpools should always be non-zero. There are many places in the rpc
layer that depend on having at least one pool. From __svc_create:
serv->sv_nrpools = npools;
serv->sv_pools =
kcalloc(serv->sv_nrpools, sizeof(struct svc_pool),
GFP_KERNEL);
if (!serv->sv_pools) {
kfree(serv);
return NULL;
}
I think kcalloc returns NULL if you pass it 0 for "n", so creation
should fail if that happens. None of the in-tree callers ever pass in 0
for the npools, but maybe that's worth an explicit check in
__svc_create(). I can add one.
>
> > + int threads = base;
> > +
> > + if (remain) {
> > + ++threads;
> > + --remain;
> > + }
> > + err = svc_set_pool_threads(serv, &serv->sv_pools[i], threads);
> > + if (err)
> > + break;
> > + }
> > + return err;
> > +}
> > EXPORT_SYMBOL_GPL(svc_set_num_threads);
> >
> > /**
> >
> > --
> > 2.52.0
--
Jeff Layton <jlayton@...nel.org>
Powered by blists - more mailing lists