> On May 10, 2022, at 8:02 AM, Sebastian Andrzej Siewior <bigeasy@xxxxxxxxxxxxx> wrote:
>
> 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.
Basically, why would bfd241600a3b ("[PATCH] knfsd: make rpc
threads pools numa aware") use get_cpu/put_cpu when
raw_smp_processor_id() was available to it? Looking through
the commit log, I can't see that it is necessary, but I
needed to convince myself that it was just a coding whim
and not done purposely to protect that function.
And, note that svc_pool_map is a read-write data structure.
I needed a little more review to convince myself that the
data structure cannot change once it has been initialized.
Third, my testing so far shows your patch does not introduce
any regression.
So I'm feeling more confident. Let's do this:
- Get rid of the @cpu argument to svc_pool_for_cpu() and
do the raw_smp_processor_id() call inside that function.
Please add a kerneldoc comment for svc_pool_for_cpu()
that states the current CPU is an implicit argument.
- Fix up the patch description as above.
- Post a v2
>> 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.
I think I'm convinced that using raw_smp_processor_id() is
safe to do, and it is certainly preferable in a performance
path to not toggle pre-emption at all.
--
Chuck Lever
