| Message ID | 1463003804.23934.154.camel@edumazet-glaptop3.roam.corp.google.com |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
On Wed, 2016-05-11 at 14:56 -0700, Eric Dumazet wrote: > On Wed, 2016-05-11 at 08:55 +0200, Peter Zijlstra wrote: > > On Tue, May 10, 2016 at 03:51:37PM -0700, Eric Dumazet wrote: > > > diff --git a/kernel/softirq.c b/kernel/softirq.c > > > index 17caf4b63342..22463217e3cf 100644 > > > --- a/kernel/softirq.c > > > +++ b/kernel/softirq.c > > > @@ -56,6 +56,7 @@ EXPORT_SYMBOL(irq_stat); > > > static struct softirq_action softirq_vec[NR_SOFTIRQS] __cacheline_aligned_in_smp; > > > > > > DEFINE_PER_CPU(struct task_struct *, ksoftirqd); > > > +DEFINE_PER_CPU(bool, ksoftirqd_scheduled); > > > > > > const char * const softirq_to_name[NR_SOFTIRQS] = { > > > "HI", "TIMER", "NET_TX", "NET_RX", "BLOCK", "BLOCK_IOPOLL", > > > @@ -73,8 +74,10 @@ static void wakeup_softirqd(void) > > > /* Interrupts are disabled: no need to stop preemption */ > > > struct task_struct *tsk = __this_cpu_read(ksoftirqd); > > > > > > - if (tsk && tsk->state != TASK_RUNNING) > > > + if (tsk && tsk->state != TASK_RUNNING) { > > > + __this_cpu_write(ksoftirqd_scheduled, true); > > > wake_up_process(tsk); > > > > Since we're already looking at tsk->state, and the wake_up_process() > > ensures the thing becomes TASK_RUNNING, you could add: > > > > static inline bool ksoftirqd_running(void) > > { > > return __this_cpu_read(ksoftirqd)->state == TASK_RUNNING; here something like: struct task_struct *tsk = __this_cpu_read(ksoftirqd); return tsk && (tsk->state == TASK_RUNNING); is needed since __this_cpu_read(ksoftirqd) can be NULL on boot. > > } > > Indeed, and the patch looks quite simple now ;) > > diff --git a/kernel/softirq.c b/kernel/softirq.c > index 17caf4b63342d7839528f367b283a386413b0362..23c364485d03618773c385d943c0ef39f5931d09 100644 > --- a/kernel/softirq.c > +++ b/kernel/softirq.c > @@ -57,6 +57,11 @@ static struct softirq_action softirq_vec[NR_SOFTIRQS] __cacheline_aligned_in_smp > > DEFINE_PER_CPU(struct task_struct *, ksoftirqd); > > +static inline bool ksoftirqd_running(void) > +{ > + return __this_cpu_read(ksoftirqd)->state == TASK_RUNNING; > +} > + > const char * const softirq_to_name[NR_SOFTIRQS] = { > "HI", "TIMER", "NET_TX", "NET_RX", "BLOCK", "BLOCK_IOPOLL", > "TASKLET", "SCHED", "HRTIMER", "RCU" > @@ -313,7 +318,7 @@ asmlinkage __visible void do_softirq(void) > > pending = local_softirq_pending(); > > - if (pending) > + if (pending && !ksoftirqd_running()) > do_softirq_own_stack(); > > local_irq_restore(flags); > @@ -340,6 +345,9 @@ void irq_enter(void) > > static inline void invoke_softirq(void) > { > + if (ksoftirqd_running()) > + return; > + > if (!force_irqthreads) { > #ifdef CONFIG_HAVE_IRQ_EXIT_ON_IRQ_STACK > /* > >
On Thu, 2016-05-12 at 22:07 +0200, Paolo Abeni wrote: > > > static inline bool ksoftirqd_running(void) > > > { > > > return __this_cpu_read(ksoftirqd)->state == TASK_RUNNING; > > here something like: > > struct task_struct *tsk = __this_cpu_read(ksoftirqd); > return tsk && (tsk->state == TASK_RUNNING); > > is needed since __this_cpu_read(ksoftirqd) can be NULL on boot. Indeed I've seen this but only when backporting to an older linux kernel this morning. Have you got this with current linux kernel ?
On Thu, 2016-05-12 at 13:49 -0700, Eric Dumazet wrote: > On Thu, 2016-05-12 at 22:07 +0200, Paolo Abeni wrote: > > > > > static inline bool ksoftirqd_running(void) > > > > { > > > > return __this_cpu_read(ksoftirqd)->state == TASK_RUNNING; > > > > here something like: > > > > struct task_struct *tsk = __this_cpu_read(ksoftirqd); > > return tsk && (tsk->state == TASK_RUNNING); > > > > is needed since __this_cpu_read(ksoftirqd) can be NULL on boot. > > Indeed I've seen this but only when backporting to an older linux kernel > this morning. > > Have you got this with current linux kernel ? Yes, on net-next updated to commit c66b2581123cd1527b6a084f39e9271cb02673b7 Author: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> Date: Sat May 7 14:09:01 2016 -0700 sh_eth: reuse sh_eth_chip_reset() Cheers, Paolo
On Thu, 2016-05-12 at 22:58 +0200, Paolo Abeni wrote: > On Thu, 2016-05-12 at 13:49 -0700, Eric Dumazet wrote: > > On Thu, 2016-05-12 at 22:07 +0200, Paolo Abeni wrote: > > > > > > > static inline bool ksoftirqd_running(void) > > > > > { > > > > > return __this_cpu_read(ksoftirqd)->state == TASK_RUNNING; > > > > > > here something like: > > > > > > struct task_struct *tsk = __this_cpu_read(ksoftirqd); > > > return tsk && (tsk->state == TASK_RUNNING); > > > > > > is needed since __this_cpu_read(ksoftirqd) can be NULL on boot. > > > > Indeed I've seen this but only when backporting to an older linux kernel > > this morning. > > > > Have you got this with current linux kernel ? > > Yes, on net-next updated to > > commit c66b2581123cd1527b6a084f39e9271cb02673b7 > Author: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> > Date: Sat May 7 14:09:01 2016 -0700 > > sh_eth: reuse sh_eth_chip_reset() > Yeah, I was unsure if the same test in wakeup_softirqd() was still relevant today. static void wakeup_softirqd(void) { /* Interrupts are disabled: no need to stop preemption */ struct task_struct *tsk = __this_cpu_read(ksoftirqd); if (tsk && tsk->state != TASK_RUNNING) wake_up_process(tsk); } I guess we could avoid the NULL test if all these per_cpu var where pointing to a dummy task_struct at boot time, before they are properly allocated.
On Wed, 2016-05-11 at 14:56 -0700, Eric Dumazet wrote: > On Wed, 2016-05-11 at 08:55 +0200, Peter Zijlstra wrote: > > On Tue, May 10, 2016 at 03:51:37PM -0700, Eric Dumazet wrote: > > > diff --git a/kernel/softirq.c b/kernel/softirq.c > > > index 17caf4b63342..22463217e3cf 100644 > > > --- a/kernel/softirq.c > > > +++ b/kernel/softirq.c > > > @@ -56,6 +56,7 @@ EXPORT_SYMBOL(irq_stat); > > > static struct softirq_action softirq_vec[NR_SOFTIRQS] __cacheline_aligned_in_smp; > > > > > > DEFINE_PER_CPU(struct task_struct *, ksoftirqd); > > > +DEFINE_PER_CPU(bool, ksoftirqd_scheduled); > > > > > > const char * const softirq_to_name[NR_SOFTIRQS] = { > > > "HI", "TIMER", "NET_TX", "NET_RX", "BLOCK", "BLOCK_IOPOLL", > > > @@ -73,8 +74,10 @@ static void wakeup_softirqd(void) > > > /* Interrupts are disabled: no need to stop preemption */ > > > struct task_struct *tsk = __this_cpu_read(ksoftirqd); > > > > > > - if (tsk && tsk->state != TASK_RUNNING) > > > + if (tsk && tsk->state != TASK_RUNNING) { > > > + __this_cpu_write(ksoftirqd_scheduled, true); > > > wake_up_process(tsk); > > > > Since we're already looking at tsk->state, and the wake_up_process() > > ensures the thing becomes TASK_RUNNING, you could add: > > > > static inline bool ksoftirqd_running(void) > > { > > return __this_cpu_read(ksoftirqd)->state == TASK_RUNNING; > > } > > Indeed, and the patch looks quite simple now ;) > > diff --git a/kernel/softirq.c b/kernel/softirq.c > index 17caf4b63342d7839528f367b283a386413b0362..23c364485d03618773c385d943c0ef39f5931d09 100644 > --- a/kernel/softirq.c > +++ b/kernel/softirq.c > @@ -57,6 +57,11 @@ static struct softirq_action softirq_vec[NR_SOFTIRQS] __cacheline_aligned_in_smp > > DEFINE_PER_CPU(struct task_struct *, ksoftirqd); > > +static inline bool ksoftirqd_running(void) > +{ > + return __this_cpu_read(ksoftirqd)->state == TASK_RUNNING; > +} > + > const char * const softirq_to_name[NR_SOFTIRQS] = { > "HI", "TIMER", "NET_TX", "NET_RX", "BLOCK", "BLOCK_IOPOLL", > "TASKLET", "SCHED", "HRTIMER", "RCU" > @@ -313,7 +318,7 @@ asmlinkage __visible void do_softirq(void) > > pending = local_softirq_pending(); > > - if (pending) > + if (pending && !ksoftirqd_running()) > do_softirq_own_stack(); > > local_irq_restore(flags); > @@ -340,6 +345,9 @@ void irq_enter(void) > > static inline void invoke_softirq(void) > { > + if (ksoftirqd_running()) > + return; > + > if (!force_irqthreads) { > #ifdef CONFIG_HAVE_IRQ_EXIT_ON_IRQ_STACK > /* In this version of the path, the chunk affecting __local_bh_enable_ip() has been removed. I think it is beneficial, because it allows avoiding a local_irq_save()/local_irq_restore() pairs per local_bh_enable under heavy load. Cheers, Paolo
On Fri, May 13, 2016 at 9:50 AM, Paolo Abeni <pabeni@redhat.com> wrote: >> Indeed, and the patch looks quite simple now ;) >> >> diff --git a/kernel/softirq.c b/kernel/softirq.c >> index 17caf4b63342d7839528f367b283a386413b0362..23c364485d03618773c385d943c0ef39f5931d09 100644 >> --- a/kernel/softirq.c >> +++ b/kernel/softirq.c >> @@ -57,6 +57,11 @@ static struct softirq_action softirq_vec[NR_SOFTIRQS] __cacheline_aligned_in_smp >> >> DEFINE_PER_CPU(struct task_struct *, ksoftirqd); >> >> +static inline bool ksoftirqd_running(void) >> +{ >> + return __this_cpu_read(ksoftirqd)->state == TASK_RUNNING; >> +} >> + >> const char * const softirq_to_name[NR_SOFTIRQS] = { >> "HI", "TIMER", "NET_TX", "NET_RX", "BLOCK", "BLOCK_IOPOLL", >> "TASKLET", "SCHED", "HRTIMER", "RCU" >> @@ -313,7 +318,7 @@ asmlinkage __visible void do_softirq(void) >> >> pending = local_softirq_pending(); >> >> - if (pending) >> + if (pending && !ksoftirqd_running()) >> do_softirq_own_stack(); >> >> local_irq_restore(flags); >> @@ -340,6 +345,9 @@ void irq_enter(void) >> >> static inline void invoke_softirq(void) >> { >> + if (ksoftirqd_running()) >> + return; >> + >> if (!force_irqthreads) { >> #ifdef CONFIG_HAVE_IRQ_EXIT_ON_IRQ_STACK >> /* > > In this version of the path, the chunk affecting __local_bh_enable_ip() > has been removed. > > I think it is beneficial, because it allows avoiding a > local_irq_save()/local_irq_restore() pairs per local_bh_enable under heavy load. > Interesting, do you have any numbers ? I believe I did this so that we factorize the logic in do_softirq() and keep the code local to kernel/softirq.c Otherwise, netif_rx_ni() could also process softirq while ksoftirqd was scheduled, so I would have to 'export' the ksoftirqd_running(void) helper in an include file. I noticed that simply doing a "ping -n gateway" while the UDP flood was occurring, my udp receiver had quite a different efficiency. Thanks.
On Fri, 2016-05-13 at 10:03 -0700, Eric Dumazet wrote: > On Fri, May 13, 2016 at 9:50 AM, Paolo Abeni <pabeni@redhat.com> wrote: > > >> Indeed, and the patch looks quite simple now ;) > >> > >> diff --git a/kernel/softirq.c b/kernel/softirq.c > >> index 17caf4b63342d7839528f367b283a386413b0362..23c364485d03618773c385d943c0ef39f5931d09 100644 > >> --- a/kernel/softirq.c > >> +++ b/kernel/softirq.c > >> @@ -57,6 +57,11 @@ static struct softirq_action softirq_vec[NR_SOFTIRQS] __cacheline_aligned_in_smp > >> > >> DEFINE_PER_CPU(struct task_struct *, ksoftirqd); > >> > >> +static inline bool ksoftirqd_running(void) > >> +{ > >> + return __this_cpu_read(ksoftirqd)->state == TASK_RUNNING; > >> +} > >> + > >> const char * const softirq_to_name[NR_SOFTIRQS] = { > >> "HI", "TIMER", "NET_TX", "NET_RX", "BLOCK", "BLOCK_IOPOLL", > >> "TASKLET", "SCHED", "HRTIMER", "RCU" > >> @@ -313,7 +318,7 @@ asmlinkage __visible void do_softirq(void) > >> > >> pending = local_softirq_pending(); > >> > >> - if (pending) > >> + if (pending && !ksoftirqd_running()) > >> do_softirq_own_stack(); > >> > >> local_irq_restore(flags); > >> @@ -340,6 +345,9 @@ void irq_enter(void) > >> > >> static inline void invoke_softirq(void) > >> { > >> + if (ksoftirqd_running()) > >> + return; > >> + > >> if (!force_irqthreads) { > >> #ifdef CONFIG_HAVE_IRQ_EXIT_ON_IRQ_STACK > >> /* > > > > In this version of the path, the chunk affecting __local_bh_enable_ip() > > has been removed. > > > > I think it is beneficial, because it allows avoiding a > > local_irq_save()/local_irq_restore() pairs per local_bh_enable under heavy load. > > > > Interesting, do you have any numbers ? The difference is small, in the noise range: [with this patch applied] super_netperf 100 -H 192.168.122.1 -t UDP_STREAM -l 60 -- -m 1 9.00 [adding the test into __local_bh_enable_ip(), too] super_netperf 100 -H 192.168.122.1 -t UDP_STREAM -l 60 -- -m 1 9.14 but reproducible, in my experiments. I have similar data for different number of flows. > I believe I did this so that we factorize the logic in do_softirq() > and keep the code local to kernel/softirq.c > > Otherwise, netif_rx_ni() could also process softirq while ksoftirqd > was scheduled, > so I would have to 'export' the ksoftirqd_running(void) helper in an > include file. The idea could be to add the test in __local_bh_enable_ip(), maintaining the test also in do_softirq() (as currently done, i.e for local_softirq_pending()) Cheers, Paolo
On Fri, May 13, 2016 at 10:19 AM, Paolo Abeni <pabeni@redhat.com> wrote: > The difference is small, in the noise range: > > [with this patch applied] > super_netperf 100 -H 192.168.122.1 -t UDP_STREAM -l 60 -- -m 1 > 9.00 > > [adding the test into __local_bh_enable_ip(), too] > super_netperf 100 -H 192.168.122.1 -t UDP_STREAM -l 60 -- -m 1 > 9.14 > > but reproducible, in my experiments. > I have similar data for different number of flows. > >> I believe I did this so that we factorize the logic in do_softirq() >> and keep the code local to kernel/softirq.c >> >> Otherwise, netif_rx_ni() could also process softirq while ksoftirqd >> was scheduled, >> so I would have to 'export' the ksoftirqd_running(void) helper in an >> include file. > > The idea could be to add the test in __local_bh_enable_ip(), maintaining > the test also in do_softirq() (as currently done, i.e for > local_softirq_pending()) > Then I guess even the !in_interrupt() test we do is expensive and could be avoided, since do_softirq() is doing it again in the unlikely case it really is needed. @@ -162,7 +170,8 @@ void __local_bh_enable_ip(unsigned long ip, unsigned int cnt) */ preempt_count_sub(cnt - 1); - if (unlikely(!in_interrupt() && local_softirq_pending())) { + if (unlikely(local_softirq_pending()) && + !ksoftirqd_running()) { /* * Run softirq if any pending. And do it in its own stack * as we may be calling this deep in a task call stack already.
On Fri, 2016-05-13 at 10:36 -0700, Eric Dumazet wrote: > On Fri, May 13, 2016 at 10:19 AM, Paolo Abeni <pabeni@redhat.com> wrote: > > > The difference is small, in the noise range: > > > > [with this patch applied] > > super_netperf 100 -H 192.168.122.1 -t UDP_STREAM -l 60 -- -m 1 > > 9.00 > > > > [adding the test into __local_bh_enable_ip(), too] > > super_netperf 100 -H 192.168.122.1 -t UDP_STREAM -l 60 -- -m 1 > > 9.14 > > > > but reproducible, in my experiments. > > I have similar data for different number of flows. > > > >> I believe I did this so that we factorize the logic in do_softirq() > >> and keep the code local to kernel/softirq.c > >> > >> Otherwise, netif_rx_ni() could also process softirq while ksoftirqd > >> was scheduled, > >> so I would have to 'export' the ksoftirqd_running(void) helper in an > >> include file. > > > > The idea could be to add the test in __local_bh_enable_ip(), maintaining > > the test also in do_softirq() (as currently done, i.e for > > local_softirq_pending()) > > > > Then I guess even the !in_interrupt() test we do is expensive and > could be avoided, > since do_softirq() is doing it again in the unlikely case it really is needed. > > @@ -162,7 +170,8 @@ void __local_bh_enable_ip(unsigned long ip, > unsigned int cnt) > */ > preempt_count_sub(cnt - 1); > > - if (unlikely(!in_interrupt() && local_softirq_pending())) { > + if (unlikely(local_softirq_pending()) && > + !ksoftirqd_running()) { > /* > * Run softirq if any pending. And do it in its own stack > * as we may be calling this deep in a task call stack already. I'm sorry for the not-so-prompt reply. I had to use a different H/W, so I had to re-run the tests with all the patch flavors to get comparable results. While I can confirm that adding the '!ksoftirqd_running()' condition improves the throughput a little, but in a reproducible way, removing the '!in_interrupt()' don't change the result measurably, in my environment. While running the test against a kernel with the above chunk applied I got a couple of: [ 702.791025] NOHZ: local_softirq_pending 08 Not seen with the other versions of this patch. Cheers, Paolo
On Mon, May 16, 2016 at 6:10 AM, Paolo Abeni <pabeni@redhat.com> wrote: > > I'm sorry for the not-so-prompt reply. I had to use a different H/W, so > I had to re-run the tests with all the patch flavors to get comparable > results. > > While I can confirm that adding the '!ksoftirqd_running()' condition > improves the throughput a little, but in a reproducible way, removing > the '!in_interrupt()' don't change the result measurably, in my > environment. > > While running the test against a kernel with the above chunk applied I > got a couple of: > > [ 702.791025] NOHZ: local_softirq_pending 08 > > Not seen with the other versions of this patch. I've seen this message in all versions, depending on the workload. Either a barrier of some kind is missing, or we uncover an existing bug. Note that in my tests on an older base kernel (something based on 3.11 but with thousands of patches), I would not have the scary rcu messages that I got with current upstream kernels.
diff --git a/kernel/softirq.c b/kernel/softirq.c index 17caf4b63342d7839528f367b283a386413b0362..23c364485d03618773c385d943c0ef39f5931d09 100644 --- a/kernel/softirq.c +++ b/kernel/softirq.c @@ -57,6 +57,11 @@ static struct softirq_action softirq_vec[NR_SOFTIRQS] __cacheline_aligned_in_smp DEFINE_PER_CPU(struct task_struct *, ksoftirqd); +static inline bool ksoftirqd_running(void) +{ + return __this_cpu_read(ksoftirqd)->state == TASK_RUNNING; +} + const char * const softirq_to_name[NR_SOFTIRQS] = { "HI", "TIMER", "NET_TX", "NET_RX", "BLOCK", "BLOCK_IOPOLL", "TASKLET", "SCHED", "HRTIMER", "RCU" @@ -313,7 +318,7 @@ asmlinkage __visible void do_softirq(void) pending = local_softirq_pending(); - if (pending) + if (pending && !ksoftirqd_running()) do_softirq_own_stack(); local_irq_restore(flags); @@ -340,6 +345,9 @@ void irq_enter(void) static inline void invoke_softirq(void) { + if (ksoftirqd_running()) + return; + if (!force_irqthreads) { #ifdef CONFIG_HAVE_IRQ_EXIT_ON_IRQ_STACK /*