On Mon, Jul 31, 2023 at 04:48:28PM +1000, NeilBrown wrote:
> Based on its name you would think that rqst_should_sleep() would be
> read-only, not changing anything. But it fact it will clear
> SP_TASK_PENDING if that was set. This is surprising, and it blurs the
> line between "check for work to do" and "dequeue work to do".
I agree that rqst_should_sleep() sounds like it should be a
predicate without side effects.
> So change the "test_and_clear" to simple "test" and clear the bit once
> the thread has decided to wake up and return to the caller.
>
> With this, it makes sense to *always* set SP_TASK_PENDING when asked,
> rather than only to set it if no thread could be woken up.
I'm lost here. Why does always setting TASK_PENDING now make sense?
If there's no task pending, won't this trigger a wake up when there
is nothing to do?
> Signed-off-by: NeilBrown <neilb@xxxxxxx>
> ---
> net/sunrpc/svc_xprt.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
> index cd92cb54132d..380fb3caea4c 100644
> --- a/net/sunrpc/svc_xprt.c
> +++ b/net/sunrpc/svc_xprt.c
> @@ -581,8 +581,8 @@ void svc_wake_up(struct svc_serv *serv)
> {
> struct svc_pool *pool = &serv->sv_pools[0];
>
> - if (!svc_pool_wake_idle_thread(serv, pool))
> - set_bit(SP_TASK_PENDING, &pool->sp_flags);
> + set_bit(SP_TASK_PENDING, &pool->sp_flags);
> + svc_pool_wake_idle_thread(serv, pool);
> }
> EXPORT_SYMBOL_GPL(svc_wake_up);
>
> @@ -704,7 +704,7 @@ rqst_should_sleep(struct svc_rqst *rqstp)
> struct svc_pool *pool = rqstp->rq_pool;
>
> /* did someone call svc_wake_up? */
> - if (test_and_clear_bit(SP_TASK_PENDING, &pool->sp_flags))
> + if (test_bit(SP_TASK_PENDING, &pool->sp_flags))
> return false;
>
> /* was a socket queued? */
> @@ -750,6 +750,7 @@ static struct svc_xprt *svc_get_next_xprt(struct svc_rqst *rqstp)
>
> set_bit(RQ_BUSY, &rqstp->rq_flags);
> smp_mb__after_atomic();
> + clear_bit(SP_TASK_PENDING, &pool->sp_flags);
Why wouldn't this go before the smp_mb__after_atomic()?
> rqstp->rq_xprt = svc_xprt_dequeue(pool);
> if (rqstp->rq_xprt) {
> trace_svc_pool_awoken(rqstp);
> @@ -761,6 +762,7 @@ static struct svc_xprt *svc_get_next_xprt(struct svc_rqst *rqstp)
> percpu_counter_inc(&pool->sp_threads_no_work);
> return NULL;
> out_found:
> + clear_bit(SP_TASK_PENDING, &pool->sp_flags);
clear_bit_unlock ?
> /* Normally we will wait up to 5 seconds for any required
> * cache information to be provided.
> */
> --
> 2.40.1
>
--
Chuck Lever
