net: Handle threadirqs in __napi_schedule_irqoff

[PATCH] net: Handle threadirqs in __napi_schedule_irqoff

Posted by Sean Anderson 1 year, 4 months ago

The threadirqs kernel parameter can be used to force threaded IRQs even
on non-PREEMPT_RT kernels. Use force_irqthreads to determine if we can
skip disabling local interrupts. This defaults to false on regular
kernels, and is always true on PREEMPT_RT kernels.

Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
---

 net/core/dev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 1e740faf9e78..112e871bc2b0 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -6202,7 +6202,7 @@ EXPORT_SYMBOL(napi_schedule_prep);
  */
 void __napi_schedule_irqoff(struct napi_struct *n)
 {
-	if (!IS_ENABLED(CONFIG_PREEMPT_RT))
+	if (!force_irqthreads())
 		____napi_schedule(this_cpu_ptr(&softnet_data), n);
 	else
 		__napi_schedule(n);
-- 
2.35.1.1320.gc452695387.dirty

Re: [PATCH] net: Handle threadirqs in __napi_schedule_irqoff

Posted by Sebastian Andrzej Siewior 1 year, 4 months ago

On 2024-09-13 11:09:54 [-0400], Sean Anderson wrote:
> The threadirqs kernel parameter can be used to force threaded IRQs even
> on non-PREEMPT_RT kernels. Use force_irqthreads to determine if we can
> skip disabling local interrupts. This defaults to false on regular
> kernels, and is always true on PREEMPT_RT kernels.

Is this fixing a behaviour or is this from the clean up/ make it pretty
category?
The forced-threaded interrupts run with disabled interrupts on !RT so
this change should not fix anything.

> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>

Sebastian

Re: [PATCH] net: Handle threadirqs in __napi_schedule_irqoff

Posted by Sean Anderson 1 year, 4 months ago

On 9/13/24 12:14, Sebastian Andrzej Siewior wrote:
> On 2024-09-13 11:09:54 [-0400], Sean Anderson wrote:
>> The threadirqs kernel parameter can be used to force threaded IRQs even
>> on non-PREEMPT_RT kernels. Use force_irqthreads to determine if we can
>> skip disabling local interrupts. This defaults to false on regular
>> kernels, and is always true on PREEMPT_RT kernels.
> 
> Is this fixing a behaviour or is this from the clean up/ make it pretty
> category?

It's in response to [1].

[1] https://lore.kernel.org/netdev/20240912084322.148d7fb2@kernel.org/

> The forced-threaded interrupts run with disabled interrupts on !RT so
> this change should not fix anything.

OK, so maybe this isn't necessary at all?

--Sean

Re: [PATCH] net: Handle threadirqs in __napi_schedule_irqoff

Posted by Sebastian Andrzej Siewior 1 year, 4 months ago

On 2024-09-13 12:19:05 [-0400], Sean Anderson wrote:
> It's in response to [1].
> 
> [1] https://lore.kernel.org/netdev/20240912084322.148d7fb2@kernel.org/

thanks

> > The forced-threaded interrupts run with disabled interrupts on !RT so
> > this change should not fix anything.
> 
> OK, so maybe this isn't necessary at all?

Based on your reasoning so far, yes, not needed.

> --Sean

Sebastian

Re: [PATCH] net: Handle threadirqs in __napi_schedule_irqoff

Posted by Eric Dumazet 1 year, 4 months ago

On Fri, Sep 13, 2024 at 5:10 PM Sean Anderson <sean.anderson@linux.dev> wrote:
>
> The threadirqs kernel parameter can be used to force threaded IRQs even
> on non-PREEMPT_RT kernels. Use force_irqthreads to determine if we can
> skip disabling local interrupts. This defaults to false on regular
> kernels, and is always true on PREEMPT_RT kernels.
>
> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
> ---
>
>  net/core/dev.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 1e740faf9e78..112e871bc2b0 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -6202,7 +6202,7 @@ EXPORT_SYMBOL(napi_schedule_prep);
>   */
>  void __napi_schedule_irqoff(struct napi_struct *n)
>  {
> -       if (!IS_ENABLED(CONFIG_PREEMPT_RT))
> +       if (!force_irqthreads())
>                 ____napi_schedule(this_cpu_ptr(&softnet_data), n);
>         else
>                 __napi_schedule(n);
> --
> 2.35.1.1320.gc452695387.dirty
>

Seems reasonable, can you update the comment (kdoc) as well ?

It says :

 * On PREEMPT_RT enabled kernels this maps to __napi_schedule()
 * because the interrupt disabled assumption might not be true
 * due to force-threaded interrupts and spinlock substitution.

Also always specify net or net-next for networking patches.

Thanks.

Re: [PATCH] net: Handle threadirqs in __napi_schedule_irqoff

Posted by Sean Anderson 1 year, 4 months ago

On 9/13/24 11:16, Eric Dumazet wrote:
> On Fri, Sep 13, 2024 at 5:10 PM Sean Anderson <sean.anderson@linux.dev> wrote:
>>
>> The threadirqs kernel parameter can be used to force threaded IRQs even
>> on non-PREEMPT_RT kernels. Use force_irqthreads to determine if we can
>> skip disabling local interrupts. This defaults to false on regular
>> kernels, and is always true on PREEMPT_RT kernels.
>>
>> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
>> ---
>>
>>  net/core/dev.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/net/core/dev.c b/net/core/dev.c
>> index 1e740faf9e78..112e871bc2b0 100644
>> --- a/net/core/dev.c
>> +++ b/net/core/dev.c
>> @@ -6202,7 +6202,7 @@ EXPORT_SYMBOL(napi_schedule_prep);
>>   */
>>  void __napi_schedule_irqoff(struct napi_struct *n)
>>  {
>> -       if (!IS_ENABLED(CONFIG_PREEMPT_RT))
>> +       if (!force_irqthreads())
>>                 ____napi_schedule(this_cpu_ptr(&softnet_data), n);
>>         else
>>                 __napi_schedule(n);
>> --
>> 2.35.1.1320.gc452695387.dirty
>>
> 
> Seems reasonable, can you update the comment (kdoc) as well ?
> 
> It says :
> 
>  * On PREEMPT_RT enabled kernels this maps to __napi_schedule()
>  * because the interrupt disabled assumption might not be true
>  * due to force-threaded interrupts and spinlock substitution.

OK

> Also always specify net or net-next for networking patches.

Ah, sorry. Should be net-next.

--Sean

Re: [PATCH] net: Handle threadirqs in __napi_schedule_irqoff

Posted by Brett Creeley 1 year, 4 months ago


On 9/13/2024 8:23 AM, Sean Anderson wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
> 
> 
> On 9/13/24 11:16, Eric Dumazet wrote:
>> On Fri, Sep 13, 2024 at 5:10 PM Sean Anderson <sean.anderson@linux.dev> wrote:
>>>
>>> The threadirqs kernel parameter can be used to force threaded IRQs even
>>> on non-PREEMPT_RT kernels. Use force_irqthreads to determine if we can
>>> skip disabling local interrupts. This defaults to false on regular
>>> kernels, and is always true on PREEMPT_RT kernels.
>>>
>>> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
>>> ---
>>>
>>>   net/core/dev.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/net/core/dev.c b/net/core/dev.c
>>> index 1e740faf9e78..112e871bc2b0 100644
>>> --- a/net/core/dev.c
>>> +++ b/net/core/dev.c
>>> @@ -6202,7 +6202,7 @@ EXPORT_SYMBOL(napi_schedule_prep);
>>>    */
>>>   void __napi_schedule_irqoff(struct napi_struct *n)
>>>   {
>>> -       if (!IS_ENABLED(CONFIG_PREEMPT_RT))
>>> +       if (!force_irqthreads())
>>>                  ____napi_schedule(this_cpu_ptr(&softnet_data), n);
>>>          else
>>>                  __napi_schedule(n);
>>> --
>>> 2.35.1.1320.gc452695387.dirty
>>>
>>
>> Seems reasonable, can you update the comment (kdoc) as well ?
>>
>> It says :
>>
>>   * On PREEMPT_RT enabled kernels this maps to __napi_schedule()
>>   * because the interrupt disabled assumption might not be true
>>   * due to force-threaded interrupts and spinlock substitution.
> 
> OK
> 
>> Also always specify net or net-next for networking patches.
> 
> Ah, sorry. Should be net-next.

Is this worthy for a fixes/net tag?

Thanks,

Brett
> 
> --Sean
> 
>

Re: [PATCH] net: Handle threadirqs in __napi_schedule_irqoff

Posted by Sean Anderson 1 year, 4 months ago

On 9/13/24 12:08, Brett Creeley wrote:
> 
> 
> On 9/13/2024 8:23 AM, Sean Anderson wrote:
>> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>>
>>
>> On 9/13/24 11:16, Eric Dumazet wrote:
>>> On Fri, Sep 13, 2024 at 5:10 PM Sean Anderson <sean.anderson@linux.dev> wrote:
>>>>
>>>> The threadirqs kernel parameter can be used to force threaded IRQs even
>>>> on non-PREEMPT_RT kernels. Use force_irqthreads to determine if we can
>>>> skip disabling local interrupts. This defaults to false on regular
>>>> kernels, and is always true on PREEMPT_RT kernels.
>>>>
>>>> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
>>>> ---
>>>>
>>>>   net/core/dev.c | 2 +-
>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/net/core/dev.c b/net/core/dev.c
>>>> index 1e740faf9e78..112e871bc2b0 100644
>>>> --- a/net/core/dev.c
>>>> +++ b/net/core/dev.c
>>>> @@ -6202,7 +6202,7 @@ EXPORT_SYMBOL(napi_schedule_prep);
>>>>    */
>>>>   void __napi_schedule_irqoff(struct napi_struct *n)
>>>>   {
>>>> -       if (!IS_ENABLED(CONFIG_PREEMPT_RT))
>>>> +       if (!force_irqthreads())
>>>>                  ____napi_schedule(this_cpu_ptr(&softnet_data), n);
>>>>          else
>>>>                  __napi_schedule(n);
>>>> -- 
>>>> 2.35.1.1320.gc452695387.dirty
>>>>
>>>
>>> Seems reasonable, can you update the comment (kdoc) as well ?
>>>
>>> It says :
>>>
>>>   * On PREEMPT_RT enabled kernels this maps to __napi_schedule()
>>>   * because the interrupt disabled assumption might not be true
>>>   * due to force-threaded interrupts and spinlock substitution.
>>
>> OK
>>
>>> Also always specify net or net-next for networking patches.
>>
>> Ah, sorry. Should be net-next.
> 
> Is this worthy for a fixes/net tag?

Maybe? Commit 8380c81d5c4f ("net: Treat __napi_schedule_irqoff() as
__napi_schedule() on PREEMPT_RT") originally introduced the condition on
PREEMPT_RT but didn't include any fixes. And that's probably because
there's nothing wrong with the original behavior as long as you add
IRQF_NO_THREAD to your interrupt. Although at the time threadirqs had
existed for a while, so maybe this commit should fix that one.

--Sean