softirq: Make softirqs soft-interruptible (+ per vector disablement)
Subsystems (106)
Frederic Weisbecker
maintainerRFC PATCH 25/30 softirq: Push down softirq mask to __local_bh_disable_ip()
In reply to Frederic WeisbeckerNow that all callers are ready, we can push down the softirq enabled mask to the core from callers such as spin_lock_bh(), local_bh_disable(), rcu_read_lock_bh(), etc...
It is applied to the CPU vector enabled mask on __local_bh_disable_ip() which then returns the old value to be restored on __local_bh_enable_ip().
--- include/linux/bottom_half.h | 19 ++++++++++--------- include/linux/rwlock_api_smp.h | 14 ++++++++------ include/linux/spinlock_api_smp.h | 10 +++++----- kernel/softirq.c | 28 +++++++++++++++++++--------- 4 files changed, 42 insertions(+), 29 deletions(-)
include/linux/bottom_half.h +10 -9
@@ -37,9 +37,10 @@ enum#ifdef CONFIG_TRACE_IRQFLAGS -extern void __local_bh_disable_ip (unsigned long ip,unsigned int cnt); +extern unsigned int __local_bh_disable_ip (unsigned long ip,unsigned int cnt, +unsigned int mask);#else -static __always_inline void __local_bh_disable_ip(unsigned long ip, unsigned int cnt) +static __always_inline unsigned int __local_bh_disable_ip(unsigned long ip, unsigned int cnt) {preempt_count_add (cnt);barrier ();
@@ -48,21 +49,21 @@ static __always_inline void __local_bh_disable_ip(unsigned long ip, unsigned intstatic inline unsigned int local_bh_disable (unsigned int mask) { -__local_bh_disable_ip (_THIS_IP_,SOFTIRQ_DISABLE_OFFSET ); -return 0 ; +return __local_bh_disable_ip (_THIS_IP_,SOFTIRQ_DISABLE_OFFSET , mask); } -extern void local_bh_enable_no_softirq (void ); -extern void __local_bh_enable_ip (unsigned long ip,unsigned int cnt); +extern void local_bh_enable_no_softirq (unsigned int bh); +extern void __local_bh_enable_ip (unsigned long ip, +unsigned int cnt,unsigned int bh); -static inline void local_bh_enable_ip (unsigned long ip) +static inline void local_bh_enable_ip (unsigned long ip,unsigned int bh) { -__local_bh_enable_ip (ip,SOFTIRQ_DISABLE_OFFSET ); +__local_bh_enable_ip (ip,SOFTIRQ_DISABLE_OFFSET , bh); }static inline void local_bh_enable (unsigned int bh) { -__local_bh_enable_ip (_THIS_IP_,SOFTIRQ_DISABLE_OFFSET ); +__local_bh_enable_ip (_THIS_IP_,SOFTIRQ_DISABLE_OFFSET , bh); }extern void local_bh_disable_all (void );
include/linux/rwlock_api_smp.h +8 -6
@@ -173,10 +173,11 @@ static inline void __raw_read_lock_irq(rwlock_t *lock)static inline unsigned int __raw_read_lock_bh (rwlock_t *lock,unsigned int mask) { -__local_bh_disable_ip (_RET_IP_,SOFTIRQ_LOCK_OFFSET ); +unsigned int bh; + bh =__local_bh_disable_ip (_RET_IP_,SOFTIRQ_LOCK_OFFSET , mask);rwlock_acquire_read (&lock->dep_map,0 ,0 , _RET_IP_);LOCK_CONTENDED (lock, do_raw_read_trylock, do_raw_read_lock); -return 0 ; +return bh; }static inline unsigned long __raw_write_lock_irqsave (rwlock_t *lock)
@@ -202,10 +203,11 @@ static inline void __raw_write_lock_irq(rwlock_t *lock)static inline unsigned int __raw_write_lock_bh (rwlock_t *lock,unsigned int mask) { -__local_bh_disable_ip (_RET_IP_,SOFTIRQ_LOCK_OFFSET ); +unsigned int bh; + bh =__local_bh_disable_ip (_RET_IP_,SOFTIRQ_LOCK_OFFSET , mask);rwlock_acquire (&lock->dep_map,0 ,0 , _RET_IP_);LOCK_CONTENDED (lock, do_raw_write_trylock, do_raw_write_lock); -return 0 ; +return bh; }static inline void __raw_write_lock (rwlock_t *lock)
@@ -253,7 +255,7 @@ static inline void __raw_read_unlock_bh(rwlock_t *lock, {rwlock_release (&lock->dep_map,1 , _RET_IP_);do_raw_read_unlock (lock); -__local_bh_enable_ip (_RET_IP_,SOFTIRQ_LOCK_OFFSET ); +__local_bh_enable_ip (_RET_IP_,SOFTIRQ_LOCK_OFFSET , bh); } static inline void __raw_write_unlock_irqrestore(rwlock_t *lock,
@@ -278,7 +280,7 @@ static inline void __raw_write_unlock_bh(rwlock_t *lock, {rwlock_release (&lock->dep_map,1 , _RET_IP_);do_raw_write_unlock (lock); -__local_bh_enable_ip (_RET_IP_,SOFTIRQ_LOCK_OFFSET ); +__local_bh_enable_ip (_RET_IP_,SOFTIRQ_LOCK_OFFSET , bh); }#endif /* __LINUX_RWLOCK_API_SMP_H */
include/linux/spinlock_api_smp.h +5 -5
@@ -132,9 +132,9 @@ static inline void __raw_spin_lock_irq(raw_spinlock_t *lock)static inline unsigned int __raw_spin_lock_bh (raw_spinlock_t *lock,unsigned int mask) { -unsigned int bh =0 ; +unsigned int bh; -__local_bh_disable_ip (_RET_IP_,SOFTIRQ_LOCK_OFFSET ); + bh =__local_bh_disable_ip (_RET_IP_,SOFTIRQ_LOCK_OFFSET , mask);spin_acquire (&lock->dep_map,0 ,0 , _RET_IP_);LOCK_CONTENDED (lock, do_raw_spin_trylock, do_raw_spin_lock);
@@ -179,19 +179,19 @@ static inline void __raw_spin_unlock_bh(raw_spinlock_t *lock, {spin_release (&lock->dep_map,1 , _RET_IP_);do_raw_spin_unlock (lock); -__local_bh_enable_ip (_RET_IP_,SOFTIRQ_LOCK_OFFSET ); +__local_bh_enable_ip (_RET_IP_,SOFTIRQ_LOCK_OFFSET , bh); }static inline int __raw_spin_trylock_bh (raw_spinlock_t *lock,unsigned int *bh,unsigned int mask) { -__local_bh_disable_ip (_RET_IP_,SOFTIRQ_LOCK_OFFSET ); + *bh =__local_bh_disable_ip (_RET_IP_,SOFTIRQ_LOCK_OFFSET , mask);if (do_raw_spin_trylock (lock)) {spin_acquire (&lock->dep_map,0 ,1 , _RET_IP_);return 1 ; } -__local_bh_enable_ip (_RET_IP_,SOFTIRQ_LOCK_OFFSET ); +__local_bh_enable_ip (_RET_IP_,SOFTIRQ_LOCK_OFFSET , *bh);return 0 ; }
kernel/softirq.c +19 -9
@@ -107,13 +107,16 @@ static bool ksoftirqd_running(unsigned long pending)* where hardirqs are disabled legitimately: */ #ifdef CONFIG_TRACE_IRQFLAGS -void __local_bh_disable_ip (unsigned long ip,unsigned int cnt) +unsigned int __local_bh_disable_ip (unsigned long ip,unsigned int cnt, +unsigned int mask) {unsigned long flags; +unsigned int enabled;WARN_ON_ONCE (in_irq ());raw_local_irq_save (flags); +/* * The preempt tracer hooks into preempt_count_add and will break * lockdep because it calls back into lockdep after SOFTIRQ_OFFSET
@@ -127,6 +130,9 @@ void __local_bh_disable_ip(unsigned long ip, unsigned int cnt)*/ if (softirq_count () == (cnt &SOFTIRQ_MASK ))trace_softirqs_off (ip); + + enabled =local_softirq_enabled (); +softirq_enabled_nand (mask);raw_local_irq_restore (flags);if (preempt_count () == cnt) {
@@ -135,6 +141,7 @@ void __local_bh_disable_ip(unsigned long ip, unsigned int cnt)#endif trace_preempt_off(CALLER_ADDR0, get_lock_parent_ip()); } +return enabled; }EXPORT_SYMBOL (__local_bh_disable_ip);#endif /* CONFIG_TRACE_IRQFLAGS */
@@ -143,11 +150,13 @@ EXPORT_SYMBOL(__local_bh_disable_ip);* Special-case - softirqs can safely be enabled by __do_softirq(), * without processing still-pending softirqs: */ -void local_bh_enable_no_softirq (void ) +void local_bh_enable_no_softirq (unsigned int bh) {WARN_ON_ONCE (in_irq ());lockdep_assert_irqs_disabled (); +softirq_enabled_set (bh); +if (preempt_count () ==SOFTIRQ_DISABLE_OFFSET )trace_preempt_on (CALLER_ADDR0 ,get_lock_parent_ip ());
@@ -155,17 +164,18 @@ void local_bh_enable_no_softirq(void)trace_softirqs_on (_RET_IP_);__preempt_count_sub (SOFTIRQ_DISABLE_OFFSET ); - }EXPORT_SYMBOL (local_bh_enable_no_softirq); -void __local_bh_enable_ip (unsigned long ip,unsigned int cnt) +void __local_bh_enable_ip (unsigned long ip,unsigned int cnt,unsigned int bh) {WARN_ON_ONCE (in_irq ());lockdep_assert_irqs_enabled ();#ifdef CONFIG_TRACE_IRQFLAGS local_irq_disable ();#endif +softirq_enabled_set (bh); +/* * Are softirqs going to be turned on now: */
@@ -177,6 +187,7 @@ void __local_bh_enable_ip(unsigned long ip, unsigned int cnt)*/ preempt_count_sub (cnt -1 ); +if (unlikely (!in_interrupt () &&local_softirq_pending ())) {/* * Run softirq if any pending. And do it in its own stack
@@ -246,9 +257,6 @@ static void local_bh_exit(void)__preempt_count_sub (SOFTIRQ_OFFSET ); } - - -/* * We restart softirq processing for at most MAX_SOFTIRQ_RESTART times, * but break the loop if need_resched() is set or after 2 ms.
@@ -395,15 +403,17 @@ asmlinkage __visible void do_softirq(void)*/ void irq_enter (void ) { +unsigned int bh; +rcu_irq_enter ();if (is_idle_task (current) && !in_interrupt ()) {/* * Prevent raise_softirq from needlessly waking up ksoftirqd * here, as softirq will be serviced on return from interrupt. */ -local_bh_disable (SOFTIRQ_ALL_MASK ); + bh =local_bh_disable (SOFTIRQ_ALL_MASK );tick_irq_enter (); -local_bh_enable_no_softirq (); +local_bh_enable_no_softirq (bh); }__irq_enter ();
Signature · 1 line
2.7.4
Frederic Weisbecker
maintainerRFC PATCH 26/30 softirq: Increment the softirq offset on top of enabled bits
In reply to Frederic WeisbeckerNow we can rely on the vector enabled bits to know if some vector is disabled. Hence we can also now drive the softirq offset on top of it.
As a result, the softirq offset don't need to nest anymore as the vector enable mask does it on the stack on its behalf:
// Start with local_bh_disabled() == SOFTIRQ_ALL_MASK
...
bh = local_bh_disable(BIT(NET_RX_SOFTIRQ)) {
bh = local_bh_disabled();
local_bh_disabled() &= ~BIT(NET_RX_SOFTIRQ);
// First vector disabled, inc preempt count
preempt_count += SOFTIRQ_DISABLE_OFFSET;
return bh;
}
....
bh2 = local_bh_disable(BIT(BLOCK_SOFTIRQ)) {
bh2 = local_bh_disabled();
local_bh_disabled() &= ~BIT(NET_RX_SOFTIRQ);
// No need to inc preempt count
return bh2;
}
...
local_bh_enable(bh2) {
local_bh_disabled() = bh2;
// No need to dec preempt count
}
...
local_bh_enable(bh1) {
local_bh_disabled() = bh;
preempt_count -= SOFTIRQ_DISABLE_OFFSET;
}--- kernel/softirq.c | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-)
kernel/softirq.c +16 -9
@@ -117,6 +117,10 @@ unsigned int __local_bh_disable_ip(unsigned long ip, unsigned int cnt,raw_local_irq_save (flags); + enabled =local_softirq_enabled (); +if (enabled !=SOFTIRQ_ALL_MASK ) + cnt &= ~SOFTIRQ_MASK ; +/* * The preempt tracer hooks into preempt_count_add and will break * lockdep because it calls back into lockdep after SOFTIRQ_OFFSET
@@ -131,7 +135,6 @@ unsigned int __local_bh_disable_ip(unsigned long ip, unsigned int cnt,if (softirq_count () == (cnt &SOFTIRQ_MASK ))trace_softirqs_off (ip); - enabled =local_softirq_enabled ();softirq_enabled_nand (mask);raw_local_irq_restore (flags);
@@ -157,6 +160,9 @@ void local_bh_enable_no_softirq(unsigned int bh)softirq_enabled_set (bh); +if (bh !=SOFTIRQ_ALL_MASK ) +return ; +if (preempt_count () ==SOFTIRQ_DISABLE_OFFSET )trace_preempt_on (CALLER_ADDR0 ,get_lock_parent_ip ());
@@ -175,18 +181,18 @@ void __local_bh_enable_ip(unsigned long ip, unsigned int cnt, unsigned int bh)local_irq_disable ();#endif softirq_enabled_set (bh); - -/* -* Are softirqs going to be turned on now: -*/ -if (softirq_count () ==SOFTIRQ_DISABLE_OFFSET ) +if (bh !=SOFTIRQ_ALL_MASK ) { + cnt &= ~SOFTIRQ_MASK ; + }else if (!(softirq_count () &SOFTIRQ_OFFSET )) { +/* Are softirqs going to be turned on now: */ trace_softirqs_on (ip); + }/* * Keep preemption disabled until we are done with * softirq processing: */ -preempt_count_sub (cnt -1 ); - +if (cnt) +preempt_count_sub (cnt -1 );if (unlikely (!in_interrupt () &&local_softirq_pending ())) {/*
@@ -196,7 +202,8 @@ void __local_bh_enable_ip(unsigned long ip, unsigned int cnt, unsigned int bh)do_softirq (); } -preempt_count_dec (); +if (cnt) +preempt_count_dec ();#ifdef CONFIG_TRACE_IRQFLAGS local_irq_enable ();#endif
Signature · 1 line
2.7.4
Frederic Weisbecker
maintainerRFC PATCH 27/30 softirq: Swap softirq serving VS disable on preempt mask layout
In reply to Frederic WeisbeckerThe current softirq_count() layout is designed as followed:
* Serving the softirq is done under SOFTIRQ_OFFSET. It makes the softirq_count() odd and since it can't nest, due to softirq serving not being re-entrant, it's fine to differenciate it from softirq disablement that use even values.
* Disable the softirq is done under SOFTIRQ_OFFSET * 2. This can nest, so increment of even values is fine to differenciate it from serving softirqs.
Now the design is going to change:
* Serving softirqs will need to be re-entrant to allow a vector to interrupt another.
* Disable softirqs can't nest anymore at the softirq_count() level. This is all driven by the vector disabled mask now.
In order to support this new layout, simply swap them. Serving softirqs now use even value increments and disable softirqs now use odd value toggle.
--- include/linux/bottom_half.h | 6 +++--- include/linux/preempt.h | 9 +++++---- kernel/softirq.c | 21 +++++++++++---------- kernel/trace/ring_buffer.c | 2 +- kernel/trace/trace.c | 2 +- 5 files changed, 21 insertions(+), 19 deletions(-)
include/linux/bottom_half.h +3 -3
@@ -49,7 +49,7 @@ static __always_inline unsigned int __local_bh_disable_ip(unsigned long ip, unsistatic inline unsigned int local_bh_disable (unsigned int mask) { -return __local_bh_disable_ip (_THIS_IP_,SOFTIRQ_DISABLE_OFFSET , mask); +return __local_bh_disable_ip (_THIS_IP_,SOFTIRQ_OFFSET , mask); }extern void local_bh_enable_no_softirq (unsigned int bh);
@@ -58,12 +58,12 @@ extern void __local_bh_enable_ip(unsigned long ip,static inline void local_bh_enable_ip (unsigned long ip,unsigned int bh) { -__local_bh_enable_ip (ip,SOFTIRQ_DISABLE_OFFSET , bh); +__local_bh_enable_ip (ip,SOFTIRQ_OFFSET , bh); }static inline void local_bh_enable (unsigned int bh) { -__local_bh_enable_ip (_THIS_IP_,SOFTIRQ_DISABLE_OFFSET , bh); +__local_bh_enable_ip (_THIS_IP_,SOFTIRQ_OFFSET , bh); }extern void local_bh_disable_all (void );
include/linux/preempt.h +5 -4
@@ -51,7 +51,8 @@#define HARDIRQ_OFFSET (1UL << HARDIRQ_SHIFT)#define NMI_OFFSET (1UL << NMI_SHIFT) -#define SOFTIRQ_DISABLE_OFFSET (2 * SOFTIRQ_OFFSET) +#define SOFTIRQ_SERVING_OFFSET (2 * SOFTIRQ_OFFSET) +#define SOFTIRQ_SERVING_MASK (SOFTIRQ_MASK & ~SOFTIRQ_OFFSET)/* We use the MSB mostly because its available */ #define PREEMPT_NEED_RESCHED 0x80000000
@@ -101,10 +102,10 @@#define in_irq () (hardirq_count())#define in_softirq () (softirq_count())#define in_interrupt () (irq_count()) -#define in_serving_softirq () (softirq_count() & SOFTIRQ_OFFSET) +#define in_serving_softirq () (softirq_count() & ~SOFTIRQ_OFFSET)#define in_nmi () (preempt_count() & NMI_MASK)#define in_task () (!(preempt_count() & \ - (NMI_MASK | HARDIRQ_MASK | SOFTIRQ_OFFSET))) + (NMI_MASK | HARDIRQ_MASK | SOFTIRQ_SERVING_MASK)))/* * The preempt_count offset after preempt_disable();
@@ -133,7 +134,7 @@* * Work as expected. */ -#define SOFTIRQ_LOCK_OFFSET (SOFTIRQ_DISABLE_OFFSET + PREEMPT_LOCK_OFFSET) +#define SOFTIRQ_LOCK_OFFSET (SOFTIRQ_OFFSET + PREEMPT_LOCK_OFFSET)/* * Are we running in atomic context? WARNING: this macro cannot
kernel/softirq.c +11 -10
@@ -163,13 +163,13 @@ void local_bh_enable_no_softirq(unsigned int bh)if (bh !=SOFTIRQ_ALL_MASK )return ; -if (preempt_count () ==SOFTIRQ_DISABLE_OFFSET ) +if (preempt_count () ==SOFTIRQ_OFFSET )trace_preempt_on (CALLER_ADDR0 ,get_lock_parent_ip ()); -if (softirq_count () ==SOFTIRQ_DISABLE_OFFSET ) +if (softirq_count () ==SOFTIRQ_OFFSET )trace_softirqs_on (_RET_IP_); -__preempt_count_sub (SOFTIRQ_DISABLE_OFFSET ); +__preempt_count_sub (SOFTIRQ_OFFSET ); }EXPORT_SYMBOL (local_bh_enable_no_softirq);
@@ -181,9 +181,10 @@ void __local_bh_enable_ip(unsigned long ip, unsigned int cnt, unsigned int bh)local_irq_disable ();#endif softirq_enabled_set (bh); +if (bh !=SOFTIRQ_ALL_MASK ) { cnt &= ~SOFTIRQ_MASK ; - }else if (!(softirq_count () &SOFTIRQ_OFFSET )) { + }else if (!(softirq_count () &SOFTIRQ_SERVING_MASK )) {/* Are softirqs going to be turned on now: */ trace_softirqs_on (ip); }
@@ -235,15 +236,15 @@ static void local_bh_enter(unsigned long ip)* We must manually increment preempt_count here and manually * call the trace_preempt_off later. */ -__preempt_count_add (SOFTIRQ_OFFSET ); +__preempt_count_add (SOFTIRQ_SERVING_OFFSET );/* * Were softirqs turned off above: */ -if (softirq_count () ==SOFTIRQ_OFFSET ) +if (softirq_count () ==SOFTIRQ_SERVING_OFFSET )trace_softirqs_off (ip);raw_local_irq_restore (flags); -if (preempt_count () ==SOFTIRQ_OFFSET ) { +if (preempt_count () ==SOFTIRQ_SERVING_OFFSET ) {#ifdef CONFIG_DEBUG_PREEMPT current->preempt_disable_ip =get_lock_parent_ip ();#endif
@@ -255,13 +256,13 @@ static void local_bh_exit(void) {lockdep_assert_irqs_disabled (); -if (preempt_count () ==SOFTIRQ_OFFSET ) +if (preempt_count () ==SOFTIRQ_SERVING_OFFSET )trace_preempt_on (CALLER_ADDR0 ,get_lock_parent_ip ()); -if (softirq_count () ==SOFTIRQ_OFFSET ) +if (softirq_count () ==SOFTIRQ_SERVING_OFFSET )trace_softirqs_on (_RET_IP_); -__preempt_count_sub (SOFTIRQ_OFFSET ); +__preempt_count_sub (SOFTIRQ_SERVING_OFFSET ); }/*
kernel/trace/ring_buffer.c +1 -1
@@ -2655,7 +2655,7 @@ trace_recursive_lock(struct ring_buffer_per_cpu *cpu_buffer)unsigned long pc =preempt_count ();int bit; -if (!(pc & (NMI_MASK |HARDIRQ_MASK |SOFTIRQ_OFFSET ))) +if (in_task ()) bit =RB_CTX_NORMAL ;else bit = pc & NMI_MASK ? RB_CTX_NMI :
kernel/trace/trace.c +1 -1
@@ -2143,7 +2143,7 @@ tracing_generic_entry_update(struct trace_entry *entry, unsigned long flags,#endif ((pc &NMI_MASK ) ?TRACE_FLAG_NMI :0 ) | ((pc &HARDIRQ_MASK ) ?TRACE_FLAG_HARDIRQ :0 ) | - ((pc &SOFTIRQ_OFFSET ) ?TRACE_FLAG_SOFTIRQ :0 ) | + ((pc &SOFTIRQ_SERVING_MASK ) ?TRACE_FLAG_SOFTIRQ :0 ) | (tif_need_resched () ?TRACE_FLAG_NEED_RESCHED :0 ) | (test_preempt_need_resched () ?TRACE_FLAG_PREEMPT_RESCHED :0 ); }
Signature · 1 line
2.7.4
Frederic Weisbecker
maintainerRFC PATCH 28/30 softirq: Disable vector on execution
In reply to Frederic WeisbeckerFrom: Frederic Weisbecker <[email protected]>
Disable a vector while it is being processed. This prepare for softirq re-entrancy with an obvious single constraint: a vector can't be interrupted by itself.
--- kernel/softirq.c | 3 +++ 1 file changed, 3 insertions(+)
kernel/softirq.c +3 -0
@@ -357,7 +357,10 @@ asmlinkage __visible void __softirq_entry __do_softirq(void)kstat_incr_softirqs_this_cpu (vec_nr);trace_softirq_entry (vec_nr); +softirq_enabled_nand (BIT (vec_nr)); +barrier (); h->action (h); +softirq_enabled_or (BIT (vec_nr));trace_softirq_exit (vec_nr);if (unlikely (prev_count !=preempt_count ())) { pr_err("huh, entered softirq %u %s %p with preempt_count %08x, exited with %08x?\n",
Signature · 1 line
2.7.4
Frederic Weisbecker
maintainerRFC PATCH 29/30 softirq: Make softirq processing softinterruptible
In reply to Frederic WeisbeckerFrom: Frederic Weisbecker <[email protected]>
Make do_softirq() re-entrant and allow a vector, being either processed or disabled, to be interrupted by another vector. This way a vector won't be able to monopolize the CPU for a long while at the expense of the others that may rely on some predictable latency, especially on softirq disabled sections that used to disable all vectors.
--- kernel/softirq.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)
kernel/softirq.c +8 -8
@@ -195,7 +195,7 @@ void __local_bh_enable_ip(unsigned long ip, unsigned int cnt, unsigned int bh)if (cnt)preempt_count_sub (cnt -1 ); -if (unlikely (!in_interrupt () &&local_softirq_pending ())) { +if (unlikely (!in_irq () && (local_softirq_pending () &local_softirq_enabled ()))) {/* * 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.
@@ -387,7 +387,7 @@ asmlinkage __visible void __softirq_entry __do_softirq(void)lockdep_softirq_end (in_hardirq);account_irq_exit_time (current);local_bh_exit (); -WARN_ON_ONCE (in_interrupt ()); +WARN_ON_ONCE (in_irq ());current_restore_flags (old_flags,PF_MEMALLOC ); }
@@ -396,12 +396,12 @@ asmlinkage __visible void do_softirq(void)__u32 pending;unsigned long flags; -if (in_interrupt ()) +if (in_irq ())return ;local_irq_save (flags); - pending =local_softirq_pending (); + pending =local_softirq_pending () &local_softirq_enabled ();if (pending && !ksoftirqd_running (pending))do_softirq_own_stack ();
@@ -432,7 +432,7 @@ void irq_enter(void)static inline void invoke_softirq (void ) { -if (ksoftirqd_running (local_softirq_pending ())) +if (ksoftirqd_running (local_softirq_pending () &local_softirq_enabled ()))return ;if (!force_irqthreads) {
@@ -481,7 +481,7 @@ void irq_exit(void)#endif account_irq_exit_time (current);preempt_count_sub (HARDIRQ_OFFSET ); -if (!in_interrupt () &&local_softirq_pending ()) +if (!in_irq () && (local_softirq_pending () &local_softirq_enabled ()))invoke_softirq ();tick_irq_exit ();
@@ -712,13 +712,13 @@ void __init softirq_init(void)static int ksoftirqd_should_run (unsigned int cpu) { -return local_softirq_pending (); +return local_softirq_pending () &local_softirq_enabled (); }static void run_ksoftirqd (unsigned int cpu) {local_irq_disable (); -if (local_softirq_pending ()) { +if (local_softirq_pending () &local_softirq_enabled ()) {/* * We can safely run softirq on inline stack, as we are not deep * in the task stack here.
Signature · 1 line
2.7.4
Re: [RFC PATCH 29/30] softirq: Make softirq processing softinterruptible
Hi Frederic,
Quoted 8 lines · Frederic Weisbecker
From: Frederic Weisbecker <[email protected]>
Make
do_softirq()re-entrant and allow a vector, being either processed or disabled, to be interrupted by another vector. This way a vector won't be able to monopolize the CPU for a long while at the expense of the others that may rely on some predictable latency, especially on softirq disabled sections that used to disable all vectors.
I understand that a long running softirq can be preempted/interrupted by other softirqs which is not possible today. I have few questions on your patches.
(1) When softirq processing is pushed to ksoftirqd, then the long running softirq can still block other softirqs (not in SOFTIRQ_NOW_MASK) for a while. correct?
(2) When softirqs processing happens asynchronously, a particular softirq like TASKLET can keep interrupting an already running softirq like TIMER/NET_RX, correct? In worse case scenario, a long running softirq like NET_RX interrupt a TIMER softirq. But I guess this is something expected with this. i.e each softirq is independent and whichever comes recent gets to interrupt the previously running softirqs.
Thanks,
Pavan
Signature · 2 lines
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.
Frederic Weisbecker
maintainerRe: [RFC PATCH 29/30] softirq: Make softirq processing softinterruptible
Hi Pavan,
Quoted 11 lines · Pavan Kondeti
Hi Frederic,
Quoted 8 lines · Frederic Weisbecker
From: Frederic Weisbecker <[email protected]>
Make
do_softirq()re-entrant and allow a vector, being either processed or disabled, to be interrupted by another vector. This way a vector won't be able to monopolize the CPU for a long while at the expense of the others that may rely on some predictable latency, especially on softirq disabled sections that used to disable all vectors.
Quoted 7 lines
I understand that a long running softirq can be preempted/interrupted by other softirqs which is not possible today. I have few questions on your patches.
(1) When softirq processing is pushed to ksoftirqd, then the long running softirq can still block other softirqs (not in
SOFTIRQ_NOW_MASK) for a while. correct?
No, Ksoftirqd is treated the same as IRQ tail processing here: a vector can interrupt another. So for example, a NET_RX softirq running in Ksoftirqd can be interrupted by a TIMER softirq running in hardirq tail.
Quoted 7 lines
(2) When softirqs processing happens asynchronously, a particular softirq like TASKLET can keep interrupting an already running softirq like TIMER/NET_RX, correct? In worse case scenario, a long running softirq like
NET_RXinterrupt a TIMER softirq. But I guess this is something expected with this. i.e each softirq is independent and whichever comes recent gets to interrupt the previously running softirqs.
Exactly, and that's inherent with interrupts in general. The only way to work around that is to thread each vector independantly but that's a whole different dimension :-)
Thanks!
Re: [RFC PATCH 29/30] softirq: Make softirq processing softinterruptible
Hi Frederic,
Quoted 18 lines · Frederic Weisbecker
Hi Pavan,
Quoted 15 lines · Pavan Kondeti
Hi Frederic,
Quoted 8 lines · Frederic Weisbecker
From: Frederic Weisbecker <[email protected]>
Make
do_softirq()re-entrant and allow a vector, being either processed or disabled, to be interrupted by another vector. This way a vector won't be able to monopolize the CPU for a long while at the expense of the others that may rely on some predictable latency, especially on softirq disabled sections that used to disable all vectors.I understand that a long running softirq can be preempted/interrupted by other softirqs which is not possible today. I have few questions on your patches.
Quoted 8 lines
Quoted 3 lines
(1) When softirq processing is pushed to ksoftirqd, then the long running softirq can still block other softirqs (not in
SOFTIRQ_NOW_MASK) for a while. correct?No, Ksoftirqd is treated the same as IRQ tail processing here: a vector can interrupt another. So for example, a
NET_RXsoftirq running in Ksoftirqd can be interrupted by a TIMER softirq running in hardirq tail.
When ksoftirqd is running, we are only allowing softirqs in SOFTIRQ_NOW_MASK to run after serving an interrupt. So I don't see how TIMER which is not in SOFTIRQ_NOW_MASK can interrupt a NET_RX softirq running in ksoftirqd context.
Quoted 8 lines
Quoted 7 lines
(2) When softirqs processing happens asynchronously, a particular softirq like TASKLET can keep interrupting an already running softirq like TIMER/NET_RX, correct? In worse case scenario, a long running softirq like
NET_RXinterrupt a TIMER softirq. But I guess this is something expected with this. i.e each softirq is independent and whichever comes recent gets to interrupt the previously running softirqs.
Quoted 4 lines
Exactly, and that's inherent with interrupts in general. The only way to work around that is to thread each vector independantly but that's a whole different dimension :-)
Right.
Assigning a thread for each vector also may not solve this problem because preemption would be disabled while a softirq vector is running in its own thread.
I guess there is no hard priorities among softirq vectors. Earlier it was like first come first serve, now it is not. If we had priorities defined, (don't know how :-)) we could disable the lower prio vectors while a higher prio vector is being handled. This way we could gaurantee that TIMER softirq or HI-TASKLET won't be starved while a long running softirq like NET_RX/NET_TX/RCU is running.
Thanks,
Pavan
Signature · 2 lines
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.
Frederic Weisbecker
maintainerRFC PATCH 30/30 softirq: Tasklet/net-rx fixup
In reply to Frederic WeisbeckerFrom: Frederic Weisbecker <[email protected]>
Tasklets and net-rx vectors don't quite get along. If one is interrupted by another, we may run into a nasty spin_lock recursion:
[ 135.427198] Call Trace: [ 135.429650] <IRQ> [ 135.431690] dump_stack+0x67/0x95 [ 135.435024] spin_bug+0x95/0xf0 [ 135.438187] do_raw_spin_lock+0x77/0xa0 [ 135.442079] _raw_spin_lock_nested+0x40/0x50 [ 135.446439] ? tcp_v4_rcv+0x9da/0xb10 [ 135.450131] tcp_v4_rcv+0x9da/0xb10 [ 135.453650] ? ip_local_deliver+0x78/0x260 [ 135.457758] ip_local_deliver+0xdf/0x260 [ 135.461728] ip_rcv+0x4e/0x80 [ 135.464716] __netif_receive_skb_one_core+0x55/0x80 [ 135.469623] __netif_receive_skb+0x1b/0x70 [ 135.473757] netif_receive_skb_internal+0x92/0x390 [ 135.478574] napi_gro_receive+0xdf/0x1a0 [ 135.482545] rtl8169_poll+0x2b8/0x670 [ 135.486211] net_rx_action+0x1f8/0x3e0 [ 135.489989] __do_softirq+0x1a0/0x63c [ 135.493691] irq_exit+0x10f/0x120 [ 135.497033] do_IRQ+0x71/0x130 [ 135.500137] common_interrupt+0xf/0xf [ 135.503839] RIP: 0010:_raw_spin_unlock_irqrestore+0x59/0x70 [ 135.509471] Code: 75 21 53 9d e8 e8 1d a5 ff bf 01 00 00 00 e8 8e f6 97 ff 65 8b 05 ef 06 8d 7e 85 c0 74 0e 5b 41 5c 5d c3 e8 c9 20 a5 ff 53 9d <eb> dd e8 90 d4 8b ff 5b 41 5c 5d c3 66 66 2e 0f 1f 84 00 00 00 00 [ 135.528347] RSP: 0018:ffff88021fb03d28 EFLAGS: 00000246 ORIG_RAX: ffffffffffffffde [ 135.535989] RAX: ffff880217762480 RBX: 0000000000000246 RCX: 0000000000000002 [ 135.543201] RDX: 0000000000000000 RSI: ffff880217762c70 RDI: ffff880217762480 [ 135.550332] RBP: ffff88021fb03d38 R08: 0000000000000001 R09: 0000000000000000 [ 135.557519] R10: 0000000000000000 R11: 0000000000000000 R12: ffff88021fa59b40 [ 135.564719] R13: ffff88021fa59b40 R14: 00000000fffd78b5 R15: 000000000e000001 [ 135.571905] ? common_interrupt+0xa/0xf [ 135.575783] mod_timer+0x196/0x440 [ 135.579221] sk_reset_timer+0x18/0x30 [ 135.582940] tcp_schedule_loss_probe+0xe9/0x120 [ 135.587515] tcp_write_xmit+0x2c4/0x1240 [ 135.591468] tcp_tsq_write.part.46+0x5e/0xb0 [ 135.595756] tcp_tsq_handler+0xa3/0xb0 [ 135.599534] tcp_tasklet_func+0xdc/0x120 [ 135.603488] tasklet_action_common.isra.17+0xa3/0xb0 [ 135.608471] tasklet_action+0x2d/0x30 [ 135.612161] __do_softirq+0x1a0/0x63c [ 135.615847] irq_exit+0x10f/0x120 [ 135.619173] do_IRQ+0x71/0x130 [ 135.622251] common_interrupt+0xf/0xf [ 135.625949] </IRQ>
This is an ugly workaround until we find a proper solution.
--- kernel/softirq.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)
kernel/softirq.c +13 -0
@@ -320,6 +320,7 @@ asmlinkage __visible void __softirq_entry __do_softirq(void)unsigned long old_flags = current->flags;int max_restart =MAX_SOFTIRQ_RESTART ;struct softirq_action *h; +bool tasklet_enabled = false, net_rx_enabled = false;bool in_hardirq;__u32 pending;int softirq_bit;
@@ -338,6 +339,10 @@ asmlinkage __visible void __softirq_entry __do_softirq(void) in_hardirq =lockdep_softirq_start (); restart: +if (local_softirq_enabled () &TASKLET_SOFTIRQ ) + tasklet_enabled = true; +if (local_softirq_enabled () &NET_RX_SOFTIRQ ) + net_rx_enabled = true;/* Reset the pending bitmask before enabling irqs */ softirq_pending_nand (pending);
@@ -358,8 +363,16 @@ asmlinkage __visible void __softirq_entry __do_softirq(void)trace_softirq_entry (vec_nr);softirq_enabled_nand (BIT (vec_nr)); +if (vec_nr ==NET_RX_SOFTIRQ && tasklet_enabled) +softirq_enabled_nand (BIT (TASKLET_SOFTIRQ )); +if (vec_nr ==TASKLET_SOFTIRQ && net_rx_enabled) +softirq_enabled_nand (BIT (NET_RX_SOFTIRQ ));barrier (); h->action (h); +if (vec_nr ==TASKLET_SOFTIRQ && net_rx_enabled) +softirq_enabled_or (BIT (NET_RX_SOFTIRQ )); +if (vec_nr ==NET_RX_SOFTIRQ && tasklet_enabled) +softirq_enabled_or (BIT (TASKLET_SOFTIRQ ));softirq_enabled_or (BIT (vec_nr));trace_softirq_exit (vec_nr);if (unlikely (prev_count !=preempt_count ())) {
Signature · 1 line
2.7.4
Quoted 1 line · On Thu, 11 Oct 2018 01:11:47 +0200 Frederic Weisbecker <[email protected]>…
945 files changed, 13857 insertions(+), 9767 deletions(-)
Impressive :)
I have to ask a dumb question, though. Might it not be better to add a new set of functions like:
local_softirq_disable(mask); spin_lock_softirq(lock, mask);
Then just define the existing functions to call the new ones with SOFTIRQ_ALL_MASK? It would achieve something like the same result with far less churn and conflict potential; then individual call sites could be changed at leisure? For extra credit, somebody could do a checkpatch rule to keep new calls to the _bh functions from being added.
Thanks,
jon
Quoted 10 lines · Jonathan Corbet
I have to ask a dumb question, though. Might it not be better to add a new set of functions like:
local_softirq_disable(mask); spin_lock_softirq(lock, mask);Then just define the existing functions to call the new ones with
SOFTIRQ_ALL_MASK? It would achieve something like the same result with far less churn and conflict potential; then individual call sites could be changed at leisure?
I was thinking the exact same thing...
Thanks,
Richard
Frederic Weisbecker
maintainerQuoted 6 lines · Jonathan Corbet
Quoted 1 line · On Thu, 11 Oct 2018 01:11:47 +0200 Frederic Weisbecker <[email protected]>…
945 files changed, 13857 insertions(+), 9767 deletions(-)Impressive :)
In the wrong way :)
Quoted 4 lines
I have to ask a dumb question, though. Might it not be better to add a new set of functions like:
Quoted 8 lines
local_softirq_disable(mask); spin_lock_softirq(lock, mask);Then just define the existing functions to call the new ones with
SOFTIRQ_ALL_MASK? It would achieve something like the same result with far less churn and conflict potential; then individual call sites could be changed at leisure? For extra credit, somebody could do a checkpatch rule to keep new calls to the_bhfunctions from being added.
So it's not a dumb question at all. That's in fact the core of the suggestions I got while discussing that lately on IRC with the initial Cc list.
That's definetly the direction I'll take on v2: keeping the current API, introduce new ones with per vector granularity and convert iteratively. The diffstat will shrink tremendously and it can make the change eventually maintainable.
The reason why I didn't take that approach first in this version is because of a little technical detail:
_ Serving softirqs is done under SOFTIRQ_OFFSET: (1 << SOFTIRQ_SHIFT)
_ Disabling softirqs is done under SOFTIRQ_OFFSET * 2
We do that to differentiate both state. Serving softirqs can't nest whereas disabling softirqs can nest. So we just need to check the value is odd to identify a serving softirq state.
Now things are going to change as serving softirqs will be able to nest too. And having that bh saved state allowed me to make softirqs disablement not nesting. So I just needed to invert both ways to account. I wanted to do that because otherwise we need to share SOFTIRQ_MASK for two counters, which makes a maximum of 16 for both. That's enough for serving softirqs as it's couldn't nest further than NR_SOFTIRQS, but disabling softirqs depth was unpredictable, even though 16 levels is already insane, anyway...
There is an easy alternative though:
local_bh_enter()
{
bool nesting = false;
if (preempt_count() & SOFTIRQ_OFFSET)
nesting = true;
else
preempt_count() |= SOFTIRQ_OFFSET;
return nesting;
}
local_bh_exit(bool nesting)
{
if (nesting)
preempt_count() &= ~SOFTIRQ_OFFSET;
}
do_softirq()
{
bool nesting = local_bh_enter();
// process softirqs ....
local_bh_exit(nesting);
}But I guess it was just too obvious for me to be considered :-S