On 2022-05-09 16:56:47 [+0000], Chuck Lever III wrote:
> Hi Sebastian-
Hi Chuck,
> > On May 4, 2022, at 1:24 PM, Sebastian Andrzej Siewior <bigeasy@xxxxxxxxxxxxx> wrote:
> >
> > svc_xprt_enqueue() disables preemption via get_cpu() and then asks for a
> > pool of a specific CPU (current) via svc_pool_for_cpu().
> > With disabled preemption it acquires svc_pool::sp_lock, a spinlock_t,
> > which is a sleeping lock on PREEMPT_RT and can't be acquired with
> > disabled preemption.
>
> I found this paragraph a little unclear. If you repost, I'd suggest:
>
> svc_xprt_enqueue() disables preemption via get_cpu() and then asks
> for a pool of a specific CPU (current) via svc_pool_for_cpu().
> While preemption is disabled, svc_xprt_enqueue() acquires
> svc_pool::sp_lock with bottom-halfs disabled, which can sleep on
> PREEMPT_RT.
Sure.
> > diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
> > index 5b59e2103526e..79965deec5b12 100644
> > --- a/net/sunrpc/svc_xprt.c
> > +++ b/net/sunrpc/svc_xprt.c
> > @@ -461,8 +460,7 @@ void svc_xprt_enqueue(struct svc_xprt *xprt)
> > if (test_and_set_bit(XPT_BUSY, &xprt->xpt_flags))
> > return;
> >
> > - cpu = get_cpu();
> > - pool = svc_pool_for_cpu(xprt->xpt_server, cpu);
> > + pool = svc_pool_for_cpu(xprt->xpt_server, raw_smp_processor_id());
>
> The pre-2014 code here was this:
>
> cpu = get_cpu();
> pool = svc_pool_for_cpu(xprt->xpt_server, cpu);
> put_cpu();
>
> Your explanation covers the rationale for leaving pre-emption
> enabled in the body of svc_xprt_enqueue(), but it's not clear
> to me that rationale also applies to svc_pool_for_cpu().
I don't see why svc_pool_for_cpu() should be protected by disabling
preemption. It returns a "struct svc_pool" depending on the CPU number.
In the SVC_POOL_PERNODE case it will return the same pointer/ struct for
two different CPUs which belong to the same node.
> Protecting the svc_pool data structures with RCU might be
> better, but would amount to a more extensive change. To address
> the pre-emption issue quickly, you could simply revert this
> call site back to its pre-2014 form and postpone deeper changes.
You mean before commit
983c684466e02 ("SUNRPC: get rid of the request wait queue")
I'm not sure how RCU would help here. It would ensure that the pool does
not vanish within the RCU section. The pool remains around until
svc_destroy() and I assume that everything pool related (like
svc_pool::sp_sockets) is gone by then.
> I have an NFS server in my lab on NUMA hardware and can run
> some tests this week with this patch.
I'm a little confused now. I could repost the patch with an updated
description as you suggested _or_ limit the get_cpu()/preempt-disabled
section to be only around svc_pool_for_cpu(). I don't see the reason for
it but will do as requested (if you want me to) since it doesn't cause
any problems.
Sebastian
