On Tue, Jul 18, 2023 at 04:38:08PM +1000, NeilBrown wrote:
> No callers of svc_pool_wake_idle_thread() care which thread was woken -
> except one that wants to trace the wakeup. For now we drop that
> tracepoint.
That's an important tracepoint, IMO.
It might be better to have svc_pool_wake_idle_thread() return void
right from it's introduction, and move the tracepoint into that
function. I can do that and respin if you agree.
> One caller wants to know if anything was woken to set SP_CONGESTED, so
> set that inside the function instead.
>
> Now svc_pool_wake_idle_thread() can "return" void.
>
> Signed-off-by: NeilBrown <neilb@xxxxxxx>
> ---
> include/linux/sunrpc/svc.h | 2 +-
> net/sunrpc/svc.c | 13 +++++--------
> net/sunrpc/svc_xprt.c | 18 +++---------------
> 3 files changed, 9 insertions(+), 24 deletions(-)
>
> diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
> index ea3ce1315416..b39c613fbe06 100644
> --- a/include/linux/sunrpc/svc.h
> +++ b/include/linux/sunrpc/svc.h
> @@ -454,7 +454,7 @@ int svc_register(const struct svc_serv *, struct net *, const int,
>
> void svc_wake_up(struct svc_serv *);
> void svc_reserve(struct svc_rqst *rqstp, int space);
> -struct svc_rqst *svc_pool_wake_idle_thread(struct svc_serv *serv,
> +void svc_pool_wake_idle_thread(struct svc_serv *serv,
> struct svc_pool *pool);
> struct svc_pool *svc_pool_for_cpu(struct svc_serv *serv);
> char * svc_print_addr(struct svc_rqst *, char *, size_t);
> diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
> index b18175ef74ec..fd49e7b12c94 100644
> --- a/net/sunrpc/svc.c
> +++ b/net/sunrpc/svc.c
> @@ -696,13 +696,10 @@ svc_prepare_thread(struct svc_serv *serv, struct svc_pool *pool, int node)
> * @serv: RPC service
> * @pool: service thread pool
> *
> - * Returns an idle service thread (now marked BUSY), or NULL
> - * if no service threads are available. Finding an idle service
> - * thread and marking it BUSY is atomic with respect to other
> - * calls to svc_pool_wake_idle_thread().
> + * Wake an idle thread if there is one, else mark the pool as congested.
> */
> -struct svc_rqst *svc_pool_wake_idle_thread(struct svc_serv *serv,
> - struct svc_pool *pool)
> +void svc_pool_wake_idle_thread(struct svc_serv *serv,
> + struct svc_pool *pool)
> {
> struct svc_rqst *rqstp;
>
> @@ -715,13 +712,13 @@ struct svc_rqst *svc_pool_wake_idle_thread(struct svc_serv *serv,
> wake_up_process(rqstp->rq_task);
> rcu_read_unlock();
> percpu_counter_inc(&pool->sp_threads_woken);
> - return rqstp;
> + return;
> }
> rcu_read_unlock();
>
> trace_svc_pool_starved(serv, pool);
> percpu_counter_inc(&pool->sp_threads_starved);
> - return NULL;
> + set_bit(SP_CONGESTED, &pool->sp_flags);
> }
> EXPORT_SYMBOL_GPL(svc_pool_wake_idle_thread);
>
> diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
> index 948605e7043b..964c97dbb36c 100644
> --- a/net/sunrpc/svc_xprt.c
> +++ b/net/sunrpc/svc_xprt.c
> @@ -456,7 +456,6 @@ static bool svc_xprt_ready(struct svc_xprt *xprt)
> */
> void svc_xprt_enqueue(struct svc_xprt *xprt)
> {
> - struct svc_rqst *rqstp;
> struct svc_pool *pool;
>
> if (!svc_xprt_ready(xprt))
> @@ -477,13 +476,7 @@ void svc_xprt_enqueue(struct svc_xprt *xprt)
> list_add_tail(&xprt->xpt_ready, &pool->sp_sockets);
> spin_unlock_bh(&pool->sp_lock);
>
> - rqstp = svc_pool_wake_idle_thread(xprt->xpt_server, pool);
> - if (!rqstp) {
> - set_bit(SP_CONGESTED, &pool->sp_flags);
> - return;
> - }
> -
> - trace_svc_xprt_enqueue(xprt, rqstp);
> + svc_pool_wake_idle_thread(xprt->xpt_server, pool);
> }
> EXPORT_SYMBOL_GPL(svc_xprt_enqueue);
>
> @@ -587,14 +580,9 @@ static void svc_xprt_release(struct svc_rqst *rqstp)
> void svc_wake_up(struct svc_serv *serv)
> {
> struct svc_pool *pool = &serv->sv_pools[0];
> - struct svc_rqst *rqstp;
>
> - rqstp = svc_pool_wake_idle_thread(serv, pool);
> - if (!rqstp) {
> - set_bit(SP_TASK_PENDING, &pool->sp_flags);
> - smp_wmb();
> - return;
> - }
> + set_bit(SP_TASK_PENDING, &pool->sp_flags);
> + svc_pool_wake_idle_thread(serv, pool);
> }
> EXPORT_SYMBOL_GPL(svc_wake_up);
>
>
>
