| Message ID | 20240913150954.2287196-1-sean.anderson@linux.dev (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: Handle threadirqs in __napi_schedule_irqoff | expand |
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.
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
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 > >
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
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
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
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
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);
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(-)