Hi, guys.
I have implemented a low-overhead method for detecting interrupt
storm in softlockup. Please review it, all comments are welcome.
Changes from v9 to v10:
- The two patches related to 'watchdog/softlockup' remain unchanged.
- The majority of the work related to 'genirq' is contributed by
Thomas, indicated by adding 'Originally-by' tag. And I'd like to
express my gratitude for Thomas's contributions and guidance here.
- Adopt Thomas's change log for the snapshot mechanism for interrupt
statistics.
- Split unrelated change in patch #2 into a separate patch #3.
Changes from v8 to v9:
- Patch #1 remains unchanged.
- From Thomas Gleixner, split patch #2 into two patches. Interrupt
infrastructure first and then the actual usage site in the
watchdog code.
Changes from v7 to v8:
- From Thomas Gleixner, implement statistics within the interrupt
core code and provide sensible interfaces for the watchdog code.
- Patch #1 remains unchanged. Patch #2 has significant changes
based on Thomas's suggestions, which is why I have removed
Liu Song and Douglas's Reviewed-by from patch #2. Please review
it again, and all comments are welcome.
Changes from v6 to v7:
- Remove "READ_ONCE" in "start_counting_irqs"
- Replace the hard-coded 5 with "NUM_SAMPLE_PERIODS" macro in
"set_sample_period".
- Add empty lines to help with reading the code.
- Remove the branch that processes IRQs where "counts_diff = 0".
- Add the Reviewed-by of Liu Song and Douglas.
Changes from v5 to v6:
- Use "./scripts/checkpatch.pl --strict" to get a few extra
style nits and fix them.
- Squash patch #3 into patch #1, and wrapp the help text to
80 columns.
- Sort existing headers alphabetically in watchdog.c
- Drop "softlockup_hardirq_cpus", just read "hardirq_counts"
and see if it's non-NULL.
- Store "nr_irqs" in a local variable.
- Simplify the calculation of "cpu_diff".
Changes from v4 to v5:
- Rearranging variable placement to make code look neater.
Changes from v3 to v4:
- Renaming some variable and function names to make the code logic
more readable.
- Change the code location to avoid predeclaring.
- Just swap rather than a double loop in tabulate_irq_count.
- Since nr_irqs has the potential to grow at runtime, bounds-check
logic has been implemented.
- Add SOFTLOCKUP_DETECTOR_INTR_STORM Kconfig knob.
Changes from v2 to v3:
- From Liu Song, using enum instead of macro for cpu_stats, shortening
the name 'idx_to_stat' to 'stats', adding 'get_16bit_precesion' instead
of using right shift operations, and using 'struct irq_counts'.
- From kernel robot test, using '__this_cpu_read' and '__this_cpu_write'
instead of accessing to an per-cpu array directly, in order to avoid
this warning.
'sparse: incorrect type in initializer (different modifiers)'
Changes from v1 to v2:
- From Douglas, optimize the memory of cpustats. With the maximum number
of CPUs, that's now this.
2 * 8192 * 4 + 1 * 8192 * 5 * 4 + 1 * 8192 = 237,568 bytes.
- From Liu Song, refactor the code format and add necessary comments.
- From Douglas, use interrupt counts instead of interrupt time to
determine the cause of softlockup.
- Remove the cmdline parameter added in PATCHv1.
Bitao Hu (4):
watchdog/softlockup: low-overhead detection of interrupt storm
genirq: Provide a snapshot mechanism for interrupt statistics
genirq: Avoid summation loops for /proc/interrupts
watchdog/softlockup: report the most frequent interrupts
arch/mips/dec/setup.c | 2 +-
arch/parisc/kernel/smp.c | 2 +-
arch/powerpc/kvm/book3s_hv_rm_xics.c | 2 +-
include/linux/irqdesc.h | 11 +-
include/linux/kernel_stat.h | 3 +
kernel/irq/internals.h | 2 +-
kernel/irq/irqdesc.c | 36 ++++-
kernel/irq/proc.c | 12 +-
kernel/watchdog.c | 213 ++++++++++++++++++++++++++-
lib/Kconfig.debug | 13 ++
scripts/gdb/linux/interrupts.py | 6 +-
11 files changed, 276 insertions(+), 26 deletions(-)
--
2.37.1 (Apple Git-137.1)
The soft lockup detector lacks a mechanism to identify interrupt storms
as root cause of a lockup. To enable this the detector needs a
mechanism to snapshot the interrupt count statistics on a CPU when the
detector observes a potential lockup scenario and compare that against
the interrupt count when it warns about the lockup later on. The number
of interrupts in that period give a hint whether the lockup might be
caused by an interrupt storm.
Instead of having extra storage in the lockup detector and accessing
the internals of the interrupt descriptor directly, convert the per CPU
irq_desc::kstat_irq member to a data structure which contains the
counter plus a snapshot member and provide interfaces to take a
snapshot of all interrupts on the current CPU and to retrieve the delta
of a specific interrupt later on.
Originally-by: Thomas Gleixner <[email protected]>
Signed-off-by: Bitao Hu <[email protected]>
---
arch/mips/dec/setup.c | 2 +-
arch/parisc/kernel/smp.c | 2 +-
arch/powerpc/kvm/book3s_hv_rm_xics.c | 2 +-
include/linux/irqdesc.h | 9 ++++++--
include/linux/kernel_stat.h | 3 +++
kernel/irq/internals.h | 2 +-
kernel/irq/irqdesc.c | 34 ++++++++++++++++++++++------
kernel/irq/proc.c | 5 ++--
scripts/gdb/linux/interrupts.py | 6 ++---
9 files changed, 46 insertions(+), 19 deletions(-)
diff --git a/arch/mips/dec/setup.c b/arch/mips/dec/setup.c
index 6c3704f51d0d..87f0a1436bf9 100644
--- a/arch/mips/dec/setup.c
+++ b/arch/mips/dec/setup.c
@@ -756,7 +756,7 @@ void __init arch_init_irq(void)
NULL))
pr_err("Failed to register fpu interrupt\n");
desc_fpu = irq_to_desc(irq_fpu);
- fpu_kstat_irq = this_cpu_ptr(desc_fpu->kstat_irqs);
+ fpu_kstat_irq = this_cpu_ptr(&desc_fpu->kstat_irqs->cnt);
}
if (dec_interrupt[DEC_IRQ_CASCADE] >= 0) {
if (request_irq(dec_interrupt[DEC_IRQ_CASCADE], no_action,
diff --git a/arch/parisc/kernel/smp.c b/arch/parisc/kernel/smp.c
index 444154271f23..800eb64e91ad 100644
--- a/arch/parisc/kernel/smp.c
+++ b/arch/parisc/kernel/smp.c
@@ -344,7 +344,7 @@ static int smp_boot_one_cpu(int cpuid, struct task_struct *idle)
struct irq_desc *desc = irq_to_desc(i);
if (desc && desc->kstat_irqs)
- *per_cpu_ptr(desc->kstat_irqs, cpuid) = 0;
+ *per_cpu_ptr(desc->kstat_irqs, cpuid) = (struct irqstat) { };
}
#endif
diff --git a/arch/powerpc/kvm/book3s_hv_rm_xics.c b/arch/powerpc/kvm/book3s_hv_rm_xics.c
index e42984878503..f2636414d82a 100644
--- a/arch/powerpc/kvm/book3s_hv_rm_xics.c
+++ b/arch/powerpc/kvm/book3s_hv_rm_xics.c
@@ -837,7 +837,7 @@ static inline void this_cpu_inc_rm(unsigned int __percpu *addr)
*/
static void kvmppc_rm_handle_irq_desc(struct irq_desc *desc)
{
- this_cpu_inc_rm(desc->kstat_irqs);
+ this_cpu_inc_rm(&desc->kstat_irqs->cnt);
__this_cpu_inc(kstat.irqs_sum);
}
diff --git a/include/linux/irqdesc.h b/include/linux/irqdesc.h
index d9451d456a73..2912b1998670 100644
--- a/include/linux/irqdesc.h
+++ b/include/linux/irqdesc.h
@@ -17,6 +17,11 @@ struct irq_desc;
struct irq_domain;
struct pt_regs;
+struct irqstat {
+ unsigned int cnt;
+ unsigned int ref;
+};
+
/**
* struct irq_desc - interrupt descriptor
* @irq_common_data: per irq and chip data passed down to chip functions
@@ -55,7 +60,7 @@ struct pt_regs;
struct irq_desc {
struct irq_common_data irq_common_data;
struct irq_data irq_data;
- unsigned int __percpu *kstat_irqs;
+ struct irqstat __percpu *kstat_irqs;
irq_flow_handler_t handle_irq;
struct irqaction *action; /* IRQ action list */
unsigned int status_use_accessors;
@@ -119,7 +124,7 @@ extern struct irq_desc irq_desc[NR_IRQS];
static inline unsigned int irq_desc_kstat_cpu(struct irq_desc *desc,
unsigned int cpu)
{
- return desc->kstat_irqs ? *per_cpu_ptr(desc->kstat_irqs, cpu) : 0;
+ return desc->kstat_irqs ? per_cpu(desc->kstat_irqs->cnt, cpu) : 0;
}
static inline struct irq_desc *irq_data_to_desc(struct irq_data *data)
diff --git a/include/linux/kernel_stat.h b/include/linux/kernel_stat.h
index 9935f7ecbfb9..98b3043ea5e6 100644
--- a/include/linux/kernel_stat.h
+++ b/include/linux/kernel_stat.h
@@ -79,6 +79,9 @@ static inline unsigned int kstat_cpu_softirqs_sum(int cpu)
return sum;
}
+extern void kstat_snapshot_irqs(void);
+extern unsigned int kstat_get_irq_since_snapshot(unsigned int irq);
+
/*
* Number of interrupts per specific IRQ source, since bootup
*/
diff --git a/kernel/irq/internals.h b/kernel/irq/internals.h
index bcc7f21db9ee..1d92532c2aae 100644
--- a/kernel/irq/internals.h
+++ b/kernel/irq/internals.h
@@ -258,7 +258,7 @@ static inline void irq_state_set_masked(struct irq_desc *desc)
static inline void __kstat_incr_irqs_this_cpu(struct irq_desc *desc)
{
- __this_cpu_inc(*desc->kstat_irqs);
+ __this_cpu_inc(desc->kstat_irqs->cnt);
__this_cpu_inc(kstat.irqs_sum);
}
diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c
index 27ca1c866f29..9cd17080b2d8 100644
--- a/kernel/irq/irqdesc.c
+++ b/kernel/irq/irqdesc.c
@@ -122,7 +122,7 @@ static void desc_set_defaults(unsigned int irq, struct irq_desc *desc, int node,
desc->name = NULL;
desc->owner = owner;
for_each_possible_cpu(cpu)
- *per_cpu_ptr(desc->kstat_irqs, cpu) = 0;
+ *per_cpu_ptr(desc->kstat_irqs, cpu) = (struct irqstat) { };
desc_smp_init(desc, node, affinity);
}
@@ -418,8 +418,8 @@ static struct irq_desc *alloc_desc(int irq, int node, unsigned int flags,
desc = kzalloc_node(sizeof(*desc), GFP_KERNEL, node);
if (!desc)
return NULL;
- /* allocate based on nr_cpu_ids */
- desc->kstat_irqs = alloc_percpu(unsigned int);
+
+ desc->kstat_irqs = alloc_percpu(struct irqstat);
if (!desc->kstat_irqs)
goto err_desc;
@@ -593,7 +593,7 @@ int __init early_irq_init(void)
count = ARRAY_SIZE(irq_desc);
for (i = 0; i < count; i++) {
- desc[i].kstat_irqs = alloc_percpu(unsigned int);
+ desc[i].kstat_irqs = alloc_percpu(struct irqstat);
alloc_masks(&desc[i], node);
raw_spin_lock_init(&desc[i].lock);
lockdep_set_class(&desc[i].lock, &irq_desc_lock_class);
@@ -952,8 +952,7 @@ unsigned int kstat_irqs_cpu(unsigned int irq, int cpu)
{
struct irq_desc *desc = irq_to_desc(irq);
- return desc && desc->kstat_irqs ?
- *per_cpu_ptr(desc->kstat_irqs, cpu) : 0;
+ return desc && desc->kstat_irqs ? per_cpu(desc->kstat_irqs->cnt, cpu) : 0;
}
static bool irq_is_nmi(struct irq_desc *desc)
@@ -975,10 +974,31 @@ static unsigned int kstat_irqs(unsigned int irq)
return data_race(desc->tot_count);
for_each_possible_cpu(cpu)
- sum += data_race(*per_cpu_ptr(desc->kstat_irqs, cpu));
+ sum += data_race(per_cpu(desc->kstat_irqs->cnt, cpu));
return sum;
}
+void kstat_snapshot_irqs(void)
+{
+ struct irq_desc *desc;
+ unsigned int irq;
+
+ for_each_irq_desc(irq, desc) {
+ if (!desc->kstat_irqs)
+ continue;
+ this_cpu_write(desc->kstat_irqs->ref, this_cpu_read(desc->kstat_irqs->cnt));
+ }
+}
+
+unsigned int kstat_get_irq_since_snapshot(unsigned int irq)
+{
+ struct irq_desc *desc = irq_to_desc(irq);
+
+ if (!desc || !desc->kstat_irqs)
+ return 0;
+ return this_cpu_read(desc->kstat_irqs->cnt) - this_cpu_read(desc->kstat_irqs->ref);
+}
+
/**
* kstat_irqs_usr - Get the statistics for an interrupt from thread context
* @irq: The interrupt number
diff --git a/kernel/irq/proc.c b/kernel/irq/proc.c
index 623b8136e9af..6954e0a02047 100644
--- a/kernel/irq/proc.c
+++ b/kernel/irq/proc.c
@@ -490,7 +490,7 @@ int show_interrupts(struct seq_file *p, void *v)
if (desc->kstat_irqs) {
for_each_online_cpu(j)
- any_count |= data_race(*per_cpu_ptr(desc->kstat_irqs, j));
+ any_count |= data_race(per_cpu(desc->kstat_irqs->cnt, j));
}
if ((!desc->action || irq_desc_is_chained(desc)) && !any_count)
@@ -498,8 +498,7 @@ int show_interrupts(struct seq_file *p, void *v)
seq_printf(p, "%*d: ", prec, i);
for_each_online_cpu(j)
- seq_printf(p, "%10u ", desc->kstat_irqs ?
- *per_cpu_ptr(desc->kstat_irqs, j) : 0);
+ seq_printf(p, "%10u ", desc->kstat_irqs ? per_cpu(desc->kstat_irqs->cnt, j) : 0);
raw_spin_lock_irqsave(&desc->lock, flags);
if (desc->irq_data.chip) {
diff --git a/scripts/gdb/linux/interrupts.py b/scripts/gdb/linux/interrupts.py
index ef478e273791..7e50f3b9dfad 100644
--- a/scripts/gdb/linux/interrupts.py
+++ b/scripts/gdb/linux/interrupts.py
@@ -37,7 +37,7 @@ def show_irq_desc(prec, irq):
any_count = 0
if desc['kstat_irqs']:
for cpu in cpus.each_online_cpu():
- any_count += cpus.per_cpu(desc['kstat_irqs'], cpu)
+ any_count += cpus.per_cpu(desc['kstat_irqs'], cpu)['cnt']
if (desc['action'] == 0 or irq_desc_is_chained(desc)) and any_count == 0:
return text;
@@ -45,7 +45,7 @@ def show_irq_desc(prec, irq):
text += "%*d: " % (prec, irq)
for cpu in cpus.each_online_cpu():
if desc['kstat_irqs']:
- count = cpus.per_cpu(desc['kstat_irqs'], cpu)
+ count = cpus.per_cpu(desc['kstat_irqs'], cpu)['cnt']
else:
count = 0
text += "%10u" % (count)
@@ -177,7 +177,7 @@ def arm_common_show_interrupts(prec):
if desc == 0:
continue
for cpu in cpus.each_online_cpu():
- text += "%10u" % (cpus.per_cpu(desc['kstat_irqs'], cpu))
+ text += "%10u" % (cpus.per_cpu(desc['kstat_irqs'], cpu)['cnt'])
text += " %s" % (ipi_types[ipi].string())
text += "\n"
return text
--
2.37.1 (Apple Git-137.1)
We could use the irq_desc::tot_count member to avoid the summation
loop for interrupts which are not marked as 'PER_CPU' interrupts in
'show_interrupts'. This could reduce the time overhead of reading
/proc/interrupts.
Originally-by: Thomas Gleixner <[email protected]>
Signed-off-by: Bitao Hu <[email protected]>
---
include/linux/irqdesc.h | 2 ++
kernel/irq/irqdesc.c | 2 +-
kernel/irq/proc.c | 9 +++++++--
3 files changed, 10 insertions(+), 3 deletions(-)
diff --git a/include/linux/irqdesc.h b/include/linux/irqdesc.h
index 2912b1998670..1ee96d7232b4 100644
--- a/include/linux/irqdesc.h
+++ b/include/linux/irqdesc.h
@@ -121,6 +121,8 @@ static inline void irq_unlock_sparse(void) { }
extern struct irq_desc irq_desc[NR_IRQS];
#endif
+extern bool irq_is_nmi(struct irq_desc *desc);
+
static inline unsigned int irq_desc_kstat_cpu(struct irq_desc *desc,
unsigned int cpu)
{
diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c
index 9cd17080b2d8..56a767957a9d 100644
--- a/kernel/irq/irqdesc.c
+++ b/kernel/irq/irqdesc.c
@@ -955,7 +955,7 @@ unsigned int kstat_irqs_cpu(unsigned int irq, int cpu)
return desc && desc->kstat_irqs ? per_cpu(desc->kstat_irqs->cnt, cpu) : 0;
}
-static bool irq_is_nmi(struct irq_desc *desc)
+bool irq_is_nmi(struct irq_desc *desc)
{
return desc->istate & IRQS_NMI;
}
diff --git a/kernel/irq/proc.c b/kernel/irq/proc.c
index 6954e0a02047..b3b1b93f0410 100644
--- a/kernel/irq/proc.c
+++ b/kernel/irq/proc.c
@@ -489,8 +489,13 @@ int show_interrupts(struct seq_file *p, void *v)
goto outsparse;
if (desc->kstat_irqs) {
- for_each_online_cpu(j)
- any_count |= data_race(per_cpu(desc->kstat_irqs->cnt, j));
+ if (!irq_settings_is_per_cpu_devid(desc) &&
+ !irq_settings_is_per_cpu(desc) &&
+ !irq_is_nmi(desc))
+ any_count = data_race(desc->tot_count);
+ else
+ for_each_online_cpu(j)
+ any_count |= data_race(per_cpu(desc->kstat_irqs->cnt, j));
}
if ((!desc->action || irq_desc_is_chained(desc)) && !any_count)
--
2.37.1 (Apple Git-137.1)
When the watchdog determines that the current soft lockup is due
to an interrupt storm based on CPU utilization, reporting the
most frequent interrupts could be good enough for further
troubleshooting.
Below is an example of interrupt storm. The call tree does not
provide useful information, but we can analyze which interrupt
caused the soft lockup by comparing the counts of interrupts.
[ 638.870231] watchdog: BUG: soft lockup - CPU#9 stuck for 26s! [swapper/9:0]
[ 638.870825] CPU#9 Utilization every 4s during lockup:
[ 638.871194] #1: 0% system, 0% softirq, 100% hardirq, 0% idle
[ 638.871652] #2: 0% system, 0% softirq, 100% hardirq, 0% idle
[ 638.872107] #3: 0% system, 0% softirq, 100% hardirq, 0% idle
[ 638.872563] #4: 0% system, 0% softirq, 100% hardirq, 0% idle
[ 638.873018] #5: 0% system, 0% softirq, 100% hardirq, 0% idle
[ 638.873494] CPU#9 Detect HardIRQ Time exceeds 50%. Most frequent HardIRQs:
[ 638.873994] #1: 330945 irq#7
[ 638.874236] #2: 31 irq#82
[ 638.874493] #3: 10 irq#10
[ 638.874744] #4: 2 irq#89
[ 638.874992] #5: 1 irq#102
..
[ 638.875313] Call trace:
[ 638.875315] __do_softirq+0xa8/0x364
Signed-off-by: Bitao Hu <[email protected]>
---
kernel/watchdog.c | 115 ++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 111 insertions(+), 4 deletions(-)
diff --git a/kernel/watchdog.c b/kernel/watchdog.c
index 69e72d7e461d..c9d49ae8d045 100644
--- a/kernel/watchdog.c
+++ b/kernel/watchdog.c
@@ -12,22 +12,25 @@
#define pr_fmt(fmt) "watchdog: " fmt
-#include <linux/mm.h>
#include <linux/cpu.h>
-#include <linux/nmi.h>
#include <linux/init.h>
+#include <linux/irq.h>
+#include <linux/irqdesc.h>
#include <linux/kernel_stat.h>
+#include <linux/kvm_para.h>
#include <linux/math64.h>
+#include <linux/mm.h>
#include <linux/module.h>
+#include <linux/nmi.h>
+#include <linux/stop_machine.h>
#include <linux/sysctl.h>
#include <linux/tick.h>
+
#include <linux/sched/clock.h>
#include <linux/sched/debug.h>
#include <linux/sched/isolation.h>
-#include <linux/stop_machine.h>
#include <asm/irq_regs.h>
-#include <linux/kvm_para.h>
static DEFINE_MUTEX(watchdog_mutex);
@@ -417,13 +420,104 @@ static void print_cpustat(void)
}
}
+#define HARDIRQ_PERCENT_THRESH 50
+#define NUM_HARDIRQ_REPORT 5
+struct irq_counts {
+ int irq;
+ u32 counts;
+};
+
+static DEFINE_PER_CPU(bool, snapshot_taken);
+
+/* Tabulate the most frequent interrupts. */
+static void tabulate_irq_count(struct irq_counts *irq_counts, int irq, u32 counts, int rank)
+{
+ int i;
+ struct irq_counts new_count = {irq, counts};
+
+ for (i = 0; i < rank; i++) {
+ if (counts > irq_counts[i].counts)
+ swap(new_count, irq_counts[i]);
+ }
+}
+
+/*
+ * If the hardirq time exceeds HARDIRQ_PERCENT_THRESH% of the sample_period,
+ * then the cause of softlockup might be interrupt storm. In this case, it
+ * would be useful to start interrupt counting.
+ */
+static bool need_counting_irqs(void)
+{
+ u8 util;
+ int tail = __this_cpu_read(cpustat_tail);
+
+ tail = (tail + NUM_HARDIRQ_REPORT - 1) % NUM_HARDIRQ_REPORT;
+ util = __this_cpu_read(cpustat_util[tail][STATS_HARDIRQ]);
+ return util > HARDIRQ_PERCENT_THRESH;
+}
+
+static void start_counting_irqs(void)
+{
+ if (!__this_cpu_read(snapshot_taken)) {
+ kstat_snapshot_irqs();
+ __this_cpu_write(snapshot_taken, true);
+ }
+}
+
+static void stop_counting_irqs(void)
+{
+ __this_cpu_write(snapshot_taken, false);
+}
+
+static void print_irq_counts(void)
+{
+ unsigned int i, count;
+ struct irq_counts irq_counts_sorted[NUM_HARDIRQ_REPORT] = {
+ {-1, 0}, {-1, 0}, {-1, 0}, {-1, 0}, {-1, 0}
+ };
+
+ if (__this_cpu_read(snapshot_taken)) {
+ for_each_active_irq(i) {
+ count = kstat_get_irq_since_snapshot(i);
+ tabulate_irq_count(irq_counts_sorted, i, count, NUM_HARDIRQ_REPORT);
+ }
+
+ /*
+ * We do not want the "watchdog: " prefix on every line,
+ * hence we use "printk" instead of "pr_crit".
+ */
+ printk(KERN_CRIT "CPU#%d Detect HardIRQ Time exceeds %d%%. Most frequent HardIRQs:\n",
+ smp_processor_id(), HARDIRQ_PERCENT_THRESH);
+
+ for (i = 0; i < NUM_HARDIRQ_REPORT; i++) {
+ if (irq_counts_sorted[i].irq == -1)
+ break;
+
+ printk(KERN_CRIT "\t#%u: %-10u\tirq#%d\n",
+ i + 1, irq_counts_sorted[i].counts,
+ irq_counts_sorted[i].irq);
+ }
+
+ /*
+ * If the hardirq time is less than HARDIRQ_PERCENT_THRESH% in the last
+ * sample_period, then we suspect the interrupt storm might be subsiding.
+ */
+ if (!need_counting_irqs())
+ stop_counting_irqs();
+ }
+}
+
static void report_cpu_status(void)
{
print_cpustat();
+ print_irq_counts();
}
#else
static inline void update_cpustat(void) { }
static inline void report_cpu_status(void) { }
+static inline bool need_counting_irqs(void) { return false; }
+static inline void start_counting_irqs(void) { }
+static inline void stop_counting_irqs(void) { }
#endif
/*
@@ -527,6 +621,18 @@ static int is_softlockup(unsigned long touch_ts,
unsigned long now)
{
if ((watchdog_enabled & WATCHDOG_SOFTOCKUP_ENABLED) && watchdog_thresh) {
+ /*
+ * If period_ts has not been updated during a sample_period, then
+ * in the subsequent few sample_periods, period_ts might also not
+ * be updated, which could indicate a potential softlockup. In
+ * this case, if we suspect the cause of the potential softlockup
+ * might be interrupt storm, then we need to count the interrupts
+ * to find which interrupt is storming.
+ */
+ if (time_after_eq(now, period_ts + get_softlockup_thresh() / NUM_SAMPLE_PERIODS) &&
+ need_counting_irqs())
+ start_counting_irqs();
+
/* Warn about unreasonable delays. */
if (time_after(now, period_ts + get_softlockup_thresh()))
return now - touch_ts;
@@ -549,6 +655,7 @@ static DEFINE_PER_CPU(struct cpu_stop_work, softlockup_stop_work);
static int softlockup_fn(void *data)
{
update_touch_ts();
+ stop_counting_irqs();
complete(this_cpu_ptr(&softlockup_completion));
return 0;
--
2.37.1 (Apple Git-137.1)
在 2024/2/26 10:09, Bitao Hu 写道:
> The soft lockup detector lacks a mechanism to identify interrupt storms
> as root cause of a lockup. To enable this the detector needs a
> mechanism to snapshot the interrupt count statistics on a CPU when the
> detector observes a potential lockup scenario and compare that against
> the interrupt count when it warns about the lockup later on. The number
> of interrupts in that period give a hint whether the lockup might be
> caused by an interrupt storm.
>
> Instead of having extra storage in the lockup detector and accessing
> the internals of the interrupt descriptor directly, convert the per CPU
> irq_desc::kstat_irq member to a data structure which contains the
> counter plus a snapshot member and provide interfaces to take a
> snapshot of all interrupts on the current CPU and to retrieve the delta
> of a specific interrupt later on.
>
> Originally-by: Thomas Gleixner <[email protected]>
> Signed-off-by: Bitao Hu <[email protected]>
> ---
> arch/mips/dec/setup.c | 2 +-
> arch/parisc/kernel/smp.c | 2 +-
> arch/powerpc/kvm/book3s_hv_rm_xics.c | 2 +-
> include/linux/irqdesc.h | 9 ++++++--
> include/linux/kernel_stat.h | 3 +++
> kernel/irq/internals.h | 2 +-
> kernel/irq/irqdesc.c | 34 ++++++++++++++++++++++------
> kernel/irq/proc.c | 5 ++--
> scripts/gdb/linux/interrupts.py | 6 ++---
> 9 files changed, 46 insertions(+), 19 deletions(-)
>
> diff --git a/arch/mips/dec/setup.c b/arch/mips/dec/setup.c
> index 6c3704f51d0d..87f0a1436bf9 100644
> --- a/arch/mips/dec/setup.c
> +++ b/arch/mips/dec/setup.c
> @@ -756,7 +756,7 @@ void __init arch_init_irq(void)
> NULL))
> pr_err("Failed to register fpu interrupt\n");
> desc_fpu = irq_to_desc(irq_fpu);
> - fpu_kstat_irq = this_cpu_ptr(desc_fpu->kstat_irqs);
> + fpu_kstat_irq = this_cpu_ptr(&desc_fpu->kstat_irqs->cnt);
> }
> if (dec_interrupt[DEC_IRQ_CASCADE] >= 0) {
> if (request_irq(dec_interrupt[DEC_IRQ_CASCADE], no_action,
> diff --git a/arch/parisc/kernel/smp.c b/arch/parisc/kernel/smp.c
> index 444154271f23..800eb64e91ad 100644
> --- a/arch/parisc/kernel/smp.c
> +++ b/arch/parisc/kernel/smp.c
> @@ -344,7 +344,7 @@ static int smp_boot_one_cpu(int cpuid, struct task_struct *idle)
> struct irq_desc *desc = irq_to_desc(i);
>
> if (desc && desc->kstat_irqs)
> - *per_cpu_ptr(desc->kstat_irqs, cpuid) = 0;
> + *per_cpu_ptr(desc->kstat_irqs, cpuid) = (struct irqstat) { };
> }
> #endif
>
> diff --git a/arch/powerpc/kvm/book3s_hv_rm_xics.c b/arch/powerpc/kvm/book3s_hv_rm_xics.c
> index e42984878503..f2636414d82a 100644
> --- a/arch/powerpc/kvm/book3s_hv_rm_xics.c
> +++ b/arch/powerpc/kvm/book3s_hv_rm_xics.c
> @@ -837,7 +837,7 @@ static inline void this_cpu_inc_rm(unsigned int __percpu *addr)
> */
> static void kvmppc_rm_handle_irq_desc(struct irq_desc *desc)
> {
> - this_cpu_inc_rm(desc->kstat_irqs);
> + this_cpu_inc_rm(&desc->kstat_irqs->cnt);
> __this_cpu_inc(kstat.irqs_sum);
> }
>
> diff --git a/include/linux/irqdesc.h b/include/linux/irqdesc.h
> index d9451d456a73..2912b1998670 100644
> --- a/include/linux/irqdesc.h
> +++ b/include/linux/irqdesc.h
> @@ -17,6 +17,11 @@ struct irq_desc;
> struct irq_domain;
> struct pt_regs;
>
> +struct irqstat {
> + unsigned int cnt;
> + unsigned int ref;
> +};
> +
> /**
> * struct irq_desc - interrupt descriptor
> * @irq_common_data: per irq and chip data passed down to chip functions
> @@ -55,7 +60,7 @@ struct pt_regs;
> struct irq_desc {
> struct irq_common_data irq_common_data;
> struct irq_data irq_data;
> - unsigned int __percpu *kstat_irqs;
> + struct irqstat __percpu *kstat_irqs;
> irq_flow_handler_t handle_irq;
> struct irqaction *action; /* IRQ action list */
> unsigned int status_use_accessors;
> @@ -119,7 +124,7 @@ extern struct irq_desc irq_desc[NR_IRQS];
> static inline unsigned int irq_desc_kstat_cpu(struct irq_desc *desc,
> unsigned int cpu)
> {
> - return desc->kstat_irqs ? *per_cpu_ptr(desc->kstat_irqs, cpu) : 0;
> + return desc->kstat_irqs ? per_cpu(desc->kstat_irqs->cnt, cpu) : 0;
> }
>
> static inline struct irq_desc *irq_data_to_desc(struct irq_data *data)
> diff --git a/include/linux/kernel_stat.h b/include/linux/kernel_stat.h
> index 9935f7ecbfb9..98b3043ea5e6 100644
> --- a/include/linux/kernel_stat.h
> +++ b/include/linux/kernel_stat.h
> @@ -79,6 +79,9 @@ static inline unsigned int kstat_cpu_softirqs_sum(int cpu)
> return sum;
> }
>
> +extern void kstat_snapshot_irqs(void);
> +extern unsigned int kstat_get_irq_since_snapshot(unsigned int irq);
> +
> /*
> * Number of interrupts per specific IRQ source, since bootup
> */
> diff --git a/kernel/irq/internals.h b/kernel/irq/internals.h
> index bcc7f21db9ee..1d92532c2aae 100644
> --- a/kernel/irq/internals.h
> +++ b/kernel/irq/internals.h
> @@ -258,7 +258,7 @@ static inline void irq_state_set_masked(struct irq_desc *desc)
>
> static inline void __kstat_incr_irqs_this_cpu(struct irq_desc *desc)
> {
> - __this_cpu_inc(*desc->kstat_irqs);
> + __this_cpu_inc(desc->kstat_irqs->cnt);
> __this_cpu_inc(kstat.irqs_sum);
> }
>
> diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c
> index 27ca1c866f29..9cd17080b2d8 100644
> --- a/kernel/irq/irqdesc.c
> +++ b/kernel/irq/irqdesc.c
> @@ -122,7 +122,7 @@ static void desc_set_defaults(unsigned int irq, struct irq_desc *desc, int node,
> desc->name = NULL;
> desc->owner = owner;
> for_each_possible_cpu(cpu)
> - *per_cpu_ptr(desc->kstat_irqs, cpu) = 0;
> + *per_cpu_ptr(desc->kstat_irqs, cpu) = (struct irqstat) { };
> desc_smp_init(desc, node, affinity);
> }
>
> @@ -418,8 +418,8 @@ static struct irq_desc *alloc_desc(int irq, int node, unsigned int flags,
> desc = kzalloc_node(sizeof(*desc), GFP_KERNEL, node);
> if (!desc)
> return NULL;
> - /* allocate based on nr_cpu_ids */
> - desc->kstat_irqs = alloc_percpu(unsigned int);
> +
> + desc->kstat_irqs = alloc_percpu(struct irqstat);
> if (!desc->kstat_irqs)
> goto err_desc;
>
> @@ -593,7 +593,7 @@ int __init early_irq_init(void)
> count = ARRAY_SIZE(irq_desc);
>
> for (i = 0; i < count; i++) {
> - desc[i].kstat_irqs = alloc_percpu(unsigned int);
> + desc[i].kstat_irqs = alloc_percpu(struct irqstat);
> alloc_masks(&desc[i], node);
> raw_spin_lock_init(&desc[i].lock);
> lockdep_set_class(&desc[i].lock, &irq_desc_lock_class);
> @@ -952,8 +952,7 @@ unsigned int kstat_irqs_cpu(unsigned int irq, int cpu)
> {
> struct irq_desc *desc = irq_to_desc(irq);
>
> - return desc && desc->kstat_irqs ?
> - *per_cpu_ptr(desc->kstat_irqs, cpu) : 0;
> + return desc && desc->kstat_irqs ? per_cpu(desc->kstat_irqs->cnt, cpu) : 0;
> }
>
> static bool irq_is_nmi(struct irq_desc *desc)
> @@ -975,10 +974,31 @@ static unsigned int kstat_irqs(unsigned int irq)
> return data_race(desc->tot_count);
>
> for_each_possible_cpu(cpu)
> - sum += data_race(*per_cpu_ptr(desc->kstat_irqs, cpu));
> + sum += data_race(per_cpu(desc->kstat_irqs->cnt, cpu));
> return sum;
> }
>
> +void kstat_snapshot_irqs(void)
> +{
> + struct irq_desc *desc;
> + unsigned int irq;
> +
> + for_each_irq_desc(irq, desc) {
> + if (!desc->kstat_irqs)
> + continue;
> + this_cpu_write(desc->kstat_irqs->ref, this_cpu_read(desc->kstat_irqs->cnt));
> + }
> +}
> +
> +unsigned int kstat_get_irq_since_snapshot(unsigned int irq)
> +{
> + struct irq_desc *desc = irq_to_desc(irq);
> +
> + if (!desc || !desc->kstat_irqs)
> + return 0;
> + return this_cpu_read(desc->kstat_irqs->cnt) - this_cpu_read(desc->kstat_irqs->ref);
> +}
> +
> /**
> * kstat_irqs_usr - Get the statistics for an interrupt from thread context
> * @irq: The interrupt number
> diff --git a/kernel/irq/proc.c b/kernel/irq/proc.c
> index 623b8136e9af..6954e0a02047 100644
> --- a/kernel/irq/proc.c
> +++ b/kernel/irq/proc.c
> @@ -490,7 +490,7 @@ int show_interrupts(struct seq_file *p, void *v)
>
> if (desc->kstat_irqs) {
> for_each_online_cpu(j)
> - any_count |= data_race(*per_cpu_ptr(desc->kstat_irqs, j));
> + any_count |= data_race(per_cpu(desc->kstat_irqs->cnt, j));
> }
>
> if ((!desc->action || irq_desc_is_chained(desc)) && !any_count)
> @@ -498,8 +498,7 @@ int show_interrupts(struct seq_file *p, void *v)
>
> seq_printf(p, "%*d: ", prec, i);
> for_each_online_cpu(j)
> - seq_printf(p, "%10u ", desc->kstat_irqs ?
> - *per_cpu_ptr(desc->kstat_irqs, j) : 0);
> + seq_printf(p, "%10u ", desc->kstat_irqs ? per_cpu(desc->kstat_irqs->cnt, j) : 0);
>
> raw_spin_lock_irqsave(&desc->lock, flags);
> if (desc->irq_data.chip) {
> diff --git a/scripts/gdb/linux/interrupts.py b/scripts/gdb/linux/interrupts.py
> index ef478e273791..7e50f3b9dfad 100644
> --- a/scripts/gdb/linux/interrupts.py
> +++ b/scripts/gdb/linux/interrupts.py
> @@ -37,7 +37,7 @@ def show_irq_desc(prec, irq):
> any_count = 0
> if desc['kstat_irqs']:
> for cpu in cpus.each_online_cpu():
> - any_count += cpus.per_cpu(desc['kstat_irqs'], cpu)
> + any_count += cpus.per_cpu(desc['kstat_irqs'], cpu)['cnt']
>
> if (desc['action'] == 0 or irq_desc_is_chained(desc)) and any_count == 0:
> return text;
> @@ -45,7 +45,7 @@ def show_irq_desc(prec, irq):
> text += "%*d: " % (prec, irq)
> for cpu in cpus.each_online_cpu():
> if desc['kstat_irqs']:
> - count = cpus.per_cpu(desc['kstat_irqs'], cpu)
> + count = cpus.per_cpu(desc['kstat_irqs'], cpu)['cnt']
> else:
> count = 0
> text += "%10u" % (count)
> @@ -177,7 +177,7 @@ def arm_common_show_interrupts(prec):
> if desc == 0:
> continue
> for cpu in cpus.each_online_cpu():
> - text += "%10u" % (cpus.per_cpu(desc['kstat_irqs'], cpu))
> + text += "%10u" % (cpus.per_cpu(desc['kstat_irqs'], cpu)['cnt'])
> text += " %s" % (ipi_types[ipi].string())
> text += "\n"
> return text
Looks good.
For the newly added struct irqstat, adding annotated comments to explain
each field would be beneficial.
Reviewed-by: Liu Song <[email protected]>
在 2024/2/26 10:09, Bitao Hu 写道:
> We could use the irq_desc::tot_count member to avoid the summation
> loop for interrupts which are not marked as 'PER_CPU' interrupts in
> 'show_interrupts'. This could reduce the time overhead of reading
> /proc/interrupts.
>
> Originally-by: Thomas Gleixner <[email protected]>
> Signed-off-by: Bitao Hu <[email protected]>
> ---
> include/linux/irqdesc.h | 2 ++
> kernel/irq/irqdesc.c | 2 +-
> kernel/irq/proc.c | 9 +++++++--
> 3 files changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/include/linux/irqdesc.h b/include/linux/irqdesc.h
> index 2912b1998670..1ee96d7232b4 100644
> --- a/include/linux/irqdesc.h
> +++ b/include/linux/irqdesc.h
> @@ -121,6 +121,8 @@ static inline void irq_unlock_sparse(void) { }
> extern struct irq_desc irq_desc[NR_IRQS];
> #endif
>
> +extern bool irq_is_nmi(struct irq_desc *desc);
> +
> static inline unsigned int irq_desc_kstat_cpu(struct irq_desc *desc,
> unsigned int cpu)
> {
> diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c
> index 9cd17080b2d8..56a767957a9d 100644
> --- a/kernel/irq/irqdesc.c
> +++ b/kernel/irq/irqdesc.c
> @@ -955,7 +955,7 @@ unsigned int kstat_irqs_cpu(unsigned int irq, int cpu)
> return desc && desc->kstat_irqs ? per_cpu(desc->kstat_irqs->cnt, cpu) : 0;
> }
>
> -static bool irq_is_nmi(struct irq_desc *desc)
> +bool irq_is_nmi(struct irq_desc *desc)
> {
> return desc->istate & IRQS_NMI;
> }
> diff --git a/kernel/irq/proc.c b/kernel/irq/proc.c
> index 6954e0a02047..b3b1b93f0410 100644
> --- a/kernel/irq/proc.c
> +++ b/kernel/irq/proc.c
> @@ -489,8 +489,13 @@ int show_interrupts(struct seq_file *p, void *v)
> goto outsparse;
>
> if (desc->kstat_irqs) {
> - for_each_online_cpu(j)
> - any_count |= data_race(per_cpu(desc->kstat_irqs->cnt, j));
> + if (!irq_settings_is_per_cpu_devid(desc) &&
> + !irq_settings_is_per_cpu(desc) &&
> + !irq_is_nmi(desc))
> + any_count = data_race(desc->tot_count);
> + else
> + for_each_online_cpu(j)
> + any_count |= data_race(per_cpu(desc->kstat_irqs->cnt, j));
> }
>
> if ((!desc->action || irq_desc_is_chained(desc)) && !any_count)
The modification borrows from the implementation of |kstat_irqs. Looks
good.|
|Reviewed-by: Liu Song <[email protected]> |
||
在 2024/2/26 10:09, Bitao Hu 写道:
> When the watchdog determines that the current soft lockup is due
> to an interrupt storm based on CPU utilization, reporting the
> most frequent interrupts could be good enough for further
> troubleshooting.
>
> Below is an example of interrupt storm. The call tree does not
> provide useful information, but we can analyze which interrupt
> caused the soft lockup by comparing the counts of interrupts.
>
> [ 638.870231] watchdog: BUG: soft lockup - CPU#9 stuck for 26s! [swapper/9:0]
> [ 638.870825] CPU#9 Utilization every 4s during lockup:
> [ 638.871194] #1: 0% system, 0% softirq, 100% hardirq, 0% idle
> [ 638.871652] #2: 0% system, 0% softirq, 100% hardirq, 0% idle
> [ 638.872107] #3: 0% system, 0% softirq, 100% hardirq, 0% idle
> [ 638.872563] #4: 0% system, 0% softirq, 100% hardirq, 0% idle
> [ 638.873018] #5: 0% system, 0% softirq, 100% hardirq, 0% idle
> [ 638.873494] CPU#9 Detect HardIRQ Time exceeds 50%. Most frequent HardIRQs:
> [ 638.873994] #1: 330945 irq#7
> [ 638.874236] #2: 31 irq#82
> [ 638.874493] #3: 10 irq#10
> [ 638.874744] #4: 2 irq#89
> [ 638.874992] #5: 1 irq#102
> ...
> [ 638.875313] Call trace:
> [ 638.875315] __do_softirq+0xa8/0x364
>
> Signed-off-by: Bitao Hu <[email protected]>
> ---
> kernel/watchdog.c | 115 ++++++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 111 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/watchdog.c b/kernel/watchdog.c
> index 69e72d7e461d..c9d49ae8d045 100644
> --- a/kernel/watchdog.c
> +++ b/kernel/watchdog.c
> @@ -12,22 +12,25 @@
>
> #define pr_fmt(fmt) "watchdog: " fmt
>
> -#include <linux/mm.h>
> #include <linux/cpu.h>
> -#include <linux/nmi.h>
> #include <linux/init.h>
> +#include <linux/irq.h>
> +#include <linux/irqdesc.h>
> #include <linux/kernel_stat.h>
> +#include <linux/kvm_para.h>
> #include <linux/math64.h>
> +#include <linux/mm.h>
> #include <linux/module.h>
> +#include <linux/nmi.h>
> +#include <linux/stop_machine.h>
> #include <linux/sysctl.h>
> #include <linux/tick.h>
> +
> #include <linux/sched/clock.h>
> #include <linux/sched/debug.h>
> #include <linux/sched/isolation.h>
> -#include <linux/stop_machine.h>
>
> #include <asm/irq_regs.h>
> -#include <linux/kvm_para.h>
>
> static DEFINE_MUTEX(watchdog_mutex);
>
> @@ -417,13 +420,104 @@ static void print_cpustat(void)
> }
> }
>
> +#define HARDIRQ_PERCENT_THRESH 50
> +#define NUM_HARDIRQ_REPORT 5
> +struct irq_counts {
> + int irq;
> + u32 counts;
> +};
> +
> +static DEFINE_PER_CPU(bool, snapshot_taken);
> +
> +/* Tabulate the most frequent interrupts. */
> +static void tabulate_irq_count(struct irq_counts *irq_counts, int irq, u32 counts, int rank)
> +{
> + int i;
> + struct irq_counts new_count = {irq, counts};
> +
> + for (i = 0; i < rank; i++) {
> + if (counts > irq_counts[i].counts)
> + swap(new_count, irq_counts[i]);
> + }
> +}
> +
> +/*
> + * If the hardirq time exceeds HARDIRQ_PERCENT_THRESH% of the sample_period,
> + * then the cause of softlockup might be interrupt storm. In this case, it
> + * would be useful to start interrupt counting.
> + */
> +static bool need_counting_irqs(void)
> +{
> + u8 util;
> + int tail = __this_cpu_read(cpustat_tail);
> +
> + tail = (tail + NUM_HARDIRQ_REPORT - 1) % NUM_HARDIRQ_REPORT;
> + util = __this_cpu_read(cpustat_util[tail][STATS_HARDIRQ]);
> + return util > HARDIRQ_PERCENT_THRESH;
> +}
> +
> +static void start_counting_irqs(void)
> +{
> + if (!__this_cpu_read(snapshot_taken)) {
> + kstat_snapshot_irqs();
> + __this_cpu_write(snapshot_taken, true);
> + }
> +}
> +
> +static void stop_counting_irqs(void)
> +{
> + __this_cpu_write(snapshot_taken, false);
> +}
> +
> +static void print_irq_counts(void)
> +{
> + unsigned int i, count;
> + struct irq_counts irq_counts_sorted[NUM_HARDIRQ_REPORT] = {
> + {-1, 0}, {-1, 0}, {-1, 0}, {-1, 0}, {-1, 0}
> + };
> +
> + if (__this_cpu_read(snapshot_taken)) {
> + for_each_active_irq(i) {
> + count = kstat_get_irq_since_snapshot(i);
> + tabulate_irq_count(irq_counts_sorted, i, count, NUM_HARDIRQ_REPORT);
> + }
> +
> + /*
> + * We do not want the "watchdog: " prefix on every line,
> + * hence we use "printk" instead of "pr_crit".
> + */
> + printk(KERN_CRIT "CPU#%d Detect HardIRQ Time exceeds %d%%. Most frequent HardIRQs:\n",
> + smp_processor_id(), HARDIRQ_PERCENT_THRESH);
> +
> + for (i = 0; i < NUM_HARDIRQ_REPORT; i++) {
> + if (irq_counts_sorted[i].irq == -1)
> + break;
> +
> + printk(KERN_CRIT "\t#%u: %-10u\tirq#%d\n",
> + i + 1, irq_counts_sorted[i].counts,
> + irq_counts_sorted[i].irq);
> + }
> +
> + /*
> + * If the hardirq time is less than HARDIRQ_PERCENT_THRESH% in the last
> + * sample_period, then we suspect the interrupt storm might be subsiding.
> + */
> + if (!need_counting_irqs())
> + stop_counting_irqs();
> + }
> +}
> +
> static void report_cpu_status(void)
> {
> print_cpustat();
> + print_irq_counts();
> }
> #else
> static inline void update_cpustat(void) { }
> static inline void report_cpu_status(void) { }
> +static inline bool need_counting_irqs(void) { return false; }
> +static inline void start_counting_irqs(void) { }
> +static inline void stop_counting_irqs(void) { }
> #endif
>
> /*
> @@ -527,6 +621,18 @@ static int is_softlockup(unsigned long touch_ts,
> unsigned long now)
> {
> if ((watchdog_enabled & WATCHDOG_SOFTOCKUP_ENABLED) && watchdog_thresh) {
> + /*
> + * If period_ts has not been updated during a sample_period, then
> + * in the subsequent few sample_periods, period_ts might also not
> + * be updated, which could indicate a potential softlockup. In
> + * this case, if we suspect the cause of the potential softlockup
> + * might be interrupt storm, then we need to count the interrupts
> + * to find which interrupt is storming.
> + */
> + if (time_after_eq(now, period_ts + get_softlockup_thresh() / NUM_SAMPLE_PERIODS) &&
> + need_counting_irqs())
> + start_counting_irqs();
> +
> /* Warn about unreasonable delays. */
> if (time_after(now, period_ts + get_softlockup_thresh()))
> return now - touch_ts;
> @@ -549,6 +655,7 @@ static DEFINE_PER_CPU(struct cpu_stop_work, softlockup_stop_work);
> static int softlockup_fn(void *data)
> {
> update_touch_ts();
> + stop_counting_irqs();
> complete(this_cpu_ptr(&softlockup_completion));
>
> return 0;
Looks good.
Reviewed-by: Liu Song <[email protected]>
On Mon, Feb 26 2024 at 10:09, Bitao Hu wrote:
> We could use the irq_desc::tot_count member to avoid the summation
> loop for interrupts which are not marked as 'PER_CPU' interrupts in
> 'show_interrupts'. This could reduce the time overhead of reading
> /proc/interrupts.
"Could" is not really a technical term. Either we do or we do not. Also
please provide context for your change and avoid the 'We'.
> --- a/include/linux/irqdesc.h
> +++ b/include/linux/irqdesc.h
> @@ -121,6 +121,8 @@ static inline void irq_unlock_sparse(void) { }
> extern struct irq_desc irq_desc[NR_IRQS];
> #endif
>
> +extern bool irq_is_nmi(struct irq_desc *desc);
> +
If at all this wants to be in kernel/irq/internal.h. There is zero
reason to expose this globally.
> -static bool irq_is_nmi(struct irq_desc *desc)
> +bool irq_is_nmi(struct irq_desc *desc)
> {
> return desc->istate & IRQS_NMI;
> }
If at all this really wants to be a static inline in internals.h, but
instead of blindly copying code this can be done smarter:
unsigned int kstat_irq_desc(struct irq_desc *desc)
{
unsigned int sum = 0;
int cpu;
if (!irq_settings_is_per_cpu_devid(desc) &&
!irq_settings_is_per_cpu(desc) &&
!irq_is_nmi(desc))
return data_race(desc->tot_count);
for_each_possible_cpu(cpu)
sum += data_race(*per_cpu_ptr(desc->kstat_irqs, cpu));
return sum;
}
and then let kstat_irqs() and show_interrupts() use it. See?
With that a proper changelog would be:
show_interrupts() unconditionally accumulates the per CPU interrupt
statistics to determine whether an interrupt was ever raised.
This can be avoided for all interrupts which are not strictly per CPU
and not of type NMI because those interrupts provide already an
accumulated counter. The required logic is already implemented in
kstat_irqs().
Split the inner access logic out of kstat_irqs() and use it for
kstat_irqs() and show_interrupts() to avoid the accumulation loop
when possible.
Thanks,
tglx
Hi,
On 2024/2/27 17:26, Thomas Gleixner wrote:
> On Mon, Feb 26 2024 at 10:09, Bitao Hu wrote:
>> We could use the irq_desc::tot_count member to avoid the summation
>> loop for interrupts which are not marked as 'PER_CPU' interrupts in
>> 'show_interrupts'. This could reduce the time overhead of reading
>> /proc/interrupts.
>
> "Could" is not really a technical term. Either we do or we do not. Also
> please provide context for your change and avoid the 'We'.
OK.
>
>> --- a/include/linux/irqdesc.h
>> +++ b/include/linux/irqdesc.h
>> @@ -121,6 +121,8 @@ static inline void irq_unlock_sparse(void) { }
>> extern struct irq_desc irq_desc[NR_IRQS];
>> #endif
>>
>> +extern bool irq_is_nmi(struct irq_desc *desc);
>> +
>
> If at all this wants to be in kernel/irq/internal.h. There is zero
> reason to expose this globally.
>
>> -static bool irq_is_nmi(struct irq_desc *desc)
>> +bool irq_is_nmi(struct irq_desc *desc)
>> {
>> return desc->istate & IRQS_NMI;
>> }
>
> If at all this really wants to be a static inline in internals.h, but
> instead of blindly copying code this can be done smarter:
>
> unsigned int kstat_irq_desc(struct irq_desc *desc)
> {
> unsigned int sum = 0;
> int cpu;
>
> if (!irq_settings_is_per_cpu_devid(desc) &&
> !irq_settings_is_per_cpu(desc) &&
> !irq_is_nmi(desc))
> return data_race(desc->tot_count);
>
> for_each_possible_cpu(cpu)
> sum += data_race(*per_cpu_ptr(desc->kstat_irqs, cpu));
> return sum;
> }
>
> and then let kstat_irqs() and show_interrupts() use it. See?
I have a concern. kstat_irqs() uses for_each_possible_cpu() for
summation. However, show_interrupts() uses for_each_online_cpu(),
which means it only outputs interrupt statistics for online cpus.
If we use for_each_possible_cpu() in show_interrupts() to calculate
'any_count', there could be a problem with the following scenario:
If an interrupt has a count of zero on online cpus but a non-zero
count on possible cpus, then 'any_count' would not be zero, and the
statistics for that interrupt would be output, which is not the
desired behavior for show_interrupts(). Therefore, I think it's not
good to have kstat_irqs() and show_interrupts() both use the same
logic. What do you think?
>
> With that a proper changelog would be:
>
> show_interrupts() unconditionally accumulates the per CPU interrupt
> statistics to determine whether an interrupt was ever raised.
>
> This can be avoided for all interrupts which are not strictly per CPU
> and not of type NMI because those interrupts provide already an
> accumulated counter. The required logic is already implemented in
> kstat_irqs().
>
> Split the inner access logic out of kstat_irqs() and use it for
> kstat_irqs() and show_interrupts() to avoid the accumulation loop
> when possible.
>
Best Regards,
Bitao Hu
On Tue, Feb 27 2024 at 19:20, Bitao Hu wrote:
> On 2024/2/27 17:26, Thomas Gleixner wrote:
>>
>> and then let kstat_irqs() and show_interrupts() use it. See?
>
> I have a concern. kstat_irqs() uses for_each_possible_cpu() for
> summation. However, show_interrupts() uses for_each_online_cpu(),
> which means it only outputs interrupt statistics for online cpus.
> If we use for_each_possible_cpu() in show_interrupts() to calculate
> 'any_count', there could be a problem with the following scenario:
> If an interrupt has a count of zero on online cpus but a non-zero
> count on possible cpus, then 'any_count' would not be zero, and the
> statistics for that interrupt would be output, which is not the
> desired behavior for show_interrupts(). Therefore, I think it's not
> good to have kstat_irqs() and show_interrupts() both use the same
> logic. What do you think?
Good point. But you simply can have
unsigned int kstat_irq_desc(struct irq_desc *desc, const struct cpumask *mask)
and hand in the appropriate cpumask, which still shares the code, no?
Thanks,
tglx
On 2024/2/27 23:39, Thomas Gleixner wrote:
> On Tue, Feb 27 2024 at 19:20, Bitao Hu wrote:
>> On 2024/2/27 17:26, Thomas Gleixner wrote:
>>>
>>> and then let kstat_irqs() and show_interrupts() use it. See?
>>
>> I have a concern. kstat_irqs() uses for_each_possible_cpu() for
>> summation. However, show_interrupts() uses for_each_online_cpu(),
>> which means it only outputs interrupt statistics for online cpus.
>> If we use for_each_possible_cpu() in show_interrupts() to calculate
>> 'any_count', there could be a problem with the following scenario:
>> If an interrupt has a count of zero on online cpus but a non-zero
>> count on possible cpus, then 'any_count' would not be zero, and the
>> statistics for that interrupt would be output, which is not the
>> desired behavior for show_interrupts(). Therefore, I think it's not
>> good to have kstat_irqs() and show_interrupts() both use the same
>> logic. What do you think?
>
> Good point. But you simply can have
>
> unsigned int kstat_irq_desc(struct irq_desc *desc, const struct cpumask *mask)
>
> and hand in the appropriate cpumask, which still shares the code, no?
>
Alright, that is a good approach.