[PATCH] softirq: Prevent looping on disabled tasklets

[PATCH] softirq: Prevent looping on disabled tasklets

[Chris Wilson]

Disabling a tasklet causes it not to run during tasklet_action, but is
put back onto the runnable tasklet list, and a new softirq raised. As
the softirq is raised from within __do_softirq() this causing
__do_softirq() to loop constantly until its timeslice expires and is
transferred to the ksoftirq thread. ksoftirq then permanently spins,
as on each action, the disabled tasklet keeps reraising the softirq.

Break this vicious cycle by moving the softirq from the action to the
final tasklet_enable().

This behaviour appears to be historic (since the first git import).
However, the looping until timeslice duration (to a max of 2ms) was
first introduced in commit c10d73671ad3 ("softirq: reduce latencies"),
with the restart limit restored in commit 34376a50fb1f ("Fix lockup
related to stop_machine being stuck in __do_softirq.")

Reported-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Hannes Reinecke <hare@suse.com>
Cc: Jens Axboe <axboe@kernel.dk>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Alexander Potapenko <glider@google.com>
Cc: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: "Peter Zijlstra (Intel)" <peterz@infradead.org>
Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: Johannes Thumshirn <jthumshirn@suse.de>
Cc: Emese Revfy <re.emese@gmail.com>
Cc: Sagi Grimberg <sagi@grimberg.me>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Tom Herbert <therbert@google.com>
Cc: Ben Hutchings <bhutchings@solarflare.com>
---
 include/linux/interrupt.h | 7 +++++--
 kernel/softirq.c          | 2 --
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
index 53144e78a369..12750f00d00d 100644
--- a/include/linux/interrupt.h
+++ b/include/linux/interrupt.h
@@ -613,8 +613,11 @@ static inline void tasklet_disable(struct tasklet_struct *t)
 
 static inline void tasklet_enable(struct tasklet_struct *t)
 {
-	smp_mb__before_atomic();
-	atomic_dec(&t->count);
+	if (!atomic_dec_and_test(&t->count))
+		return;
+
+	if (test_bit(TASKLET_STATE_SCHED, &t->state))
+		raise_softirq(HI_SOFTIRQ | TASKLET_SOFTIRQ);
 }
 
 extern void tasklet_kill(struct tasklet_struct *t);
diff --git a/kernel/softirq.c b/kernel/softirq.c
index 744fa611cae0..5a359eb1a541 100644
--- a/kernel/softirq.c
+++ b/kernel/softirq.c
@@ -527,7 +527,6 @@ static __latent_entropy void tasklet_action(struct softirq_action *a)
 		t->next = NULL;
 		*__this_cpu_read(tasklet_vec.tail) = t;
 		__this_cpu_write(tasklet_vec.tail, &(t->next));
-		__raise_softirq_irqoff(TASKLET_SOFTIRQ);
 		local_irq_enable();
 	}
 }
@@ -563,7 +562,6 @@ static __latent_entropy void tasklet_hi_action(struct softirq_action *a)
 		t->next = NULL;
 		*__this_cpu_read(tasklet_hi_vec.tail) = t;
 		__this_cpu_write(tasklet_hi_vec.tail, &(t->next));
-		__raise_softirq_irqoff(HI_SOFTIRQ);
 		local_irq_enable();
 	}
 }
-- 
2.11.0

Re: [PATCH] softirq: Prevent looping on disabled tasklets

[Chris Wilson]

On Sun, Feb 12, 2017 at 02:00:19PM +0000, Chris Wilson wrote:
> Disabling a tasklet causes it not to run during tasklet_action, but is
> put back onto the runnable tasklet list, and a new softirq raised. As
> the softirq is raised from within __do_softirq() this causing
> __do_softirq() to loop constantly until its timeslice expires and is
> transferred to the ksoftirq thread. ksoftirq then permanently spins,
> as on each action, the disabled tasklet keeps reraising the softirq.
> 
> Break this vicious cycle by moving the softirq from the action to the
> final tasklet_enable().
> 
> This behaviour appears to be historic (since the first git import).
> However, the looping until timeslice duration (to a max of 2ms) was
> first introduced in commit c10d73671ad3 ("softirq: reduce latencies"),
> with the restart limit restored in commit 34376a50fb1f ("Fix lockup
> related to stop_machine being stuck in __do_softirq.")
> 
> Reported-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Hannes Reinecke <hare@suse.com>
> Cc: Jens Axboe <axboe@kernel.dk>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Alexander Potapenko <glider@google.com>
> Cc: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: "Peter Zijlstra (Intel)" <peterz@infradead.org>
> Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> Cc: Johannes Thumshirn <jthumshirn@suse.de>
> Cc: Emese Revfy <re.emese@gmail.com>
> Cc: Sagi Grimberg <sagi@grimberg.me>
> Cc: Eric Dumazet <edumazet@google.com>
> Cc: Tom Herbert <therbert@google.com>
> Cc: Ben Hutchings <bhutchings@solarflare.com>
> ---
>  include/linux/interrupt.h | 7 +++++--
>  kernel/softirq.c          | 2 --
>  2 files changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
> index 53144e78a369..12750f00d00d 100644
> --- a/include/linux/interrupt.h
> +++ b/include/linux/interrupt.h
> @@ -613,8 +613,11 @@ static inline void tasklet_disable(struct tasklet_struct *t)
>  
>  static inline void tasklet_enable(struct tasklet_struct *t)
>  {
> -	smp_mb__before_atomic();
> -	atomic_dec(&t->count);
> +	if (!atomic_dec_and_test(&t->count))
> +		return;
> +
> +	if (test_bit(TASKLET_STATE_SCHED, &t->state))
> +		raise_softirq(HI_SOFTIRQ | TASKLET_SOFTIRQ);

raise_softirq is not exported, so let's move tasklet_enable()
out-of-line and export that.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH v2] softirq: Prevent looping on disabled tasklets

[Chris Wilson]

Disabling a tasklet causes it not to run during tasklet_action, but is
put back onto the runnable tasklet list, and a new softirq raised. As
the softirq is raised from within __do_softirq() this causing
__do_softirq() to loop constantly until its timeslice expires and is
transferred to the ksoftirq thread. ksoftirq then permanently spins,
as on each action, the disabled tasklet keeps reraising the softirq.

Break this vicious cycle by moving the softirq from the action to the
final tasklet_enable().

This behaviour appears to be historic (since the first git import).
However, the looping until timeslice duration (to a max of 2ms) was
first introduced in commit c10d73671ad3 ("softirq: reduce latencies"),
with the restart limit restored in commit 34376a50fb1f ("Fix lockup
related to stop_machine being stuck in __do_softirq.")

v2: Export tasklet_enable() to work with modules.

Reported-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Hannes Reinecke <hare@suse.com>
Cc: Jens Axboe <axboe@kernel.dk>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Alexander Potapenko <glider@google.com>
Cc: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: "Peter Zijlstra (Intel)" <peterz@infradead.org>
Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: Johannes Thumshirn <jthumshirn@suse.de>
Cc: Emese Revfy <re.emese@gmail.com>
Cc: Sagi Grimberg <sagi@grimberg.me>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Tom Herbert <therbert@google.com>
Cc: Ben Hutchings <bhutchings@solarflare.com>
---
 include/linux/interrupt.h |  7 +------
 kernel/softirq.c          | 12 ++++++++++--
 2 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
index 53144e78a369..a1fa88e7e509 100644
--- a/include/linux/interrupt.h
+++ b/include/linux/interrupt.h
@@ -611,12 +611,7 @@ static inline void tasklet_disable(struct tasklet_struct *t)
 	smp_mb();
 }
 
-static inline void tasklet_enable(struct tasklet_struct *t)
-{
-	smp_mb__before_atomic();
-	atomic_dec(&t->count);
-}
-
+extern void tasklet_enable(struct tasklet_struct *t);
 extern void tasklet_kill(struct tasklet_struct *t);
 extern void tasklet_kill_immediate(struct tasklet_struct *t, unsigned int cpu);
 extern void tasklet_init(struct tasklet_struct *t,
diff --git a/kernel/softirq.c b/kernel/softirq.c
index 080eb57789c4..ab8d9aeccb46 100644
--- a/kernel/softirq.c
+++ b/kernel/softirq.c
@@ -535,7 +535,6 @@ static __latent_entropy void tasklet_action(struct softirq_action *a)
 		t->next = NULL;
 		*__this_cpu_read(tasklet_vec.tail) = t;
 		__this_cpu_write(tasklet_vec.tail, &(t->next));
-		__raise_softirq_irqoff(TASKLET_SOFTIRQ);
 		local_irq_enable();
 	}
 }
@@ -571,7 +570,6 @@ static __latent_entropy void tasklet_hi_action(struct softirq_action *a)
 		t->next = NULL;
 		*__this_cpu_read(tasklet_hi_vec.tail) = t;
 		__this_cpu_write(tasklet_hi_vec.tail, &(t->next));
-		__raise_softirq_irqoff(HI_SOFTIRQ);
 		local_irq_enable();
 	}
 }
@@ -587,6 +585,16 @@ void tasklet_init(struct tasklet_struct *t,
 }
 EXPORT_SYMBOL(tasklet_init);
 
+void tasklet_enable(struct tasklet_struct *t)
+{
+	if (!atomic_dec_and_test(&t->count))
+		return;
+
+	if (test_bit(TASKLET_STATE_SCHED, &t->state))
+		raise_softirq(HI_SOFTIRQ | TASKLET_SOFTIRQ);
+}
+EXPORT_SYMBOL(tasklet_enable);
+
 void tasklet_kill(struct tasklet_struct *t)
 {
 	if (in_interrupt())
-- 
2.11.0

Re: [Intel-gfx] [PATCH] softirq: Prevent looping on disabled tasklets

[kbuild test robot]

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

Hi Chris,

[auto build test ERROR on linus/master]
[also build test ERROR on v4.10-rc7 next-20170210]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Chris-Wilson/softirq-Prevent-looping-on-disabled-tasklets/20170212-220435
config: x86_64-lkp (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

>> ERROR: "raise_softirq" [net/mac80211/mac80211.ko] undefined!

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 24664 bytes --]

Re: [Intel-gfx] [PATCH] softirq: Prevent looping on disabled tasklets

[kbuild test robot]

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

Hi Chris,

[auto build test ERROR on linus/master]
[also build test ERROR on v4.10-rc7 next-20170210]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Chris-Wilson/softirq-Prevent-looping-on-disabled-tasklets/20170212-220435
config: x86_64-rhel (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

>> ERROR: "raise_softirq" [sound/pci/asihpi/snd-asihpi.ko] undefined!
>> ERROR: "raise_softirq" [net/mac802154/mac802154.ko] undefined!
   ERROR: "raise_softirq" [net/mac80211/mac80211.ko] undefined!
>> ERROR: "raise_softirq" [drivers/usb/atm/usbatm.ko] undefined!
>> ERROR: "raise_softirq" [drivers/net/ethernet/jme.ko] undefined!
>> ERROR: "raise_softirq" [drivers/media/pci/mantis/mantis_core.ko] undefined!
>> ERROR: "raise_softirq" [drivers/hv/hv_vmbus.ko] undefined!
>> ERROR: "raise_softirq" [drivers/firewire/firewire-ohci.ko] undefined!

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 38265 bytes --]

[PATCH v3] softirq: Prevent looping on disabled tasklets

[Chris Wilson]

Disabling a tasklet causes it not to run during tasklet_action, but is
put back onto the runnable tasklet list, and a new softirq raised. As
the softirq is raised from within __do_softirq() this causing
__do_softirq() to loop constantly until its timeslice expires and is
transferred to the ksoftirq thread. ksoftirq then permanently spins,
as on each action, the disabled tasklet keeps reraising the softirq.

Break this vicious cycle by moving the softirq from the action to the
final tasklet_enable().

This behaviour appears to be historic (since the first git import).
However, the looping until timeslice duration (to a max of 2ms) was
first introduced in commit c10d73671ad3 ("softirq: reduce latencies"),
with the restart limit restored in commit 34376a50fb1f ("Fix lockup
related to stop_machine being stuck in __do_softirq.")

v2: Export tasklet_enable() to work with modules.
v3: Restore the looping over a failed tasklet_trylock()

Reported-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Hannes Reinecke <hare@suse.com>
Cc: Jens Axboe <axboe@kernel.dk>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Alexander Potapenko <glider@google.com>
Cc: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: "Peter Zijlstra (Intel)" <peterz@infradead.org>
Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: Johannes Thumshirn <jthumshirn@suse.de>
Cc: Emese Revfy <re.emese@gmail.com>
Cc: Sagi Grimberg <sagi@grimberg.me>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Tom Herbert <therbert@google.com>
Cc: Ben Hutchings <bhutchings@solarflare.com>
---
 include/linux/interrupt.h |  7 +------
 kernel/softirq.c          | 20 ++++++++++++++++++--
 2 files changed, 19 insertions(+), 8 deletions(-)

diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
index 53144e78a369..a1fa88e7e509 100644
--- a/include/linux/interrupt.h
+++ b/include/linux/interrupt.h
@@ -611,12 +611,7 @@ static inline void tasklet_disable(struct tasklet_struct *t)
 	smp_mb();
 }
 
-static inline void tasklet_enable(struct tasklet_struct *t)
-{
-	smp_mb__before_atomic();
-	atomic_dec(&t->count);
-}
-
+extern void tasklet_enable(struct tasklet_struct *t);
 extern void tasklet_kill(struct tasklet_struct *t);
 extern void tasklet_kill_immediate(struct tasklet_struct *t, unsigned int cpu);
 extern void tasklet_init(struct tasklet_struct *t,
diff --git a/kernel/softirq.c b/kernel/softirq.c
index 080eb57789c4..47c8933d315e 100644
--- a/kernel/softirq.c
+++ b/kernel/softirq.c
@@ -516,6 +516,7 @@ static __latent_entropy void tasklet_action(struct softirq_action *a)
 
 	while (list) {
 		struct tasklet_struct *t = list;
+		bool raise_softirq = true;
 
 		list = list->next;
 
@@ -529,13 +530,15 @@ static __latent_entropy void tasklet_action(struct softirq_action *a)
 				continue;
 			}
 			tasklet_unlock(t);
+			raise_softirq = false;
 		}
 
 		local_irq_disable();
 		t->next = NULL;
 		*__this_cpu_read(tasklet_vec.tail) = t;
 		__this_cpu_write(tasklet_vec.tail, &(t->next));
-		__raise_softirq_irqoff(TASKLET_SOFTIRQ);
+		if (raise_softirq)
+			__raise_softirq_irqoff(TASKLET_SOFTIRQ);
 		local_irq_enable();
 	}
 }
@@ -552,6 +555,7 @@ static __latent_entropy void tasklet_hi_action(struct softirq_action *a)
 
 	while (list) {
 		struct tasklet_struct *t = list;
+		bool raise_softirq = true;
 
 		list = list->next;
 
@@ -565,13 +569,15 @@ static __latent_entropy void tasklet_hi_action(struct softirq_action *a)
 				continue;
 			}
 			tasklet_unlock(t);
+			raise_softirq = false;
 		}
 
 		local_irq_disable();
 		t->next = NULL;
 		*__this_cpu_read(tasklet_hi_vec.tail) = t;
 		__this_cpu_write(tasklet_hi_vec.tail, &(t->next));
-		__raise_softirq_irqoff(HI_SOFTIRQ);
+		if (raise_softirq)
+			__raise_softirq_irqoff(HI_SOFTIRQ);
 		local_irq_enable();
 	}
 }
@@ -587,6 +593,16 @@ void tasklet_init(struct tasklet_struct *t,
 }
 EXPORT_SYMBOL(tasklet_init);
 
+void tasklet_enable(struct tasklet_struct *t)
+{
+	if (!atomic_dec_and_test(&t->count))
+		return;
+
+	if (test_bit(TASKLET_STATE_SCHED, &t->state))
+		raise_softirq(HI_SOFTIRQ | TASKLET_SOFTIRQ);
+}
+EXPORT_SYMBOL(tasklet_enable);
+
 void tasklet_kill(struct tasklet_struct *t)
 {
 	if (in_interrupt())
-- 
2.11.0

Re: [PATCH v3] softirq: Prevent looping on disabled tasklets

[Chris Wilson]

On Sun, Feb 12, 2017 at 03:46:09PM +0000, Chris Wilson wrote:
> +void tasklet_enable(struct tasklet_struct *t)
> +{
> +	if (!atomic_dec_and_test(&t->count))
> +		return;
> +
> +	if (test_bit(TASKLET_STATE_SCHED, &t->state))
> +		raise_softirq(HI_SOFTIRQ | TASKLET_SOFTIRQ);

And of course this can't work as raise_softirq() is local to the cpu.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH v3] softirq: Prevent looping on disabled tasklets

[Thomas Gleixner]

On Sun, 12 Feb 2017, Chris Wilson wrote:

> On Sun, Feb 12, 2017 at 03:46:09PM +0000, Chris Wilson wrote:
> > +void tasklet_enable(struct tasklet_struct *t)
> > +{
> > +	if (!atomic_dec_and_test(&t->count))
> > +		return;
> > +
> > +	if (test_bit(TASKLET_STATE_SCHED, &t->state))
> > +		raise_softirq(HI_SOFTIRQ | TASKLET_SOFTIRQ);
> 
> And of course this can't work as raise_softirq() is local to the cpu.

Indeed. tasklets are a horror by design. We rather should make them go away.

Thanks,

	tglx