On Tue, Aug 15, 2023 at 11:54:20AM +1000, NeilBrown wrote:
> With an llist we don't need to take a lock to add a thread to the list,
> though we still need a lock to remove it. That will go in the next
> patch.
>
> Unlike double-linked lists, a thread cannot reliably remove itself from
> the list. Only the first thread can be removed, and that can change
> asynchronously. So some care is needed.
>
> We already check if there is pending work to do, so we are unlikely to
> add ourselves to the idle list and then want to remove ourselves again.
>
> If we DO find something needs to be done after adding ourselves to the
> list, we simply wake up the first thread on the list. If that was us,
> we successfully removed ourselves and can continue. If it was some
> other thread, they will do the work that needs to be done. We can
> safely sleep until woken.
>
> We also remove the test on freezing() from rqst_should_sleep(). Instead
> we always try_to_freeze() before scheduling, which is needed as we now
> schedule() in a loop waiting to be removed from the idle queue.
>
> Signed-off-by: NeilBrown <neilb@xxxxxxx>
> ---
> include/linux/sunrpc/svc.h | 14 ++++++-----
> net/sunrpc/svc.c | 13 +++++-----
> net/sunrpc/svc_xprt.c | 50 +++++++++++++++++++-------------------
> 3 files changed, 40 insertions(+), 37 deletions(-)
>
> diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
> index 22b3018ebf62..5216f95411e3 100644
> --- a/include/linux/sunrpc/svc.h
> +++ b/include/linux/sunrpc/svc.h
> @@ -37,7 +37,7 @@ struct svc_pool {
> struct list_head sp_sockets; /* pending sockets */
> unsigned int sp_nrthreads; /* # of threads in pool */
> struct list_head sp_all_threads; /* all server threads */
> - struct list_head sp_idle_threads; /* idle server threads */
> + struct llist_head sp_idle_threads; /* idle server threads */
>
> /* statistics on pool operation */
> struct percpu_counter sp_messages_arrived;
> @@ -186,7 +186,7 @@ extern u32 svc_max_payload(const struct svc_rqst *rqstp);
> */
> struct svc_rqst {
> struct list_head rq_all; /* all threads list */
> - struct list_head rq_idle; /* On the idle list */
> + struct llist_node rq_idle; /* On the idle list */
> struct rcu_head rq_rcu_head; /* for RCU deferred kfree */
> struct svc_xprt * rq_xprt; /* transport ptr */
>
> @@ -270,22 +270,24 @@ enum {
> * svc_thread_set_busy - mark a thread as busy
> * @rqstp: the thread which is now busy
> *
> - * If rq_idle is "empty", the thread must be busy.
> + * By convention a thread is busy if rq_idle.next points to rq_idle.
> + * This ensures it is not on the idle list.
> */
> static inline void svc_thread_set_busy(struct svc_rqst *rqstp)
> {
> - INIT_LIST_HEAD(&rqstp->rq_idle);
> + rqstp->rq_idle.next = &rqstp->rq_idle;
> }
>
> /**
> * svc_thread_busy - check if a thread as busy
> * @rqstp: the thread which might be busy
> *
> - * If rq_idle is "empty", the thread must be busy.
> + * By convention a thread is busy if rq_idle.next points to rq_idle.
> + * This ensures it is not on the idle list.
> */
> static inline bool svc_thread_busy(struct svc_rqst *rqstp)
> {
> - return list_empty(&rqstp->rq_idle);
> + return rqstp->rq_idle.next == &rqstp->rq_idle;
> }
>
> #define SVC_NET(rqst) (rqst->rq_xprt ? rqst->rq_xprt->xpt_net : rqst->rq_bc_net)
> diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
> index 051f08c48418..addbf28ea50a 100644
> --- a/net/sunrpc/svc.c
> +++ b/net/sunrpc/svc.c
> @@ -510,7 +510,7 @@ __svc_create(struct svc_program *prog, unsigned int bufsize, int npools,
> pool->sp_id = i;
> INIT_LIST_HEAD(&pool->sp_sockets);
> INIT_LIST_HEAD(&pool->sp_all_threads);
> - INIT_LIST_HEAD(&pool->sp_idle_threads);
> + init_llist_head(&pool->sp_idle_threads);
> spin_lock_init(&pool->sp_lock);
>
> percpu_counter_init(&pool->sp_messages_arrived, 0, GFP_KERNEL);
> @@ -701,15 +701,16 @@ svc_prepare_thread(struct svc_serv *serv, struct svc_pool *pool, int node)
> void svc_pool_wake_idle_thread(struct svc_pool *pool)
> {
> struct svc_rqst *rqstp;
> + struct llist_node *ln;
>
> rcu_read_lock();
> spin_lock_bh(&pool->sp_lock);
> - rqstp = list_first_entry_or_null(&pool->sp_idle_threads,
> - struct svc_rqst, rq_idle);
> - if (rqstp)
> - list_del_init(&rqstp->rq_idle);
> + ln = llist_del_first(&pool->sp_idle_threads);
> spin_unlock_bh(&pool->sp_lock);
> - if (rqstp) {
> + if (ln) {
> + rqstp = llist_entry(ln, struct svc_rqst, rq_idle);
> + svc_thread_set_busy(rqstp);
> +
> WRITE_ONCE(rqstp->rq_qtime, ktime_get());
> wake_up_process(rqstp->rq_task);
> rcu_read_unlock();
> diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
> index fa0d854a5596..7cb71effda0b 100644
> --- a/net/sunrpc/svc_xprt.c
> +++ b/net/sunrpc/svc_xprt.c
> @@ -715,10 +715,6 @@ rqst_should_sleep(struct svc_rqst *rqstp)
> if (svc_thread_should_stop(rqstp))
> return false;
>
> - /* are we freezing? */
> - if (freezing(current))
> - return false;
> -
> #if defined(CONFIG_SUNRPC_BACKCHANNEL)
> if (svc_is_backchannel(rqstp)) {
> if (!list_empty(&rqstp->rq_server->sv_cb_list))
> @@ -735,29 +731,32 @@ static void svc_rqst_wait_for_work(struct svc_rqst *rqstp)
>
> if (rqst_should_sleep(rqstp)) {
> set_current_state(TASK_IDLE);
> - spin_lock_bh(&pool->sp_lock);
> - list_add(&rqstp->rq_idle, &pool->sp_idle_threads);
> - spin_unlock_bh(&pool->sp_lock);
> + llist_add(&rqstp->rq_idle, &pool->sp_idle_threads);
> +
> + if (unlikely(!rqst_should_sleep(rqstp)))
> + /* maybe there were no idle threads when some work
> + * became ready and so nothing was woken. We've just
> + * become idle so someone can to the work - maybe us.
> + * But we cannot reliably remove ourselves from the
> + * idle list - we can only remove the first task which
> + * might be us, and might not.
> + * So remove and wake it, then schedule(). If it was
> + * us, we won't sleep. If it is some other thread, they
> + * will do the work.
> + */
> + svc_pool_wake_idle_thread(pool);
>
> - /* Need to check should_sleep() again after
> - * setting task state in case a wakeup happened
> - * between testing and setting.
> + /* We mustn't continue while on the idle list, and we
> + * cannot remove outselves reliably. The only "work"
> + * we can do while on the idle list is to freeze.
> + * So loop until someone removes us
> */
> - if (rqst_should_sleep(rqstp)) {
> + while (!svc_thread_busy(rqstp)) {
> + try_to_freeze();
For testing, I've applied up to this patch. When nfsd is started
I now get this splat:
do not call blocking ops when !TASK_RUNNING; state=402 set at [<000000001e3d6995>] svc_recv+0x40/0x252 [sunrpc]
WARNING: CPU: 3 PID: 1228 at kernel/sched/core.c:10112 __might_sleep+0x52/0x6a
Modules linked in: 8021q garp stp mrp llc rfkill rpcrdma rdma_ucm ib_srpt ib_isert iscsi_target_mod snd_hda_codec_realtek target_core_mod intel>
CPU: 3 PID: 1228 Comm: lockd Not tainted 6.5.0-rc6-00060-gd10a6b1ad2a1 #1
Hardware name: Supermicro X10SRA-F/X10SRA-F, BIOS 2.0b 06/12/2017
RIP: 0010:__might_sleep+0x52/0x6a
Code: 00 00 74 28 80 3d 45 40 d5 01 00 75 1f 48 8b 90 f0 1a 00 00 48 c7 c7 b3 d6 49 9b c6 05 2e 40 d5 01 01 48 89 d1 e8 e6 a2 fc ff <0f> 0b 44 >
RSP: 0018:ffffb3e3836abe68 EFLAGS: 00010286
RAX: 000000000000006f RBX: ffffffffc0c20599 RCX: 0000000000000027
RDX: 0000000000000027 RSI: 00000000ffffdfff RDI: 0000000000000001
RBP: ffffb3e3836abe78 R08: 0000000000000000 R09: ffffb3e380e70020
R10: 0000000000000000 R11: ffffa0ae94aa4c00 R12: 0000000000000035
R13: ffffa0ae94bfa040 R14: ffffa0ae9e85c010 R15: ffffa0ae94bfa040
FS: 0000000000000000(0000) GS:ffffa0bdbfd80000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007f59c2611760 CR3: 000000069ec34006 CR4: 00000000003706e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
<TASK>
? show_regs+0x5d/0x64
? __might_sleep+0x52/0x6a
? __warn+0xab/0x132
? report_bug+0xd0/0x144
? __might_sleep+0x52/0x6a
? handle_bug+0x45/0x74
? exc_invalid_op+0x18/0x68
? asm_exc_invalid_op+0x1b/0x20
? __might_sleep+0x52/0x6a
? __might_sleep+0x52/0x6a
try_to_freeze.isra.0+0x15/0x3d [sunrpc]
svc_recv+0x97/0x252 [sunrpc]
? __pfx_lockd+0x10/0x10 [lockd]
lockd+0x6b/0xda [lockd]
kthread+0x10d/0x115
? __pfx_kthread+0x10/0x10
ret_from_fork+0x2a/0x43
? __pfx_kthread+0x10/0x10
ret_from_fork_asm+0x1b/0x30
</TASK>
> schedule();
> - } else {
> - __set_current_state(TASK_RUNNING);
> - cond_resched();
> - }
> -
> - /* We *must* be removed from the list before we can continue.
> - * If we were woken, this is already done
> - */
> - if (!svc_thread_busy(rqstp)) {
> - spin_lock_bh(&pool->sp_lock);
> - list_del_init(&rqstp->rq_idle);
> - spin_unlock_bh(&pool->sp_lock);
> + set_current_state(TASK_IDLE);
> }
> + __set_current_state(TASK_RUNNING);
> } else {
> cond_resched();
> }
> @@ -870,9 +869,10 @@ void svc_recv(struct svc_rqst *rqstp)
> struct svc_xprt *xprt = rqstp->rq_xprt;
>
> /* Normally we will wait up to 5 seconds for any required
> - * cache information to be provided.
> + * cache information to be provided. When there are no
> + * idle threads, we reduce the wait time.
> */
> - if (!list_empty(&pool->sp_idle_threads))
> + if (pool->sp_idle_threads.first)
> rqstp->rq_chandle.thread_wait = 5 * HZ;
> else
> rqstp->rq_chandle.thread_wait = 1 * HZ;
> --
> 2.40.1
>
--
Chuck Lever
