So this set should hopefully address all reviews from the v2, and fix all reports from the extremely useful (as always) Kbuild testing bot. It also completes support for all archs. Unfortunately the already huge pile is ever growing. I should be able to offload 2 or 3 of those patches but the rest really belong together. Changes since v2: * Fix unused function warning (reported by Kbuild test robot) - locking/lockdep: Move valid_state() inside CONFIG_TRACE_IRQFLAGS && CONFIG_PROVE_LOCKING * Added Reviewed-by tags from davem * Reorder changes introducing u64 to the proper patches (reported by Linus) * Fix lock usage mask iteration by avoid shifting beyond 63 (reported by Linus) - locking/lockdep: Introduce lock usage mask iterator * Fix mark_lock() verbosity by handling all usages in the ask - locking/lockdep: Report all usages on mark_lock() verbosity mode * Add SPDX licence identifier on softirq_vector.h (reported by Sebastian) * Fix several build errors on s390 (reported by Kbuild test robot) - arch/softirq: Rename softirq_pending fields to softirq_data - softirq: Introduce disabled softirq vectors bits - softirq: Check enabled vectors before processing * Initialize softirq enabled field on all other archs than x86 - parisc: Init softirq enabled field - powerpc: Init softirq enabled field - softirq: Init softirq enabled field for default irq_stat definition * Fix spin_[un]lock_bh_mask() on UP, also some build errors against uninlined spinlocks (reported by Kbuild test robot) - locking: Introduce spin_[un]lock_bh_mask() * Socket lock may also execute on HRTIMER_SOFTIRQ, include it on the mask (reported by Sebastian) - net: Make softirq vector masking finegrained on release_sock() git://git.kernel.org/pub/scm/linux/kernel/git/frederic/linux-dynticks.git softirq/soft-interruptible-v2 HEAD: 63c89028058f5920d4b5a9d38452fa4623469583 Thanks, Frederic --- Frederic Weisbecker (37): locking/lockdep: Move valid_state() inside CONFIG_TRACE_IRQFLAGS && CONFIG_PROVE_LOCKING locking/lockdep: Use expanded masks on find_usage_*() functions locking/lockdep: Introduce struct lock_usage locking/lockdep: Convert usage_mask to u64 locking/lockdep: Introduce lock usage mask iterator locking/lockdep: Test all incompatible scenario at once in check_irq_usage() locking/lockdep: Prepare valid_state() to handle plain masks locking/lockdep: Prepare check_usage_*() to handle plain masks locking/lockdep: Prepare state_verbose() to handle all softirqs locking/lockdep: Make mark_lock() fastpath to work with multiple usage at once locking/lockdep: Save stack trace for each softirq vector involved locking/lockdep: Report all usages on mark_lock() verbosity mode softirq: Macrofy softirq vectors locking/lockdep: Define per vector softirq lock usage states softirq: Pass softirq vector number to lockdep on vector execution x86: Revert "x86/irq: Demote irq_cpustat_t::__softirq_pending to u16" arch/softirq: Rename softirq_pending fields to softirq_data softirq: Normalize softirq_pending naming scheme softirq: Convert softirq_pending_*() to set/clear mask scheme softirq: Introduce disabled softirq vectors bits softirq: Rename _local_bh_enable() to local_bh_enable_no_softirq() softirq: Move vectors bits to bottom_half.h x86: Init softirq enabled field parisc: Init softirq enabled field powerpc: Init softirq enabled field softirq: Init softirq enabled field for default irq_stat definition softirq: Check enabled vectors before processing softirq: Remove stale comment softirq: Uninline !CONFIG_TRACE_IRQFLAGS __local_bh_disable_ip() softirq: Prepare for mixing all/per-vector masking softirq: Support per vector masking locking/lockdep: Remove redundant softirqs on check locking/lockdep: Update check_flags() according to new layout locking/lockdep: Branch the new vec-finegrained softirq masking to lockdep softirq: Allow to soft interrupt vector-specific masked contexts locking: Introduce spin_[un]lock_bh_mask() net: Make softirq vector masking finegrained on release_sock() arch/arm/include/asm/hardirq.h | 2 +- arch/arm64/include/asm/hardirq.h | 2 +- arch/h8300/kernel/asm-offsets.c | 2 +- arch/ia64/include/asm/hardirq.h | 2 +- arch/ia64/include/asm/processor.h | 2 +- arch/m68k/include/asm/hardirq.h | 2 +- arch/m68k/kernel/asm-offsets.c | 2 +- arch/parisc/include/asm/hardirq.h | 2 +- arch/parisc/kernel/irq.c | 6 +- arch/powerpc/include/asm/hardirq.h | 2 +- arch/powerpc/kernel/irq.c | 5 +- arch/s390/include/asm/hardirq.h | 11 +- arch/s390/include/asm/lowcore.h | 2 +- arch/s390/lib/delay.c | 2 +- arch/sh/include/asm/hardirq.h | 2 +- arch/sparc/include/asm/cpudata_64.h | 2 +- arch/sparc/include/asm/hardirq_64.h | 4 +- arch/um/include/asm/hardirq.h | 2 +- arch/x86/include/asm/hardirq.h | 2 +- arch/x86/kernel/irq.c | 5 +- drivers/s390/char/sclp.c | 2 +- drivers/s390/cio/cio.c | 2 +- include/asm-generic/hardirq.h | 2 +- include/linux/bottom_half.h | 41 +++- include/linux/interrupt.h | 87 ++++--- include/linux/irqflags.h | 12 +- include/linux/lockdep.h | 5 +- include/linux/softirq_vector.h | 12 + include/linux/spinlock.h | 14 ++ include/linux/spinlock_api_smp.h | 28 +++ include/linux/spinlock_api_up.h | 13 + kernel/locking/lockdep.c | 475 ++++++++++++++++++++++++------------ kernel/locking/lockdep_internals.h | 50 +++- kernel/locking/lockdep_proc.c | 2 +- kernel/locking/lockdep_states.h | 4 +- kernel/locking/spinlock.c | 19 ++ kernel/softirq.c | 163 +++++++++---- lib/locking-selftest.c | 4 +- net/core/sock.c | 8 +- 39 files changed, 715 insertions(+), 289 deletions(-)
valid_state() and print_usage*() functions are not used beyond locking correctness checks. So move them inside the appropriate CONFIG_TRACE_IRQFLAGS && CONFIG_PROVE_LOCKING section. Sadly the "unused function" warning wouldn't fire because valid_state() is inline. Signed-off-by: Frederic Weisbecker <frederic@kernel.org> Cc: Mauro Carvalho Chehab <mchehab+samsung@kernel.org> Cc: Joel Fernandes <joel@joelfernandes.org> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Pavan Kondeti <pkondeti@codeaurora.org> Cc: Paul E . McKenney <paulmck@linux.vnet.ibm.com> Cc: David S . Miller <davem@davemloft.net> Cc: Ingo Molnar <mingo@kernel.org> Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Cc: Linus Torvalds <torvalds@linux-foundation.org> Cc: Peter Zijlstra <peterz@infradead.org> --- kernel/locking/lockdep.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c index 608f74ed8bb9..a989a3e9ead7 100644 --- a/kernel/locking/lockdep.c +++ b/kernel/locking/lockdep.c @@ -2409,6 +2409,12 @@ static void check_chain_key(struct task_struct *curr) #endif } +static int mark_lock(struct task_struct *curr, struct held_lock *this, + enum lock_usage_bit new_bit); + +#if defined(CONFIG_TRACE_IRQFLAGS) && defined(CONFIG_PROVE_LOCKING) + + static void print_usage_bug_scenario(struct held_lock *lock) { @@ -2478,10 +2484,6 @@ valid_state(struct task_struct *curr, struct held_lock *this, return 1; } -static int mark_lock(struct task_struct *curr, struct held_lock *this, - enum lock_usage_bit new_bit); - -#if defined(CONFIG_TRACE_IRQFLAGS) && defined(CONFIG_PROVE_LOCKING) /* * print irq inversion bug: -- 2.21.0
In order to perform softirq vector-finegrained locking validation we'll need to be able to check multiple vector usages at once. Prepare the low level usage mask check functions for that purpose. Reviewed-by: David S. Miller <davem@davemloft.net> Signed-off-by: Frederic Weisbecker <frederic@kernel.org> Cc: Mauro Carvalho Chehab <mchehab+samsung@kernel.org> Cc: Joel Fernandes <joel@joelfernandes.org> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Pavan Kondeti <pkondeti@codeaurora.org> Cc: Paul E . McKenney <paulmck@linux.vnet.ibm.com> Cc: David S . Miller <davem@davemloft.net> Cc: Ingo Molnar <mingo@kernel.org> Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Cc: Linus Torvalds <torvalds@linux-foundation.org> Cc: Peter Zijlstra <peterz@infradead.org> --- kernel/locking/lockdep.c | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c index a989a3e9ead7..11db7ba29660 100644 --- a/kernel/locking/lockdep.c +++ b/kernel/locking/lockdep.c @@ -1335,9 +1335,9 @@ check_redundant(struct lock_list *root, struct lock_class *target, * without creating any illegal irq-safe -> irq-unsafe lock dependency. */ -static inline int usage_match(struct lock_list *entry, void *bit) +static inline int usage_match(struct lock_list *entry, void *mask) { - return entry->class->usage_mask & (1 << (enum lock_usage_bit)bit); + return entry->class->usage_mask & *(unsigned long *)mask; } @@ -1353,14 +1353,14 @@ static inline int usage_match(struct lock_list *entry, void *bit) * Return <0 on error. */ static int -find_usage_forwards(struct lock_list *root, enum lock_usage_bit bit, +find_usage_forwards(struct lock_list *root, unsigned long usage_mask, struct lock_list **target_entry) { int result; debug_atomic_inc(nr_find_usage_forwards_checks); - result = __bfs_forwards(root, (void *)bit, usage_match, target_entry); + result = __bfs_forwards(root, &usage_mask, usage_match, target_entry); return result; } @@ -1376,14 +1376,14 @@ find_usage_forwards(struct lock_list *root, enum lock_usage_bit bit, * Return <0 on error. */ static int -find_usage_backwards(struct lock_list *root, enum lock_usage_bit bit, +find_usage_backwards(struct lock_list *root, unsigned long usage_mask, struct lock_list **target_entry) { int result; debug_atomic_inc(nr_find_usage_backwards_checks); - result = __bfs_backwards(root, (void *)bit, usage_match, target_entry); + result = __bfs_backwards(root, &usage_mask, usage_match, target_entry); return result; } @@ -1588,7 +1588,7 @@ check_usage(struct task_struct *curr, struct held_lock *prev, this.parent = NULL; this.class = hlock_class(prev); - ret = find_usage_backwards(&this, bit_backwards, &target_entry); + ret = find_usage_backwards(&this, BIT(bit_backwards), &target_entry); if (ret < 0) return print_bfs_bug(ret); if (ret == 1) @@ -1596,7 +1596,7 @@ check_usage(struct task_struct *curr, struct held_lock *prev, that.parent = NULL; that.class = hlock_class(next); - ret = find_usage_forwards(&that, bit_forwards, &target_entry1); + ret = find_usage_forwards(&that, BIT(bit_forwards), &target_entry1); if (ret < 0) return print_bfs_bug(ret); if (ret == 1) @@ -2555,7 +2555,7 @@ print_irq_inversion_bug(struct task_struct *curr, */ static int check_usage_forwards(struct task_struct *curr, struct held_lock *this, - enum lock_usage_bit bit, const char *irqclass) + unsigned long usage_mask, const char *irqclass) { int ret; struct lock_list root; @@ -2563,7 +2563,7 @@ check_usage_forwards(struct task_struct *curr, struct held_lock *this, root.parent = NULL; root.class = hlock_class(this); - ret = find_usage_forwards(&root, bit, &target_entry); + ret = find_usage_forwards(&root, usage_mask, &target_entry); if (ret < 0) return print_bfs_bug(ret); if (ret == 1) @@ -2579,7 +2579,7 @@ check_usage_forwards(struct task_struct *curr, struct held_lock *this, */ static int check_usage_backwards(struct task_struct *curr, struct held_lock *this, - enum lock_usage_bit bit, const char *irqclass) + unsigned long usage_mask, const char *irqclass) { int ret; struct lock_list root; @@ -2587,7 +2587,7 @@ check_usage_backwards(struct task_struct *curr, struct held_lock *this, root.parent = NULL; root.class = hlock_class(this); - ret = find_usage_backwards(&root, bit, &target_entry); + ret = find_usage_backwards(&root, usage_mask, &target_entry); if (ret < 0) return print_bfs_bug(ret); if (ret == 1) @@ -2646,7 +2646,7 @@ static inline int state_verbose(enum lock_usage_bit bit, } typedef int (*check_usage_f)(struct task_struct *, struct held_lock *, - enum lock_usage_bit bit, const char *name); + unsigned long usage_mask, const char *name); static int mark_lock_irq(struct task_struct *curr, struct held_lock *this, @@ -2678,7 +2678,7 @@ mark_lock_irq(struct task_struct *curr, struct held_lock *this, * states. */ if ((!read || !dir || STRICT_READ_CHECKS) && - !usage(curr, this, excl_bit, state_name(new_bit & ~LOCK_USAGE_READ_MASK))) + !usage(curr, this, BIT(excl_bit), state_name(new_bit & ~LOCK_USAGE_READ_MASK))) return 0; /* @@ -2689,7 +2689,7 @@ mark_lock_irq(struct task_struct *curr, struct held_lock *this, return 0; if (STRICT_READ_CHECKS && - !usage(curr, this, excl_bit + LOCK_USAGE_READ_MASK, + !usage(curr, this, BIT(excl_bit + LOCK_USAGE_READ_MASK), state_name(new_bit + LOCK_USAGE_READ_MASK))) return 0; } -- 2.21.0
In order to support softirq per-vector locking validation, we could simply iterate over all enabled vectors and perform separate validation for all of them on every lock event. We can expect that to introduce a severe performance penalty though. Therefore, we instead plan to validate the LOCK_USED_IN_*_SOFTIRQ and LOCK_ENABLED_*_SOFTIRQ events with grouping the involved softirq vector bits. It implies that we'll rather validate expanded usage mask than usage bit in the end. Before the usage mask to be expanded though, we need to play with the usage bit that defines the nature of the event relevant to a group of vectors. Introduce struct lock_usage to implement that. This is made of the classical lock usage bit that defines the nature of a lock usage along which we carry the vectors to which that usage applies. Once high level functions are done dealing with the nature of the lock usage, lower level functions dealing with validation can expand the struct lock_usage to a usage mask through lock_usage_mask(). For now, vector is always 0 until we get the proper vector finegrained informations on softirq usage events. Reviewed-by: David S. Miller <davem@davemloft.net> Signed-off-by: Frederic Weisbecker <frederic@kernel.org> Cc: Mauro Carvalho Chehab <mchehab+samsung@kernel.org> Cc: Joel Fernandes <joel@joelfernandes.org> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Pavan Kondeti <pkondeti@codeaurora.org> Cc: Paul E . McKenney <paulmck@linux.vnet.ibm.com> Cc: David S . Miller <davem@davemloft.net> Cc: Ingo Molnar <mingo@kernel.org> Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Cc: Linus Torvalds <torvalds@linux-foundation.org> Cc: Peter Zijlstra <peterz@infradead.org> --- kernel/locking/lockdep.c | 122 +++++++++++++++++------------ kernel/locking/lockdep_internals.h | 6 ++ 2 files changed, 79 insertions(+), 49 deletions(-) diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c index 11db7ba29660..4fc859c0a799 100644 --- a/kernel/locking/lockdep.c +++ b/kernel/locking/lockdep.c @@ -468,6 +468,11 @@ static inline unsigned long lock_flag(enum lock_usage_bit bit) return 1UL << bit; } +static unsigned long lock_usage_mask(struct lock_usage *usage) +{ + return lock_flag(usage->bit); +} + static char get_usage_char(struct lock_class *class, enum lock_usage_bit bit) { char c = '.'; @@ -2410,7 +2415,7 @@ static void check_chain_key(struct task_struct *curr) } static int mark_lock(struct task_struct *curr, struct held_lock *this, - enum lock_usage_bit new_bit); + struct lock_usage *new_usage); #if defined(CONFIG_TRACE_IRQFLAGS) && defined(CONFIG_PROVE_LOCKING) @@ -2484,7 +2489,6 @@ valid_state(struct task_struct *curr, struct held_lock *this, return 1; } - /* * print irq inversion bug: */ @@ -2650,11 +2654,14 @@ typedef int (*check_usage_f)(struct task_struct *, struct held_lock *, static int mark_lock_irq(struct task_struct *curr, struct held_lock *this, - enum lock_usage_bit new_bit) + struct lock_usage *new_usage) { - int excl_bit = exclusive_bit(new_bit); - int read = new_bit & LOCK_USAGE_READ_MASK; - int dir = new_bit & LOCK_USAGE_DIR_MASK; + struct lock_usage excl_usage = { + .bit = exclusive_bit(new_usage->bit), + .vector = new_usage->vector + }; + int read = new_usage->bit & LOCK_USAGE_READ_MASK; + int dir = new_usage->bit & LOCK_USAGE_DIR_MASK; /* * mark USED_IN has to look forwards -- to ensure no dependency @@ -2670,7 +2677,7 @@ mark_lock_irq(struct task_struct *curr, struct held_lock *this, * Validate that this particular lock does not have conflicting * usage states. */ - if (!valid_state(curr, this, new_bit, excl_bit)) + if (!valid_state(curr, this, new_usage->bit, excl_usage.bit)) return 0; /* @@ -2678,23 +2685,24 @@ mark_lock_irq(struct task_struct *curr, struct held_lock *this, * states. */ if ((!read || !dir || STRICT_READ_CHECKS) && - !usage(curr, this, BIT(excl_bit), state_name(new_bit & ~LOCK_USAGE_READ_MASK))) + !usage(curr, this, lock_usage_mask(&excl_usage), state_name(new_usage->bit & ~LOCK_USAGE_READ_MASK))) return 0; /* * Check for read in write conflicts */ if (!read) { - if (!valid_state(curr, this, new_bit, excl_bit + LOCK_USAGE_READ_MASK)) + excl_usage.bit += LOCK_USAGE_READ_MASK; + if (!valid_state(curr, this, new_usage->bit, excl_usage.bit)) return 0; if (STRICT_READ_CHECKS && - !usage(curr, this, BIT(excl_bit + LOCK_USAGE_READ_MASK), - state_name(new_bit + LOCK_USAGE_READ_MASK))) + !usage(curr, this, lock_usage_mask(&excl_usage), + state_name(new_usage->bit + LOCK_USAGE_READ_MASK))) return 0; } - if (state_verbose(new_bit, hlock_class(this))) + if (state_verbose(new_usage->bit, hlock_class(this))) return 2; return 1; @@ -2704,24 +2712,24 @@ mark_lock_irq(struct task_struct *curr, struct held_lock *this, * Mark all held locks with a usage bit: */ static int -mark_held_locks(struct task_struct *curr, enum lock_usage_bit base_bit) +mark_held_locks(struct task_struct *curr, struct lock_usage *base_usage) { struct held_lock *hlock; int i; for (i = 0; i < curr->lockdep_depth; i++) { - enum lock_usage_bit hlock_bit = base_bit; + struct lock_usage hlock_usage = *base_usage; hlock = curr->held_locks + i; if (hlock->read) - hlock_bit += LOCK_USAGE_READ_MASK; + hlock_usage.bit += LOCK_USAGE_READ_MASK; - BUG_ON(hlock_bit >= LOCK_USAGE_STATES); + BUG_ON(hlock_usage.bit >= LOCK_USAGE_STATES); if (!hlock->check) continue; - if (!mark_lock(curr, hlock, hlock_bit)) + if (!mark_lock(curr, hlock, &hlock_usage)) return 0; } @@ -2734,6 +2742,7 @@ mark_held_locks(struct task_struct *curr, enum lock_usage_bit base_bit) static void __trace_hardirqs_on_caller(unsigned long ip) { struct task_struct *curr = current; + struct lock_usage usage = { .bit = LOCK_ENABLED_HARDIRQ }; /* we'll do an OFF -> ON transition: */ curr->hardirqs_enabled = 1; @@ -2742,16 +2751,18 @@ static void __trace_hardirqs_on_caller(unsigned long ip) * We are going to turn hardirqs on, so set the * usage bit for all held locks: */ - if (!mark_held_locks(curr, LOCK_ENABLED_HARDIRQ)) + if (!mark_held_locks(curr, &usage)) return; /* * If we have softirqs enabled, then set the usage * bit for all held locks. (disabled hardirqs prevented * this bit from being set before) */ - if (curr->softirqs_enabled) - if (!mark_held_locks(curr, LOCK_ENABLED_SOFTIRQ)) + if (curr->softirqs_enabled) { + usage.bit = LOCK_ENABLED_SOFTIRQ; + if (!mark_held_locks(curr, &usage)) return; + } curr->hardirq_enable_ip = ip; curr->hardirq_enable_event = ++curr->irq_events; @@ -2834,6 +2845,9 @@ void lockdep_hardirqs_off(unsigned long ip) void trace_softirqs_on(unsigned long ip) { struct task_struct *curr = current; + struct lock_usage usage = { + .bit = LOCK_ENABLED_SOFTIRQ, + }; if (unlikely(!debug_locks || current->lockdep_recursion)) return; @@ -2864,7 +2878,7 @@ void trace_softirqs_on(unsigned long ip) * enabled too: */ if (curr->hardirqs_enabled) - mark_held_locks(curr, LOCK_ENABLED_SOFTIRQ); + mark_held_locks(curr, &usage); current->lockdep_recursion = 0; } @@ -2902,46 +2916,55 @@ void trace_softirqs_off(unsigned long ip) static int mark_irqflags(struct task_struct *curr, struct held_lock *hlock) { + struct lock_usage usage = { .vector = 0 }; /* * If non-trylock use in a hardirq or softirq context, then * mark the lock as used in these contexts: */ if (!hlock->trylock) { if (hlock->read) { - if (curr->hardirq_context) - if (!mark_lock(curr, hlock, - LOCK_USED_IN_HARDIRQ_READ)) + if (curr->hardirq_context) { + usage.bit = LOCK_USED_IN_HARDIRQ_READ; + if (!mark_lock(curr, hlock, &usage)) return 0; - if (curr->softirq_context) - if (!mark_lock(curr, hlock, - LOCK_USED_IN_SOFTIRQ_READ)) + } + if (curr->softirq_context) { + usage.bit = LOCK_USED_IN_SOFTIRQ_READ; + if (!mark_lock(curr, hlock, &usage)) return 0; + } } else { - if (curr->hardirq_context) - if (!mark_lock(curr, hlock, LOCK_USED_IN_HARDIRQ)) + if (curr->hardirq_context) { + usage.bit = LOCK_USED_IN_HARDIRQ; + if (!mark_lock(curr, hlock, &usage)) return 0; - if (curr->softirq_context) - if (!mark_lock(curr, hlock, LOCK_USED_IN_SOFTIRQ)) + } + if (curr->softirq_context) { + usage.bit = LOCK_USED_IN_SOFTIRQ; + if (!mark_lock(curr, hlock, &usage)) return 0; + } } } if (!hlock->hardirqs_off) { if (hlock->read) { - if (!mark_lock(curr, hlock, - LOCK_ENABLED_HARDIRQ_READ)) + usage.bit = LOCK_ENABLED_HARDIRQ_READ; + if (!mark_lock(curr, hlock, &usage)) return 0; - if (curr->softirqs_enabled) - if (!mark_lock(curr, hlock, - LOCK_ENABLED_SOFTIRQ_READ)) + if (curr->softirqs_enabled) { + usage.bit = LOCK_ENABLED_SOFTIRQ_READ; + if (!mark_lock(curr, hlock, &usage)) return 0; + } } else { - if (!mark_lock(curr, hlock, - LOCK_ENABLED_HARDIRQ)) + usage.bit = LOCK_ENABLED_HARDIRQ; + if (!mark_lock(curr, hlock, &usage)) return 0; - if (curr->softirqs_enabled) - if (!mark_lock(curr, hlock, - LOCK_ENABLED_SOFTIRQ)) + if (curr->softirqs_enabled) { + usage.bit = LOCK_ENABLED_SOFTIRQ; + if (!mark_lock(curr, hlock, &usage)) return 0; + } } } @@ -2980,7 +3003,7 @@ static int separate_irq_context(struct task_struct *curr, static inline int mark_lock_irq(struct task_struct *curr, struct held_lock *this, - enum lock_usage_bit new_bit) + struct lock_usage *new_usage) { WARN_ON(1); /* Impossible innit? when we don't have TRACE_IRQFLAG */ return 1; @@ -3009,9 +3032,9 @@ static inline int separate_irq_context(struct task_struct *curr, * Mark a lock with a usage bit, and validate the state transition: */ static int mark_lock(struct task_struct *curr, struct held_lock *this, - enum lock_usage_bit new_bit) + struct lock_usage *new_usage) { - unsigned int new_mask = 1 << new_bit, ret = 1; + unsigned long new_mask = lock_usage_mask(new_usage), ret = 1; /* * If already set then do not dirty the cacheline, @@ -3032,10 +3055,10 @@ static int mark_lock(struct task_struct *curr, struct held_lock *this, hlock_class(this)->usage_mask |= new_mask; - if (!save_trace(hlock_class(this)->usage_traces + new_bit)) + if (!save_trace(hlock_class(this)->usage_traces + new_usage->bit)) return 0; - switch (new_bit) { + switch (new_usage->bit) { #define LOCKDEP_STATE(__STATE) \ case LOCK_USED_IN_##__STATE: \ case LOCK_USED_IN_##__STATE##_READ: \ @@ -3043,7 +3066,7 @@ static int mark_lock(struct task_struct *curr, struct held_lock *this, case LOCK_ENABLED_##__STATE##_READ: #include "lockdep_states.h" #undef LOCKDEP_STATE - ret = mark_lock_irq(curr, this, new_bit); + ret = mark_lock_irq(curr, this, new_usage); if (!ret) return 0; break; @@ -3063,7 +3086,7 @@ static int mark_lock(struct task_struct *curr, struct held_lock *this, * We must printk outside of the graph_lock: */ if (ret == 2) { - printk("\nmarked lock as {%s}:\n", usage_str[new_bit]); + printk("\nmarked lock as {%s}:\n", usage_str[new_usage->bit]); print_lock(this); print_irqtrace_events(curr); dump_stack(); @@ -3187,6 +3210,7 @@ static int __lock_acquire(struct lockdep_map *lock, unsigned int subclass, { struct task_struct *curr = current; struct lock_class *class = NULL; + struct lock_usage usage = { .bit = LOCK_USED }; struct held_lock *hlock; unsigned int depth; int chain_head = 0; @@ -3280,7 +3304,7 @@ static int __lock_acquire(struct lockdep_map *lock, unsigned int subclass, return 0; /* mark it as used: */ - if (!mark_lock(curr, hlock, LOCK_USED)) + if (!mark_lock(curr, hlock, &usage)) return 0; /* diff --git a/kernel/locking/lockdep_internals.h b/kernel/locking/lockdep_internals.h index 2ebb9d0ea91c..e714c823f594 100644 --- a/kernel/locking/lockdep_internals.h +++ b/kernel/locking/lockdep_internals.h @@ -26,6 +26,12 @@ enum lock_usage_bit { #define LOCK_USAGE_DIR_MASK 2 #define LOCK_USAGE_STATE_MASK (~(LOCK_USAGE_READ_MASK | LOCK_USAGE_DIR_MASK)) +struct lock_usage { + enum lock_usage_bit bit; + unsigned long vector; /* Softirq vector */ +}; + + /* * Usage-state bitmasks: */ -- 2.21.0
The usage mask is going to expand to validate softirq related usages in a per-vector finegrained way. The current bitmap layout is: LOCK_USED HARDIRQ bits \ / \ / 0 0000 0000 | | SOFTIRQ bits The new one will be: TIMER_SOFTIRQ LOCK_USED bits HARDIRQ bits \ | | \ | | 0 0000 [...] 0000 0000 0000 | | | | RCU_SOFTIRQ HI_SOFTIRQ bits bits So we have 4 hardirq bits + NR_SOFTIRQS * 4 + 1 bit (LOCK_USED) = 45 bits. Therefore we need a 64 bits mask. Reviewed-by: David S. Miller <davem@davemloft.net> Signed-off-by: Frederic Weisbecker <frederic@kernel.org> Cc: Mauro Carvalho Chehab <mchehab+samsung@kernel.org> Cc: Joel Fernandes <joel@joelfernandes.org> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Pavan Kondeti <pkondeti@codeaurora.org> Cc: Paul E . McKenney <paulmck@linux.vnet.ibm.com> Cc: David S . Miller <davem@davemloft.net> Cc: Ingo Molnar <mingo@kernel.org> Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Cc: Linus Torvalds <torvalds@linux-foundation.org> Cc: Peter Zijlstra <peterz@infradead.org> --- include/linux/lockdep.h | 2 +- kernel/locking/lockdep.c | 24 ++++++++++++------------ 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h index c5335df2372f..06669f20a30a 100644 --- a/include/linux/lockdep.h +++ b/include/linux/lockdep.h @@ -83,7 +83,7 @@ struct lock_class { /* * IRQ/softirq usage tracking bits: */ - unsigned long usage_mask; + u64 usage_mask; struct stack_trace usage_traces[XXX_LOCK_USAGE_STATES]; /* diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c index 4fc859c0a799..004278969afc 100644 --- a/kernel/locking/lockdep.c +++ b/kernel/locking/lockdep.c @@ -463,12 +463,12 @@ const char * __get_key_name(struct lockdep_subclass_key *key, char *str) return kallsyms_lookup((unsigned long)key, NULL, NULL, NULL, str); } -static inline unsigned long lock_flag(enum lock_usage_bit bit) +static inline u64 lock_flag(enum lock_usage_bit bit) { - return 1UL << bit; + return BIT_ULL(bit); } -static unsigned long lock_usage_mask(struct lock_usage *usage) +static u64 lock_usage_mask(struct lock_usage *usage) { return lock_flag(usage->bit); } @@ -1342,7 +1342,7 @@ check_redundant(struct lock_list *root, struct lock_class *target, static inline int usage_match(struct lock_list *entry, void *mask) { - return entry->class->usage_mask & *(unsigned long *)mask; + return entry->class->usage_mask & *(u64 *)mask; } @@ -1358,7 +1358,7 @@ static inline int usage_match(struct lock_list *entry, void *mask) * Return <0 on error. */ static int -find_usage_forwards(struct lock_list *root, unsigned long usage_mask, +find_usage_forwards(struct lock_list *root, u64 usage_mask, struct lock_list **target_entry) { int result; @@ -1381,7 +1381,7 @@ find_usage_forwards(struct lock_list *root, unsigned long usage_mask, * Return <0 on error. */ static int -find_usage_backwards(struct lock_list *root, unsigned long usage_mask, +find_usage_backwards(struct lock_list *root, u64 usage_mask, struct lock_list **target_entry) { int result; @@ -1405,7 +1405,7 @@ static void print_lock_class_header(struct lock_class *class, int depth) printk(KERN_CONT " {\n"); for (bit = 0; bit < LOCK_USAGE_STATES; bit++) { - if (class->usage_mask & (1 << bit)) { + if (class->usage_mask & lock_flag(bit)) { int len = depth; len += printk("%*s %s", depth, "", usage_str[bit]); @@ -2484,7 +2484,7 @@ static inline int valid_state(struct task_struct *curr, struct held_lock *this, enum lock_usage_bit new_bit, enum lock_usage_bit bad_bit) { - if (unlikely(hlock_class(this)->usage_mask & (1 << bad_bit))) + if (unlikely(hlock_class(this)->usage_mask & lock_flag(bad_bit))) return print_usage_bug(curr, this, bad_bit, new_bit); return 1; } @@ -2559,7 +2559,7 @@ print_irq_inversion_bug(struct task_struct *curr, */ static int check_usage_forwards(struct task_struct *curr, struct held_lock *this, - unsigned long usage_mask, const char *irqclass) + u64 usage_mask, const char *irqclass) { int ret; struct lock_list root; @@ -2583,7 +2583,7 @@ check_usage_forwards(struct task_struct *curr, struct held_lock *this, */ static int check_usage_backwards(struct task_struct *curr, struct held_lock *this, - unsigned long usage_mask, const char *irqclass) + u64 usage_mask, const char *irqclass) { int ret; struct lock_list root; @@ -2650,7 +2650,7 @@ static inline int state_verbose(enum lock_usage_bit bit, } typedef int (*check_usage_f)(struct task_struct *, struct held_lock *, - unsigned long usage_mask, const char *name); + u64 usage_mask, const char *name); static int mark_lock_irq(struct task_struct *curr, struct held_lock *this, @@ -3034,7 +3034,7 @@ static inline int separate_irq_context(struct task_struct *curr, static int mark_lock(struct task_struct *curr, struct held_lock *this, struct lock_usage *new_usage) { - unsigned long new_mask = lock_usage_mask(new_usage), ret = 1; + u64 new_mask = lock_usage_mask(new_usage), ret = 1; /* * If already set then do not dirty the cacheline, -- 2.21.0
Introduce a mask iterator that can be used to get the bit numbers from a lock usage mask. We don't want the overhead of the bitmap library for a fixed small 64 bit mask. So we just want a simple loop relying on an ffs() like function. Yet we want to be careful not to shift further 64 bits at once as it has unpredictable behaviour, depending on architecture backend. Therefore the shift on each iteration is cut in two parts: 1) Shift by the first set bit number (can't exceed 63) 2) Shift again by 1 because __ffs64() counts from zero Inspired-by: Linus Torvalds <torvalds@linux-foundation.org> Signed-off-by: Frederic Weisbecker <frederic@kernel.org> Cc: Mauro Carvalho Chehab <mchehab+samsung@kernel.org> Cc: Joel Fernandes <joel@joelfernandes.org> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Pavan Kondeti <pkondeti@codeaurora.org> Cc: Paul E . McKenney <paulmck@linux.vnet.ibm.com> Cc: David S . Miller <davem@davemloft.net> Cc: Ingo Molnar <mingo@kernel.org> Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Cc: Linus Torvalds <torvalds@linux-foundation.org> Cc: Peter Zijlstra <peterz@infradead.org> --- kernel/locking/lockdep.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c index 004278969afc..1a335176cb61 100644 --- a/kernel/locking/lockdep.c +++ b/kernel/locking/lockdep.c @@ -463,6 +463,23 @@ const char * __get_key_name(struct lockdep_subclass_key *key, char *str) return kallsyms_lookup((unsigned long)key, NULL, NULL, NULL, str); } +static u64 mask_iter(u64 *mask, int *bit) +{ + u64 old_mask = *mask; + + if (old_mask) { + long fs = __ffs64(old_mask); + *bit += fs; + *mask >>= fs; + *mask >>= 1; + } + + return old_mask; +} + +#define for_each_bit_nr(mask, bit) \ + for (bit = 0; mask_iter(&mask, &bit); bit++) + static inline u64 lock_flag(enum lock_usage_bit bit) { return BIT_ULL(bit); -- 2.21.0
check_prev_add_irq() tests all incompatible scenarios one after the other while adding a lock (@next) to a tree dependency (@prev): LOCK_USED_IN_HARDIRQ vs LOCK_ENABLED_HARDIRQ LOCK_USED_IN_HARDIRQ_READ vs LOCK_ENABLED_HARDIRQ LOCK_USED_IN_SOFTIRQ vs LOCK_ENABLED_SOFTIRQ LOCK_USED_IN_SOFTIRQ_READ vs LOCK_ENABLED_SOFTIRQ Also for these four scenarios, we must at least iterate the @prev backward dependency. Then if it matches the relevant LOCK_USED_* bit, we must also iterate the @next forward dependency. Therefore in the best case we iterate 4 times, in the worst case 8 times. Now things are going to become much worse as we plan to add as much LOCK_USED_IN_{$VEC}_SOFTIRQ[_READ] as we have softirq vectors, currently 10. So the expanded test would be at least 22 loops and at most 44. We can't really afford that. So we take a different approach to fix this: 1) Iterate through @prev backward dependencies and accumulate all the IRQ uses in a single mask. 2) Iterate through @next forward dependencies and try to find a lock whose usage is exclusive to the accumulated usages gathered in the previous step. If we find one (call it @lockA), we have found an incompatible use. 3) Iterate again through @prev backward dependency and find the lock whose usage matches @lockA in term of incompatibility. Call that lock @lockB. 4) Report the incompatible usages of @lockA and @lockB If no incompatible use is found, the verification never goes beyond step 2 which means at most two iterations. Reviewed-by: David S. Miller <davem@davemloft.net> Signed-off-by: Frederic Weisbecker <frederic@kernel.org> Cc: Mauro Carvalho Chehab <mchehab+samsung@kernel.org> Cc: Joel Fernandes <joel@joelfernandes.org> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Pavan Kondeti <pkondeti@codeaurora.org> Cc: Paul E . McKenney <paulmck@linux.vnet.ibm.com> Cc: David S . Miller <davem@davemloft.net> Cc: Ingo Molnar <mingo@kernel.org> Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Cc: Linus Torvalds <torvalds@linux-foundation.org> Cc: Peter Zijlstra <peterz@infradead.org> --- kernel/locking/lockdep.c | 193 +++++++++++++++++++++++++-------------- 1 file changed, 126 insertions(+), 67 deletions(-) diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c index 1a335176cb61..ac1efd16f3e7 100644 --- a/kernel/locking/lockdep.c +++ b/kernel/locking/lockdep.c @@ -1351,6 +1351,14 @@ check_redundant(struct lock_list *root, struct lock_class *target, } #if defined(CONFIG_TRACE_IRQFLAGS) && defined(CONFIG_PROVE_LOCKING) + +static inline int usage_accumulate(struct lock_list *entry, void *mask) +{ + *(u64 *)mask |= entry->class->usage_mask; + + return 0; +} + /* * Forwards and backwards subgraph searching, for the purposes of * proving that two subgraphs can be connected by a new dependency @@ -1362,8 +1370,6 @@ static inline int usage_match(struct lock_list *entry, void *mask) return entry->class->usage_mask & *(u64 *)mask; } - - /* * Find a node in the forwards-direction dependency sub-graph starting * at @root->class that matches @bit. @@ -1597,39 +1603,6 @@ print_bad_irq_dependency(struct task_struct *curr, return 0; } -static int -check_usage(struct task_struct *curr, struct held_lock *prev, - struct held_lock *next, enum lock_usage_bit bit_backwards, - enum lock_usage_bit bit_forwards, const char *irqclass) -{ - int ret; - struct lock_list this, that; - struct lock_list *uninitialized_var(target_entry); - struct lock_list *uninitialized_var(target_entry1); - - this.parent = NULL; - - this.class = hlock_class(prev); - ret = find_usage_backwards(&this, BIT(bit_backwards), &target_entry); - if (ret < 0) - return print_bfs_bug(ret); - if (ret == 1) - return ret; - - that.parent = NULL; - that.class = hlock_class(next); - ret = find_usage_forwards(&that, BIT(bit_forwards), &target_entry1); - if (ret < 0) - return print_bfs_bug(ret); - if (ret == 1) - return ret; - - return print_bad_irq_dependency(curr, &this, &that, - target_entry, target_entry1, - prev, next, - bit_backwards, bit_forwards, irqclass); -} - static const char *state_names[] = { #define LOCKDEP_STATE(__STATE) \ __stringify(__STATE), @@ -1660,45 +1633,132 @@ static int exclusive_bit(int new_bit) return state | (dir ^ LOCK_USAGE_DIR_MASK); } +static u64 __invert_mask(u64 mask, bool read) +{ + u64 excl_mask = 0; + int bit; + + for_each_bit_nr(mask, bit) { + int excl = exclusive_bit(bit); + excl_mask |= lock_flag(excl); + if (read) + excl_mask |= lock_flag(excl | LOCK_USAGE_READ_MASK); + } + + return excl_mask; +} + +static u64 exclusive_mask(u64 mask) +{ + return __invert_mask(mask, false); +} + +/* + * Retrieve the _possible_ original mask to which @mask is + * exclusive. Ie: this is the opposite of exclusive_mask(). + * Note that 2 possible original bits can match an exclusive + * bit: one has LOCK_USAGE_READ_MASK set, the other has it + * cleared. So both are returned for each exclusive bit. + */ +static u64 original_mask(u64 mask) +{ + return __invert_mask(mask, true); +} + +/* + * Find the first pair of bit match between an original + * usage mask and an exclusive usage mask. + */ +static int find_exclusive_match(u64 mask, u64 excl_mask, + enum lock_usage_bit *bit, + enum lock_usage_bit *excl_bit) +{ + int nr; + + for_each_bit_nr(mask, nr) { + int excl = exclusive_bit(nr); + if (excl_mask & lock_flag(excl)) { + *bit = nr; + *excl_bit = excl; + return 0; + } + } + return -1; +} + +/* + * Prove that the new dependency does not connect a hardirq-safe(-read) + * lock with a hardirq-unsafe lock - to achieve this we search + * the backwards-subgraph starting at <prev>, and the + * forwards-subgraph starting at <next>: + */ static int check_irq_usage(struct task_struct *curr, struct held_lock *prev, - struct held_lock *next, enum lock_usage_bit bit) + struct held_lock *next) { + enum lock_usage_bit forward_bit = 0, backward_bit = 0; + struct lock_list *uninitialized_var(target_entry1); + struct lock_list *uninitialized_var(target_entry); + u64 usage_mask = 0, forward_mask, backward_mask; + struct lock_list this, that; + int ret; + /* - * Prove that the new dependency does not connect a hardirq-safe - * lock with a hardirq-unsafe lock - to achieve this we search - * the backwards-subgraph starting at <prev>, and the - * forwards-subgraph starting at <next>: + * Step 1: gather all hard/soft IRQs usages backward in an + * accumulated usage mask. */ - if (!check_usage(curr, prev, next, bit, - exclusive_bit(bit), state_name(bit))) - return 0; + this.parent = NULL; + this.class = hlock_class(prev); + + ret = __bfs_backwards(&this, &usage_mask, usage_accumulate, NULL); + if (ret < 0) + return print_bfs_bug(ret); - bit++; /* _READ */ + usage_mask &= LOCKF_USED_IN_IRQ | LOCKF_USED_IN_IRQ_READ; + if (!usage_mask) + return 1; /* - * Prove that the new dependency does not connect a hardirq-safe-read - * lock with a hardirq-unsafe lock - to achieve this we search - * the backwards-subgraph starting at <prev>, and the - * forwards-subgraph starting at <next>: + * Step 2: find exclusive uses forward that match the previous + * backward accumulated mask. */ - if (!check_usage(curr, prev, next, bit, - exclusive_bit(bit), state_name(bit))) - return 0; + forward_mask = exclusive_mask(usage_mask); - return 1; -} + that.parent = NULL; + that.class = hlock_class(next); -static int -check_prev_add_irq(struct task_struct *curr, struct held_lock *prev, - struct held_lock *next) -{ -#define LOCKDEP_STATE(__STATE) \ - if (!check_irq_usage(curr, prev, next, LOCK_USED_IN_##__STATE)) \ - return 0; -#include "lockdep_states.h" -#undef LOCKDEP_STATE + ret = find_usage_forwards(&that, forward_mask, &target_entry1); + if (ret < 0) + return print_bfs_bug(ret); + if (ret == 1) + return ret; + + /* + * Step 3: we found a bad match! Now retrieve a lock from the backward + * list whose usage mask matches the exclusive usage mask from the + * lock found on the forward list. + */ + backward_mask = original_mask(target_entry1->class->usage_mask); + + ret = find_usage_backwards(&this, backward_mask, &target_entry); + if (ret < 0) + return print_bfs_bug(ret); + if (ret == 1) + return print_bfs_bug(ret); + + /* + * Step 4: narrow down to a pair of incompatible usage bits + * and report it. + */ + ret = find_exclusive_match(target_entry->class->usage_mask, + target_entry1->class->usage_mask, + &backward_bit, &forward_bit); + WARN_ON_ONCE(ret == -1); - return 1; + return print_bad_irq_dependency(curr, &this, &that, + target_entry, target_entry1, + prev, next, + backward_bit, forward_bit, + state_name(backward_bit)); } static void inc_chains(void) @@ -1715,9 +1775,8 @@ static void inc_chains(void) #else -static inline int -check_prev_add_irq(struct task_struct *curr, struct held_lock *prev, - struct held_lock *next) +static inline int check_irq_usage(struct task_struct *curr, + struct held_lock *prev, struct held_lock *next) { return 1; } @@ -1879,7 +1938,7 @@ check_prev_add(struct task_struct *curr, struct held_lock *prev, else if (unlikely(ret < 0)) return print_bfs_bug(ret); - if (!check_prev_add_irq(curr, prev, next)) + if (!check_irq_usage(curr, prev, next)) return 0; /* -- 2.21.0
mark_lock_irq() is going to deal with lock usages that gather multiple softirq vectors at once. Therefore the validation through valid_state() will need to handle expanded usage masks. So enhance valid_state() to that purpose. Reviewed-by: David S. Miller <davem@davemloft.net> Signed-off-by: Frederic Weisbecker <frederic@kernel.org> Cc: Mauro Carvalho Chehab <mchehab+samsung@kernel.org> Cc: Joel Fernandes <joel@joelfernandes.org> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Pavan Kondeti <pkondeti@codeaurora.org> Cc: Paul E . McKenney <paulmck@linux.vnet.ibm.com> Cc: David S . Miller <davem@davemloft.net> Cc: Ingo Molnar <mingo@kernel.org> Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Cc: Linus Torvalds <torvalds@linux-foundation.org> Cc: Peter Zijlstra <peterz@infradead.org> --- kernel/locking/lockdep.c | 39 ++++++++++++++++++++++++++++++++++----- 1 file changed, 34 insertions(+), 5 deletions(-) diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c index ac1efd16f3e7..2321b5e16cdf 100644 --- a/kernel/locking/lockdep.c +++ b/kernel/locking/lockdep.c @@ -487,7 +487,21 @@ static inline u64 lock_flag(enum lock_usage_bit bit) static u64 lock_usage_mask(struct lock_usage *usage) { - return lock_flag(usage->bit); + u64 vectors = usage->vector; + u64 mask = 0ULL; + int nr; + + if (!vectors) + return lock_flag(usage->bit); + + /* Only softirqs can have non-zero vectors */ + WARN_ON_ONCE(usage->bit < LOCK_USED_IN_SOFTIRQ || + usage->bit > LOCK_ENABLED_SOFTIRQ_READ); + + for_each_bit_nr(vectors, nr) + mask |= lock_flag(usage->bit) << (4 * nr); + + return mask; } static char get_usage_char(struct lock_class *class, enum lock_usage_bit bit) @@ -2558,10 +2572,23 @@ print_usage_bug(struct task_struct *curr, struct held_lock *this, */ static inline int valid_state(struct task_struct *curr, struct held_lock *this, - enum lock_usage_bit new_bit, enum lock_usage_bit bad_bit) + u64 new_mask, u64 bad_mask) { - if (unlikely(hlock_class(this)->usage_mask & lock_flag(bad_bit))) + u64 bad_intersec; + + bad_intersec = hlock_class(this)->usage_mask & bad_mask; + + if (unlikely(bad_intersec)) { + enum lock_usage_bit new_bit, bad_bit; + int err; + + err = find_exclusive_match(new_mask, + bad_intersec, &new_bit, &bad_bit); + if (WARN_ON_ONCE(err < 0)) + return err; + return print_usage_bug(curr, this, bad_bit, new_bit); + } return 1; } @@ -2753,7 +2780,8 @@ mark_lock_irq(struct task_struct *curr, struct held_lock *this, * Validate that this particular lock does not have conflicting * usage states. */ - if (!valid_state(curr, this, new_usage->bit, excl_usage.bit)) + if (!valid_state(curr, this, lock_usage_mask(new_usage), + lock_usage_mask(&excl_usage))) return 0; /* @@ -2769,7 +2797,8 @@ mark_lock_irq(struct task_struct *curr, struct held_lock *this, */ if (!read) { excl_usage.bit += LOCK_USAGE_READ_MASK; - if (!valid_state(curr, this, new_usage->bit, excl_usage.bit)) + if (!valid_state(curr, this, lock_usage_mask(new_usage), + lock_usage_mask(&excl_usage))) return 0; if (STRICT_READ_CHECKS && -- 2.21.0
mark_lock_irq() is going to deal with lock usages that gather multiple softirq vectors at once. Therefore the validation through check_usage_backwards() and check_usage_forwards() will need to handle expanded usage masks. So enhance those functions to that purpose. Reviewed-by: David S. Miller <davem@davemloft.net> Signed-off-by: Frederic Weisbecker <frederic@kernel.org> Cc: Mauro Carvalho Chehab <mchehab+samsung@kernel.org> Cc: Joel Fernandes <joel@joelfernandes.org> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Pavan Kondeti <pkondeti@codeaurora.org> Cc: Paul E . McKenney <paulmck@linux.vnet.ibm.com> Cc: David S . Miller <davem@davemloft.net> Cc: Ingo Molnar <mingo@kernel.org> Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Cc: Linus Torvalds <torvalds@linux-foundation.org> Cc: Peter Zijlstra <peterz@infradead.org> --- kernel/locking/lockdep.c | 30 ++++++++++++++++++++++++------ 1 file changed, 24 insertions(+), 6 deletions(-) diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c index 2321b5e16cdf..d1bfe5eff708 100644 --- a/kernel/locking/lockdep.c +++ b/kernel/locking/lockdep.c @@ -2662,9 +2662,12 @@ print_irq_inversion_bug(struct task_struct *curr, */ static int check_usage_forwards(struct task_struct *curr, struct held_lock *this, - u64 usage_mask, const char *irqclass) + u64 usage_mask, bool read) { int ret; + u64 intersec; + int intersec_bit; + const char *irqclass; struct lock_list root; struct lock_list *uninitialized_var(target_entry); @@ -2676,8 +2679,14 @@ check_usage_forwards(struct task_struct *curr, struct held_lock *this, if (ret == 1) return ret; + intersec = usage_mask & target_entry->class->usage_mask; + intersec_bit = __ffs64(intersec); + if (read) + intersec_bit += LOCK_USAGE_READ_MASK; + irqclass = state_name(intersec_bit); + return print_irq_inversion_bug(curr, &root, target_entry, - this, 1, irqclass); + this, 1, irqclass); } /* @@ -2686,9 +2695,12 @@ check_usage_forwards(struct task_struct *curr, struct held_lock *this, */ static int check_usage_backwards(struct task_struct *curr, struct held_lock *this, - u64 usage_mask, const char *irqclass) + u64 usage_mask, bool read) { int ret; + u64 intersec; + int intersec_bit; + const char *irqclass; struct lock_list root; struct lock_list *uninitialized_var(target_entry); @@ -2700,6 +2712,12 @@ check_usage_backwards(struct task_struct *curr, struct held_lock *this, if (ret == 1) return ret; + intersec = usage_mask & target_entry->class->usage_mask; + intersec_bit = __ffs64(intersec); + if (read) + intersec_bit += LOCK_USAGE_READ_MASK; + irqclass = state_name(intersec_bit); + return print_irq_inversion_bug(curr, &root, target_entry, this, 0, irqclass); } @@ -2753,7 +2771,7 @@ static inline int state_verbose(enum lock_usage_bit bit, } typedef int (*check_usage_f)(struct task_struct *, struct held_lock *, - u64 usage_mask, const char *name); + u64 usage_mask, bool read); static int mark_lock_irq(struct task_struct *curr, struct held_lock *this, @@ -2789,7 +2807,7 @@ mark_lock_irq(struct task_struct *curr, struct held_lock *this, * states. */ if ((!read || !dir || STRICT_READ_CHECKS) && - !usage(curr, this, lock_usage_mask(&excl_usage), state_name(new_usage->bit & ~LOCK_USAGE_READ_MASK))) + !usage(curr, this, lock_usage_mask(&excl_usage), false)) return 0; /* @@ -2803,7 +2821,7 @@ mark_lock_irq(struct task_struct *curr, struct held_lock *this, if (STRICT_READ_CHECKS && !usage(curr, this, lock_usage_mask(&excl_usage), - state_name(new_usage->bit + LOCK_USAGE_READ_MASK))) + true)) return 0; } -- 2.21.0
Loosely handle all the softirq vectors verbosity under the same function. Reviewed-by: David S. Miller <davem@davemloft.net> Signed-off-by: Frederic Weisbecker <frederic@kernel.org> Cc: Mauro Carvalho Chehab <mchehab+samsung@kernel.org> Cc: Joel Fernandes <joel@joelfernandes.org> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Pavan Kondeti <pkondeti@codeaurora.org> Cc: Paul E . McKenney <paulmck@linux.vnet.ibm.com> Cc: David S . Miller <davem@davemloft.net> Cc: Ingo Molnar <mingo@kernel.org> Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Cc: Linus Torvalds <torvalds@linux-foundation.org> Cc: Peter Zijlstra <peterz@infradead.org> --- kernel/locking/lockdep.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c index d1bfe5eff708..0988de06a7ed 100644 --- a/kernel/locking/lockdep.c +++ b/kernel/locking/lockdep.c @@ -2757,17 +2757,13 @@ static int SOFTIRQ_verbose(struct lock_class *class) #define STRICT_READ_CHECKS 1 -static int (*state_verbose_f[])(struct lock_class *class) = { -#define LOCKDEP_STATE(__STATE) \ - __STATE##_verbose, -#include "lockdep_states.h" -#undef LOCKDEP_STATE -}; - static inline int state_verbose(enum lock_usage_bit bit, struct lock_class *class) { - return state_verbose_f[bit >> 2](class); + if (bit < LOCK_USED_IN_SOFTIRQ) + return HARDIRQ_verbose(class); + else + return SOFTIRQ_verbose(class); } typedef int (*check_usage_f)(struct task_struct *, struct held_lock *, -- 2.21.0
Now that mark_lock() is going to handle multiple softirq vectors at once for a given lock usage, the current fast path optimization that simply check if the new bit usage is already present won't work anymore. Indeed if the new usage is only partially present, such as for some softirq vectors and not for others, we may spuriously ignore all the verifications for the new vectors. What we must check instead is a bit different: we have to make sure that the new usage with all its vectors are entirely present in the current usage mask before ignoring further checks. Reviewed-by: David S. Miller <davem@davemloft.net> Signed-off-by: Frederic Weisbecker <frederic@kernel.org> Cc: Mauro Carvalho Chehab <mchehab+samsung@kernel.org> Cc: Joel Fernandes <joel@joelfernandes.org> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Pavan Kondeti <pkondeti@codeaurora.org> Cc: Paul E . McKenney <paulmck@linux.vnet.ibm.com> Cc: David S . Miller <davem@davemloft.net> Cc: Ingo Molnar <mingo@kernel.org> Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Cc: Linus Torvalds <torvalds@linux-foundation.org> Cc: Peter Zijlstra <peterz@infradead.org> --- kernel/locking/lockdep.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c index 0988de06a7ed..9a5f2dbc3812 100644 --- a/kernel/locking/lockdep.c +++ b/kernel/locking/lockdep.c @@ -3159,7 +3159,7 @@ static int mark_lock(struct task_struct *curr, struct held_lock *this, * If already set then do not dirty the cacheline, * nor do any checks: */ - if (likely(hlock_class(this)->usage_mask & new_mask)) + if (likely(!(new_mask & ~hlock_class(this)->usage_mask))) return 1; if (!graph_lock()) @@ -3167,7 +3167,7 @@ static int mark_lock(struct task_struct *curr, struct held_lock *this, /* * Make sure we didn't race: */ - if (unlikely(hlock_class(this)->usage_mask & new_mask)) { + if (unlikely(!(new_mask & ~hlock_class(this)->usage_mask))) { graph_unlock(); return 1; } -- 2.21.0
We are going to save as much traces as we have softirq vectors involved in a given usage. Expand the stack trace record code accordingly. Reviewed-by: David S. Miller <davem@davemloft.net> Signed-off-by: Frederic Weisbecker <frederic@kernel.org> Cc: Mauro Carvalho Chehab <mchehab+samsung@kernel.org> Cc: Joel Fernandes <joel@joelfernandes.org> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Pavan Kondeti <pkondeti@codeaurora.org> Cc: Paul E . McKenney <paulmck@linux.vnet.ibm.com> Cc: David S . Miller <davem@davemloft.net> Cc: Ingo Molnar <mingo@kernel.org> Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Cc: Linus Torvalds <torvalds@linux-foundation.org> Cc: Peter Zijlstra <peterz@infradead.org> --- include/linux/lockdep.h | 3 ++- kernel/locking/lockdep.c | 14 +++++++++++++- 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h index 06669f20a30a..69d2dac3d821 100644 --- a/include/linux/lockdep.h +++ b/include/linux/lockdep.h @@ -31,8 +31,9 @@ extern int lock_stat; /* * We'd rather not expose kernel/lockdep_states.h this wide, but we do need * the total number of states... :-( + * 1 bit for LOCK_USED, 4 bits for hardirqs and 4 * NR_SOFTIRQS bits */ -#define XXX_LOCK_USAGE_STATES (1+2*4) +#define XXX_LOCK_USAGE_STATES (1 + (1 + 10) * 4) /* * NR_LOCKDEP_CACHING_CLASSES ... Number of classes diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c index 9a5f2dbc3812..a369e7de3ade 100644 --- a/kernel/locking/lockdep.c +++ b/kernel/locking/lockdep.c @@ -3147,6 +3147,18 @@ static inline int separate_irq_context(struct task_struct *curr, #endif /* defined(CONFIG_TRACE_IRQFLAGS) && defined(CONFIG_PROVE_LOCKING) */ +static int save_trace_mask(struct lock_class *class, u64 mask) +{ + int bit; + + + for_each_bit_nr(mask, bit) + if (!save_trace(class->usage_traces + bit)) + return -1; + + return 0; +} + /* * Mark a lock with a usage bit, and validate the state transition: */ @@ -3174,7 +3186,7 @@ static int mark_lock(struct task_struct *curr, struct held_lock *this, hlock_class(this)->usage_mask |= new_mask; - if (!save_trace(hlock_class(this)->usage_traces + new_usage->bit)) + if (save_trace_mask(hlock_class(this), new_mask) < 0) return 0; switch (new_usage->bit) { -- 2.21.0
Since mark_lock() may now be passed more than one usage at once, make sure to expand the lock usage structure in order to report all of them while in verbose mode logging. Signed-off-by: Frederic Weisbecker <frederic@kernel.org> Cc: Mauro Carvalho Chehab <mchehab+samsung@kernel.org> Cc: Joel Fernandes <joel@joelfernandes.org> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Pavan Kondeti <pkondeti@codeaurora.org> Cc: Paul E . McKenney <paulmck@linux.vnet.ibm.com> Cc: David S . Miller <davem@davemloft.net> Cc: Ingo Molnar <mingo@kernel.org> Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Cc: Linus Torvalds <torvalds@linux-foundation.org> Cc: Peter Zijlstra <peterz@infradead.org> --- kernel/locking/lockdep.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c index a369e7de3ade..a99fd5fade54 100644 --- a/kernel/locking/lockdep.c +++ b/kernel/locking/lockdep.c @@ -3217,7 +3217,11 @@ static int mark_lock(struct task_struct *curr, struct held_lock *this, * We must printk outside of the graph_lock: */ if (ret == 2) { - printk("\nmarked lock as {%s}:\n", usage_str[new_usage->bit]); + int bit; + + for_each_bit_nr(new_mask, bit) + printk("\nmarked lock as {%s}:\n", usage_str[bit]); + print_lock(this); print_irqtrace_events(curr); dump_stack(); -- 2.21.0
Define the softirq vectors through macros so that we can later include them in the automated definition of lockdep usage states. Reviewed-by: David S. Miller <davem@davemloft.net> Signed-off-by: Frederic Weisbecker <frederic@kernel.org> Cc: Mauro Carvalho Chehab <mchehab+samsung@kernel.org> Cc: Joel Fernandes <joel@joelfernandes.org> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Pavan Kondeti <pkondeti@codeaurora.org> Cc: Paul E . McKenney <paulmck@linux.vnet.ibm.com> Cc: David S . Miller <davem@davemloft.net> Cc: Ingo Molnar <mingo@kernel.org> Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Cc: Linus Torvalds <torvalds@linux-foundation.org> Cc: Peter Zijlstra <peterz@infradead.org> --- include/linux/interrupt.h | 17 ++++------------- include/linux/softirq_vector.h | 12 ++++++++++++ 2 files changed, 16 insertions(+), 13 deletions(-) create mode 100644 include/linux/softirq_vector.h diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h index c672f34235e7..e871f361f1f1 100644 --- a/include/linux/interrupt.h +++ b/include/linux/interrupt.h @@ -474,21 +474,12 @@ extern bool force_irqthreads; tasklets are more than enough. F.e. all serial device BHs et al. should be converted to tasklets, not to softirqs. */ - enum { - HI_SOFTIRQ=0, - TIMER_SOFTIRQ, - NET_TX_SOFTIRQ, - NET_RX_SOFTIRQ, - BLOCK_SOFTIRQ, - IRQ_POLL_SOFTIRQ, - TASKLET_SOFTIRQ, - SCHED_SOFTIRQ, - HRTIMER_SOFTIRQ, /* Unused, but kept as tools rely on the - numbering. Sigh! */ - RCU_SOFTIRQ, /* Preferable RCU should always be the last softirq */ - +#define SOFTIRQ_VECTOR(__SVEC) \ + __SVEC##_SOFTIRQ, +#include <linux/softirq_vector.h> +#undef SOFTIRQ_VECTOR NR_SOFTIRQS }; diff --git a/include/linux/softirq_vector.h b/include/linux/softirq_vector.h new file mode 100644 index 000000000000..c0308cbc916f --- /dev/null +++ b/include/linux/softirq_vector.h @@ -0,0 +1,12 @@ +/* SPDX-License-Identifier: GPL-2.0 */ + +SOFTIRQ_VECTOR(HI) +SOFTIRQ_VECTOR(TIMER) +SOFTIRQ_VECTOR(NET_TX) +SOFTIRQ_VECTOR(NET_RX) +SOFTIRQ_VECTOR(BLOCK) +SOFTIRQ_VECTOR(IRQ_POLL) +SOFTIRQ_VECTOR(TASKLET) +SOFTIRQ_VECTOR(SCHED) +SOFTIRQ_VECTOR(HRTIMER) +SOFTIRQ_VECTOR(RCU) /* Preferable RCU should always be the last softirq */ -- 2.21.0
Now that the code is mostly ready to play at this finegrained level, reuse the macrofied vector list to define the usual 4 lock usage states for each softirq vectors. This way we can perform the validations independently for each of them. The new usage mask layout becomes: TIMER_SOFTIRQ LOCK_USED bits HARDIRQ bits \ | | \ | | 0 0000 [...] 0000 0000 0000 | | | | RCU_SOFTIRQ HI_SOFTIRQ bits bits Reviewed-by: David S. Miller <davem@davemloft.net> Signed-off-by: Frederic Weisbecker <frederic@kernel.org> Cc: Mauro Carvalho Chehab <mchehab+samsung@kernel.org> Cc: Joel Fernandes <joel@joelfernandes.org> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Pavan Kondeti <pkondeti@codeaurora.org> Cc: Paul E . McKenney <paulmck@linux.vnet.ibm.com> Cc: David S . Miller <davem@davemloft.net> Cc: Ingo Molnar <mingo@kernel.org> Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Cc: Linus Torvalds <torvalds@linux-foundation.org> Cc: Peter Zijlstra <peterz@infradead.org> --- kernel/locking/lockdep_internals.h | 43 +++++++++++++++++++++++++++--- kernel/locking/lockdep_states.h | 4 ++- 2 files changed, 42 insertions(+), 5 deletions(-) diff --git a/kernel/locking/lockdep_internals.h b/kernel/locking/lockdep_internals.h index e714c823f594..4b0c03f0f7ce 100644 --- a/kernel/locking/lockdep_internals.h +++ b/kernel/locking/lockdep_internals.h @@ -22,9 +22,15 @@ enum lock_usage_bit { LOCK_USAGE_STATES }; -#define LOCK_USAGE_READ_MASK 1 -#define LOCK_USAGE_DIR_MASK 2 -#define LOCK_USAGE_STATE_MASK (~(LOCK_USAGE_READ_MASK | LOCK_USAGE_DIR_MASK)) +#define LOCK_USAGE_READ_MASK 1 +#define LOCK_USAGE_DIR_MASK 2 +#define LOCK_USAGE_STATE_MASK (~(LOCK_USAGE_READ_MASK | LOCK_USAGE_DIR_MASK)) + +/* Base softirq bits that get shifted by the index in stuct lock_usage::vector */ +#define LOCK_USED_IN_SOFTIRQ LOCK_USED_IN_HI_SOFTIRQ +#define LOCK_USED_IN_SOFTIRQ_READ LOCK_USED_IN_HI_SOFTIRQ_READ +#define LOCK_ENABLED_SOFTIRQ LOCK_ENABLED_HI_SOFTIRQ +#define LOCK_ENABLED_SOFTIRQ_READ LOCK_ENABLED_HI_SOFTIRQ_READ struct lock_usage { enum lock_usage_bit bit; @@ -35,7 +41,7 @@ struct lock_usage { /* * Usage-state bitmasks: */ -#define __LOCKF(__STATE) LOCKF_##__STATE = (1 << LOCK_##__STATE), +#define __LOCKF(__STATE) LOCKF_##__STATE = (1ULL << LOCK_##__STATE), enum { #define LOCKDEP_STATE(__STATE) \ @@ -48,11 +54,40 @@ enum { __LOCKF(USED) }; +#define LOCKF_ENABLED_SOFTIRQ \ + (LOCKF_ENABLED_HI_SOFTIRQ | LOCKF_ENABLED_TIMER_SOFTIRQ | \ + LOCKF_ENABLED_NET_TX_SOFTIRQ | LOCKF_ENABLED_NET_RX_SOFTIRQ | \ + LOCKF_ENABLED_BLOCK_SOFTIRQ | LOCKF_ENABLED_IRQ_POLL_SOFTIRQ | \ + LOCKF_ENABLED_TASKLET_SOFTIRQ | LOCKF_ENABLED_SCHED_SOFTIRQ | \ + LOCKF_ENABLED_HRTIMER_SOFTIRQ | LOCKF_ENABLED_RCU_SOFTIRQ) #define LOCKF_ENABLED_IRQ (LOCKF_ENABLED_HARDIRQ | LOCKF_ENABLED_SOFTIRQ) + + +#define LOCKF_USED_IN_SOFTIRQ \ + (LOCKF_USED_IN_HI_SOFTIRQ | LOCKF_USED_IN_TIMER_SOFTIRQ | \ + LOCKF_USED_IN_NET_TX_SOFTIRQ | LOCKF_USED_IN_NET_RX_SOFTIRQ | \ + LOCKF_USED_IN_BLOCK_SOFTIRQ | LOCKF_USED_IN_IRQ_POLL_SOFTIRQ | \ + LOCKF_USED_IN_TASKLET_SOFTIRQ | LOCKF_USED_IN_SCHED_SOFTIRQ | \ + LOCKF_USED_IN_HRTIMER_SOFTIRQ | LOCKF_USED_IN_RCU_SOFTIRQ) #define LOCKF_USED_IN_IRQ (LOCKF_USED_IN_HARDIRQ | LOCKF_USED_IN_SOFTIRQ) + +#define LOCKF_ENABLED_SOFTIRQ_READ \ + (LOCKF_ENABLED_HI_SOFTIRQ_READ | LOCKF_ENABLED_TIMER_SOFTIRQ_READ | \ + LOCKF_ENABLED_NET_TX_SOFTIRQ_READ | LOCKF_ENABLED_NET_RX_SOFTIRQ_READ | \ + LOCKF_ENABLED_BLOCK_SOFTIRQ_READ | LOCKF_ENABLED_IRQ_POLL_SOFTIRQ_READ | \ + LOCKF_ENABLED_TASKLET_SOFTIRQ_READ | LOCKF_ENABLED_SCHED_SOFTIRQ_READ | \ + LOCKF_ENABLED_HRTIMER_SOFTIRQ_READ | LOCKF_ENABLED_RCU_SOFTIRQ_READ) #define LOCKF_ENABLED_IRQ_READ \ (LOCKF_ENABLED_HARDIRQ_READ | LOCKF_ENABLED_SOFTIRQ_READ) + + +#define LOCKF_USED_IN_SOFTIRQ_READ \ + (LOCKF_USED_IN_HI_SOFTIRQ_READ | LOCKF_USED_IN_TIMER_SOFTIRQ_READ | \ + LOCKF_USED_IN_NET_TX_SOFTIRQ_READ | LOCKF_USED_IN_NET_RX_SOFTIRQ_READ | \ + LOCKF_USED_IN_BLOCK_SOFTIRQ_READ | LOCKF_USED_IN_IRQ_POLL_SOFTIRQ_READ | \ + LOCKF_USED_IN_TASKLET_SOFTIRQ_READ | LOCKF_USED_IN_SCHED_SOFTIRQ_READ | \ + LOCKF_USED_IN_HRTIMER_SOFTIRQ_READ | LOCKF_USED_IN_RCU_SOFTIRQ_READ) #define LOCKF_USED_IN_IRQ_READ \ (LOCKF_USED_IN_HARDIRQ_READ | LOCKF_USED_IN_SOFTIRQ_READ) diff --git a/kernel/locking/lockdep_states.h b/kernel/locking/lockdep_states.h index 35ca09f2ed0b..34ff569d2024 100644 --- a/kernel/locking/lockdep_states.h +++ b/kernel/locking/lockdep_states.h @@ -5,4 +5,6 @@ * you add one, or come up with a nice dynamic solution. */ LOCKDEP_STATE(HARDIRQ) -LOCKDEP_STATE(SOFTIRQ) +#define SOFTIRQ_VECTOR(__SVEC) LOCKDEP_STATE(__SVEC##_SOFTIRQ) +#include <linux/softirq_vector.h> +#undef SOFTIRQ_VECTOR -- 2.21.0
Pass the softirq vector number to lockdep on callback execution so that we know which one is involved while holding a lock. We will then be able to pick up the proper LOCK_USED_IN_*_SOFTIRQ index to perform the finegrained verifications. Reviewed-by: David S. Miller <davem@davemloft.net> Signed-off-by: Frederic Weisbecker <frederic@kernel.org> Cc: Mauro Carvalho Chehab <mchehab+samsung@kernel.org> Cc: Joel Fernandes <joel@joelfernandes.org> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Pavan Kondeti <pkondeti@codeaurora.org> Cc: Paul E . McKenney <paulmck@linux.vnet.ibm.com> Cc: David S . Miller <davem@davemloft.net> Cc: Ingo Molnar <mingo@kernel.org> Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Cc: Linus Torvalds <torvalds@linux-foundation.org> Cc: Peter Zijlstra <peterz@infradead.org> --- include/linux/irqflags.h | 12 ++++++------ kernel/softirq.c | 8 ++++---- lib/locking-selftest.c | 4 ++-- 3 files changed, 12 insertions(+), 12 deletions(-) diff --git a/include/linux/irqflags.h b/include/linux/irqflags.h index 21619c92c377..623030a74866 100644 --- a/include/linux/irqflags.h +++ b/include/linux/irqflags.h @@ -43,13 +43,13 @@ do { \ do { \ current->hardirq_context--; \ } while (0) -# define lockdep_softirq_enter() \ +# define lockdep_softirq_enter(__VEC) \ do { \ - current->softirq_context++; \ + current->softirq_context |= BIT(__VEC); \ } while (0) -# define lockdep_softirq_exit() \ +# define lockdep_softirq_exit(__VEC) \ do { \ - current->softirq_context--; \ + current->softirq_context &= ~BIT(__VEC);\ } while (0) #else # define trace_hardirqs_on() do { } while (0) @@ -60,8 +60,8 @@ do { \ # define trace_softirqs_enabled(p) 0 # define trace_hardirq_enter() do { } while (0) # define trace_hardirq_exit() do { } while (0) -# define lockdep_softirq_enter() do { } while (0) -# define lockdep_softirq_exit() do { } while (0) +# define lockdep_softirq_enter(vec) do { } while (0) +# define lockdep_softirq_exit(vec) do { } while (0) #endif #if defined(CONFIG_IRQSOFF_TRACER) || \ diff --git a/kernel/softirq.c b/kernel/softirq.c index d28813306b2c..2dcaef813acb 100644 --- a/kernel/softirq.c +++ b/kernel/softirq.c @@ -229,18 +229,15 @@ static inline bool lockdep_softirq_start(void) trace_hardirq_exit(); } - lockdep_softirq_enter(); - return in_hardirq; } static inline void lockdep_softirq_end(bool in_hardirq) { - lockdep_softirq_exit(); - if (in_hardirq) trace_hardirq_enter(); } + #else static inline bool lockdep_softirq_start(void) { return false; } static inline void lockdep_softirq_end(bool in_hardirq) { } @@ -288,9 +285,12 @@ asmlinkage __visible void __softirq_entry __do_softirq(void) kstat_incr_softirqs_this_cpu(vec_nr); + lockdep_softirq_enter(vec_nr); trace_softirq_entry(vec_nr); h->action(h); trace_softirq_exit(vec_nr); + lockdep_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", vec_nr, softirq_to_name[vec_nr], h->action, diff --git a/lib/locking-selftest.c b/lib/locking-selftest.c index 1e1bbf171eca..2bd782d7f2e5 100644 --- a/lib/locking-selftest.c +++ b/lib/locking-selftest.c @@ -197,11 +197,11 @@ static void init_shared_classes(void) #define SOFTIRQ_ENTER() \ local_bh_disable(); \ local_irq_disable(); \ - lockdep_softirq_enter(); \ + lockdep_softirq_enter(0); \ WARN_ON(!in_softirq()); #define SOFTIRQ_EXIT() \ - lockdep_softirq_exit(); \ + lockdep_softirq_exit(0); \ local_irq_enable(); \ local_bh_enable(); -- 2.21.0
This reverts commit 9aee5f8a7e30330d0a8f4c626dc924ca5590aba5. We are going to need the 16 high bits above in order to implement a softirq enable mask. x86 is the only architecture that doesn't use unsigned int to implement softirq_pending. Reviewed-by: David S. Miller <davem@davemloft.net> Signed-off-by: Frederic Weisbecker <frederic@kernel.org> Cc: Mauro Carvalho Chehab <mchehab+samsung@kernel.org> Cc: Joel Fernandes <joel@joelfernandes.org> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Pavan Kondeti <pkondeti@codeaurora.org> Cc: Paul E . McKenney <paulmck@linux.vnet.ibm.com> Cc: David S . Miller <davem@davemloft.net> Cc: Ingo Molnar <mingo@kernel.org> Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Cc: Linus Torvalds <torvalds@linux-foundation.org> Cc: Peter Zijlstra <peterz@infradead.org> --- arch/x86/include/asm/hardirq.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/x86/include/asm/hardirq.h b/arch/x86/include/asm/hardirq.h index d9069bb26c7f..a8e8e126b421 100644 --- a/arch/x86/include/asm/hardirq.h +++ b/arch/x86/include/asm/hardirq.h @@ -5,7 +5,7 @@ #include <linux/threads.h> typedef struct { - u16 __softirq_pending; + unsigned int __softirq_pending; #if IS_ENABLED(CONFIG_KVM_INTEL) u8 kvm_cpu_l1tf_flush_l1d; #endif -- 2.21.0
We are going to extend the softirq bits with an enabled vector mask. Provide the field with a more generic name to later layout the pending states on the lower bits and the enabled states on the higher bits. Reviewed-by: David S. Miller <davem@davemloft.net> Signed-off-by: Frederic Weisbecker <frederic@kernel.org> Cc: Mauro Carvalho Chehab <mchehab+samsung@kernel.org> Cc: Joel Fernandes <joel@joelfernandes.org> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Pavan Kondeti <pkondeti@codeaurora.org> Cc: Paul E . McKenney <paulmck@linux.vnet.ibm.com> Cc: David S . Miller <davem@davemloft.net> Cc: Ingo Molnar <mingo@kernel.org> Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Cc: Linus Torvalds <torvalds@linux-foundation.org> Cc: Peter Zijlstra <peterz@infradead.org> --- arch/arm/include/asm/hardirq.h | 2 +- arch/arm64/include/asm/hardirq.h | 2 +- arch/h8300/kernel/asm-offsets.c | 2 +- arch/ia64/include/asm/hardirq.h | 2 +- arch/ia64/include/asm/processor.h | 2 +- arch/m68k/include/asm/hardirq.h | 2 +- arch/m68k/kernel/asm-offsets.c | 2 +- arch/parisc/include/asm/hardirq.h | 2 +- arch/powerpc/include/asm/hardirq.h | 2 +- arch/s390/include/asm/hardirq.h | 6 +++--- arch/s390/include/asm/lowcore.h | 2 +- arch/sh/include/asm/hardirq.h | 2 +- arch/sparc/include/asm/cpudata_64.h | 2 +- arch/sparc/include/asm/hardirq_64.h | 4 ++-- arch/um/include/asm/hardirq.h | 2 +- arch/x86/include/asm/hardirq.h | 2 +- include/asm-generic/hardirq.h | 2 +- include/linux/interrupt.h | 10 +++++----- 18 files changed, 25 insertions(+), 25 deletions(-) diff --git a/arch/arm/include/asm/hardirq.h b/arch/arm/include/asm/hardirq.h index cba23eaa6072..e5b06dd41b88 100644 --- a/arch/arm/include/asm/hardirq.h +++ b/arch/arm/include/asm/hardirq.h @@ -9,7 +9,7 @@ #define NR_IPI 7 typedef struct { - unsigned int __softirq_pending; + unsigned int __softirq_data; #ifdef CONFIG_SMP unsigned int ipi_irqs[NR_IPI]; #endif diff --git a/arch/arm64/include/asm/hardirq.h b/arch/arm64/include/asm/hardirq.h index 1473fc2f7ab7..e9add887e2f2 100644 --- a/arch/arm64/include/asm/hardirq.h +++ b/arch/arm64/include/asm/hardirq.h @@ -23,7 +23,7 @@ #define NR_IPI 7 typedef struct { - unsigned int __softirq_pending; + unsigned int __softirq_data; unsigned int ipi_irqs[NR_IPI]; } ____cacheline_aligned irq_cpustat_t; diff --git a/arch/h8300/kernel/asm-offsets.c b/arch/h8300/kernel/asm-offsets.c index 85e60509f0a8..719d4cff704e 100644 --- a/arch/h8300/kernel/asm-offsets.c +++ b/arch/h8300/kernel/asm-offsets.c @@ -32,7 +32,7 @@ int main(void) /* offsets into the irq_cpustat_t struct */ DEFINE(CPUSTAT_SOFTIRQ_PENDING, offsetof(irq_cpustat_t, - __softirq_pending)); + __softirq_data)); /* offsets into the thread struct */ OFFSET(THREAD_KSP, thread_struct, ksp); diff --git a/arch/ia64/include/asm/hardirq.h b/arch/ia64/include/asm/hardirq.h index ccde7c2ba00f..004f6093a11a 100644 --- a/arch/ia64/include/asm/hardirq.h +++ b/arch/ia64/include/asm/hardirq.h @@ -13,7 +13,7 @@ #define __ARCH_IRQ_STAT 1 -#define local_softirq_pending_ref ia64_cpu_info.softirq_pending +#define local_softirq_data_ref ia64_cpu_info.softirq_data #include <linux/threads.h> #include <linux/irq.h> diff --git a/arch/ia64/include/asm/processor.h b/arch/ia64/include/asm/processor.h index c91ef98ed6bf..72a70c42b701 100644 --- a/arch/ia64/include/asm/processor.h +++ b/arch/ia64/include/asm/processor.h @@ -188,7 +188,7 @@ union ia64_rr { * state comes earlier: */ struct cpuinfo_ia64 { - unsigned int softirq_pending; + unsigned int softirq_data; unsigned long itm_delta; /* # of clock cycles between clock ticks */ unsigned long itm_next; /* interval timer mask value to use for next clock tick */ unsigned long nsec_per_cyc; /* (1000000000<<IA64_NSEC_PER_CYC_SHIFT)/itc_freq */ diff --git a/arch/m68k/include/asm/hardirq.h b/arch/m68k/include/asm/hardirq.h index 11793165445d..6ad364cc3799 100644 --- a/arch/m68k/include/asm/hardirq.h +++ b/arch/m68k/include/asm/hardirq.h @@ -15,7 +15,7 @@ static inline void ack_bad_irq(unsigned int irq) /* entry.S is sensitive to the offsets of these fields */ typedef struct { - unsigned int __softirq_pending; + unsigned int __softirq_data; } ____cacheline_aligned irq_cpustat_t; #include <linux/irq_cpustat.h> /* Standard mappings for irq_cpustat_t above */ diff --git a/arch/m68k/kernel/asm-offsets.c b/arch/m68k/kernel/asm-offsets.c index ccea355052ef..93b6bea52c8b 100644 --- a/arch/m68k/kernel/asm-offsets.c +++ b/arch/m68k/kernel/asm-offsets.c @@ -64,7 +64,7 @@ int main(void) #endif /* offsets into the irq_cpustat_t struct */ - DEFINE(CPUSTAT_SOFTIRQ_PENDING, offsetof(irq_cpustat_t, __softirq_pending)); + DEFINE(CPUSTAT_SOFTIRQ_PENDING, offsetof(irq_cpustat_t, __softirq_data)); /* signal defines */ DEFINE(LSIGSEGV, SIGSEGV); diff --git a/arch/parisc/include/asm/hardirq.h b/arch/parisc/include/asm/hardirq.h index 1a1235a9d533..28d8ceeab077 100644 --- a/arch/parisc/include/asm/hardirq.h +++ b/arch/parisc/include/asm/hardirq.h @@ -17,7 +17,7 @@ #endif typedef struct { - unsigned int __softirq_pending; + unsigned int __softirq_data; unsigned int kernel_stack_usage; unsigned int irq_stack_usage; #ifdef CONFIG_SMP diff --git a/arch/powerpc/include/asm/hardirq.h b/arch/powerpc/include/asm/hardirq.h index f1e9067bd5ac..d3a896b402da 100644 --- a/arch/powerpc/include/asm/hardirq.h +++ b/arch/powerpc/include/asm/hardirq.h @@ -6,7 +6,7 @@ #include <linux/irq.h> typedef struct { - unsigned int __softirq_pending; + unsigned int __softirq_data; unsigned int timer_irqs_event; unsigned int broadcast_irqs_event; unsigned int timer_irqs_others; diff --git a/arch/s390/include/asm/hardirq.h b/arch/s390/include/asm/hardirq.h index dfbc3c6c0674..e26325fe287d 100644 --- a/arch/s390/include/asm/hardirq.h +++ b/arch/s390/include/asm/hardirq.h @@ -13,9 +13,9 @@ #include <asm/lowcore.h> -#define local_softirq_pending() (S390_lowcore.softirq_pending) -#define set_softirq_pending(x) (S390_lowcore.softirq_pending = (x)) -#define or_softirq_pending(x) (S390_lowcore.softirq_pending |= (x)) +#define local_softirq_pending() (S390_lowcore.softirq_data) +#define set_softirq_pending(x) (S390_lowcore.softirq_data = (x)) +#define or_softirq_pending(x) (S390_lowcore.softirq_data |= (x)) #define __ARCH_IRQ_STAT #define __ARCH_HAS_DO_SOFTIRQ diff --git a/arch/s390/include/asm/lowcore.h b/arch/s390/include/asm/lowcore.h index cc0947e08b6f..0b6f519a38df 100644 --- a/arch/s390/include/asm/lowcore.h +++ b/arch/s390/include/asm/lowcore.h @@ -127,7 +127,7 @@ struct lowcore { /* SMP info area */ __u32 cpu_nr; /* 0x0398 */ - __u32 softirq_pending; /* 0x039c */ + __u32 softirq_data; /* 0x039c */ __u32 preempt_count; /* 0x03a0 */ __u32 spinlock_lockval; /* 0x03a4 */ __u32 spinlock_index; /* 0x03a8 */ diff --git a/arch/sh/include/asm/hardirq.h b/arch/sh/include/asm/hardirq.h index edaea3559a23..e364a63babd3 100644 --- a/arch/sh/include/asm/hardirq.h +++ b/arch/sh/include/asm/hardirq.h @@ -6,7 +6,7 @@ #include <linux/irq.h> typedef struct { - unsigned int __softirq_pending; + unsigned int __softirq_data; unsigned int __nmi_count; /* arch dependent */ } ____cacheline_aligned irq_cpustat_t; diff --git a/arch/sparc/include/asm/cpudata_64.h b/arch/sparc/include/asm/cpudata_64.h index 9c3fc03abe9a..9e52c6bf69b9 100644 --- a/arch/sparc/include/asm/cpudata_64.h +++ b/arch/sparc/include/asm/cpudata_64.h @@ -11,7 +11,7 @@ typedef struct { /* Dcache line 1 */ - unsigned int __softirq_pending; /* must be 1st, see rtrap.S */ + unsigned int __softirq_data; /* must be 1st, see rtrap.S */ unsigned int __nmi_count; unsigned long clock_tick; /* %tick's per second */ unsigned long __pad; diff --git a/arch/sparc/include/asm/hardirq_64.h b/arch/sparc/include/asm/hardirq_64.h index 75b92bfe04b5..8ff0458870e1 100644 --- a/arch/sparc/include/asm/hardirq_64.h +++ b/arch/sparc/include/asm/hardirq_64.h @@ -11,8 +11,8 @@ #define __ARCH_IRQ_STAT -#define local_softirq_pending_ref \ - __cpu_data.__softirq_pending +#define local_softirq_data_ref \ + __cpu_data.__softirq_data void ack_bad_irq(unsigned int irq); diff --git a/arch/um/include/asm/hardirq.h b/arch/um/include/asm/hardirq.h index b426796d26fd..96844938b92e 100644 --- a/arch/um/include/asm/hardirq.h +++ b/arch/um/include/asm/hardirq.h @@ -6,7 +6,7 @@ #include <linux/threads.h> typedef struct { - unsigned int __softirq_pending; + unsigned int __softirq_data; } ____cacheline_aligned irq_cpustat_t; #include <linux/irq_cpustat.h> /* Standard mappings for irq_cpustat_t above */ diff --git a/arch/x86/include/asm/hardirq.h b/arch/x86/include/asm/hardirq.h index a8e8e126b421..875f7de3b65b 100644 --- a/arch/x86/include/asm/hardirq.h +++ b/arch/x86/include/asm/hardirq.h @@ -5,7 +5,7 @@ #include <linux/threads.h> typedef struct { - unsigned int __softirq_pending; + unsigned int __softirq_data; #if IS_ENABLED(CONFIG_KVM_INTEL) u8 kvm_cpu_l1tf_flush_l1d; #endif diff --git a/include/asm-generic/hardirq.h b/include/asm-generic/hardirq.h index d14214dfc10b..4ea87b50a257 100644 --- a/include/asm-generic/hardirq.h +++ b/include/asm-generic/hardirq.h @@ -6,7 +6,7 @@ #include <linux/threads.h> typedef struct { - unsigned int __softirq_pending; + unsigned int __softirq_data; } ____cacheline_aligned irq_cpustat_t; #include <linux/irq_cpustat.h> /* Standard mappings for irq_cpustat_t above */ diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h index e871f361f1f1..a32bdd9bb103 100644 --- a/include/linux/interrupt.h +++ b/include/linux/interrupt.h @@ -449,13 +449,13 @@ extern bool force_irqthreads; #ifndef local_softirq_pending -#ifndef local_softirq_pending_ref -#define local_softirq_pending_ref irq_stat.__softirq_pending +#ifndef local_softirq_data_ref +#define local_softirq_data_ref irq_stat.__softirq_data #endif -#define local_softirq_pending() (__this_cpu_read(local_softirq_pending_ref)) -#define set_softirq_pending(x) (__this_cpu_write(local_softirq_pending_ref, (x))) -#define or_softirq_pending(x) (__this_cpu_or(local_softirq_pending_ref, (x))) +#define local_softirq_pending() (__this_cpu_read(local_softirq_data_ref)) +#define set_softirq_pending(x) (__this_cpu_write(local_softirq_data_ref, (x))) +#define or_softirq_pending(x) (__this_cpu_or(local_softirq_data_ref, (x))) #endif /* local_softirq_pending */ -- 2.21.0
From: Frederic Weisbecker <fweisbec@gmail.com> Use the subsystem as the prefix to name the __softirq_data accessors. They are going to be extended and want a more greppable and standard naming sheme. Reviewed-by: David S. Miller <davem@davemloft.net> Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com> Cc: Mauro Carvalho Chehab <mchehab+samsung@kernel.org> Cc: Joel Fernandes <joel@joelfernandes.org> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Pavan Kondeti <pkondeti@codeaurora.org> Cc: Paul E . McKenney <paulmck@linux.vnet.ibm.com> Cc: David S . Miller <davem@davemloft.net> Cc: Ingo Molnar <mingo@kernel.org> Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Cc: Linus Torvalds <torvalds@linux-foundation.org> Cc: Peter Zijlstra <peterz@infradead.org> --- arch/s390/include/asm/hardirq.h | 4 ++-- include/linux/interrupt.h | 24 ++++++++++++------------ kernel/softirq.c | 4 ++-- 3 files changed, 16 insertions(+), 16 deletions(-) diff --git a/arch/s390/include/asm/hardirq.h b/arch/s390/include/asm/hardirq.h index e26325fe287d..3103680487cf 100644 --- a/arch/s390/include/asm/hardirq.h +++ b/arch/s390/include/asm/hardirq.h @@ -14,8 +14,8 @@ #include <asm/lowcore.h> #define local_softirq_pending() (S390_lowcore.softirq_data) -#define set_softirq_pending(x) (S390_lowcore.softirq_data = (x)) -#define or_softirq_pending(x) (S390_lowcore.softirq_data |= (x)) +#define softirq_pending_set(x) (S390_lowcore.softirq_data = (x)) +#define softirq_pending_or(x) (S390_lowcore.softirq_data |= (x)) #define __ARCH_IRQ_STAT #define __ARCH_HAS_DO_SOFTIRQ diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h index a32bdd9bb103..a5dec926531b 100644 --- a/include/linux/interrupt.h +++ b/include/linux/interrupt.h @@ -447,18 +447,6 @@ extern bool force_irqthreads; #define force_irqthreads (0) #endif -#ifndef local_softirq_pending - -#ifndef local_softirq_data_ref -#define local_softirq_data_ref irq_stat.__softirq_data -#endif - -#define local_softirq_pending() (__this_cpu_read(local_softirq_data_ref)) -#define set_softirq_pending(x) (__this_cpu_write(local_softirq_data_ref, (x))) -#define or_softirq_pending(x) (__this_cpu_or(local_softirq_data_ref, (x))) - -#endif /* local_softirq_pending */ - /* Some architectures might implement lazy enabling/disabling of * interrupts. In some cases, such as stop_machine, we might want * to ensure that after a local_irq_disable(), interrupts have @@ -485,6 +473,18 @@ enum #define SOFTIRQ_STOP_IDLE_MASK (~(1 << RCU_SOFTIRQ)) +#ifndef local_softirq_pending + +#ifndef local_softirq_data_ref +#define local_softirq_data_ref irq_stat.__softirq_data +#endif + +#define local_softirq_pending() (__this_cpu_read(local_softirq_data_ref)) +#define softirq_pending_set(x) (__this_cpu_write(local_softirq_data_ref, (x))) +#define softirq_pending_or(x) (__this_cpu_or(local_softirq_data_ref, (x))) + +#endif /* local_softirq_pending */ + /* map softirq index to softirq name. update 'softirq_to_name' in * kernel/softirq.c when adding a new softirq. */ diff --git a/kernel/softirq.c b/kernel/softirq.c index 2dcaef813acb..2137c54baf98 100644 --- a/kernel/softirq.c +++ b/kernel/softirq.c @@ -268,7 +268,7 @@ asmlinkage __visible void __softirq_entry __do_softirq(void) restart: /* Reset the pending bitmask before enabling irqs */ - set_softirq_pending(0); + softirq_pending_set(0); local_irq_enable(); @@ -449,7 +449,7 @@ void raise_softirq(unsigned int nr) void __raise_softirq_irqoff(unsigned int nr) { trace_softirq_raise(nr); - or_softirq_pending(1UL << nr); + softirq_pending_or(1UL << nr); } void open_softirq(int nr, void (*action)(struct softirq_action *)) -- 2.21.0
We are going to need a nand version of softirq_pending_or() in order to clear specific bits from the pending mask. But instead of naming the mutators after their implementation, rather name them after their high level purpose. Suggested-by: Linus Torvalds <torvalds@linux-foundation.org> Reviewed-by: David S. Miller <davem@davemloft.net> Signed-off-by: Frederic Weisbecker <frederic@kernel.org> Cc: Mauro Carvalho Chehab <mchehab+samsung@kernel.org> Cc: Joel Fernandes <joel@joelfernandes.org> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Pavan Kondeti <pkondeti@codeaurora.org> Cc: Paul E . McKenney <paulmck@linux.vnet.ibm.com> Cc: David S . Miller <davem@davemloft.net> Cc: Ingo Molnar <mingo@kernel.org> Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Cc: Linus Torvalds <torvalds@linux-foundation.org> Cc: Peter Zijlstra <peterz@infradead.org> --- arch/s390/include/asm/hardirq.h | 4 ++-- include/linux/interrupt.h | 5 +++-- kernel/softirq.c | 4 ++-- 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/arch/s390/include/asm/hardirq.h b/arch/s390/include/asm/hardirq.h index 3103680487cf..54e81f520175 100644 --- a/arch/s390/include/asm/hardirq.h +++ b/arch/s390/include/asm/hardirq.h @@ -14,8 +14,8 @@ #include <asm/lowcore.h> #define local_softirq_pending() (S390_lowcore.softirq_data) -#define softirq_pending_set(x) (S390_lowcore.softirq_data = (x)) -#define softirq_pending_or(x) (S390_lowcore.softirq_data |= (x)) +#define softirq_pending_clear_mask(x) (S390_lowcore.softirq_data &= ~(x)) +#define softirq_pending_set_mask(x) (S390_lowcore.softirq_data |= (x)) #define __ARCH_IRQ_STAT #define __ARCH_HAS_DO_SOFTIRQ diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h index a5dec926531b..c402770ae45b 100644 --- a/include/linux/interrupt.h +++ b/include/linux/interrupt.h @@ -472,6 +472,7 @@ enum }; #define SOFTIRQ_STOP_IDLE_MASK (~(1 << RCU_SOFTIRQ)) +#define SOFTIRQ_ALL_MASK (BIT(NR_SOFTIRQS) - 1) #ifndef local_softirq_pending @@ -480,8 +481,8 @@ enum #endif #define local_softirq_pending() (__this_cpu_read(local_softirq_data_ref)) -#define softirq_pending_set(x) (__this_cpu_write(local_softirq_data_ref, (x))) -#define softirq_pending_or(x) (__this_cpu_or(local_softirq_data_ref, (x))) +#define softirq_pending_clear_mask(x) (__this_cpu_and(local_softirq_data_ref, ~(x))) +#define softirq_pending_set_mask(x) (__this_cpu_or(local_softirq_data_ref, (x))) #endif /* local_softirq_pending */ diff --git a/kernel/softirq.c b/kernel/softirq.c index 2137c54baf98..5f167fc43ab9 100644 --- a/kernel/softirq.c +++ b/kernel/softirq.c @@ -268,7 +268,7 @@ asmlinkage __visible void __softirq_entry __do_softirq(void) restart: /* Reset the pending bitmask before enabling irqs */ - softirq_pending_set(0); + softirq_pending_clear_mask(SOFTIRQ_ALL_MASK); local_irq_enable(); @@ -449,7 +449,7 @@ void raise_softirq(unsigned int nr) void __raise_softirq_irqoff(unsigned int nr) { trace_softirq_raise(nr); - softirq_pending_or(1UL << nr); + softirq_pending_set_mask(1UL << nr); } void open_softirq(int nr, void (*action)(struct softirq_action *)) -- 2.21.0
Disabling the softirqs is currently an all-or-nothing operation: either all softirqs are enabled or none of them. However we plan to introduce a per vector granularity of this ability to improve latency response and make each softirq vector interruptible by the others. The first step carried here is to provide the necessary APIs to control the per-vector enable bits. Reviewed-by: David S. Miller <davem@davemloft.net> Signed-off-by: Frederic Weisbecker <frederic@kernel.org> Cc: Mauro Carvalho Chehab <mchehab+samsung@kernel.org> Cc: Joel Fernandes <joel@joelfernandes.org> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Pavan Kondeti <pkondeti@codeaurora.org> Cc: Paul E . McKenney <paulmck@linux.vnet.ibm.com> Cc: David S . Miller <davem@davemloft.net> Cc: Ingo Molnar <mingo@kernel.org> Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Cc: Linus Torvalds <torvalds@linux-foundation.org> Cc: Peter Zijlstra <peterz@infradead.org> --- arch/s390/include/asm/hardirq.h | 9 ++++-- include/linux/interrupt.h | 55 ++++++++++++++++++++++++++++++--- 2 files changed, 57 insertions(+), 7 deletions(-) diff --git a/arch/s390/include/asm/hardirq.h b/arch/s390/include/asm/hardirq.h index 54e81f520175..13c57289e7ec 100644 --- a/arch/s390/include/asm/hardirq.h +++ b/arch/s390/include/asm/hardirq.h @@ -13,9 +13,14 @@ #include <asm/lowcore.h> -#define local_softirq_pending() (S390_lowcore.softirq_data) -#define softirq_pending_clear_mask(x) (S390_lowcore.softirq_data &= ~(x)) +#define local_softirq_data() (S390_lowcore.softirq_data) +#define local_softirq_pending() (local_softirq_data() & SOFTIRQ_PENDING_MASK) +#define local_softirq_enabled() (local_softirq_data() >> SOFTIRQ_ENABLED_SHIFT) +#define softirq_enabled_set_mask(x) (S390_lowcore.softirq_data |= ((x) << SOFTIRQ_ENABLED_SHIFT)) +#define softirq_enabled_clear_mask(x) (S390_lowcore.softirq_data &= ~((x) << SOFTIRQ_ENABLED_SHIFT)) +#define softirq_enabled_set(x) (S390_lowcore.softirq_data = local_softirq_pending() | ((x) << SOFTIRQ_ENABLED_SHIFT)) #define softirq_pending_set_mask(x) (S390_lowcore.softirq_data |= (x)) +#define softirq_pending_clear_mask(x) (S390_lowcore.softirq_data &= ~(x)) #define __ARCH_IRQ_STAT #define __ARCH_HAS_DO_SOFTIRQ diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h index c402770ae45b..0c3590e4fcac 100644 --- a/include/linux/interrupt.h +++ b/include/linux/interrupt.h @@ -474,17 +474,62 @@ enum #define SOFTIRQ_STOP_IDLE_MASK (~(1 << RCU_SOFTIRQ)) #define SOFTIRQ_ALL_MASK (BIT(NR_SOFTIRQS) - 1) -#ifndef local_softirq_pending +#define SOFTIRQ_ENABLED_SHIFT 16 +#define SOFTIRQ_PENDING_MASK (BIT(SOFTIRQ_ENABLED_SHIFT) - 1) + + +#ifndef local_softirq_data #ifndef local_softirq_data_ref #define local_softirq_data_ref irq_stat.__softirq_data #endif -#define local_softirq_pending() (__this_cpu_read(local_softirq_data_ref)) -#define softirq_pending_clear_mask(x) (__this_cpu_and(local_softirq_data_ref, ~(x))) -#define softirq_pending_set_mask(x) (__this_cpu_or(local_softirq_data_ref, (x))) +static inline unsigned int local_softirq_data(void) +{ + return __this_cpu_read(local_softirq_data_ref); +} -#endif /* local_softirq_pending */ +static inline unsigned int local_softirq_enabled(void) +{ + return local_softirq_data() >> SOFTIRQ_ENABLED_SHIFT; +} + +static inline unsigned int local_softirq_pending(void) +{ + return local_softirq_data() & SOFTIRQ_PENDING_MASK; +} + +static inline void softirq_enabled_clear_mask(unsigned int enabled) +{ + enabled <<= SOFTIRQ_ENABLED_SHIFT; + __this_cpu_and(local_softirq_data_ref, ~enabled); +} + +static inline void softirq_enabled_set_mask(unsigned int enabled) +{ + enabled <<= SOFTIRQ_ENABLED_SHIFT; + __this_cpu_or(local_softirq_data_ref, enabled); +} + +static inline void softirq_enabled_set(unsigned int enabled) +{ + unsigned int data; + + data = enabled << SOFTIRQ_ENABLED_SHIFT; + data |= local_softirq_pending(); + __this_cpu_write(local_softirq_data_ref, data); +} + +static inline void softirq_pending_clear_mask(unsigned int pending) +{ + __this_cpu_and(local_softirq_data_ref, ~pending); +} + +static inline void softirq_pending_set_mask(unsigned int pending) +{ + __this_cpu_or(local_softirq_data_ref, pending); +} +#endif /* local_softirq_data */ /* map softirq index to softirq name. update 'softirq_to_name' in * kernel/softirq.c when adding a new softirq. -- 2.21.0
The bottom half masking APIs have become interestingly confusing with all these flavours: local_bh_enable() _local_bh_enable() local_bh_enable_ip() __local_bh_enable_ip() _local_bh_enable() is an exception here because it's the only version that won't execute do_softirq() in the end. Clarify this straight in the name. It may help reviewers who are already familiar with functions such as preempt_enable_no_resched(). Reviewed-by: David S. Miller <davem@davemloft.net> Signed-off-by: Frederic Weisbecker <frederic@kernel.org> Cc: Mauro Carvalho Chehab <mchehab+samsung@kernel.org> Cc: Joel Fernandes <joel@joelfernandes.org> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Pavan Kondeti <pkondeti@codeaurora.org> Cc: Paul E . McKenney <paulmck@linux.vnet.ibm.com> Cc: David S . Miller <davem@davemloft.net> Cc: Ingo Molnar <mingo@kernel.org> Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Cc: Linus Torvalds <torvalds@linux-foundation.org> Cc: Peter Zijlstra <peterz@infradead.org> --- arch/s390/lib/delay.c | 2 +- drivers/s390/char/sclp.c | 2 +- drivers/s390/cio/cio.c | 2 +- include/linux/bottom_half.h | 2 +- kernel/softirq.c | 12 ++++++------ 5 files changed, 10 insertions(+), 10 deletions(-) diff --git a/arch/s390/lib/delay.c b/arch/s390/lib/delay.c index d4aa10795605..3f83ee9446b7 100644 --- a/arch/s390/lib/delay.c +++ b/arch/s390/lib/delay.c @@ -91,7 +91,7 @@ void __udelay(unsigned long long usecs) if (raw_irqs_disabled_flags(flags)) { local_bh_disable(); __udelay_disabled(usecs); - _local_bh_enable(); + local_bh_enable_no_softirq(); goto out; } __udelay_enabled(usecs); diff --git a/drivers/s390/char/sclp.c b/drivers/s390/char/sclp.c index e9aa71cdfc44..6c6b7456b368 100644 --- a/drivers/s390/char/sclp.c +++ b/drivers/s390/char/sclp.c @@ -572,7 +572,7 @@ sclp_sync_wait(void) local_irq_disable(); __ctl_load(cr0, 0, 0); if (!irq_context) - _local_bh_enable(); + local_bh_enable_no_softirq(); local_tick_enable(old_tick); local_irq_restore(flags); } diff --git a/drivers/s390/cio/cio.c b/drivers/s390/cio/cio.c index de744ca158fd..e3fb83b3c6c1 100644 --- a/drivers/s390/cio/cio.c +++ b/drivers/s390/cio/cio.c @@ -607,7 +607,7 @@ void cio_tsch(struct subchannel *sch) inc_irq_stat(IRQIO_CIO); if (!irq_context) { irq_exit(); - _local_bh_enable(); + local_bh_enable_no_softirq(); } } diff --git a/include/linux/bottom_half.h b/include/linux/bottom_half.h index a19519f4241d..a104f815efcf 100644 --- a/include/linux/bottom_half.h +++ b/include/linux/bottom_half.h @@ -19,7 +19,7 @@ static inline void local_bh_disable(void) __local_bh_disable_ip(_THIS_IP_, SOFTIRQ_DISABLE_OFFSET); } -extern void _local_bh_enable(void); +extern void local_bh_enable_no_softirq(void); extern void __local_bh_enable_ip(unsigned long ip, unsigned int cnt); static inline void local_bh_enable_ip(unsigned long ip) diff --git a/kernel/softirq.c b/kernel/softirq.c index 5f167fc43ab9..d305b4c8d1a7 100644 --- a/kernel/softirq.c +++ b/kernel/softirq.c @@ -139,7 +139,7 @@ void __local_bh_disable_ip(unsigned long ip, unsigned int cnt) EXPORT_SYMBOL(__local_bh_disable_ip); #endif /* CONFIG_TRACE_IRQFLAGS */ -static void __local_bh_enable(unsigned int cnt) +static void __local_bh_enable_no_softirq(unsigned int cnt) { lockdep_assert_irqs_disabled(); @@ -156,12 +156,12 @@ static void __local_bh_enable(unsigned int cnt) * Special-case - softirqs can safely be enabled by __do_softirq(), * without processing still-pending softirqs: */ -void _local_bh_enable(void) +void local_bh_enable_no_softirq(void) { WARN_ON_ONCE(in_irq()); - __local_bh_enable(SOFTIRQ_DISABLE_OFFSET); + __local_bh_enable_no_softirq(SOFTIRQ_DISABLE_OFFSET); } -EXPORT_SYMBOL(_local_bh_enable); +EXPORT_SYMBOL(local_bh_enable_no_softirq); void __local_bh_enable_ip(unsigned long ip, unsigned int cnt) { @@ -316,7 +316,7 @@ asmlinkage __visible void __softirq_entry __do_softirq(void) lockdep_softirq_end(in_hardirq); account_irq_exit_time(current); - __local_bh_enable(SOFTIRQ_OFFSET); + __local_bh_enable_no_softirq(SOFTIRQ_OFFSET); WARN_ON_ONCE(in_interrupt()); current_restore_flags(old_flags, PF_MEMALLOC); } @@ -352,7 +352,7 @@ void irq_enter(void) */ local_bh_disable(); tick_irq_enter(); - _local_bh_enable(); + local_bh_enable_no_softirq(); } __irq_enter(); -- 2.21.0
From: Frederic Weisbecker <fweisbec@gmail.com> Using the bottom-half masking APIs defined in linux/bottom-half.h won't be possible without passing the relevant softirq vectors that are currently defined in linux/interrupt.h Yet we can't include linux/interrupt.h from linux/bottom-half.h due to circular dependencies. Now the vector bits belong to bottom halves anyway, so moving them there is more natural and avoid nasty header dances. Reviewed-by: David S. Miller <davem@davemloft.net> Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com> Cc: Mauro Carvalho Chehab <mchehab+samsung@kernel.org> Cc: Joel Fernandes <joel@joelfernandes.org> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Pavan Kondeti <pkondeti@codeaurora.org> Cc: Paul E . McKenney <paulmck@linux.vnet.ibm.com> Cc: David S . Miller <davem@davemloft.net> Cc: Ingo Molnar <mingo@kernel.org> Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Cc: Linus Torvalds <torvalds@linux-foundation.org> Cc: Peter Zijlstra <peterz@infradead.org> --- include/linux/bottom_half.h | 24 ++++++++++++++++++++++++ include/linux/interrupt.h | 21 --------------------- 2 files changed, 24 insertions(+), 21 deletions(-) diff --git a/include/linux/bottom_half.h b/include/linux/bottom_half.h index a104f815efcf..39aaf9189226 100644 --- a/include/linux/bottom_half.h +++ b/include/linux/bottom_half.h @@ -4,6 +4,30 @@ #include <linux/preempt.h> + +/* + * PLEASE, avoid to allocate new softirqs, if you need not _really_ high + * frequency threaded job scheduling. For almost all the purposes + * tasklets are more than enough. F.e. all serial device BHs et + * al. should be converted to tasklets, not to softirqs. + */ +enum +{ +#define SOFTIRQ_VECTOR(__SVEC) \ + __SVEC##_SOFTIRQ, +#include <linux/softirq_vector.h> +#undef SOFTIRQ_VECTOR + NR_SOFTIRQS +}; + +#define SOFTIRQ_STOP_IDLE_MASK (~(1 << RCU_SOFTIRQ)) +#define SOFTIRQ_ALL_MASK (BIT(NR_SOFTIRQS) - 1) + +#define SOFTIRQ_ENABLED_SHIFT 16 +#define SOFTIRQ_PENDING_MASK (BIT(SOFTIRQ_ENABLED_SHIFT) - 1) + + + #ifdef CONFIG_TRACE_IRQFLAGS extern void __local_bh_disable_ip(unsigned long ip, unsigned int cnt); #else diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h index 0c3590e4fcac..1f4bd62ae218 100644 --- a/include/linux/interrupt.h +++ b/include/linux/interrupt.h @@ -457,27 +457,6 @@ extern bool force_irqthreads; #define hard_irq_disable() do { } while(0) #endif -/* PLEASE, avoid to allocate new softirqs, if you need not _really_ high - frequency threaded job scheduling. For almost all the purposes - tasklets are more than enough. F.e. all serial device BHs et - al. should be converted to tasklets, not to softirqs. - */ -enum -{ -#define SOFTIRQ_VECTOR(__SVEC) \ - __SVEC##_SOFTIRQ, -#include <linux/softirq_vector.h> -#undef SOFTIRQ_VECTOR - NR_SOFTIRQS -}; - -#define SOFTIRQ_STOP_IDLE_MASK (~(1 << RCU_SOFTIRQ)) -#define SOFTIRQ_ALL_MASK (BIT(NR_SOFTIRQS) - 1) - -#define SOFTIRQ_ENABLED_SHIFT 16 -#define SOFTIRQ_PENDING_MASK (BIT(SOFTIRQ_ENABLED_SHIFT) - 1) - - #ifndef local_softirq_data #ifndef local_softirq_data_ref -- 2.21.0
From: Frederic Weisbecker <fweisbec@gmail.com> All softirqs must be set enabled on boot. Reviewed-by: David S. Miller <davem@davemloft.net> Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com> Cc: Mauro Carvalho Chehab <mchehab+samsung@kernel.org> Cc: Joel Fernandes <joel@joelfernandes.org> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Pavan Kondeti <pkondeti@codeaurora.org> Cc: Paul E . McKenney <paulmck@linux.vnet.ibm.com> Cc: David S . Miller <davem@davemloft.net> Cc: Ingo Molnar <mingo@kernel.org> Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Cc: Linus Torvalds <torvalds@linux-foundation.org> Cc: Peter Zijlstra <peterz@infradead.org> --- arch/x86/kernel/irq.c | 5 ++++- include/linux/bottom_half.h | 2 ++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/arch/x86/kernel/irq.c b/arch/x86/kernel/irq.c index 59b5f2ea7c2f..b859861733a4 100644 --- a/arch/x86/kernel/irq.c +++ b/arch/x86/kernel/irq.c @@ -11,6 +11,7 @@ #include <linux/delay.h> #include <linux/export.h> #include <linux/irq.h> +#include <linux/bottom_half.h> #include <asm/apic.h> #include <asm/io_apic.h> @@ -22,7 +23,9 @@ #define CREATE_TRACE_POINTS #include <asm/trace/irq_vectors.h> -DEFINE_PER_CPU_SHARED_ALIGNED(irq_cpustat_t, irq_stat); +DEFINE_PER_CPU_SHARED_ALIGNED(irq_cpustat_t, irq_stat) = { + .__softirq_data = SOFTIRQ_DATA_INIT, +}; EXPORT_PER_CPU_SYMBOL(irq_stat); DEFINE_PER_CPU(struct pt_regs *, irq_regs); diff --git a/include/linux/bottom_half.h b/include/linux/bottom_half.h index 39aaf9189226..240419382978 100644 --- a/include/linux/bottom_half.h +++ b/include/linux/bottom_half.h @@ -26,6 +26,8 @@ enum #define SOFTIRQ_ENABLED_SHIFT 16 #define SOFTIRQ_PENDING_MASK (BIT(SOFTIRQ_ENABLED_SHIFT) - 1) +#define SOFTIRQ_DATA_INIT (SOFTIRQ_ALL_MASK << SOFTIRQ_ENABLED_SHIFT) + #ifdef CONFIG_TRACE_IRQFLAGS -- 2.21.0
From: Frederic Weisbecker <fweisbec@gmail.com> All softirqs must be set enabled on boot. Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com> Cc: Mauro Carvalho Chehab <mchehab+samsung@kernel.org> Cc: Joel Fernandes <joel@joelfernandes.org> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Pavan Kondeti <pkondeti@codeaurora.org> Cc: Paul E . McKenney <paulmck@linux.vnet.ibm.com> Cc: David S . Miller <davem@davemloft.net> Cc: Ingo Molnar <mingo@kernel.org> Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Cc: Linus Torvalds <torvalds@linux-foundation.org> Cc: Peter Zijlstra <peterz@infradead.org> --- arch/parisc/kernel/irq.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/arch/parisc/kernel/irq.c b/arch/parisc/kernel/irq.c index 0ca254085a66..b905a7e8e556 100644 --- a/arch/parisc/kernel/irq.c +++ b/arch/parisc/kernel/irq.c @@ -28,6 +28,7 @@ #include <linux/kernel_stat.h> #include <linux/seq_file.h> #include <linux/types.h> +#include <linux/bottom_half.h> #include <asm/io.h> #include <asm/smp.h> @@ -152,7 +153,10 @@ static struct irq_chip cpu_interrupt_type = { .irq_retrigger = NULL, }; -DEFINE_PER_CPU_SHARED_ALIGNED(irq_cpustat_t, irq_stat); +DEFINE_PER_CPU_SHARED_ALIGNED(irq_cpustat_t, irq_stat) = { + .__softirq_data = SOFTIRQ_DATA_INIT, +}; + #define irq_stats(x) (&per_cpu(irq_stat, x)) /* -- 2.21.0
From: Frederic Weisbecker <fweisbec@gmail.com> All softirqs must be set enabled on boot. Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com> Cc: Mauro Carvalho Chehab <mchehab+samsung@kernel.org> Cc: Joel Fernandes <joel@joelfernandes.org> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Pavan Kondeti <pkondeti@codeaurora.org> Cc: Paul E . McKenney <paulmck@linux.vnet.ibm.com> Cc: David S . Miller <davem@davemloft.net> Cc: Ingo Molnar <mingo@kernel.org> Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Cc: Linus Torvalds <torvalds@linux-foundation.org> Cc: Peter Zijlstra <peterz@infradead.org> --- arch/powerpc/kernel/irq.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/arch/powerpc/kernel/irq.c b/arch/powerpc/kernel/irq.c index 916ddc4aac44..f5e35bbc2804 100644 --- a/arch/powerpc/kernel/irq.c +++ b/arch/powerpc/kernel/irq.c @@ -54,6 +54,7 @@ #include <linux/debugfs.h> #include <linux/of.h> #include <linux/of_irq.h> +#include <linux/bottom_half.h> #include <linux/uaccess.h> #include <asm/io.h> @@ -78,7 +79,9 @@ #include <asm/trace.h> #include <asm/cpu_has_feature.h> -DEFINE_PER_CPU_SHARED_ALIGNED(irq_cpustat_t, irq_stat); +DEFINE_PER_CPU_SHARED_ALIGNED(irq_cpustat_t, irq_stat) = { + .__softirq_data = SOFTIRQ_DATA_INIT, +}; EXPORT_PER_CPU_SYMBOL(irq_stat); int __irq_offset_value; -- 2.21.0
From: Frederic Weisbecker <fweisbec@gmail.com> All softirqs must be set enabled on boot. Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com> Cc: Mauro Carvalho Chehab <mchehab+samsung@kernel.org> Cc: Joel Fernandes <joel@joelfernandes.org> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Pavan Kondeti <pkondeti@codeaurora.org> Cc: Paul E . McKenney <paulmck@linux.vnet.ibm.com> Cc: David S . Miller <davem@davemloft.net> Cc: Ingo Molnar <mingo@kernel.org> Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Cc: Linus Torvalds <torvalds@linux-foundation.org> Cc: Peter Zijlstra <peterz@infradead.org> --- kernel/softirq.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/kernel/softirq.c b/kernel/softirq.c index d305b4c8d1a7..e957615102dc 100644 --- a/kernel/softirq.c +++ b/kernel/softirq.c @@ -49,7 +49,9 @@ */ #ifndef __ARCH_IRQ_STAT -DEFINE_PER_CPU_ALIGNED(irq_cpustat_t, irq_stat); +DEFINE_PER_CPU_ALIGNED(irq_cpustat_t, irq_stat) = { + .__softirq_data = SOFTIRQ_DATA_INIT, +}; EXPORT_PER_CPU_SYMBOL(irq_stat); #endif -- 2.21.0
There is no need to process softirqs if none of those pending are enabled. Check about that early to avoid unnecessary overhead. Reviewed-by: David S. Miller <davem@davemloft.net> Signed-off-by: Frederic Weisbecker <frederic@kernel.org> Cc: Mauro Carvalho Chehab <mchehab+samsung@kernel.org> Cc: Joel Fernandes <joel@joelfernandes.org> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Pavan Kondeti <pkondeti@codeaurora.org> Cc: Paul E . McKenney <paulmck@linux.vnet.ibm.com> Cc: David S . Miller <davem@davemloft.net> Cc: Ingo Molnar <mingo@kernel.org> Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Cc: Linus Torvalds <torvalds@linux-foundation.org> Cc: Peter Zijlstra <peterz@infradead.org> --- include/linux/interrupt.h | 5 +++++ kernel/softirq.c | 21 +++++++++++---------- 2 files changed, 16 insertions(+), 10 deletions(-) diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h index 1f4bd62ae218..2bb8b7fcfb15 100644 --- a/include/linux/interrupt.h +++ b/include/linux/interrupt.h @@ -510,6 +510,11 @@ static inline void softirq_pending_set_mask(unsigned int pending) } #endif /* local_softirq_data */ +static inline int softirq_pending_enabled(void) +{ + return local_softirq_pending() & local_softirq_enabled(); +} + /* map softirq index to softirq name. update 'softirq_to_name' in * kernel/softirq.c when adding a new softirq. */ diff --git a/kernel/softirq.c b/kernel/softirq.c index e957615102dc..6c703bbb718b 100644 --- a/kernel/softirq.c +++ b/kernel/softirq.c @@ -183,7 +183,7 @@ void __local_bh_enable_ip(unsigned long ip, unsigned int cnt) */ preempt_count_sub(cnt - 1); - if (unlikely(!in_interrupt() && local_softirq_pending())) { + if (unlikely(!in_interrupt() && softirq_pending_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. @@ -252,7 +252,7 @@ asmlinkage __visible void __softirq_entry __do_softirq(void) int max_restart = MAX_SOFTIRQ_RESTART; struct softirq_action *h; bool in_hardirq; - __u32 pending; + __u32 pending, enabled; int softirq_bit; /* @@ -262,7 +262,8 @@ asmlinkage __visible void __softirq_entry __do_softirq(void) */ current->flags &= ~PF_MEMALLOC; - pending = local_softirq_pending(); + enabled = local_softirq_enabled(); + pending = local_softirq_pending() & enabled; account_irq_enter_time(current); __local_bh_disable_ip(_RET_IP_, SOFTIRQ_OFFSET); @@ -270,7 +271,7 @@ asmlinkage __visible void __softirq_entry __do_softirq(void) restart: /* Reset the pending bitmask before enabling irqs */ - softirq_pending_clear_mask(SOFTIRQ_ALL_MASK); + softirq_pending_clear_mask(pending); local_irq_enable(); @@ -307,7 +308,7 @@ asmlinkage __visible void __softirq_entry __do_softirq(void) rcu_softirq_qs(); local_irq_disable(); - pending = local_softirq_pending(); + pending = local_softirq_pending() & enabled; if (pending) { if (time_before(jiffies, end) && !need_resched() && --max_restart) @@ -333,7 +334,7 @@ asmlinkage __visible void do_softirq(void) local_irq_save(flags); - pending = local_softirq_pending(); + pending = softirq_pending_enabled(); if (pending && !ksoftirqd_running(pending)) do_softirq_own_stack(); @@ -362,7 +363,7 @@ void irq_enter(void) static inline void invoke_softirq(void) { - if (ksoftirqd_running(local_softirq_pending())) + if (ksoftirqd_running(softirq_pending_enabled())) return; if (!force_irqthreads) { @@ -411,7 +412,7 @@ void irq_exit(void) #endif account_irq_exit_time(current); preempt_count_sub(HARDIRQ_OFFSET); - if (!in_interrupt() && local_softirq_pending()) + if (!in_interrupt() && softirq_pending_enabled()) invoke_softirq(); tick_irq_exit(); @@ -642,13 +643,13 @@ void __init softirq_init(void) static int ksoftirqd_should_run(unsigned int cpu) { - return local_softirq_pending(); + return softirq_pending_enabled(); } static void run_ksoftirqd(unsigned int cpu) { local_irq_disable(); - if (local_softirq_pending()) { + if (softirq_pending_enabled()) { /* * We can safely run softirq on inline stack, as we are not deep * in the task stack here. -- 2.21.0
__local_bh_disable_ip() is neither for strict internal use nor does it require the caller to disable hardirqs. Probaby a celebration for ancient behaviour. Reviewed-by: David S. Miller <davem@davemloft.net> Signed-off-by: Frederic Weisbecker <frederic@kernel.org> Cc: Mauro Carvalho Chehab <mchehab+samsung@kernel.org> Cc: Joel Fernandes <joel@joelfernandes.org> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Pavan Kondeti <pkondeti@codeaurora.org> Cc: Paul E . McKenney <paulmck@linux.vnet.ibm.com> Cc: David S . Miller <davem@davemloft.net> Cc: Ingo Molnar <mingo@kernel.org> Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Cc: Linus Torvalds <torvalds@linux-foundation.org> Cc: Peter Zijlstra <peterz@infradead.org> --- kernel/softirq.c | 4 ---- 1 file changed, 4 deletions(-) diff --git a/kernel/softirq.c b/kernel/softirq.c index 6c703bbb718b..60d1706ad47e 100644 --- a/kernel/softirq.c +++ b/kernel/softirq.c @@ -104,10 +104,6 @@ static bool ksoftirqd_running(unsigned long pending) * softirq and whether we just have bh disabled. */ -/* - * This one is for softirq.c-internal use, - * where hardirqs are disabled legitimately: - */ #ifdef CONFIG_TRACE_IRQFLAGS void __local_bh_disable_ip(unsigned long ip, unsigned int cnt) { -- 2.21.0
The common code between both versions of __local_bh_disable_ip(), whether CONFIG_TRACE_IRQFLAGS is on or off, is going to grow up in order to support vector masking granularity. Merge these versions together to prepare for that. Reviewed-by: David S. Miller <davem@davemloft.net> Signed-off-by: Frederic Weisbecker <frederic@kernel.org> Cc: Mauro Carvalho Chehab <mchehab+samsung@kernel.org> Cc: Joel Fernandes <joel@joelfernandes.org> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Pavan Kondeti <pkondeti@codeaurora.org> Cc: Paul E . McKenney <paulmck@linux.vnet.ibm.com> Cc: David S . Miller <davem@davemloft.net> Cc: Ingo Molnar <mingo@kernel.org> Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Cc: Linus Torvalds <torvalds@linux-foundation.org> Cc: Peter Zijlstra <peterz@infradead.org> --- include/linux/bottom_half.h | 10 ---------- kernel/softirq.c | 10 ++++++---- 2 files changed, 6 insertions(+), 14 deletions(-) diff --git a/include/linux/bottom_half.h b/include/linux/bottom_half.h index 240419382978..ef9e4c752f56 100644 --- a/include/linux/bottom_half.h +++ b/include/linux/bottom_half.h @@ -28,17 +28,7 @@ enum #define SOFTIRQ_DATA_INIT (SOFTIRQ_ALL_MASK << SOFTIRQ_ENABLED_SHIFT) - - -#ifdef CONFIG_TRACE_IRQFLAGS extern void __local_bh_disable_ip(unsigned long ip, unsigned int cnt); -#else -static __always_inline void __local_bh_disable_ip(unsigned long ip, unsigned int cnt) -{ - preempt_count_add(cnt); - barrier(); -} -#endif static inline void local_bh_disable(void) { diff --git a/kernel/softirq.c b/kernel/softirq.c index 60d1706ad47e..40aa915c5e4a 100644 --- a/kernel/softirq.c +++ b/kernel/softirq.c @@ -104,14 +104,14 @@ static bool ksoftirqd_running(unsigned long pending) * softirq and whether we just have bh disabled. */ -#ifdef CONFIG_TRACE_IRQFLAGS void __local_bh_disable_ip(unsigned long ip, unsigned int cnt) { +#ifdef CONFIG_TRACE_IRQFLAGS unsigned long flags; - WARN_ON_ONCE(in_irq()); - raw_local_irq_save(flags); +#endif + WARN_ON_ONCE(in_irq()); /* * The preempt tracer hooks into preempt_count_add and will break * lockdep because it calls back into lockdep after SOFTIRQ_OFFSET @@ -125,7 +125,10 @@ void __local_bh_disable_ip(unsigned long ip, unsigned int cnt) */ if (softirq_count() == (cnt & SOFTIRQ_MASK)) trace_softirqs_off(ip); + +#ifdef CONFIG_TRACE_IRQFLAGS raw_local_irq_restore(flags); +#endif if (preempt_count() == cnt) { #ifdef CONFIG_DEBUG_PREEMPT @@ -135,7 +138,6 @@ void __local_bh_disable_ip(unsigned long ip, unsigned int cnt) } } EXPORT_SYMBOL(__local_bh_disable_ip); -#endif /* CONFIG_TRACE_IRQFLAGS */ static void __local_bh_enable_no_softirq(unsigned int cnt) { -- 2.21.0
In order to be able to mix and nest full and per vector softirq masking, we need to be able to track the nesting state using a "full masking" counter and a mask of "individual disabled vectors". Start with introducing the full masking counter. For now it's a simple mirror of softirq_count() because there is no per vector masking API yet. When this full masking counter is non 0, all softirq vectors are explicitly disabled. Suggested-by: Linus Torvalds <torvalds@linux-foundation.org> Reviewed-by: David S. Miller <davem@davemloft.net> Signed-off-by: Frederic Weisbecker <frederic@kernel.org> Cc: Mauro Carvalho Chehab <mchehab+samsung@kernel.org> Cc: Joel Fernandes <joel@joelfernandes.org> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Pavan Kondeti <pkondeti@codeaurora.org> Cc: Paul E . McKenney <paulmck@linux.vnet.ibm.com> Cc: David S . Miller <davem@davemloft.net> Cc: Ingo Molnar <mingo@kernel.org> Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Cc: Linus Torvalds <torvalds@linux-foundation.org> Cc: Peter Zijlstra <peterz@infradead.org> --- kernel/softirq.c | 33 ++++++++++++++++++++++----------- 1 file changed, 22 insertions(+), 11 deletions(-) diff --git a/kernel/softirq.c b/kernel/softirq.c index 40aa915c5e4a..2cddaaff3bfa 100644 --- a/kernel/softirq.c +++ b/kernel/softirq.c @@ -59,6 +59,12 @@ static struct softirq_action softirq_vec[NR_SOFTIRQS] __cacheline_aligned_in_smp DEFINE_PER_CPU(struct task_struct *, ksoftirqd); +struct softirq_nesting { + unsigned int disabled_all; +}; + +static DEFINE_PER_CPU(struct softirq_nesting, softirq_nesting); + const char * const softirq_to_name[NR_SOFTIRQS] = { "HI", "TIMER", "NET_TX", "NET_RX", "BLOCK", "IRQ_POLL", "TASKLET", "SCHED", "HRTIMER", "RCU" @@ -120,11 +126,11 @@ void __local_bh_disable_ip(unsigned long ip, unsigned int cnt) * call the trace_preempt_off later. */ __preempt_count_add(cnt); - /* - * Were softirqs turned off above: - */ - if (softirq_count() == (cnt & SOFTIRQ_MASK)) + + if (__this_cpu_inc_return(softirq_nesting.disabled_all) == 1) { + softirq_enabled_clear_mask(SOFTIRQ_ALL_MASK); trace_softirqs_off(ip); + } #ifdef CONFIG_TRACE_IRQFLAGS raw_local_irq_restore(flags); @@ -139,6 +145,15 @@ void __local_bh_disable_ip(unsigned long ip, unsigned int cnt) } EXPORT_SYMBOL(__local_bh_disable_ip); +static void local_bh_enable_common(unsigned long ip, unsigned int cnt) +{ + if (__this_cpu_dec_return(softirq_nesting.disabled_all)) + return; + + softirq_enabled_set(SOFTIRQ_ALL_MASK); + trace_softirqs_on(ip); +} + static void __local_bh_enable_no_softirq(unsigned int cnt) { lockdep_assert_irqs_disabled(); @@ -146,8 +161,7 @@ static void __local_bh_enable_no_softirq(unsigned int cnt) if (preempt_count() == cnt) trace_preempt_on(CALLER_ADDR0, get_lock_parent_ip()); - if (softirq_count() == (cnt & SOFTIRQ_MASK)) - trace_softirqs_on(_RET_IP_); + local_bh_enable_common(_RET_IP_, cnt); __preempt_count_sub(cnt); } @@ -170,11 +184,8 @@ void __local_bh_enable_ip(unsigned long ip, unsigned int cnt) #ifdef CONFIG_TRACE_IRQFLAGS local_irq_disable(); #endif - /* - * Are softirqs going to be turned on now: - */ - if (softirq_count() == SOFTIRQ_DISABLE_OFFSET) - trace_softirqs_on(ip); + local_bh_enable_common(ip, cnt); + /* * Keep preemption disabled until we are done with * softirq processing: -- 2.21.0
Provide the low level APIs to support per-vector masking. In order to allow these to properly nest with itself and with full softirq masking APIs, we provide two mechanisms: 1) Self nesting: use a caller stack saved/restored state model similar to that of local_irq_save() and local_irq_restore(): bh = local_bh_disable_mask(BIT(NET_RX_SOFTIRQ)); [...] bh2 = local_bh_disable_mask(BIT(TIMER_SOFTIRQ)); [...] local_bh_enable_mask(bh2); local_bh_enable_mask(bh); 2) Nest against full masking: save the per-vector disabled state prior to the first full disable operation and restore it on the last full enable operation: bh = local_bh_disable_mask(BIT(NET_RX_SOFTIRQ)); [...] local_bh_disable() <---- save state with NET_RX_SOFTIRQ disabled [...] local_bh_enable() <---- restore state with NET_RX_SOFTIRQ disabled local_bh_enable_mask(bh); Suggested-by: Linus Torvalds <torvalds@linux-foundation.org> Reviewed-by: David S. Miller <davem@davemloft.net> Signed-off-by: Frederic Weisbecker <frederic@kernel.org> Cc: Mauro Carvalho Chehab <mchehab+samsung@kernel.org> Cc: Joel Fernandes <joel@joelfernandes.org> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Pavan Kondeti <pkondeti@codeaurora.org> Cc: Paul E . McKenney <paulmck@linux.vnet.ibm.com> Cc: David S . Miller <davem@davemloft.net> Cc: Ingo Molnar <mingo@kernel.org> Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Cc: Linus Torvalds <torvalds@linux-foundation.org> Cc: Peter Zijlstra <peterz@infradead.org> --- include/linux/bottom_half.h | 7 +++ kernel/softirq.c | 85 +++++++++++++++++++++++++++++++------ 2 files changed, 80 insertions(+), 12 deletions(-) diff --git a/include/linux/bottom_half.h b/include/linux/bottom_half.h index ef9e4c752f56..a6996e3f4526 100644 --- a/include/linux/bottom_half.h +++ b/include/linux/bottom_half.h @@ -35,6 +35,10 @@ static inline void local_bh_disable(void) __local_bh_disable_ip(_THIS_IP_, SOFTIRQ_DISABLE_OFFSET); } +extern unsigned int local_bh_disable_mask(unsigned long ip, + unsigned int cnt, unsigned int mask); + + extern void local_bh_enable_no_softirq(void); extern void __local_bh_enable_ip(unsigned long ip, unsigned int cnt); @@ -48,4 +52,7 @@ static inline void local_bh_enable(void) __local_bh_enable_ip(_THIS_IP_, SOFTIRQ_DISABLE_OFFSET); } +extern void local_bh_enable_mask(unsigned long ip, unsigned int cnt, + unsigned int mask); + #endif /* _LINUX_BH_H */ diff --git a/kernel/softirq.c b/kernel/softirq.c index 2cddaaff3bfa..bb841e5d9951 100644 --- a/kernel/softirq.c +++ b/kernel/softirq.c @@ -61,6 +61,7 @@ DEFINE_PER_CPU(struct task_struct *, ksoftirqd); struct softirq_nesting { unsigned int disabled_all; + unsigned int enabled_vector; }; static DEFINE_PER_CPU(struct softirq_nesting, softirq_nesting); @@ -110,8 +111,10 @@ static bool ksoftirqd_running(unsigned long pending) * softirq and whether we just have bh disabled. */ -void __local_bh_disable_ip(unsigned long ip, unsigned int cnt) +static unsigned int local_bh_disable_common(unsigned long ip, unsigned int cnt, + bool per_vec, unsigned int vec_mask) { + unsigned int enabled; #ifdef CONFIG_TRACE_IRQFLAGS unsigned long flags; @@ -127,10 +130,31 @@ void __local_bh_disable_ip(unsigned long ip, unsigned int cnt) */ __preempt_count_add(cnt); - if (__this_cpu_inc_return(softirq_nesting.disabled_all) == 1) { - softirq_enabled_clear_mask(SOFTIRQ_ALL_MASK); - trace_softirqs_off(ip); - } + enabled = local_softirq_enabled(); + + /* + * Handle nesting of full/per-vector masking. Per vector masking + * takes effect only if full masking hasn't taken place yet. + */ + if (!__this_cpu_read(softirq_nesting.disabled_all)) { + if (enabled & vec_mask) { + softirq_enabled_clear_mask(vec_mask); + if (!local_softirq_enabled()) + trace_softirqs_off(ip); + } + + /* + * Save the state prior to full masking. We'll restore it + * on next non-nesting full unmasking in case some vectors + * have been individually disabled before (case of full masking + * nesting inside per-vector masked code). + */ + if (!per_vec) + __this_cpu_write(softirq_nesting.enabled_vector, enabled); + } + + if (!per_vec) + __this_cpu_inc(softirq_nesting.disabled_all); #ifdef CONFIG_TRACE_IRQFLAGS raw_local_irq_restore(flags); @@ -142,15 +166,38 @@ void __local_bh_disable_ip(unsigned long ip, unsigned int cnt) #endif trace_preempt_off(CALLER_ADDR0, get_lock_parent_ip()); } + + return enabled; +} + +void __local_bh_disable_ip(unsigned long ip, unsigned int cnt) +{ + local_bh_disable_common(ip, cnt, false, SOFTIRQ_ALL_MASK); } EXPORT_SYMBOL(__local_bh_disable_ip); -static void local_bh_enable_common(unsigned long ip, unsigned int cnt) +unsigned int local_bh_disable_mask(unsigned long ip, unsigned int cnt, + unsigned int vec_mask) { - if (__this_cpu_dec_return(softirq_nesting.disabled_all)) - return; + return local_bh_disable_common(ip, cnt, true, vec_mask); +} +EXPORT_SYMBOL(local_bh_disable_mask); - softirq_enabled_set(SOFTIRQ_ALL_MASK); +static void local_bh_enable_common(unsigned long ip, unsigned int cnt, + bool per_vec, unsigned int mask) +{ + /* + * Restore the previous softirq mask state. If this was the last + * full unmasking, restore what was saved. + */ + if (!per_vec) { + if (__this_cpu_dec_return(softirq_nesting.disabled_all)) + return; + else + mask = __this_cpu_read(softirq_nesting.enabled_vector); + } + + softirq_enabled_set(mask); trace_softirqs_on(ip); } @@ -161,7 +208,7 @@ static void __local_bh_enable_no_softirq(unsigned int cnt) if (preempt_count() == cnt) trace_preempt_on(CALLER_ADDR0, get_lock_parent_ip()); - local_bh_enable_common(_RET_IP_, cnt); + local_bh_enable_common(_RET_IP_, cnt, false, SOFTIRQ_ALL_MASK); __preempt_count_sub(cnt); } @@ -177,14 +224,15 @@ void local_bh_enable_no_softirq(void) } EXPORT_SYMBOL(local_bh_enable_no_softirq); -void __local_bh_enable_ip(unsigned long ip, unsigned int cnt) +static void local_bh_enable_ip_mask(unsigned long ip, unsigned int cnt, + bool per_vec, unsigned int mask) { WARN_ON_ONCE(in_irq()); lockdep_assert_irqs_enabled(); #ifdef CONFIG_TRACE_IRQFLAGS local_irq_disable(); #endif - local_bh_enable_common(ip, cnt); + local_bh_enable_common(ip, cnt, per_vec, mask); /* * Keep preemption disabled until we are done with @@ -206,8 +254,21 @@ void __local_bh_enable_ip(unsigned long ip, unsigned int cnt) #endif preempt_check_resched(); } + +void __local_bh_enable_ip(unsigned long ip, unsigned int cnt) +{ + local_bh_enable_ip_mask(ip, cnt, false, SOFTIRQ_ALL_MASK); +} EXPORT_SYMBOL(__local_bh_enable_ip); +void local_bh_enable_mask(unsigned long ip, unsigned int cnt, + unsigned int mask) +{ + local_bh_enable_ip_mask(ip, cnt, true, mask); +} +EXPORT_SYMBOL(local_bh_enable_mask); + + /* * We restart softirq processing for at most MAX_SOFTIRQ_RESTART times, * but break the loop if need_resched() is set or after 2 ms. -- 2.21.0
It makes no more sense to check for redundant softirqs on because trace_softirqs_on() is no more symmetrical to trace_softirqs_off(). Indeed trace_softirqs_off() is called whenever we know that all softirq vectors have been disabled. And trace_softirqs_on() is called everytime we enable at least one vector. So curr->softirqs_enabled may well remain true throughout subsequent calls. FIXME: Perhaps we should rename those functions. Another solution would be to make curr->softirqs_enabled to record the last value of local_softirqs_enabled() so we could track again redundant calls. Reviewed-by: David S. Miller <davem@davemloft.net> Signed-off-by: Frederic Weisbecker <frederic@kernel.org> Cc: Mauro Carvalho Chehab <mchehab+samsung@kernel.org> Cc: Joel Fernandes <joel@joelfernandes.org> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Pavan Kondeti <pkondeti@codeaurora.org> Cc: Paul E . McKenney <paulmck@linux.vnet.ibm.com> Cc: David S . Miller <davem@davemloft.net> Cc: Ingo Molnar <mingo@kernel.org> Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Cc: Linus Torvalds <torvalds@linux-foundation.org> Cc: Peter Zijlstra <peterz@infradead.org> --- kernel/locking/lockdep.c | 5 ----- kernel/locking/lockdep_internals.h | 1 - kernel/locking/lockdep_proc.c | 2 +- 3 files changed, 1 insertion(+), 7 deletions(-) diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c index a99fd5fade54..ce027d436651 100644 --- a/kernel/locking/lockdep.c +++ b/kernel/locking/lockdep.c @@ -2978,11 +2978,6 @@ void trace_softirqs_on(unsigned long ip) if (DEBUG_LOCKS_WARN_ON(!irqs_disabled())) return; - if (curr->softirqs_enabled) { - debug_atomic_inc(redundant_softirqs_on); - return; - } - current->lockdep_recursion = 1; /* * We'll do an OFF -> ON transition: diff --git a/kernel/locking/lockdep_internals.h b/kernel/locking/lockdep_internals.h index 4b0c03f0f7ce..3401e4a91afb 100644 --- a/kernel/locking/lockdep_internals.h +++ b/kernel/locking/lockdep_internals.h @@ -186,7 +186,6 @@ struct lockdep_stats { int redundant_hardirqs_off; int softirqs_on_events; int softirqs_off_events; - int redundant_softirqs_on; int redundant_softirqs_off; int nr_unused_locks; int nr_redundant_checks; diff --git a/kernel/locking/lockdep_proc.c b/kernel/locking/lockdep_proc.c index 3d31f9b0059e..4157560b36d2 100644 --- a/kernel/locking/lockdep_proc.c +++ b/kernel/locking/lockdep_proc.c @@ -169,7 +169,7 @@ static void lockdep_stats_debug_show(struct seq_file *m) hr2 = debug_atomic_read(redundant_hardirqs_off), si1 = debug_atomic_read(softirqs_on_events), si2 = debug_atomic_read(softirqs_off_events), - sr1 = debug_atomic_read(redundant_softirqs_on), + sr1 = 0, sr2 = debug_atomic_read(redundant_softirqs_off); seq_printf(m, " chain lookup misses: %11llu\n", -- 2.21.0
current->softirqs_enabled used to mean that either all or no softirqs are enabled. Now things are getting a bit different with the new per vector masking extension after which current->softirqs_enabled will stay on as long as there is a single vector still enabled. Let's adapt the check to the updated semantics. We can't deduce much from softirq_count() alone anymore except when it's 0. Reviewed-by: David S. Miller <davem@davemloft.net> Signed-off-by: Frederic Weisbecker <frederic@kernel.org> Cc: Mauro Carvalho Chehab <mchehab+samsung@kernel.org> Cc: Joel Fernandes <joel@joelfernandes.org> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Pavan Kondeti <pkondeti@codeaurora.org> Cc: Paul E . McKenney <paulmck@linux.vnet.ibm.com> Cc: David S . Miller <davem@davemloft.net> Cc: Ingo Molnar <mingo@kernel.org> Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Cc: Linus Torvalds <torvalds@linux-foundation.org> Cc: Peter Zijlstra <peterz@infradead.org> --- kernel/locking/lockdep.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c index ce027d436651..aab634b07d67 100644 --- a/kernel/locking/lockdep.c +++ b/kernel/locking/lockdep.c @@ -3911,13 +3911,8 @@ static void check_flags(unsigned long flags) * check if not in hardirq contexts: */ if (!hardirq_count()) { - if (softirq_count()) { - /* like the above, but with softirqs */ - DEBUG_LOCKS_WARN_ON(current->softirqs_enabled); - } else { - /* lick the above, does it taste good? */ + if (!softirq_count()) DEBUG_LOCKS_WARN_ON(!current->softirqs_enabled); - } } if (!debug_locks) -- 2.21.0
Now that we have full support from softirqs to perform per vector masking, let's feed lockdep with the proper inputs and push the vector numbers involved in a base softirq lock usage: LOCK_ENABLED_SOFTIRQ: push local_softirq_enabled() LOCK_USED_IN_SOFTIRQ: push curr->softirq_context, modified by lockdep_softirq_enter/exit() Reviewed-by: David S. Miller <davem@davemloft.net> Signed-off-by: Frederic Weisbecker <frederic@kernel.org> Cc: Mauro Carvalho Chehab <mchehab+samsung@kernel.org> Cc: Joel Fernandes <joel@joelfernandes.org> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Pavan Kondeti <pkondeti@codeaurora.org> Cc: Paul E . McKenney <paulmck@linux.vnet.ibm.com> Cc: David S . Miller <davem@davemloft.net> Cc: Ingo Molnar <mingo@kernel.org> Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Cc: Linus Torvalds <torvalds@linux-foundation.org> Cc: Peter Zijlstra <peterz@infradead.org> --- kernel/locking/lockdep.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c index aab634b07d67..4d38ff30006d 100644 --- a/kernel/locking/lockdep.c +++ b/kernel/locking/lockdep.c @@ -2879,6 +2879,7 @@ static void __trace_hardirqs_on_caller(unsigned long ip) */ if (curr->softirqs_enabled) { usage.bit = LOCK_ENABLED_SOFTIRQ; + usage.vector = local_softirq_enabled(); if (!mark_held_locks(curr, &usage)) return; } @@ -2966,6 +2967,7 @@ void trace_softirqs_on(unsigned long ip) struct task_struct *curr = current; struct lock_usage usage = { .bit = LOCK_ENABLED_SOFTIRQ, + .vector = local_softirq_enabled() }; if (unlikely(!debug_locks || current->lockdep_recursion)) @@ -3030,7 +3032,7 @@ void trace_softirqs_off(unsigned long ip) static int mark_irqflags(struct task_struct *curr, struct held_lock *hlock) { - struct lock_usage usage = { .vector = 0 }; + struct lock_usage usage; /* * If non-trylock use in a hardirq or softirq context, then * mark the lock as used in these contexts: @@ -3039,22 +3041,26 @@ static int mark_irqflags(struct task_struct *curr, struct held_lock *hlock) if (hlock->read) { if (curr->hardirq_context) { usage.bit = LOCK_USED_IN_HARDIRQ_READ; + usage.vector = 0; if (!mark_lock(curr, hlock, &usage)) return 0; } if (curr->softirq_context) { usage.bit = LOCK_USED_IN_SOFTIRQ_READ; + usage.vector = curr->softirq_context; if (!mark_lock(curr, hlock, &usage)) return 0; } } else { if (curr->hardirq_context) { usage.bit = LOCK_USED_IN_HARDIRQ; + usage.vector = 0; if (!mark_lock(curr, hlock, &usage)) return 0; } if (curr->softirq_context) { usage.bit = LOCK_USED_IN_SOFTIRQ; + usage.vector = curr->softirq_context; if (!mark_lock(curr, hlock, &usage)) return 0; } @@ -3063,19 +3069,23 @@ static int mark_irqflags(struct task_struct *curr, struct held_lock *hlock) if (!hlock->hardirqs_off) { if (hlock->read) { usage.bit = LOCK_ENABLED_HARDIRQ_READ; + usage.vector = 0; if (!mark_lock(curr, hlock, &usage)) return 0; if (curr->softirqs_enabled) { usage.bit = LOCK_ENABLED_SOFTIRQ_READ; + usage.vector = local_softirq_enabled(); if (!mark_lock(curr, hlock, &usage)) return 0; } } else { usage.bit = LOCK_ENABLED_HARDIRQ; + usage.vector = 0; if (!mark_lock(curr, hlock, &usage)) return 0; if (curr->softirqs_enabled) { usage.bit = LOCK_ENABLED_SOFTIRQ; + usage.vector = local_softirq_enabled(); if (!mark_lock(curr, hlock, &usage)) return 0; } -- 2.21.0
Remove the old protections that prevented softirqs from interrupting any softirq-disabled context. Now that we can disable specific vectors on a given piece of code, we want to be able to soft-interrupt those places with other vectors. Reviewed-by: David S. Miller <davem@davemloft.net> Signed-off-by: Frederic Weisbecker <frederic@kernel.org> Cc: Mauro Carvalho Chehab <mchehab+samsung@kernel.org> Cc: Joel Fernandes <joel@joelfernandes.org> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Pavan Kondeti <pkondeti@codeaurora.org> Cc: Paul E . McKenney <paulmck@linux.vnet.ibm.com> Cc: David S . Miller <davem@davemloft.net> Cc: Ingo Molnar <mingo@kernel.org> Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Cc: Linus Torvalds <torvalds@linux-foundation.org> Cc: Peter Zijlstra <peterz@infradead.org> --- kernel/softirq.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/kernel/softirq.c b/kernel/softirq.c index bb841e5d9951..95156afb768f 100644 --- a/kernel/softirq.c +++ b/kernel/softirq.c @@ -240,7 +240,7 @@ static void local_bh_enable_ip_mask(unsigned long ip, unsigned int cnt, */ preempt_count_sub(cnt - 1); - if (unlikely(!in_interrupt() && softirq_pending_enabled())) { + if (unlikely(softirq_pending_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. @@ -390,7 +390,7 @@ asmlinkage __visible void __softirq_entry __do_softirq(void) lockdep_softirq_end(in_hardirq); account_irq_exit_time(current); __local_bh_enable_no_softirq(SOFTIRQ_OFFSET); - WARN_ON_ONCE(in_interrupt()); + WARN_ON_ONCE(in_irq()); current_restore_flags(old_flags, PF_MEMALLOC); } @@ -399,7 +399,7 @@ asmlinkage __visible void do_softirq(void) __u32 pending; unsigned long flags; - if (in_interrupt()) + if (in_irq()) return; local_irq_save(flags); @@ -482,7 +482,7 @@ void irq_exit(void) #endif account_irq_exit_time(current); preempt_count_sub(HARDIRQ_OFFSET); - if (!in_interrupt() && softirq_pending_enabled()) + if (!in_irq() && softirq_pending_enabled()) invoke_softirq(); tick_irq_exit(); @@ -506,7 +506,7 @@ inline void raise_softirq_irqoff(unsigned int nr) * Otherwise we wake up ksoftirqd to make sure we * schedule the softirq soon. */ - if (!in_interrupt()) + if (!in_irq()) wakeup_softirqd(); } -- 2.21.0
It allows us to extend the coverage of vector finegrained masking throughout softirq safe locking. This is especially interesting with networking that makes extensive use of it. It works the same way as local_bh_disable_mask(): bh = spin_lock_bh_mask(lock, BIT(NET_RX_SOFTIRQ)); [...] spin_unlock_bh_mask(lock, bh); Suggested-by: Linus Torvalds <torvalds@linux-foundation.org> Reviewed-by: David S. Miller <davem@davemloft.net> Signed-off-by: Frederic Weisbecker <frederic@kernel.org> Cc: Mauro Carvalho Chehab <mchehab+samsung@kernel.org> Cc: Joel Fernandes <joel@joelfernandes.org> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Pavan Kondeti <pkondeti@codeaurora.org> Cc: Paul E . McKenney <paulmck@linux.vnet.ibm.com> Cc: David S . Miller <davem@davemloft.net> Cc: Ingo Molnar <mingo@kernel.org> Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Cc: Linus Torvalds <torvalds@linux-foundation.org> Cc: Peter Zijlstra <peterz@infradead.org> --- include/linux/spinlock.h | 14 ++++++++++++++ include/linux/spinlock_api_smp.h | 28 ++++++++++++++++++++++++++++ include/linux/spinlock_api_up.h | 13 +++++++++++++ kernel/locking/spinlock.c | 19 +++++++++++++++++++ 4 files changed, 74 insertions(+) diff --git a/include/linux/spinlock.h b/include/linux/spinlock.h index e089157dcf97..57dd73ed202d 100644 --- a/include/linux/spinlock.h +++ b/include/linux/spinlock.h @@ -270,6 +270,7 @@ static inline void do_raw_spin_unlock(raw_spinlock_t *lock) __releases(lock) #define raw_spin_lock_irq(lock) _raw_spin_lock_irq(lock) #define raw_spin_lock_bh(lock) _raw_spin_lock_bh(lock) +#define raw_spin_lock_bh_mask(lock, mask) _raw_spin_lock_bh_mask(lock, mask) #define raw_spin_unlock(lock) _raw_spin_unlock(lock) #define raw_spin_unlock_irq(lock) _raw_spin_unlock_irq(lock) @@ -279,6 +280,7 @@ static inline void do_raw_spin_unlock(raw_spinlock_t *lock) __releases(lock) _raw_spin_unlock_irqrestore(lock, flags); \ } while (0) #define raw_spin_unlock_bh(lock) _raw_spin_unlock_bh(lock) +#define raw_spin_unlock_bh_mask(lock, mask) _raw_spin_unlock_bh_mask(lock, mask) #define raw_spin_trylock_bh(lock) \ __cond_lock(lock, _raw_spin_trylock_bh(lock)) @@ -334,6 +336,12 @@ static __always_inline void spin_lock_bh(spinlock_t *lock) raw_spin_lock_bh(&lock->rlock); } +static __always_inline unsigned int spin_lock_bh_mask(spinlock_t *lock, + unsigned int mask) +{ + return raw_spin_lock_bh_mask(&lock->rlock, mask); +} + static __always_inline int spin_trylock(spinlock_t *lock) { return raw_spin_trylock(&lock->rlock); @@ -374,6 +382,12 @@ static __always_inline void spin_unlock_bh(spinlock_t *lock) raw_spin_unlock_bh(&lock->rlock); } +static __always_inline void spin_unlock_bh_mask(spinlock_t *lock, + unsigned int mask) +{ + raw_spin_unlock_bh_mask(&lock->rlock, mask); +} + static __always_inline void spin_unlock_irq(spinlock_t *lock) { raw_spin_unlock_irq(&lock->rlock); diff --git a/include/linux/spinlock_api_smp.h b/include/linux/spinlock_api_smp.h index 42dfab89e740..473641abbc5c 100644 --- a/include/linux/spinlock_api_smp.h +++ b/include/linux/spinlock_api_smp.h @@ -26,6 +26,8 @@ void __lockfunc _raw_spin_lock_nest_lock(raw_spinlock_t *lock, struct lockdep_map *map) __acquires(lock); void __lockfunc _raw_spin_lock_bh(raw_spinlock_t *lock) __acquires(lock); +unsigned int __lockfunc +_raw_spin_lock_bh_mask(raw_spinlock_t *lock, unsigned int mask) __acquires(lock); void __lockfunc _raw_spin_lock_irq(raw_spinlock_t *lock) __acquires(lock); @@ -38,6 +40,9 @@ int __lockfunc _raw_spin_trylock(raw_spinlock_t *lock); int __lockfunc _raw_spin_trylock_bh(raw_spinlock_t *lock); void __lockfunc _raw_spin_unlock(raw_spinlock_t *lock) __releases(lock); void __lockfunc _raw_spin_unlock_bh(raw_spinlock_t *lock) __releases(lock); +void __lockfunc +_raw_spin_unlock_bh_mask(raw_spinlock_t *lock, unsigned int mask) + __releases(lock); void __lockfunc _raw_spin_unlock_irq(raw_spinlock_t *lock) __releases(lock); void __lockfunc _raw_spin_unlock_irqrestore(raw_spinlock_t *lock, unsigned long flags) @@ -49,6 +54,7 @@ _raw_spin_unlock_irqrestore(raw_spinlock_t *lock, unsigned long flags) #ifdef CONFIG_INLINE_SPIN_LOCK_BH #define _raw_spin_lock_bh(lock) __raw_spin_lock_bh(lock) +#define _raw_spin_lock_bh_mask(lock, mask) __raw_spin_lock_bh_mask(lock, mask) #endif #ifdef CONFIG_INLINE_SPIN_LOCK_IRQ @@ -73,6 +79,7 @@ _raw_spin_unlock_irqrestore(raw_spinlock_t *lock, unsigned long flags) #ifdef CONFIG_INLINE_SPIN_UNLOCK_BH #define _raw_spin_unlock_bh(lock) __raw_spin_unlock_bh(lock) +#define _raw_spin_unlock_bh_mask(lock, mask) __raw_spin_unlock_bh_mask(lock, mask) #endif #ifdef CONFIG_INLINE_SPIN_UNLOCK_IRQ @@ -136,6 +143,19 @@ static inline void __raw_spin_lock_bh(raw_spinlock_t *lock) LOCK_CONTENDED(lock, do_raw_spin_trylock, do_raw_spin_lock); } +static inline unsigned int __raw_spin_lock_bh_mask(raw_spinlock_t *lock, + unsigned int mask) +{ + unsigned int old_mask; + + old_mask = local_bh_disable_mask(_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); + + return old_mask; +} + + static inline void __raw_spin_lock(raw_spinlock_t *lock) { preempt_disable(); @@ -176,6 +196,14 @@ static inline void __raw_spin_unlock_bh(raw_spinlock_t *lock) __local_bh_enable_ip(_RET_IP_, SOFTIRQ_LOCK_OFFSET); } +static inline void __raw_spin_unlock_bh_mask(raw_spinlock_t *lock, + unsigned int mask) +{ + spin_release(&lock->dep_map, 1, _RET_IP_); + do_raw_spin_unlock(lock); + local_bh_enable_mask(_RET_IP_, SOFTIRQ_LOCK_OFFSET, mask); +} + static inline int __raw_spin_trylock_bh(raw_spinlock_t *lock) { __local_bh_disable_ip(_RET_IP_, SOFTIRQ_LOCK_OFFSET); diff --git a/include/linux/spinlock_api_up.h b/include/linux/spinlock_api_up.h index d0d188861ad6..b900dcf46b26 100644 --- a/include/linux/spinlock_api_up.h +++ b/include/linux/spinlock_api_up.h @@ -33,6 +33,13 @@ #define __LOCK_BH(lock) \ do { __local_bh_disable_ip(_THIS_IP_, SOFTIRQ_LOCK_OFFSET); ___LOCK(lock); } while (0) +#define __LOCK_BH_MASK(lock, mask) ({ \ + unsigned int ____old_mask; \ + ____old_mask = local_bh_disable_mask(_THIS_IP_, SOFTIRQ_LOCK_OFFSET, mask); \ + ___LOCK(lock); \ + ____old_mask; \ +}) + #define __LOCK_IRQ(lock) \ do { local_irq_disable(); __LOCK(lock); } while (0) @@ -49,6 +56,10 @@ do { __local_bh_enable_ip(_THIS_IP_, SOFTIRQ_LOCK_OFFSET); \ ___UNLOCK(lock); } while (0) +#define __UNLOCK_BH_MASK(lock, mask) \ + do { local_bh_enable_mask(_THIS_IP_, SOFTIRQ_LOCK_OFFSET, mask); \ + ___UNLOCK(lock); } while (0) + #define __UNLOCK_IRQ(lock) \ do { local_irq_enable(); __UNLOCK(lock); } while (0) @@ -60,6 +71,7 @@ #define _raw_read_lock(lock) __LOCK(lock) #define _raw_write_lock(lock) __LOCK(lock) #define _raw_spin_lock_bh(lock) __LOCK_BH(lock) +#define _raw_spin_lock_bh_mask(lock, mask) __LOCK_BH_MASK(lock, mask) #define _raw_read_lock_bh(lock) __LOCK_BH(lock) #define _raw_write_lock_bh(lock) __LOCK_BH(lock) #define _raw_spin_lock_irq(lock) __LOCK_IRQ(lock) @@ -76,6 +88,7 @@ #define _raw_read_unlock(lock) __UNLOCK(lock) #define _raw_write_unlock(lock) __UNLOCK(lock) #define _raw_spin_unlock_bh(lock) __UNLOCK_BH(lock) +#define _raw_spin_unlock_bh_mask(lock, mask) __UNLOCK_BH_MASK(lock, mask) #define _raw_write_unlock_bh(lock) __UNLOCK_BH(lock) #define _raw_read_unlock_bh(lock) __UNLOCK_BH(lock) #define _raw_spin_unlock_irq(lock) __UNLOCK_IRQ(lock) diff --git a/kernel/locking/spinlock.c b/kernel/locking/spinlock.c index 936f3d14dd6b..4245cb3cda5a 100644 --- a/kernel/locking/spinlock.c +++ b/kernel/locking/spinlock.c @@ -170,6 +170,16 @@ void __lockfunc _raw_spin_lock_bh(raw_spinlock_t *lock) EXPORT_SYMBOL(_raw_spin_lock_bh); #endif +#ifndef CONFIG_INLINE_SPIN_LOCK_BH +unsigned int __lockfunc _raw_spin_lock_bh_mask(raw_spinlock_t *lock, + unsigned int mask) +{ + return __raw_spin_lock_bh_mask(lock, mask); +} +EXPORT_SYMBOL(_raw_spin_lock_bh_mask); +#endif + + #ifdef CONFIG_UNINLINE_SPIN_UNLOCK void __lockfunc _raw_spin_unlock(raw_spinlock_t *lock) { @@ -202,6 +212,15 @@ void __lockfunc _raw_spin_unlock_bh(raw_spinlock_t *lock) EXPORT_SYMBOL(_raw_spin_unlock_bh); #endif +#ifndef CONFIG_INLINE_SPIN_UNLOCK_BH +void __lockfunc _raw_spin_unlock_bh_mask(raw_spinlock_t *lock, + unsigned int mask) +{ + __raw_spin_unlock_bh_mask(lock, mask); +} +EXPORT_SYMBOL(_raw_spin_unlock_bh_mask); +#endif + #ifndef CONFIG_INLINE_READ_TRYLOCK int __lockfunc _raw_read_trylock(rwlock_t *lock) { -- 2.21.0
This is a usage example of spin_lock_bh_mask(). NET_RX, TIMER and TASKLET are the three softirqs that have been reported by lockdep to be used for this socket lock, at least on my own usecases. Reviewed-by: David S. Miller <davem@davemloft.net> Signed-off-by: Frederic Weisbecker <frederic@kernel.org> Cc: Mauro Carvalho Chehab <mchehab+samsung@kernel.org> Cc: Joel Fernandes <joel@joelfernandes.org> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Pavan Kondeti <pkondeti@codeaurora.org> Cc: Paul E . McKenney <paulmck@linux.vnet.ibm.com> Cc: David S . Miller <davem@davemloft.net> Cc: Ingo Molnar <mingo@kernel.org> Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Cc: Linus Torvalds <torvalds@linux-foundation.org> Cc: Peter Zijlstra <peterz@infradead.org> --- net/core/sock.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/net/core/sock.c b/net/core/sock.c index 6aa2e7e0b4fb..d6c7dca82ad9 100644 --- a/net/core/sock.c +++ b/net/core/sock.c @@ -2795,7 +2795,11 @@ EXPORT_SYMBOL(lock_sock_nested); void release_sock(struct sock *sk) { - spin_lock_bh(&sk->sk_lock.slock); + unsigned int bh; + + bh = spin_lock_bh_mask(&sk->sk_lock.slock, + BIT(NET_RX_SOFTIRQ) | BIT(TIMER_SOFTIRQ) | + BIT(HRTIMER_SOFTIRQ) | BIT(TASKLET_SOFTIRQ)); if (sk->sk_backlog.tail) __release_sock(sk); @@ -2808,7 +2812,7 @@ void release_sock(struct sock *sk) sock_release_ownership(sk); if (waitqueue_active(&sk->sk_lock.wq)) wake_up(&sk->sk_lock.wq); - spin_unlock_bh(&sk->sk_lock.slock); + spin_unlock_bh_mask(&sk->sk_lock.slock, bh); } EXPORT_SYMBOL(release_sock); -- 2.21.0
On Thu, Feb 28, 2019 at 9:12 AM Frederic Weisbecker <frederic@kernel.org> wrote:
>
> So this set should hopefully address all reviews from the v2, and
> fix all reports from the extremely useful (as always) Kbuild testing
> bot. It also completes support for all archs.
The one thing I'd still like to see is some actual performance
(latency?) numbers.
Maybe they were hiding somewhere in the pile and my quick scan missed
them. But the main argument for this was that we've had the occasional
latency issues with softirqs blocking (eg the USB v4l frame dropping
etc), and I did that SOFTIRQ_NOW_MASK because it helped one particular
case.
And you don't seem to have removed that hack, and I'd really like to
see that that thing isn't needed any more.
Because otherwise the whole series seems a bit pointless, don't you
think? If it doesn't fix that fundamental issue, then what's the point
of all this churn..
See commit 3c53776e29f8 ("Mark HI and TASKLET softirq synchronous"),
which also has a couple of people listed who could hopefully re-test
the v4l latency thing with whatever USB capture dongle it was that
showed the issue.
Linus
On Thu, Feb 28, 2019 at 09:33:15AM -0800, Linus Torvalds wrote: > On Thu, Feb 28, 2019 at 9:12 AM Frederic Weisbecker <frederic@kernel.org> wrote: > > > > So this set should hopefully address all reviews from the v2, and > > fix all reports from the extremely useful (as always) Kbuild testing > > bot. It also completes support for all archs. > > The one thing I'd still like to see is some actual performance > (latency?) numbers. > > Maybe they were hiding somewhere in the pile and my quick scan missed > them. But the main argument for this was that we've had the occasional > latency issues with softirqs blocking (eg the USB v4l frame dropping > etc), and I did that SOFTIRQ_NOW_MASK because it helped one particular > case. > > And you don't seem to have removed that hack, and I'd really like to > see that that thing isn't needed any more. > > Because otherwise the whole series seems a bit pointless, don't you > think? If it doesn't fix that fundamental issue, then what's the point > of all this churn.. Numbers are indeed missing. In fact this patchset mostly just brings an infrastructure. We have yet to pinpoint the most latency-inducing softirq disabled sites and make them disable only the vectors that are involved in a given lock. And last but not least, this patchset allows us to soft-interrupt code that disabled other vectors but it doesn't yet allow us to soft-interrupt a vector itself. Not much is needed to allow that from the softirq core code. But we can't do that blindly. For example TIMER_SOFTIRQ, HRTIMER_SOFTIRQ, TASKLET_SOFTIRQ, NET_RX_SOFTIRQ can't interrupt each others because some locks can be taken on all of them (the socket lock for example). Although so many vectors involved for a single lock is probably rare but still... The only solution I see to make vectors interruptible is to proceed the same way as we do for softirq disabled sections: proceed case by case on a per handler basis. Hopefully we can operate per subsystem and we don't need to start from drivers. So the idea is the following: if the lock A can be taken from both TIMER_SOFTIRQ and BLOCK_SOFTIRQ, we do this from the timer handler for example: __do_softirq() { // all vectors disabled run_timers { random_timer_callback() { bh = local_bh_enable_mask(~(TIMER_SOFTIRQ | BLOCK_SOFTIRQ)); spin_lock(&A); do_some_work(); spin_unlock(&A); local_bh_disable_mask(bh); } } } Sounds tedious but that's the only way I can imagine to make that correct. Another way could be for locks to piggyback the vectors they are involved in on initialization: DEFINE_SPINLOCK_SOFTIRQ(A, TIMER_SOFTIRQ | BLOCK_SOFTIRQ); Then callsites can just use: bh = spin_lock_softirq(A); .... spin_unlock_softirq(A, bh); Then the lock function always arrange to only disable TIMER_SOFTIRQ | BLOCK_SOFTIRQ if not nesting, whether we are in a vector or not. The only drawback is for the relevant spin_lock_t to carry those init flags. > > See commit 3c53776e29f8 ("Mark HI and TASKLET softirq synchronous"), > which also has a couple of people listed who could hopefully re-test > the v4l latency thing with whatever USB capture dongle it was that > showed the issue. So in this case for example, I'll need to check the callbacks involved and make them disable only the vectors that need to be disabled. I should try to reproduce the issue myself. Thanks.
On 2019-02-28 18:12:25 [+0100], Frederic Weisbecker wrote: > --- a/include/linux/interrupt.h > +++ b/include/linux/interrupt.h > @@ -474,17 +474,62 @@ enum … > +static inline unsigned int local_softirq_pending(void) > +{ > + return local_softirq_data() & SOFTIRQ_PENDING_MASK; > +} … I'm still digesting but… > +static inline void softirq_enabled_set(unsigned int enabled) > +{ > + unsigned int data; > + > + data = enabled << SOFTIRQ_ENABLED_SHIFT; > + data |= local_softirq_pending(); if an interrupt occurs at this point and invokes raise_softirq_irqof() => softirq_pending_set_mask() then we lose the pending bits, correct? This may be invoked from local_bh_enable() => local_bh_enable_ip_mask() => local_bh_enable_common() => softirq_enabled_set() and as far as I can, interrupts are only disabled in the tracing case. > + __this_cpu_write(local_softirq_data_ref, data); > +} Sebastian
On Thu, Feb 28, 2019 at 7:45 PM Frederic Weisbecker <frederic@kernel.org> wrote:
>
> Numbers are indeed missing. In fact this patchset mostly just brings an
> infrastructure. We have yet to pinpoint the most latency-inducing
> softirq disabled sites and make them disable only the vectors that
> are involved in a given lock.
Note that I think we pretty much know that already: the people who
have had issues have never actually really had issues with the actual
"disable softirq" paths, they've all been about actually *running* the
softirq's (and that in turn being a latency issue for running _other_
softirqs, and in particular for delaying them into softirqd).
Now, it may well be that yes, we'll have "block softirqs" code that
has issues too, but it's absolutely been swamped by the latencies for
actually running them so far.
Note that this is all really fairly independent of the whole masking
logic. Yes, the masking logic comes into play too (allowing you to run
a subset of softirq's at a time), but on the whole the complaints I've
seen have not been "the networking softirq takes so long that it
delays USB tasklet handling", but they have been along the lines of
"the networking softirq gets invoked so often that it then floods the
system and triggers softirqd, and _that_ then makes tasklet handling
latency go up insanely".
See the difference? Not the latency of softirq's disabled, but the
latency of one group of softirqs causing problems for another when
they all get batched together (and soft-scheduled to another context
together).
Linus
On Fri, Mar 01, 2019 at 08:51:38AM -0800, Linus Torvalds wrote:
> On Thu, Feb 28, 2019 at 7:45 PM Frederic Weisbecker <frederic@kernel.org> wrote:
> >
> > Numbers are indeed missing. In fact this patchset mostly just brings an
> > infrastructure. We have yet to pinpoint the most latency-inducing
> > softirq disabled sites and make them disable only the vectors that
> > are involved in a given lock.
>
> Note that I think we pretty much know that already: the people who
> have had issues have never actually really had issues with the actual
> "disable softirq" paths, they've all been about actually *running* the
> softirq's (and that in turn being a latency issue for running _other_
> softirqs, and in particular for delaying them into softirqd).
>
> Now, it may well be that yes, we'll have "block softirqs" code that
> has issues too, but it's absolutely been swamped by the latencies for
> actually running them so far.
>
> Note that this is all really fairly independent of the whole masking
> logic. Yes, the masking logic comes into play too (allowing you to run
> a subset of softirq's at a time), but on the whole the complaints I've
> seen have not been "the networking softirq takes so long that it
> delays USB tasklet handling", but they have been along the lines of
> "the networking softirq gets invoked so often that it then floods the
> system and triggers softirqd, and _that_ then makes tasklet handling
> latency go up insanely".
>
> See the difference? Not the latency of softirq's disabled, but the
> latency of one group of softirqs causing problems for another when
> they all get batched together (and soft-scheduled to another context
> together).
I see, so that's an entirely different problem that vector
soft-interruptibility can't fix, at least not alone.
The only solution I can imagine is to have a seperate pending mask
for normal softirq processing and ksoftirqd, so that only vectors
that have been enqueued for threaded processing are delayed.
I can work on that first, but I really need to be able to reproduce
an example of the issue. The USB capture thing seems to be one the best.
Let's browse some history to see if I can find some details on the
relevant scenario.
On Thu, Feb 28, 2019 at 06:12:19PM +0100, Frederic Weisbecker wrote: > @@ -35,7 +41,7 @@ struct lock_usage { > /* > * Usage-state bitmasks: > */ > -#define __LOCKF(__STATE) LOCKF_##__STATE = (1 << LOCK_##__STATE), > +#define __LOCKF(__STATE) LOCKF_##__STATE = (1ULL << LOCK_##__STATE), > > enum { > #define LOCKDEP_STATE(__STATE) \ > @@ -48,11 +54,40 @@ enum { > __LOCKF(USED) > }; > > +#define LOCKF_ENABLED_SOFTIRQ \ > + (LOCKF_ENABLED_HI_SOFTIRQ | LOCKF_ENABLED_TIMER_SOFTIRQ | \ > + LOCKF_ENABLED_NET_TX_SOFTIRQ | LOCKF_ENABLED_NET_RX_SOFTIRQ | \ > + LOCKF_ENABLED_BLOCK_SOFTIRQ | LOCKF_ENABLED_IRQ_POLL_SOFTIRQ | \ > + LOCKF_ENABLED_TASKLET_SOFTIRQ | LOCKF_ENABLED_SCHED_SOFTIRQ | \ > + LOCKF_ENABLED_HRTIMER_SOFTIRQ | LOCKF_ENABLED_RCU_SOFTIRQ) > #define LOCKF_ENABLED_IRQ (LOCKF_ENABLED_HARDIRQ | LOCKF_ENABLED_SOFTIRQ) > + > + > +#define LOCKF_USED_IN_SOFTIRQ \ > + (LOCKF_USED_IN_HI_SOFTIRQ | LOCKF_USED_IN_TIMER_SOFTIRQ | \ > + LOCKF_USED_IN_NET_TX_SOFTIRQ | LOCKF_USED_IN_NET_RX_SOFTIRQ | \ > + LOCKF_USED_IN_BLOCK_SOFTIRQ | LOCKF_USED_IN_IRQ_POLL_SOFTIRQ | \ > + LOCKF_USED_IN_TASKLET_SOFTIRQ | LOCKF_USED_IN_SCHED_SOFTIRQ | \ > + LOCKF_USED_IN_HRTIMER_SOFTIRQ | LOCKF_USED_IN_RCU_SOFTIRQ) > #define LOCKF_USED_IN_IRQ (LOCKF_USED_IN_HARDIRQ | LOCKF_USED_IN_SOFTIRQ) > > + > +#define LOCKF_ENABLED_SOFTIRQ_READ \ > + (LOCKF_ENABLED_HI_SOFTIRQ_READ | LOCKF_ENABLED_TIMER_SOFTIRQ_READ | \ > + LOCKF_ENABLED_NET_TX_SOFTIRQ_READ | LOCKF_ENABLED_NET_RX_SOFTIRQ_READ | \ > + LOCKF_ENABLED_BLOCK_SOFTIRQ_READ | LOCKF_ENABLED_IRQ_POLL_SOFTIRQ_READ | \ > + LOCKF_ENABLED_TASKLET_SOFTIRQ_READ | LOCKF_ENABLED_SCHED_SOFTIRQ_READ | \ > + LOCKF_ENABLED_HRTIMER_SOFTIRQ_READ | LOCKF_ENABLED_RCU_SOFTIRQ_READ) > #define LOCKF_ENABLED_IRQ_READ \ > (LOCKF_ENABLED_HARDIRQ_READ | LOCKF_ENABLED_SOFTIRQ_READ) > + > + > +#define LOCKF_USED_IN_SOFTIRQ_READ \ > + (LOCKF_USED_IN_HI_SOFTIRQ_READ | LOCKF_USED_IN_TIMER_SOFTIRQ_READ | \ > + LOCKF_USED_IN_NET_TX_SOFTIRQ_READ | LOCKF_USED_IN_NET_RX_SOFTIRQ_READ | \ > + LOCKF_USED_IN_BLOCK_SOFTIRQ_READ | LOCKF_USED_IN_IRQ_POLL_SOFTIRQ_READ | \ > + LOCKF_USED_IN_TASKLET_SOFTIRQ_READ | LOCKF_USED_IN_SCHED_SOFTIRQ_READ | \ > + LOCKF_USED_IN_HRTIMER_SOFTIRQ_READ | LOCKF_USED_IN_RCU_SOFTIRQ_READ) > #define LOCKF_USED_IN_IRQ_READ \ > (LOCKF_USED_IN_HARDIRQ_READ | LOCKF_USED_IN_SOFTIRQ_READ) > > diff --git a/kernel/locking/lockdep_states.h b/kernel/locking/lockdep_states.h > index 35ca09f2ed0b..34ff569d2024 100644 > --- a/kernel/locking/lockdep_states.h > +++ b/kernel/locking/lockdep_states.h > @@ -5,4 +5,6 @@ > * you add one, or come up with a nice dynamic solution. > */ > LOCKDEP_STATE(HARDIRQ) > -LOCKDEP_STATE(SOFTIRQ) > +#define SOFTIRQ_VECTOR(__SVEC) LOCKDEP_STATE(__SVEC##_SOFTIRQ) > +#include <linux/softirq_vector.h> > +#undef SOFTIRQ_VECTOR Would something like the below help? --- Subject: locking/lockdep: Generate LOCKF_ bit composites From: Peter Zijlstra <peterz@infradead.org> Date: Tue Apr 9 13:59:03 CEST 2019 Instead of open-coding the bitmasks, generate them using the lockdep_states.h header. This prepares for additional states, which would make the manual masks tedious and error prone. Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> --- kernel/locking/lockdep_internals.h | 28 ++++++++++++++++++++++------ 1 file changed, 22 insertions(+), 6 deletions(-) --- a/kernel/locking/lockdep_internals.h +++ b/kernel/locking/lockdep_internals.h @@ -42,13 +42,29 @@ enum { __LOCKF(USED) }; -#define LOCKF_ENABLED_IRQ (LOCKF_ENABLED_HARDIRQ | LOCKF_ENABLED_SOFTIRQ) -#define LOCKF_USED_IN_IRQ (LOCKF_USED_IN_HARDIRQ | LOCKF_USED_IN_SOFTIRQ) +#define LOCKDEP_STATE(__STATE) LOCKF_ENABLED_##__STATE | +static const u64 LOCKF_ENABLED_IRQ = +#include "lockdep_states.h" + 0; +#undef LOCKDEP_STATE -#define LOCKF_ENABLED_IRQ_READ \ - (LOCKF_ENABLED_HARDIRQ_READ | LOCKF_ENABLED_SOFTIRQ_READ) -#define LOCKF_USED_IN_IRQ_READ \ - (LOCKF_USED_IN_HARDIRQ_READ | LOCKF_USED_IN_SOFTIRQ_READ) +#define LOCKDEP_STATE(__STATE) LOCKF_USED_IN_##__STATE | +static const u64 LOCKF_USED_IN_IRQ = +#include "lockdep_states.h" + 0; +#undef LOCKDEP_STATE + +#define LOCKDEP_STATE(__STATE) LOCKF_ENABLED_##__STATE##_READ | +static const u64 LOCKF_ENABLED_IRQ_READ = +#include "lockdep_states.h" + 0; +#undef LOCKDEP_STATE + +#define LOCKDEP_STATE(__STATE) LOCKF_USED_IN_##__STATE##_READ | +static const u64 LOCKF_USED_IN_IRQ_READ = +#include "lockdep_states.h" + 0; +#undef LOCKDEP_STATE /* * CONFIG_LOCKDEP_SMALL is defined for sparc. Sparc requires .text,