[PATCH] SUNRPC: Don't disable preemption while calling svc_pool_for_cpu().

[PATCH] SUNRPC: Don't disable preemption while calling svc_pool_for_cpu().

[Sebastian Andrzej Siewior]

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.

Disabling preemption is not required here. The pool is protected with a
lock so the following list access is safe even cross-CPU. The following
iteration through svc_pool::sp_all_threads is under RCU-readlock and
remaining operations within the loop are atomic and do not rely on
disabled-preemption.

Use raw_smp_processor_id() as the argument for the requested CPU in
svc_pool_for_cpu().

Reported-by: Mike Galbraith <umgwanakikbuti@gmail.com>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 net/sunrpc/svc_xprt.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

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
@@ -448,7 +448,6 @@ void svc_xprt_enqueue(struct svc_xprt *xprt)
 {
 	struct svc_pool *pool;
 	struct svc_rqst	*rqstp = NULL;
-	int cpu;
 
 	if (!svc_xprt_ready(xprt))
 		return;
@@ -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());
 
 	atomic_long_inc(&pool->sp_stats.packets);
 
@@ -485,7 +483,6 @@ void svc_xprt_enqueue(struct svc_xprt *xprt)
 	rqstp = NULL;
 out_unlock:
 	rcu_read_unlock();
-	put_cpu();
 	trace_svc_xprt_enqueue(xprt, rqstp);
 }
 EXPORT_SYMBOL_GPL(svc_xprt_enqueue);
-- 
2.36.0

Re: [PATCH] SUNRPC: Don't disable preemption while calling svc_pool_for_cpu().

[Chuck Lever III]

On May 4, 2022, at 10:24 AM, Sebastian Andrzej Siewior <bigeasy@linutronix.de> 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.
> 
> Disabling preemption is not required here. The pool is protected with a
> lock so the following list access is safe even cross-CPU. The following
> iteration through svc_pool::sp_all_threads is under RCU-readlock and
> remaining operations within the loop are atomic and do not rely on
> disabled-preemption.
> 
> Use raw_smp_processor_id() as the argument for the requested CPU in
> svc_pool_for_cpu().
> 
> Reported-by: Mike Galbraith <umgwanakikbuti@gmail.com>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

Sebastian, I will have a closer look in a few days. Meanwhile, if I’m reading the patch description right, this is a bug fix. Was there a lockdep splat (ie, how did you find this issue)? Does it belong in 5.18-rc? Should it have a Fixes: tag?


> ---
> net/sunrpc/svc_xprt.c | 5 +----
> 1 file changed, 1 insertion(+), 4 deletions(-)
> 
> 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
> @@ -448,7 +448,6 @@ void svc_xprt_enqueue(struct svc_xprt *xprt)
> {
>    struct svc_pool *pool;
>    struct svc_rqst    *rqstp = NULL;
> -    int cpu;
> 
>    if (!svc_xprt_ready(xprt))
>        return;
> @@ -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());
> 
>    atomic_long_inc(&pool->sp_stats.packets);
> 
> @@ -485,7 +483,6 @@ void svc_xprt_enqueue(struct svc_xprt *xprt)
>    rqstp = NULL;
> out_unlock:
>    rcu_read_unlock();
> -    put_cpu();
>    trace_svc_xprt_enqueue(xprt, rqstp);
> }
> EXPORT_SYMBOL_GPL(svc_xprt_enqueue);
> -- 
> 2.36.0
> 

Re: [PATCH] SUNRPC: Don't disable preemption while calling svc_pool_for_cpu().

[Sebastian Andrzej Siewior]

On 2022-05-05 03:19:57 [+0000], Chuck Lever III wrote:
> 
> Sebastian, I will have a closer look in a few days. Meanwhile, if I’m
> reading the patch description right, this is a bug fix. Was there a
> lockdep splat (ie, how did you find this issue)? Does it belong in
> 5.18-rc? Should it have a Fixes: tag?

It is a bugfix for PREEMPT_RT and I'm posting this as part of getting
PREEMPT_RT merged upstream. This is already fixed in the PREEMPT_RT
queue. 
There is no need to get this merged in the current -rc series since
!PREEMPT_RT is not affected. Next release is fine.

Sebastian

Re: [PATCH] SUNRPC: Don't disable preemption while calling svc_pool_for_cpu().

[Chuck Lever III]

Hi Sebastian-


> On May 4, 2022, at 1:24 PM, Sebastian Andrzej Siewior <bigeasy@linutronix.de> 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.


> Disabling preemption is not required here. The pool is protected with a
> lock so the following list access is safe even cross-CPU. The following
> iteration through svc_pool::sp_all_threads is under RCU-readlock and
> remaining operations within the loop are atomic and do not rely on
> disabled-preemption.
> 
> Use raw_smp_processor_id() as the argument for the requested CPU in
> svc_pool_for_cpu().
> 
> Reported-by: Mike Galbraith <umgwanakikbuti@gmail.com>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
> net/sunrpc/svc_xprt.c | 5 +----
> 1 file changed, 1 insertion(+), 4 deletions(-)
> 
> 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
> @@ -448,7 +448,6 @@ void svc_xprt_enqueue(struct svc_xprt *xprt)
> {
> 	struct svc_pool *pool;
> 	struct svc_rqst	*rqstp = NULL;
> -	int cpu;
> 
> 	if (!svc_xprt_ready(xprt))
> 		return;
> @@ -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().

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.

I have an NFS server in my lab on NUMA hardware and can run
some tests this week with this patch.


> 	atomic_long_inc(&pool->sp_stats.packets);
> 
> @@ -485,7 +483,6 @@ void svc_xprt_enqueue(struct svc_xprt *xprt)
> 	rqstp = NULL;
> out_unlock:
> 	rcu_read_unlock();
> -	put_cpu();
> 	trace_svc_xprt_enqueue(xprt, rqstp);
> }
> EXPORT_SYMBOL_GPL(svc_xprt_enqueue);
> -- 
> 2.36.0
> 

--
Chuck Lever

Re: [PATCH] SUNRPC: Don't disable preemption while calling svc_pool_for_cpu().

[Sebastian Andrzej Siewior]

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@linutronix.de> 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

Re: [PATCH] SUNRPC: Don't disable preemption while calling svc_pool_for_cpu().

[Chuck Lever III]

> On May 10, 2022, at 8:02 AM, Sebastian Andrzej Siewior <bigeasy@linutronix.de> 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@linutronix.de> 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

[PATCH v2] SUNRPC: Don't disable preemption while calling svc_pool_for_cpu().

[Sebastian Andrzej Siewior]

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.

Disabling preemption is not required here. The pool is protected with a
lock so the following list access is safe even cross-CPU. The following
iteration through svc_pool::sp_all_threads is under RCU-readlock and
remaining operations within the loop are atomic and do not rely on
disabled-preemption.

Use raw_smp_processor_id() as the argument for the requested CPU in
svc_pool_for_cpu().

Reported-by: Mike Galbraith <umgwanakikbuti@gmail.com>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
v1…v2:
   - Reword the first part of the commit message as per Chuck Lever III.

 net/sunrpc/svc_xprt.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

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
@@ -448,7 +448,6 @@ void svc_xprt_enqueue(struct svc_xprt *xprt)
 {
 	struct svc_pool *pool;
 	struct svc_rqst	*rqstp = NULL;
-	int cpu;
 
 	if (!svc_xprt_ready(xprt))
 		return;
@@ -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());
 
 	atomic_long_inc(&pool->sp_stats.packets);
 
@@ -485,7 +483,6 @@ void svc_xprt_enqueue(struct svc_xprt *xprt)
 	rqstp = NULL;
 out_unlock:
 	rcu_read_unlock();
-	put_cpu();
 	trace_svc_xprt_enqueue(xprt, rqstp);
 }
 EXPORT_SYMBOL_GPL(svc_xprt_enqueue);
-- 
2.36.0

Re: [PATCH v2] SUNRPC: Don't disable preemption while calling svc_pool_for_cpu().

[Chuck Lever III]

> On May 10, 2022, at 10:38 AM, Sebastian Andrzej Siewior <bigeasy@linutronix.de> 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().
> While preemption is disabled, svc_xprt_enqueue() acquires
> svc_pool::sp_lock with bottom-halfs disabled, which can sleep on
> PREEMPT_RT.
> 
> Disabling preemption is not required here. The pool is protected with a
> lock so the following list access is safe even cross-CPU. The following
> iteration through svc_pool::sp_all_threads is under RCU-readlock and
> remaining operations within the loop are atomic and do not rely on
> disabled-preemption.
> 
> Use raw_smp_processor_id() as the argument for the requested CPU in
> svc_pool_for_cpu().
> 
> Reported-by: Mike Galbraith <umgwanakikbuti@gmail.com>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

Thanks. I'll apply this and add a clean-up patch to move
the raw_smp_processor_id() into svc_pool_for_cpu(). Sorry
for the noise!


> ---
> v1…v2:
>   - Reword the first part of the commit message as per Chuck Lever III.
> 
> net/sunrpc/svc_xprt.c | 5 +----
> 1 file changed, 1 insertion(+), 4 deletions(-)
> 
> 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
> @@ -448,7 +448,6 @@ void svc_xprt_enqueue(struct svc_xprt *xprt)
> {
> 	struct svc_pool *pool;
> 	struct svc_rqst	*rqstp = NULL;
> -	int cpu;
> 
> 	if (!svc_xprt_ready(xprt))
> 		return;
> @@ -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());
> 
> 	atomic_long_inc(&pool->sp_stats.packets);
> 
> @@ -485,7 +483,6 @@ void svc_xprt_enqueue(struct svc_xprt *xprt)
> 	rqstp = NULL;
> out_unlock:
> 	rcu_read_unlock();
> -	put_cpu();
> 	trace_svc_xprt_enqueue(xprt, rqstp);
> }
> EXPORT_SYMBOL_GPL(svc_xprt_enqueue);
> -- 
> 2.36.0
> 

--
Chuck Lever