[request for stable inclusion] softirq: reduce latencies

[request for stable inclusion] softirq: reduce latencies

[Rui Xiang]

Hi Zefan,

c10d73671ad30f54692f7f69f0e09e75d3a8926a
softirq: reduce latencies

This commit can reduce softirq latencies in some various network workloads.

34376a50fb1fa095b9d0636fa41ed2e73125f214
Fix lockup related to stop_machine being stuck in __do_softirq.

The commit 34376a50fb1f fix a bug introduced by commit c10d73671ad3
(softirq: reduce latencies).

They look applicable to stable-3.4, but need some adjustments to backport.
The following patches are the backports.

What do you think?

Thanks.

[PATCH 1/2] softirq: reduce latencies

[Rui Xiang]

From: Eric Dumazet <edumazet@google.com>

commit c10d73671ad30f54692f7f69f0e09e75d3a8926a upstream.

In various network workloads, __do_softirq() latencies can be up
to 20 ms if HZ=1000, and 200 ms if HZ=100.

This is because we iterate 10 times in the softirq dispatcher,
and some actions can consume a lot of cycles.

This patch changes the fallback to ksoftirqd condition to :

- A time limit of 2 ms.
- need_resched() being set on current task

When one of this condition is met, we wakeup ksoftirqd for further
softirq processing if we still have pending softirqs.

Using need_resched() as the only condition can trigger RCU stalls,
as we can keep BH disabled for too long.

I ran several benchmarks and got no significant difference in
throughput, but a very significant reduction of latencies (one order
of magnitude) :

In following bench, 200 antagonist "netperf -t TCP_RR" are started in
background, using all available cpus.

Then we start one "netperf -t TCP_RR", bound to the cpu handling the NIC
IRQ (hard+soft)

Before patch :

RT_LATENCY,MIN_LATENCY,MAX_LATENCY,P50_LATENCY,P90_LATENCY,P99_LATENCY,MEAN_LATENCY,STDDEV_LATENCY
MIGRATED TCP REQUEST/RESPONSE TEST from 0.0.0.0 (0.0.0.0) port 0 AF_INET
to 7.7.7.84 () port 0 AF_INET : first burst 0 : cpu bind
RT_LATENCY=550110.424
MIN_LATENCY=146858
MAX_LATENCY=997109
P50_LATENCY=305000
P90_LATENCY=550000
P99_LATENCY=710000
MEAN_LATENCY=376989.12
STDDEV_LATENCY=184046.92

After patch :

RT_LATENCY,MIN_LATENCY,MAX_LATENCY,P50_LATENCY,P90_LATENCY,P99_LATENCY,MEAN_LATENCY,STDDEV_LATENCY
MIGRATED TCP REQUEST/RESPONSE TEST from 0.0.0.0 (0.0.0.0) port 0 AF_INET
to 7.7.7.84 () port 0 AF_INET : first burst 0 : cpu bind
RT_LATENCY=40545.492
MIN_LATENCY=9834
MAX_LATENCY=78366
P50_LATENCY=33583
P90_LATENCY=59000
P99_LATENCY=69000
MEAN_LATENCY=38364.67
STDDEV_LATENCY=12865.26

Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: David Miller <davem@davemloft.net>
Cc: Tom Herbert <therbert@google.com>
Cc: Ben Hutchings <bhutchings@solarflare.com>
Signed-off-by: David S. Miller <davem@davemloft.net>
[xr: Backported to 3.4: Adjust context]
Signed-off-by: Rui Xiang <rui.xiang@huawei.com>
---
 kernel/softirq.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/kernel/softirq.c b/kernel/softirq.c
index 671f959..db3df13 100644
--- a/kernel/softirq.c
+++ b/kernel/softirq.c
@@ -194,21 +194,21 @@ void local_bh_enable_ip(unsigned long ip)
 EXPORT_SYMBOL(local_bh_enable_ip);
 
 /*
- * We restart softirq processing MAX_SOFTIRQ_RESTART times,
- * and we fall back to softirqd after that.
+ * We restart softirq processing for at most 2 ms, 
+ * and if need_resched() is not set.
  *
- * This number has been established via experimentation.
+ * These limits have been established via experimentation. 
  * The two things to balance is latency against fairness -
  * we want to handle softirqs as soon as possible, but they
  * should not be able to lock up the box.
  */
-#define MAX_SOFTIRQ_RESTART 10
+#define MAX_SOFTIRQ_TIME  msecs_to_jiffies(2)
 
 asmlinkage void __do_softirq(void)
 {
 	struct softirq_action *h;
 	__u32 pending;
-	int max_restart = MAX_SOFTIRQ_RESTART;
+	unsigned long end = jiffies + MAX_SOFTIRQ_TIME;
 	int cpu;
 
 	pending = local_softirq_pending();
@@ -255,11 +255,12 @@ restart:
 	local_irq_disable();
 
 	pending = local_softirq_pending();
-	if (pending && --max_restart)
-		goto restart;
+	if (pending) {
+		if (time_before(jiffies, end) && !need_resched())
+			goto restart;
 
-	if (pending)
 		wakeup_softirqd();
+	}
 
 	lockdep_softirq_exit();
 
-- 
1.8.2.2

[PATCH 2/2] Fix lockup related to stop_machine being stuck in __do_softirq.

[Rui Xiang]

From: Ben Greear <greearb@candelatech.com>

commit 34376a50fb1fa095b9d0636fa41ed2e73125f214 upstream.

The stop machine logic can lock up if all but one of the migration
threads make it through the disable-irq step and the one remaining
thread gets stuck in __do_softirq.  The reason __do_softirq can hang is
that it has a bail-out based on jiffies timeout, but in the lockup case,
jiffies itself is not incremented.

To work around this, re-add the max_restart counter in __do_irq and stop
processing irqs after 10 restarts.

Thanks to Tejun Heo and Rusty Russell and others for helping me track
this down.

This was introduced in 3.9 by commit c10d73671ad3 ("softirq: reduce
latencies").

It may be worth looking into ath9k to see if it has issues with its irq
handler at a later date.

The hang stack traces look something like this:

    ------------[ cut here ]------------
    WARNING: at kernel/watchdog.c:245 watchdog_overflow_callback+0x9c/0xa7()
    Watchdog detected hard LOCKUP on cpu 2
    Modules linked in: ath9k ath9k_common ath9k_hw ath mac80211 cfg80211 nfsv4 auth_rpcgss nfs fscache nf_nat_ipv4 nf_nat veth 8021q garp stp mrp llc pktgen lockd sunrpc]
    Pid: 23, comm: migration/2 Tainted: G         C   3.9.4+ #11
    Call Trace:
     <NMI>   warn_slowpath_common+0x85/0x9f
      warn_slowpath_fmt+0x46/0x48
      watchdog_overflow_callback+0x9c/0xa7
      __perf_event_overflow+0x137/0x1cb
      perf_event_overflow+0x14/0x16
      intel_pmu_handle_irq+0x2dc/0x359
      perf_event_nmi_handler+0x19/0x1b
      nmi_handle+0x7f/0xc2
      do_nmi+0xbc/0x304
      end_repeat_nmi+0x1e/0x2e
     <<EOE>>
      cpu_stopper_thread+0xae/0x162
      smpboot_thread_fn+0x258/0x260
      kthread+0xc7/0xcf
      ret_from_fork+0x7c/0xb0
    ---[ end trace 4947dfa9b0a4cec3 ]---
    BUG: soft lockup - CPU#1 stuck for 22s! [migration/1:17]
    Modules linked in: ath9k ath9k_common ath9k_hw ath mac80211 cfg80211 nfsv4 auth_rpcgss nfs fscache nf_nat_ipv4 nf_nat veth 8021q garp stp mrp llc pktgen lockd sunrpc]
    irq event stamp: 835637905
    hardirqs last  enabled at (835637904): __do_softirq+0x9f/0x257
    hardirqs last disabled at (835637905): apic_timer_interrupt+0x6d/0x80
    softirqs last  enabled at (5654720): __do_softirq+0x1ff/0x257
    softirqs last disabled at (5654725): irq_exit+0x5f/0xbb
    CPU 1
    Pid: 17, comm: migration/1 Tainted: G        WC   3.9.4+ #11 To be filled by O.E.M. To be filled by O.E.M./To be filled by O.E.M.
    RIP: tasklet_hi_action+0xf0/0xf0
    Process migration/1
    Call Trace:
     <IRQ>
      __do_softirq+0x117/0x257
      irq_exit+0x5f/0xbb
      smp_apic_timer_interrupt+0x8a/0x98
      apic_timer_interrupt+0x72/0x80
     <EOI>
      printk+0x4d/0x4f
      stop_machine_cpu_stop+0x22c/0x274
      cpu_stopper_thread+0xae/0x162
      smpboot_thread_fn+0x258/0x260
      kthread+0xc7/0xcf
      ret_from_fork+0x7c/0xb0

Signed-off-by: Ben Greear <greearb@candelatech.com>
Acked-by: Tejun Heo <tj@kernel.org>
Acked-by: Pekka Riikonen <priikone@iki.fi>
Cc: Eric Dumazet <eric.dumazet@gmail.com>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
[xr: Backported to 3.4: Adjust context]
Signed-off-by: Rui Xiang <rui.xiang@huawei.com>
---
 kernel/softirq.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/kernel/softirq.c b/kernel/softirq.c
index db3df13..5cc401e 100644
--- a/kernel/softirq.c
+++ b/kernel/softirq.c
@@ -194,8 +194,12 @@ void local_bh_enable_ip(unsigned long ip)
 EXPORT_SYMBOL(local_bh_enable_ip);
 
 /*
- * We restart softirq processing for at most 2 ms, 
- * and if need_resched() is not set.
+ * We restart softirq processing for at most MAX_SOFTIRQ_RESTART times,
+ * but break the loop if need_resched() is set or after 2 ms.
+ * The MAX_SOFTIRQ_TIME provides a nice upper bound in most cases, but in
+ * certain cases, such as stop_machine(), jiffies may cease to
+ * increment and so we need the MAX_SOFTIRQ_RESTART limit as
+ * well to make sure we eventually return from this method.
  *
  * These limits have been established via experimentation. 
  * The two things to balance is latency against fairness -
@@ -203,6 +207,7 @@ EXPORT_SYMBOL(local_bh_enable_ip);
  * should not be able to lock up the box.
  */
 #define MAX_SOFTIRQ_TIME  msecs_to_jiffies(2)
+#define MAX_SOFTIRQ_RESTART 10
 
 asmlinkage void __do_softirq(void)
 {
@@ -210,6 +215,7 @@ asmlinkage void __do_softirq(void)
 	__u32 pending;
 	unsigned long end = jiffies + MAX_SOFTIRQ_TIME;
 	int cpu;
+	int max_restart = MAX_SOFTIRQ_RESTART;
 
 	pending = local_softirq_pending();
 	account_system_vtime(current);
@@ -256,7 +262,8 @@ restart:
 
 	pending = local_softirq_pending();
 	if (pending) {
-		if (time_before(jiffies, end) && !need_resched())
+		if (time_before(jiffies, end) && !need_resched() &&
+		    --max_restart)
 			goto restart;
 
 		wakeup_softirqd();
-- 
1.8.2.2

Re: [PATCH 2/2] Fix lockup related to stop_machine being stuck in __do_softirq.

[Linus Torvalds]

So this one kind of fell through the cracks, partly because I don't
exactly love the patch.

What is it that keeps re-arming the softirq pending bit all the time?
You mention the ath9k driver..

Also, do we really need the jiffies-based one at all? Maybe we should
just get rid of that entirely, if it's not sufficiently reliable
anyway. It's not like we should *ever* keep doing softirq's forever,
and quite frankly, when you introduce the limit of doing the loop at
most ten times, I doubt that the "2 milliseconds" limit is even
relevant any more. It would be a strange situation where ten times
through the softirq handling loop would take more than 2ms.

So I'd rather take a patch that replaces the 2ms timeout with the
10-iteration timeout. And I think it might be a good idea to have a
debug thing that says what the softirq that keepts firing was. If it's
ath9k, I guess it's NET_TX/RX_SOFTIRQ, but maybe we could have
something that tells exact what it is that re-triggers it over and
over again..

               Linus

On Wed, May 13, 2015 at 11:29 PM, Rui Xiang <rui.xiang@huawei.com> wrote:
> From: Ben Greear <greearb@candelatech.com>
>
> commit 34376a50fb1fa095b9d0636fa41ed2e73125f214 upstream.
>
> The stop machine logic can lock up if all but one of the migration
> threads make it through the disable-irq step and the one remaining
> thread gets stuck in __do_softirq.  The reason __do_softirq can hang is
> that it has a bail-out based on jiffies timeout, but in the lockup case,
> jiffies itself is not incremented.
>
> To work around this, re-add the max_restart counter in __do_irq and stop
> processing irqs after 10 restarts.
>
> Thanks to Tejun Heo and Rusty Russell and others for helping me track
> this down.
>
> This was introduced in 3.9 by commit c10d73671ad3 ("softirq: reduce
> latencies").

Re: [PATCH 2/2] Fix lockup related to stop_machine being stuck in __do_softirq.

[Ben Greear]

On 05/18/2015 12:19 PM, Linus Torvalds wrote:
> So this one kind of fell through the cracks, partly because I don't
> exactly love the patch.
> 
> What is it that keeps re-arming the softirq pending bit all the time?
> You mention the ath9k driver..

It has been a while...I don't remember all the details, but at least some
of the problem was that most CPU cores were shut down, so the system could
make very little progress.  Maybe that includes ath9k logic that would clear the soft-irq-set logic?
I also often run wifi systems at full capacity using sw-rx-crypt, so maybe the system was just
very busy for this test case.

As for your other suggestions, I don't have any opinions.  I did not know this
code well and did not want to do more than just fix the problem for worry
that I would introduce something worse.

Anyway, it seems this patch is in the official kernel by version 3.14
(and maybe sooner...that is just the one that I checked first), so I'm
not sure why this email was sent by Rui Xiang.

Thanks,
Ben

> 
> Also, do we really need the jiffies-based one at all? Maybe we should
> just get rid of that entirely, if it's not sufficiently reliable
> anyway. It's not like we should *ever* keep doing softirq's forever,
> and quite frankly, when you introduce the limit of doing the loop at
> most ten times, I doubt that the "2 milliseconds" limit is even
> relevant any more. It would be a strange situation where ten times
> through the softirq handling loop would take more than 2ms.
> 
> So I'd rather take a patch that replaces the 2ms timeout with the
> 10-iteration timeout. And I think it might be a good idea to have a
> debug thing that says what the softirq that keepts firing was. If it's
> ath9k, I guess it's NET_TX/RX_SOFTIRQ, but maybe we could have
> something that tells exact what it is that re-triggers it over and
> over again..
> 
>                Linus
> 
> On Wed, May 13, 2015 at 11:29 PM, Rui Xiang <rui.xiang@huawei.com> wrote:
>> From: Ben Greear <greearb@candelatech.com>
>>
>> commit 34376a50fb1fa095b9d0636fa41ed2e73125f214 upstream.
>>
>> The stop machine logic can lock up if all but one of the migration
>> threads make it through the disable-irq step and the one remaining
>> thread gets stuck in __do_softirq.  The reason __do_softirq can hang is
>> that it has a bail-out based on jiffies timeout, but in the lockup case,
>> jiffies itself is not incremented.
>>
>> To work around this, re-add the max_restart counter in __do_irq and stop
>> processing irqs after 10 restarts.
>>
>> Thanks to Tejun Heo and Rusty Russell and others for helping me track
>> this down.
>>
>> This was introduced in 3.9 by commit c10d73671ad3 ("softirq: reduce
>> latencies").
> 


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [PATCH 2/2] Fix lockup related to stop_machine being stuck in __do_softirq.

[Linus Torvalds]

On Mon, May 18, 2015 at 12:41 PM, Ben Greear <greearb@candelatech.com> wrote:
>
> Anyway, it seems this patch is in the official kernel by version 3.14
> (and maybe sooner...that is just the one that I checked first), so I'm
> not sure why this email was sent by Rui Xiang.

Ahh, right you are. Back in 2013.. No wonder it didn't look familiar.
My memory isn't _that_ good.

              Linus

Re: [PATCH 2/2] Fix lockup related to stop_machine being stuck in __do_softirq.

[Eric Dumazet]

On Mon, 2015-05-18 at 12:19 -0700, Linus Torvalds wrote:
> So this one kind of fell through the cracks, partly because I don't
> exactly love the patch.
> 
> What is it that keeps re-arming the softirq pending bit all the time?
> You mention the ath9k driver..
> 
> Also, do we really need the jiffies-based one at all? Maybe we should
> just get rid of that entirely, if it's not sufficiently reliable
> anyway. It's not like we should *ever* keep doing softirq's forever,
> and quite frankly, when you introduce the limit of doing the loop at
> most ten times, I doubt that the "2 milliseconds" limit is even
> relevant any more. It would be a strange situation where ten times
> through the softirq handling loop would take more than 2ms.

We've seen very often this taking more than 50ms here.

Things get better, but back in 2013, timer handlers could run for quite
a long time. The infamous one is scheduled to me removed for 4.1 (TCP
SYNACK retransmit timer)

Re: [PATCH 2/2] Fix lockup related to stop_machine being stuck in __do_softirq.

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Mon, 18 May 2015 13:34:36 -0700

> On Mon, 2015-05-18 at 12:19 -0700, Linus Torvalds wrote:
>> So this one kind of fell through the cracks, partly because I don't
>> exactly love the patch.
>> 
>> What is it that keeps re-arming the softirq pending bit all the time?
>> You mention the ath9k driver..
>> 
>> Also, do we really need the jiffies-based one at all? Maybe we should
>> just get rid of that entirely, if it's not sufficiently reliable
>> anyway. It's not like we should *ever* keep doing softirq's forever,
>> and quite frankly, when you introduce the limit of doing the loop at
>> most ten times, I doubt that the "2 milliseconds" limit is even
>> relevant any more. It would be a strange situation where ten times
>> through the softirq handling loop would take more than 2ms.
> 
> We've seen very often this taking more than 50ms here.
> 
> Things get better, but back in 2013, timer handlers could run for quite
> a long time. The infamous one is scheduled to me removed for 4.1 (TCP
> SYNACK retransmit timer)

If he is doing full crypto in software, it is definitely possible for
new packets to show up (and thus new softirq work) before existing
work is complete.

Re: [request for stable inclusion] softirq: reduce latencies

[Zefan Li]

锟斤拷 2015/5/14 14:29, Rui Xiang wrote:
> Hi Zefan,
> 
> c10d73671ad30f54692f7f69f0e09e75d3a8926a
> softirq: reduce latencies
> 
> This commit can reduce softirq latencies in some various network workloads.
> 
> 34376a50fb1fa095b9d0636fa41ed2e73125f214
> Fix lockup related to stop_machine being stuck in __do_softirq.
> 
> The commit 34376a50fb1f fix a bug introduced by commit c10d73671ad3
> (softirq: reduce latencies).
> 
> They look applicable to stable-3.4, but need some adjustments to backport.
> The following patches are the backports.
> 
> What do you think?
> 

Queued up for 3.4, as 2.6.32 and 2.634 already did the backport.

Re: [request for stable inclusion] softirq: reduce latencies

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 807 bytes --]

On Thu, 2015-05-14 at 14:29 +0800, Rui Xiang wrote:
> Hi Zefan,
> 
> c10d73671ad30f54692f7f69f0e09e75d3a8926a
> softirq: reduce latencies
> 
> This commit can reduce softirq latencies in some various network workloads.
> 
> 34376a50fb1fa095b9d0636fa41ed2e73125f214
> Fix lockup related to stop_machine being stuck in __do_softirq.
> 
> The commit 34376a50fb1f fix a bug introduced by commit c10d73671ad3
> (softirq: reduce latencies).
> 
> They look applicable to stable-3.4, but need some adjustments to backport.
> The following patches are the backports.
> 
> What do you think?

As these have been applied to both 2.6.32 and 3.4 I've queued them up
for 3.2 as well.  Thanks.

Ben.

-- 
Ben Hutchings
One of the nice things about standards is that there are so many of them.


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]