Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1400901 > unrolled thread

Re: [RFC PATCH 0/2] net: threadable napi poll loop

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [RFC PATCH 0/2] net: threadable napi poll loop Paolo Abeni <pabeni@redhat.com> - 2016-05-13 19:00 +0200
    Re: [RFC PATCH 0/2] net: threadable napi poll loop Eric Dumazet <edumazet@google.com> - 2016-05-13 19:10 +0200
      Re: [RFC PATCH 0/2] net: threadable napi poll loop Paolo Abeni <pabeni@redhat.com> - 2016-05-13 19:20 +0200
        Re: [RFC PATCH 0/2] net: threadable napi poll loop Eric Dumazet <edumazet@google.com> - 2016-05-13 19:40 +0200
          Re: [RFC PATCH 0/2] net: threadable napi poll loop Paolo Abeni <pabeni@redhat.com> - 2016-05-16 15:20 +0200
            Re: [RFC PATCH 0/2] net: threadable napi poll loop Eric Dumazet <edumazet@google.com> - 2016-05-16 15:40 +0200

#1400901 — Re: [RFC PATCH 0/2] net: threadable napi poll loop

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

[toc] | [next] | [standalone]

#1400904

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.

[toc] | [prev] | [next] | [standalone]

#1400911

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

[toc] | [prev] | [next] | [standalone]

#1400921

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.

[toc] | [prev] | [next] | [standalone]

#1401492

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

[toc] | [prev] | [next] | [standalone]

#1401502

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.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web