net: threadable napi poll loop

Currently, the softirq loop can be scheduled both inside the ksofirqd kernel thread and inside any running process. This makes nearly impossible for the process scheduler to balance in a fair way the amount of time that a given core spends performing the softirq loop.

Under high network load, the softirq loop can take nearly 100% of a given CPU, leaving very little time for use space processing. On single core hosts, this means that the user space can nearly starve; for example super_netperf UDP_STREAM tests towards a remote single core vCPU guest[1] can measure an aggregated throughput of a few thousands pps, and the same behavior can be reproduced even on bare-metal, eventually simulating a single core with taskset and/or sysfs configuration.

We already have ksoftirqd to normally cope with the case you are describing.

If it is not working as intended, please identify the bugs and fix them, instead of adding yet another tests in fast path and extra complexity in the stack.

Quoted 5 lines · Paolo Abeni

Currently, the softirq loop can be scheduled both inside the ksofirqd kernel thread and inside any running process. This makes nearly impossible for the process scheduler to balance in a fair way the amount of time that a given core spends performing the softirq loop.

Quoted 7 lines

Under high network load, the softirq loop can take nearly 100% of a given CPU, leaving very little time for use space processing. On single core hosts, this means that the user space can nearly starve; for example super_netperf UDP_STREAM tests towards a remote single core vCPU guest[1] can measure an aggregated throughput of a few thousands pps, and the same behavior can be reproduced even on bare-metal, eventually simulating a single core with taskset and/or sysfs configuration.

I hate these patches and ideas guys, sorry. That is before my breakfast, but still...

I have enough hard time dealing with loads where ksoftirqd has to compete with user threads that thought that playing with priorities was a nice idea.

Guess what, when they lose networking they complain.

We already have ksoftirqd to normally cope with the case you are describing.

If it is not working as intended, please identify the bugs and fix them, instead of adding yet another tests in fast path and extra complexity in the stack.

In the one vcpu case, allowing the user thread to consume more UDP packets from the target UDP socket will also make your NIC drop more packets, that are not necessarily packets for the same socket.

So you are shifting the attack to a different target, at the expense of more kernel bloat.

If a single core host is under network flood, i.e. ksoftirqd is scheduled and it eventually (after processing ~640 packets) will let the user space process run. The latter will execute a syscall to receive a packet, which will have to disable/enable bh at least once and that will cause the processing of another ~640 packets. To receive a single packet in user space, the kernel has to process more than one thousand packets.

Quoted 6 lines · Paolo Abeni

If a single core host is under network flood, i.e. ksoftirqd is scheduled and it eventually (after processing ~640 packets) will let the user space process run. The latter will execute a syscall to receive a packet, which will have to disable/enable bh at least once and that will cause the processing of another ~640 packets. To receive a single packet in user space, the kernel has to process more than one thousand packets.

Looks you found the bug then. Have you tried to fix it ?

Quoted 10 lines · Paolo Abeni

Quoted 7 lines

If a single core host is under network flood, i.e. ksoftirqd is scheduled and it eventually (after processing ~640 packets) will let the user space process run. The latter will execute a syscall to receive a packet, which will have to disable/enable bh at least once and that will cause the processing of another ~640 packets. To receive a single packet in user space, the kernel has to process more than one thousand packets.

Looks you found the bug then. Have you tried to fix it ?

The ksoftirq and the local_bh_enable() design are the root of the problem, they need to be touched/affected to solve it.

From: Paolo Abeni <[email protected]>
Date: Tue, 10 May 2016 22:22:50 +0200

Quoted 5 lines · Eric Dumazet

Quoted 4 lines · Paolo Abeni

Quoted 2 lines

If a single core host is under network flood, i.e. ksoftirqd is scheduled and it eventually (after processing ~640 packets) will

let the

Quoted 1 line

Quoted 1 line

Quoted 1 line

user space process run. The latter will execute a syscall to

receive a

Quoted 1 line

Quoted 1 line

Quoted 1 line

packet, which will have to disable/enable bh at least once and

that will

Quoted 1 line

Quoted 1 line

Quoted 1 line

cause the processing of another ~640 packets. To receive a

single packet

Quoted 1 line

Quoted 1 line

Quoted 1 line

in user space, the kernel has to process more than one thousand

packets.

Quoted 2 lines

Quoted 2 lines

Looks you found the bug then. Have you tried to fix it ?

 ...

Quoted 2 lines

The ksoftirq and the local_bh_enable() design are the root of the problem, they need to be touched/affected to solve it.

That's not what I read from your description, processing 640 packets before going to ksoftirqd seems to the be the absolute root problem.

Quoted 17 lines

From: Paolo Abeni <[email protected]>
Date: Tue, 10 May 2016 22:22:50 +0200

Quoted 5 lines · Eric Dumazet

Quoted 4 lines · Paolo Abeni

Quoted 2 lines

If a single core host is under network flood, i.e. ksoftirqd is scheduled and it eventually (after processing ~640 packets) will

let the

Quoted 1 line

Quoted 1 line

Quoted 1 line

user space process run. The latter will execute a syscall to

receive a

Quoted 1 line

Quoted 1 line

Quoted 1 line

packet, which will have to disable/enable bh at least once and

that will

Quoted 1 line

Quoted 1 line

Quoted 1 line

cause the processing of another ~640 packets. To receive a

single packet

Quoted 1 line

Quoted 1 line

Quoted 1 line

in user space, the kernel has to process more than one thousand

packets.

Quoted 8 lines

Quoted 2 lines

Quoted 2 lines

Looks you found the bug then. Have you tried to fix it ?

 ...

Quoted 2 lines

The ksoftirq and the local_bh_enable() design are the root of the problem, they need to be touched/affected to solve it.

That's not what I read from your description, processing 640 packets before going to ksoftirqd seems to the be the absolute root problem.

What would a fix for that look like?

Keep track of the number of processed incoming packets, and the number of packets handed off, and defer to ksoftirqd earlier if the statistics suggest packets are getting dropped on the floor?

From: Rik van Riel <[email protected]>
Date: Tue, 10 May 2016 16:50:56 -0400

Quoted 8 lines · David Miller

Quoted 7 lines

From: Paolo Abeni <[email protected]>
Date: Tue, 10 May 2016 22:22:50 +0200

Quoted 4 lines · Eric Dumazet

Quoted 3 lines · Paolo Abeni

Quoted 1 line

If a single core host is under network flood, i.e. ksoftirqd

is

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

scheduled and it eventually (after processing ~640 packets)

will

Quoted 4 lines

Quoted 4 lines

let the

Quoted 1 line

Quoted 1 line

Quoted 1 line

user space process run. The latter will execute a syscall to

receive a

Quoted 1 line

Quoted 1 line

Quoted 1 line

packet, which will have to disable/enable bh at least once

and

Quoted 4 lines

Quoted 4 lines

that will

Quoted 1 line

Quoted 1 line

Quoted 1 line

cause the processing of another ~640 packets. To receive a

single packet

Quoted 1 line

Quoted 1 line

Quoted 1 line

in user space, the kernel has to process more than one

thousand

Quoted 5 lines

Quoted 5 lines

packets.

Quoted 2 lines

Quoted 2 lines

Looks you found the bug then. Have you tried to fix it ?

 ...

Quoted 1 line

The ksoftirq and the local_bh_enable() design are the root of

the

Quoted 3 lines

Quoted 3 lines

Quoted 1 line

problem, they need to be touched/affected to solve it.

That's not what I read from your description, processing 640

packets

Quoted 1 line

Quoted 1 line

before going to ksoftirqd seems to the be the absolute root

problem.

Quoted 7 lines

What would a fix for that look like?

Keep track of the number of processed incoming packets, and the number of packets handed off, and defer to ksoftirqd earlier if the statistics suggest packets are getting dropped on the floor?

Not by packet count but by something more easily to measure and scalable to fairness like processing time.

Quoted 5 lines · Paolo Abeni

Currently, the softirq loop can be scheduled both inside the ksofirqd kernel thread and inside any running process. This makes nearly impossible for the process scheduler to balance in a fair way the amount of time that a given core spends performing the softirq loop.

Quoted 7 lines

Under high network load, the softirq loop can take nearly 100% of a given CPU, leaving very little time for use space processing. On single core hosts, this means that the user space can nearly starve; for example super_netperf UDP_STREAM tests towards a remote single core vCPU guest[1] can measure an aggregated throughput of a few thousands pps, and the same behavior can be reproduced even on bare-metal, eventually simulating a single core with taskset and/or sysfs configuration.

I hate these patches and ideas guys, sorry. That is before my breakfast, but still...

I have enough hard time dealing with loads where ksoftirqd has to compete with user threads that thought that playing with priorities was a nice idea.

Guess what, when they lose networking they complain.

We already have ksoftirqd to normally cope with the case you are describing.

If it is not working as intended, please identify the bugs and fix them, instead of adding yet another tests in fast path and extra complexity in the stack.

In the one vcpu case, allowing the user thread to consume more UDP packets from the target UDP socket will also make your NIC drop more packets, that are not necessarily packets for the same socket.

So you are shifting the attack to a different target, at the expense of more kernel bloat.

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.

Quoted 3 lines · Hannes Frederic Sowa

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.

Quoted 21 lines · Eric Dumazet

Quoted 6 lines · Hannes Frederic Sowa

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)

Quoted 8 lines

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.

Only to give the idea (it is completely untested and probably buggy)

kernel/softirq.c +0 -0

index 17caf4b63342..cb30cfd76687 100644
@@ -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",

kernel/softirq.c +0 -0

@@ -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);

kernel/softirq.c +2 -1

-	if (tsk && tsk->state != TASK_RUNNING)
+	if (tsk && tsk->state != TASK_RUNNING) {
+		__this_cpu_write(ksoftirqd_scheduled, true);

  wake_up_process(tsk);

kernel/softirq.c +1 -0

+	}

 }

 /*

kernel/softirq.c +0 -0

@@ -162,7 +165,9 @@ void __local_bh_enable_ip(unsigned long ip,
 unsigned int cnt)
  	 */
  	preempt_count_sub(cnt - 1);

kernel/softirq.c +3 -1

-	if (unlikely(!in_interrupt() && local_softirq_pending())) {
+	if (unlikely(!in_interrupt() &&
+		     local_softirq_pending() &&
+		     !__this_cpu_read(ksoftirqd_scheduled))) {

  /*

You might need another one of these in invoke_softirq()

Quoted 2 lines · Rik van Riel

You might need another one of these in invoke_softirq()

Excellent.

I gave it a quick try (without your suggestion), and host seems to survive a stress test.

Of course we do have to fix these problems :

[  147.781629] NOHZ: local_softirq_pending 48
[  147.785546] NOHZ: local_softirq_pending 48
[  147.788344] NOHZ: local_softirq_pending 48
[  147.788992] NOHZ: local_softirq_pending 48
[  147.790943] NOHZ: local_softirq_pending 48
[  147.791232] NOHZ: local_softirq_pending 24a
[  147.791258] NOHZ: local_softirq_pending 48
[  147.791366] NOHZ: local_softirq_pending 48
[  147.792118] NOHZ: local_softirq_pending 48
[  147.793428] NOHZ: local_softirq_pending 48

Quoted 12 lines · Eric Dumazet

Quoted 2 lines · Rik van Riel

You might need another one of these in invoke_softirq()

Excellent.

I gave it a quick try (without your suggestion), and host seems to survive a stress test.

Of course we do have to fix these problems :

Quoted 10 lines

[  147.781629] NOHZ: local_softirq_pending 48
[  147.785546] NOHZ: local_softirq_pending 48
[  147.788344] NOHZ: local_softirq_pending 48
[  147.788992] NOHZ: local_softirq_pending 48
[  147.790943] NOHZ: local_softirq_pending 48
[  147.791232] NOHZ: local_softirq_pending 24a
[  147.791258] NOHZ: local_softirq_pending 48
[  147.791366] NOHZ: local_softirq_pending 48
[  147.792118] NOHZ: local_softirq_pending 48
[  147.793428] NOHZ: local_softirq_pending 48

Well, with your suggestion, these warnings disappear ;)

Quoted 3 lines · Rik van Riel

You might need another one of these in invoke_softirq()

Excellent.

I gave it a quick try (without your suggestion), and host seems to survive a stress test.

Of course we do have to fix these problems :

[  147.781629] NOHZ: local_softirq_pending 48 [  147.785546] NOHZ: local_softirq_pending 48 [  147.788344] NOHZ: local_softirq_pending 48 [  147.788992] NOHZ: local_softirq_pending 48 [  147.790943] NOHZ: local_softirq_pending 48 [  147.791232] NOHZ: local_softirq_pending 24a [  147.791258] NOHZ: local_softirq_pending 48 [  147.791366] NOHZ: local_softirq_pending 48 [  147.792118] NOHZ: local_softirq_pending 48 [  147.793428] NOHZ: local_softirq_pending 48

Quoted 2 lines · Rik van Riel

You might need another one of these in invoke_softirq()

Excellent.

I gave it a quick try (without your suggestion), and host seems to survive a stress test.

Quoted 3 lines · Hannes Frederic Sowa

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.

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.

kernel/softirq.c +1 -0

index 17caf4b63342..22463217e3cf 100644
@@ -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",

kernel/softirq.c +0 -0

 		local_irq_enable();
 		cond_resched_rcu_qs();
 		return;

Quoted 13 lines · Eric Dumazet

kernel/softirq.c +1 -0

index 17caf4b63342..22463217e3cf 100644
@@ -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",

kernel/softirq.c static void wakeup_softirqd(void) · line 79

Quoted 7 lines

kernel/softirq.c +2 -1

@@ -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.

Quoted 3 lines · Hannes Frederic Sowa

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 :

Quoted 2 lines · Hannes Frederic Sowa

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

Quoted 13 lines · Eric Dumazet

kernel/softirq.c +1 -0

index 17caf4b63342..22463217e3cf 100644
@@ -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",

kernel/softirq.c static void wakeup_softirqd(void) · line 79

Quoted 7 lines

kernel/softirq.c +2 -1

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

Quoted 25 lines · Peter Zijlstra

Quoted 13 lines · Eric Dumazet

kernel/softirq.c +1 -0

index 17caf4b63342..22463217e3cf 100644
@@ -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",

kernel/softirq.c static void wakeup_softirqd(void) · line 79

Quoted 7 lines

kernel/softirq.c +2 -1

@@ -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:

Quoted 3 lines

static inline bool ksoftirqd_running(void)

{
	return __this_cpu_read(ksoftirqd)->state == TASK_RUNNING;

Quoted 1 line

}

Indeed, and the patch looks quite simple now ;)

kernel/softirq.c +9 -1

index 17caf4b63342d7839528f367b283a386413b0362..23c364485d03618773c385d943c0ef39f5931d09 100644
@@ -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
 		/*

Quoted 3 lines

Quoted 3 lines

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.

Quoted 9 lines · Paolo Abeni

Quoted 3 lines

Quoted 3 lines

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

Quoted 1 line

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 ?

Quoted 11 lines · Eric Dumazet

Quoted 9 lines · Paolo Abeni

Quoted 3 lines

Quoted 3 lines

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

Quoted 6 lines

Quoted 1 line

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 <[email protected]>
Date:   Sat May 7 14:09:01 2016 -0700

sh_eth: reuse sh_eth_chip_reset()

Quoted 25 lines · Peter Zijlstra

Quoted 13 lines · Eric Dumazet

kernel/softirq.c +1 -0

index 17caf4b63342..22463217e3cf 100644
@@ -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",

kernel/softirq.c static void wakeup_softirqd(void) · line 79

Quoted 7 lines

kernel/softirq.c +2 -1

@@ -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:

Quoted 4 lines

static inline bool ksoftirqd_running(void)

{
	return __this_cpu_read(ksoftirqd)->state == TASK_RUNNING;
}

Indeed, and the patch looks quite simple now ;)

kernel/softirq.c +6 -1

index 17caf4b63342d7839528f367b283a386413b0362..23c364485d03618773c385d943c0ef39f5931d09 100644
@@ -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);

Quoted 28 lines

Indeed, and the patch looks quite simple now ;)

kernel/softirq.c +6 -1

index 17caf4b63342d7839528f367b283a386413b0362..23c364485d03618773c385d943c0ef39f5931d09 100644
@@ -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);

kernel/softirq.c void irq_enter(void) · line 353

Quoted 9 lines

kernel/softirq.c +3 -0

@@ -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.

Quoted 41 lines · Paolo Abeni

Quoted 28 lines

Indeed, and the patch looks quite simple now ;)

kernel/softirq.c +6 -1

index 17caf4b63342d7839528f367b283a386413b0362..23c364485d03618773c385d943c0ef39f5931d09 100644
@@ -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);

kernel/softirq.c void irq_enter(void) · line 353

Quoted 9 lines

kernel/softirq.c +3 -0

@@ -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.

Quoted 3 lines

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.

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.

Quoted 3 lines

I believe I did this so that we factorize the logic in do_softirq() and keep the code local to kernel/softirq.c

Quoted 4 lines

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())

Quoted 16 lines · Paolo Abeni

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.

Quoted 3 lines

I believe I did this so that we factorize the logic in do_softirq() and keep the code local to kernel/softirq.c

Quoted 9 lines

Quoted 4 lines

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.

(quoted diff) +2 -1

@@ -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.

Quoted 6 lines · Hannes Frederic Sowa

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.

Hi Eric,

Quoted 12 lines · Eric Dumazet

Quoted 6 lines · Hannes Frederic Sowa

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.

Quoted 6 lines

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 ?

Quoted 23 lines · Paolo Abeni

Hi Eric,

Quoted 12 lines · Eric Dumazet

Quoted 6 lines · Hannes Frederic Sowa

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.

Quoted 6 lines

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.

Quoted 9 lines

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.

Quoted 7 lines

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%).

Quoted 2 lines

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.

Quoted 2 lines

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.

Quoted 9 lines

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.

Quoted 3 lines

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.

I am fine with that. It is us to show clear benefits or use cases for that. If we fail with that, no problem at all that the patches get rejected, we don't want to add bloat to the kernel, for sure! At this point I still think a possibility to run napi in kthreads will allow specific workloads to see an improvement. Maybe the simple branch in napi_schedule is just worth so people can play around with it. As it shouldn't change behavior we can later on simply remove it.

Quoted 28 lines · Paolo Abeni

Hi Eric,

Quoted 12 lines · Eric Dumazet

Quoted 6 lines · Hannes Frederic Sowa

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.

Quoted 6 lines

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

Quoted 4 lines

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.

Quoted 9 lines

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.

Quoted 3 lines

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.

Quoted 10 lines

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.

Quoted 3 lines

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.

Uh, we have likely the same issue in the net_rx_action() function, which also execute with bh disabled and check for jiffies changes even on single core hosts ?!?

Aren't jiffies updated by the timer interrupt ? and thous even with bh_disabled ?!?

Quoted 3 lines · Paolo Abeni

Uh, we have likely the same issue in the net_rx_action() function, which also execute with bh disabled and check for jiffies changes even on single core hosts ?!?

That is why we have a loop break after netdev_budget=300 packets. And a sysctl to eventually tune this.

Same issue for softirq handler, look at commit
34376a50fb1fa095b9d0636fa41ed2e73125f214

Your questions about this central piece of networking code are worrying.

Quoted 3 lines

Aren't jiffies updated by the timer interrupt ? and thous even with bh_disabled ?!?

Exactly my point : jiffie wont be updated in your code, since you block BH.

At this point we are not really sure if we should go with this simpler approach by putting NAPI itself into kthreads or leverage the threadirqs function by putting the whole interrupt into a thread and signaling NAPI that it does not reschedule itself in a softirq but to simply run at this particular context of the interrupt handler.

While the threaded irq way seems to better integrate into the kernel and also other devices could move their interrupts into the threads easily on a common policy, we don't know how to really express the necessary knobs with the current device driver model (module parameters, sysfs attributes, etc.). This is where we would like to hear some opinions. NAPI would e.g. have to query the kernel if the particular IRQ/MSI if it should be scheduled in a softirq or in a thread, so we don't have to rewrite all device drivers. This might even be needed on a per rx-queue granularity.

On Tue, 10 May 2016, Paolo Abeni wrote:

Nice patch set and very promising results!

Quoted 6 lines

At this point we are not really sure if we should go with this simpler approach by putting NAPI itself into kthreads or leverage the threadirqs function by putting the whole interrupt into a thread and signaling NAPI that it does not reschedule itself in a softirq but to simply run at this particular context of the interrupt handler.

Quoted 9 lines

While the threaded irq way seems to better integrate into the kernel and also other devices could move their interrupts into the threads easily on a common policy, we don't know how to really express the necessary knobs with the current device driver model (module parameters, sysfs attributes, etc.). This is where we would like to hear some opinions. NAPI would e.g. have to query the kernel if the particular IRQ/MSI if it should be scheduled in a softirq or in a thread, so we don't have to rewrite all device drivers. This might even be needed on a per rx-queue granularity.

Utilizing threaded irqs should be halfways simple even without touching the device driver at all.

We can do the switch to threading in two ways:

1) Let the driver request the interrupt(s) as it does now and then have a /proc/irq/NNN/threaded file which converts it to a threaded interrupt on the fly. That should be fairly trivial.

2) Let the driver request the interrupt(s) as it does now and retrieve the interrupt number which belongs to the device/queue from the network core and let the irq core switch it over to threaded.

You surely need some way to figure out whether the interrupt is threaded when you set up the device in order to flag your napi struct, but that should be not too hard to achieve.