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

Groups > linux.kernel > #1398143 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [RFC PATCH 0/2] net: threadable napi poll loop Paolo Abeni <pabeni@redhat.com> - 2016-05-10 16:20 +0200
    [RFC PATCH 2/2] net: add sysfs attribute to control napi threaded mode Paolo Abeni <pabeni@redhat.com> - 2016-05-10 16:20 +0200
    Re: [RFC PATCH 0/2] net: threadable napi poll loop Eric Dumazet <eric.dumazet@gmail.com> - 2016-05-10 16:40 +0200
      Re: [RFC PATCH 0/2] net: threadable napi poll loop David Miller <davem@davemloft.net> - 2016-05-10 18:00 +0200
      Re: [RFC PATCH 0/2] net: threadable napi poll loop Eric Dumazet <eric.dumazet@gmail.com> - 2016-05-10 18:10 +0200
        Re: [RFC PATCH 0/2] net: threadable napi poll loop Paolo Abeni <pabeni@redhat.com> - 2016-05-10 22:30 +0200
          Re: [RFC PATCH 0/2] net: threadable napi poll loop David Miller <davem@davemloft.net> - 2016-05-10 22:50 +0200
            Re: [RFC PATCH 0/2] net: threadable napi poll loop David Miller <davem@davemloft.net> - 2016-05-10 23:00 +0200
              Re: [RFC PATCH 0/2] net: threadable napi poll loop Rik van Riel <riel@redhat.com> - 2016-05-10 23:10 +0200
            Re: [RFC PATCH 0/2] net: threadable napi poll loop Rik van Riel <riel@redhat.com> - 2016-05-10 23:00 +0200
      Re: [RFC PATCH 0/2] net: threadable napi poll loop Paolo Abeni <pabeni@redhat.com> - 2016-05-10 18:10 +0200
      Re: [RFC PATCH 0/2] net: threadable napi poll loop Hannes Frederic Sowa <hannes@stressinduktion.org> - 2016-05-10 22:50 +0200
        Re: [RFC PATCH 0/2] net: threadable napi poll loop Eric Dumazet <edumazet@google.com> - 2016-05-10 23:10 +0200
          Re: [RFC PATCH 0/2] net: threadable napi poll loop Eric Dumazet <eric.dumazet@gmail.com> - 2016-05-10 23:40 +0200
            Re: [RFC PATCH 0/2] net: threadable napi poll loop Rik van Riel <riel@redhat.com> - 2016-05-10 23:40 +0200
              Re: [RFC PATCH 0/2] net: threadable napi poll loop Eric Dumazet <eric.dumazet@gmail.com> - 2016-05-11 00:00 +0200
                Re: [RFC PATCH 0/2] net: threadable napi poll loop Rik van Riel <riel@redhat.com> - 2016-05-11 00:10 +0200
                Re: [RFC PATCH 0/2] net: threadable napi poll loop Eric Dumazet <eric.dumazet@gmail.com> - 2016-05-11 00:10 +0200
                  Re: [RFC PATCH 0/2] net: threadable napi poll loop Eric Dumazet <eric.dumazet@gmail.com> - 2016-05-11 00:50 +0200
                Re: [RFC PATCH 0/2] net: threadable napi poll loop Eric Dumazet <eric.dumazet@gmail.com> - 2016-05-11 20:00 +0200
          Re: [RFC PATCH 0/2] net: threadable napi poll loop Hannes Frederic Sowa <hannes@stressinduktion.org> - 2016-05-11 00:40 +0200
            Re: [RFC PATCH 0/2] net: threadable napi poll loop Eric Dumazet <eric.dumazet@gmail.com> - 2016-05-11 01:00 +0200
              Re: [RFC PATCH 0/2] net: threadable napi poll loop Peter Zijlstra <peterz@infradead.org> - 2016-05-11 09:00 +0200
                Re: [RFC PATCH 0/2] net: threadable napi poll loop Hannes Frederic Sowa <hannes@stressinduktion.org> - 2016-05-11 15:20 +0200
                  Re: [RFC PATCH 0/2] net: threadable napi poll loop Eric Dumazet <edumazet@google.com> - 2016-05-11 16:50 +0200
                    Re: [RFC PATCH 0/2] net: threadable napi poll loop Rik van Riel <riel@redhat.com> - 2016-05-11 17:10 +0200
                    Re: [RFC PATCH 0/2] net: threadable napi poll loop Eric Dumazet <eric.dumazet@gmail.com> - 2016-05-11 18:00 +0200
                Re: [RFC PATCH 0/2] net: threadable napi poll loop Eric Dumazet <eric.dumazet@gmail.com> - 2016-05-12 00:00 +0200
                  Re: [RFC PATCH 0/2] net: threadable napi poll loop Paolo Abeni <pabeni@redhat.com> - 2016-05-12 22:10 +0200
                    Re: [RFC PATCH 0/2] net: threadable napi poll loop Eric Dumazet <eric.dumazet@gmail.com> - 2016-05-12 22:50 +0200
                      Re: [RFC PATCH 0/2] net: threadable napi poll loop Paolo Abeni <pabeni@redhat.com> - 2016-05-12 23:00 +0200
                        Re: [RFC PATCH 0/2] net: threadable napi poll loop Eric Dumazet <eric.dumazet@gmail.com> - 2016-05-12 23:10 +0200
                  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
              Re: [RFC PATCH 0/2] net: threadable napi poll loop Paolo Abeni <pabeni@redhat.com> - 2016-05-11 11:50 +0200
                Re: [RFC PATCH 0/2] net: threadable napi poll loop Eric Dumazet <eric.dumazet@gmail.com> - 2016-05-11 15:10 +0200
                  Re: [RFC PATCH 0/2] net: threadable napi poll loop Hannes Frederic Sowa <hannes@stressinduktion.org> - 2016-05-11 15:40 +0200
                    Re: [RFC PATCH 0/2] net: threadable napi poll loop Hannes Frederic Sowa <hannes@stressinduktion.org> - 2016-05-11 15:50 +0200
                  Re: [RFC PATCH 0/2] net: threadable napi poll loop Paolo Abeni <pabeni@redhat.com> - 2016-05-11 16:40 +0200
                    Re: [RFC PATCH 0/2] net: threadable napi poll loop Eric Dumazet <edumazet@google.com> - 2016-05-11 16:50 +0200
                      Re: [RFC PATCH 0/2] net: threadable napi poll loop Hannes Frederic Sowa <hannes@stressinduktion.org> - 2016-05-12 00:50 +0200
    Re: [RFC PATCH 0/2] net: threadable napi poll loop Thomas Gleixner <tglx@linutronix.de> - 2016-05-10 18:00 +0200
      Re: [RFC PATCH 0/2] net: threadable napi poll loop Paolo Abeni <pabeni@redhat.com> - 2016-05-10 22:50 +0200

Page 2 of 3 — ← Prev page 1 [2] 3  Next page →

#1398562

On 10.05.2016 23:09, Eric Dumazet wrote:
> On Tue, May 10, 2016 at 1:46 PM, Hannes Frederic Sowa
> <hannes@stressinduktion.org> wrote:
> 
>> I agree here, but I don't think this patch particularly is a lot of
>> bloat and something very interesting people can play with and extend upon.
>>
> 
> Sure, very rarely patch authors think their stuff is bloat.
> 
> I prefer to fix kernel softirq.c, or at least show me that you tried
> hard enough.
> 
> I am pretty sure that the following would work :
> 
> When ksoftirqd is scheduled, remember this in a per cpu variable
> (ksoftiqd_scheduled)
> 
> When enabling BH , do not call do_softirq() if this variable is set.
> 
> ksoftirqd would clear the variable at the right place (probably in
> run_ksoftirqd())
> 
> Sure, this might add a lot of latency regressions, but lets fix them.

Probably, yes.

We had a version which limited the number of restarts if softirqs were
invoked from local_bh_enable (so that at least timers etc. would run)
and would defer all other work to ksoftirqd. That also solved the
initial live lock problem. I do have concerns about the fairness of this
approach, but we now have to investigate this. ;)

Not only did we want to present this solely as a bugfix but also as as
performance enhancements in case of virtio (as you can see in the cover
letter). Given that a long time ago there was a tendency to remove
softirqs completely, we thought it might be very interesting, that a
threaded napi in general seems to be absolutely viable nowadays and
might offer new features.

Bye,
Hannes

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

#1398570

On Wed, 2016-05-11 at 00:32 +0200, Hannes Frederic Sowa wrote:

> Not only did we want to present this solely as a bugfix but also as as
> performance enhancements in case of virtio (as you can see in the cover
> letter). Given that a long time ago there was a tendency to remove
> softirqs completely, we thought it might be very interesting, that a
> threaded napi in general seems to be absolutely viable nowadays and
> might offer new features.

Well, you did not fix the bug, you worked around by adding yet another
layer, with another sysctl that admins or programs have to manage.

If you have a special need for virtio, do not hide it behind a 'bug fix'
but add it as a features request.

This ksoftirqd issue is real and a fix looks very reasonable.

Please try this patch, as I had very good success with it.

Thanks.


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);
+	}
 }
 
 /*
@@ -162,7 +165,9 @@ 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(!in_interrupt() &&
+		     local_softirq_pending() &&
+		     !__this_cpu_read(ksoftirqd_scheduled))) {
 		/*
 		 * 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.
@@ -340,6 +345,9 @@ void irq_enter(void)
 
 static inline void invoke_softirq(void)
 {
+	if (__this_cpu_read(ksoftirqd_scheduled))
+		return;
+
 	if (!force_irqthreads) {
 #ifdef CONFIG_HAVE_IRQ_EXIT_ON_IRQ_STACK
 		/*
@@ -660,6 +668,8 @@ static void run_ksoftirqd(unsigned int cpu)
 		 * in the task stack here.
 		 */
 		__do_softirq();
+		if (!local_softirq_pending())
+			__this_cpu_write(ksoftirqd_scheduled, false);
 		local_irq_enable();
 		cond_resched_rcu_qs();
 		return;

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

#1398717

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;
}

> +	}
>  }
>  
>  /*
> @@ -162,7 +165,9 @@ 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(!in_interrupt() &&
> +		     local_softirq_pending() &&
> +		     !__this_cpu_read(ksoftirqd_scheduled))) {

And use it here,

>  		/*
>  		 * 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.
> @@ -340,6 +345,9 @@ void irq_enter(void)
>  
>  static inline void invoke_softirq(void)
>  {
> +	if (__this_cpu_read(ksoftirqd_scheduled))

and here.

> +		return;
> +
>  	if (!force_irqthreads) {
>  #ifdef CONFIG_HAVE_IRQ_EXIT_ON_IRQ_STACK
>  		/*
> @@ -660,6 +668,8 @@ static void run_ksoftirqd(unsigned int cpu)
>  		 * in the task stack here.
>  		 */
>  		__do_softirq();
> +		if (!local_softirq_pending())
> +			__this_cpu_write(ksoftirqd_scheduled, false);

And avoid twiddling the new variable which only seems to mirror
tsk->state.

>  		local_irq_enable();
>  		cond_resched_rcu_qs();
>  		return;
> 
> 

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

#1399065

On 11.05.2016 08:55, 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;
> }

This looks racy to me as the ksoftirqd could be in the progress to stop
and we would miss another softirq invocation.

Thanks,
Hannes

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

#1399197

On Wed, May 11, 2016 at 6:13 AM, Hannes Frederic Sowa
<hannes@stressinduktion.org> wrote:

> This looks racy to me as the ksoftirqd could be in the progress to stop
> and we would miss another softirq invocation.

Looking at smpboot_thread_fn(), it looks fine :

                if (!ht->thread_should_run(td->cpu)) {
                        preempt_enable_no_resched();
                        schedule();
                } else {
                        __set_current_state(TASK_RUNNING);
                        preempt_enable();
                        ht->thread_fn(td->cpu);
                }

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

#1399225

[Multipart message — attachments visible in raw view] — view raw

On Wed, 2016-05-11 at 07:40 -0700, Eric Dumazet wrote:
> On Wed, May 11, 2016 at 6:13 AM, Hannes Frederic Sowa
> <hannes@stressinduktion.org> wrote:
> 
> > This looks racy to me as the ksoftirqd could be in the progress to
> > stop
> > and we would miss another softirq invocation.
> 
> Looking at smpboot_thread_fn(), it looks fine :
> 

Additionally, we are talking about waking up
ksoftirqd on the same CPU.

That means the wakeup code could interrupt
ksoftirqd almost going to sleep, but the
two code paths could not run simultaneously.

That does narrow the scope considerably.

-- 
All rights reversed

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

#1399291

On Wed, 2016-05-11 at 07:40 -0700, Eric Dumazet wrote:
> On Wed, May 11, 2016 at 6:13 AM, Hannes Frederic Sowa
> <hannes@stressinduktion.org> wrote:
> 
> > This looks racy to me as the ksoftirqd could be in the progress to stop
> > and we would miss another softirq invocation.
> 
> Looking at smpboot_thread_fn(), it looks fine :
> 
>                 if (!ht->thread_should_run(td->cpu)) {
>                         preempt_enable_no_resched();
>                         schedule();
>                 } else {
>                         __set_current_state(TASK_RUNNING);
>                         preempt_enable();
>                         ht->thread_fn(td->cpu);
>                 }

BTW, I wonder why we pass td->cpu as argument to ht->thread_fn(td->cpu)

This always should be the current processor id.

Or do we have an issue because we ignore it in :

static int ksoftirqd_should_run(unsigned int cpu)
{
        return local_softirq_pending();
}

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

#1399539

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
 		/*

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

#1400341

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
>  		/*
> 
> 

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

#1400365

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 ?

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

#1400372

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

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

#1400374

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.

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

#1400901

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] | [prev] | [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] | [next] | [standalone]

#1398871

Hi Eric,
On Tue, 2016-05-10 at 15:51 -0700, Eric Dumazet wrote:
> On Wed, 2016-05-11 at 00:32 +0200, Hannes Frederic Sowa wrote:
> 
> > Not only did we want to present this solely as a bugfix but also as as
> > performance enhancements in case of virtio (as you can see in the cover
> > letter). Given that a long time ago there was a tendency to remove
> > softirqs completely, we thought it might be very interesting, that a
> > threaded napi in general seems to be absolutely viable nowadays and
> > might offer new features.
> 
> Well, you did not fix the bug, you worked around by adding yet another
> layer, with another sysctl that admins or programs have to manage.
> 
> If you have a special need for virtio, do not hide it behind a 'bug fix'
> but add it as a features request.
> 
> This ksoftirqd issue is real and a fix looks very reasonable.
> 
> Please try this patch, as I had very good success with it.

Thank you for your time and your effort.

I tested your patch on the bare metal "single core" scenario, disabling
the unneeded cores with:
CPUS=`nproc`
for I in `seq 1 $CPUS`; do echo 0  >  /sys/devices/system/node/node0/cpu$I/online; done

And I got a:

[   86.925249] Broke affinity for irq <num>

for each irq number generated by a network device.

In this scenario, your patch solves the ksoftirqd issue, performing
comparable to the napi threaded patches (with a negative delta in the
noise range) and introducing a minor regression with a single flow, in
the noise range (3%).

As said in a previous mail, we actually experimented something similar,
but it felt quite hackish.

AFAICS this patch adds three more tests in the fast path and affect all
other softirq use case. I'm not sure how to check for regression there.

The napi thread patches are actually a new feature, that also fixes the
ksoftirqd issue: hunting the ksoftirqd issue has been the initial
trigger for this work. I'm sorry for not being clear enough in the cover
letter.

The napi thread patches offer additional benefits, i.e. an additional
relevant gain in the described test scenario, and do not impact on other
subsystems/kernel entities. 

I still think they are worthy, and I bet you would disagree, but could
you please articulate more which parts concern you most and/or are more
bloated ?

Thank you,

Paolo

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

#1399064

On Wed, 2016-05-11 at 11:48 +0200, Paolo Abeni wrote:
> Hi Eric,
> On Tue, 2016-05-10 at 15:51 -0700, Eric Dumazet wrote:
> > On Wed, 2016-05-11 at 00:32 +0200, Hannes Frederic Sowa wrote:
> > 
> > > Not only did we want to present this solely as a bugfix but also as as
> > > performance enhancements in case of virtio (as you can see in the cover
> > > letter). Given that a long time ago there was a tendency to remove
> > > softirqs completely, we thought it might be very interesting, that a
> > > threaded napi in general seems to be absolutely viable nowadays and
> > > might offer new features.
> > 
> > Well, you did not fix the bug, you worked around by adding yet another
> > layer, with another sysctl that admins or programs have to manage.
> > 
> > If you have a special need for virtio, do not hide it behind a 'bug fix'
> > but add it as a features request.
> > 
> > This ksoftirqd issue is real and a fix looks very reasonable.
> > 
> > Please try this patch, as I had very good success with it.
> 
> Thank you for your time and your effort.
> 
> I tested your patch on the bare metal "single core" scenario, disabling
> the unneeded cores with:
> CPUS=`nproc`
> for I in `seq 1 $CPUS`; do echo 0  >  /sys/devices/system/node/node0/cpu$I/online; done
> 
> And I got a:
> 
> [   86.925249] Broke affinity for irq <num>
> 

Was it fatal, or simply a warning that you are removing the cpu that was
the only allowed cpu in an affinity_mask ?

Looks another bug to fix then ? We disabled CPU hotplug here at Google
for our production, as it was notoriously buggy. No time to fix dozens
of issues added by a crowd of developers that do not even know a cpu can
be unplugged.

Maybe some caller of local_bh_disable()/local_bh_enable() expected that
current softirq would be processed. Obviously flaky even before the
patches.

> for each irq number generated by a network device.
> 
> In this scenario, your patch solves the ksoftirqd issue, performing
> comparable to the napi threaded patches (with a negative delta in the
> noise range) and introducing a minor regression with a single flow, in
> the noise range (3%).
> 
> As said in a previous mail, we actually experimented something similar,
> but it felt quite hackish.

Right, we are networking guys, and we feel that messing with such core
infra is not for us. So we feel comfortable adding a pure networking
patch.

> 
> AFAICS this patch adds three more tests in the fast path and affect all
> other softirq use case. I'm not sure how to check for regression there.

It is obvious to me that ksoftird mechanism is not working as intended.

Fixing it might uncover bugs from parts of the kernel relying on the
bug, indirectly or directly. Is it a good thing ?

I can not tell before trying.

Just by looking at /proc/{ksoftirqs_pid}/sched you can see the problem,
as we normally schedule ksoftird under stress but most of the time,
the softirq items were processed by another tasks as you found out.


> 
> The napi thread patches are actually a new feature, that also fixes the
> ksoftirqd issue: hunting the ksoftirqd issue has been the initial
> trigger for this work. I'm sorry for not being clear enough in the cover
> letter.
> 
> The napi thread patches offer additional benefits, i.e. an additional
> relevant gain in the described test scenario, and do not impact on other
> subsystems/kernel entities. 
> 
> I still think they are worthy, and I bet you would disagree, but could
> you please articulate more which parts concern you most and/or are more
> bloated ?

Just look at the added code. napi_threaded_poll() is very buggy, but
honestly I do not want to fix the bugs you added there. If you have only
one vcpu, how jiffies can ever change since you block BH ?

I was planning to remove cond_resched_softirq() that we no longer use
after my recent changes to TCP stack,
and you call it again (while it is obviously buggy since it does not
check if a BH is pending, only if a thread needs the cpu)

I prefer fixing the existing code, really. It took us years to
understand it and maybe fix it.

Just think of what will happen if you have 10 devices (10 new threads in
your model) and one cpu.

Instead of the nice existing netif_rx() doing 64 packets per device
rounds, you'll now rely on process scheduler behavior that has no such
granularity.

Adding more threads is the natural answer of userland programmers, but
in the kernel it is not the right answer. We already have mechanism,
just use them and fix them if they are broken.

Sorry, I really do not think your patches are the way to go.
But this thread is definitely interesting.

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

Page 2 of 3 — ← Prev page 1 [2] 3  Next page →

Back to top | Article view | linux.kernel

csiph-web