Hi,Sorry, I misunderstood your point, thinking that they should only be
On Tue, Feb 6, 2024 at 1:59 AM Bitao Hu <yaoma@xxxxxxxxxxxxxxxxx> wrote:
diff --git a/kernel/watchdog.c b/kernel/watchdog.c
index 71d5b6dfa358..26dc1ad86276 100644
--- a/kernel/watchdog.c
+++ b/kernel/watchdog.c
@@ -18,6 +18,9 @@
#include <linux/init.h>
#include <linux/kernel_stat.h>
#include <linux/math64.h>
+#include <linux/irq.h>
+#include <linux/irqdesc.h>
+#include <linux/bitops.h>
These are still not sorted alphabetically. "irq.h" and "irqdesc.h"
should go between "init.h" and "kernel_stat.h". "bitops.h" is trickier
because the existing headers are not quite sorted. Probably the best
would be to fully sort them. They should end up like this:
#include <linux/bitops.h>
#include <linux/cpu.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 <asm/irq_regs.h>
Sure, the existing variables are sufficient for making a determination.
+static void start_counting_irqs(void)
+{
+ int i;
+ struct irq_desc *desc;
+ u32 *counts = __this_cpu_read(hardirq_counts);
+ int cpu = smp_processor_id();
+
+ if (!test_bit(cpu, softlockup_hardirq_cpus)) {
I don't think you need "softlockup_hardirq_cpus", do you? Just read
"actual_nr_irqs" and see if it's non-zero? ...or read "hardirq_counts"
and see if it's non-NULL?
OK.
+ /*
+ * nr_irqs has the potential to grow at runtime. We should read
+ * it and store locally to avoid array out-of-bounds access.
+ */
+ __this_cpu_write(actual_nr_irqs, nr_irqs);
nit: IMO store nr_irqs in a local variable to avoid all of the
"__this_cpu_read" calls everywhere. Then just write it once from your
local variable.
OK, I will use "kcalloc()" here.
+ counts = kmalloc_array(__this_cpu_read(actual_nr_irqs),
+ sizeof(u32),
+ GFP_ATOMIC);
should use "kcalloc()" so the array is zeroed. That way if the set of
non-NULL "desc"s changes between calls you don't end up reading
uninitialized memory.
OK.
+static void stop_counting_irqs(void)
+{
+ u32 *counts = __this_cpu_read(hardirq_counts);
+ int cpu = smp_processor_id();
+
+ if (test_bit(cpu, softlockup_hardirq_cpus)) {
+ kfree(counts);
+ counts = NULL;
+ __this_cpu_write(hardirq_counts, counts);
nit: don't really need to set the local "counts" to NULL. Just:
__this_cpu_write(hardirq_counts, NULL);
...and actually if you take my advice above and get rid of
"softlockup_hardirq_cpus" then this function just becomes:
kfree(__this_cpu_read(hardirq_counts));
__this_cpu_write(hardirq_counts, NULL);
Since kfree() handles when you pass it NULL...
Thanks for your reminder.
+static void print_irq_counts(void)
+{
+ int i;
+ struct irq_desc *desc;
+ u32 counts_diff;
+ u32 *counts = __this_cpu_read(hardirq_counts);
+ int cpu = smp_processor_id();
+ struct irq_counts irq_counts_sorted[NUM_HARDIRQ_REPORT] = {
+ {-1, 0}, {-1, 0}, {-1, 0}, {-1, 0},
+ };
+
+ if (test_bit(cpu, softlockup_hardirq_cpus)) {
+ for_each_irq_desc(i, desc) {
+ if (!desc)
+ continue;
The "if" test above isn't needed. The "for_each_irq_desc()" macro
already checks for NULL.
Although "alloc_desc" wil allocate both "desc" and "kstat_irqs" at the
+ /*
+ * We need to bounds-check in case someone on a different CPU
+ * expanded nr_irqs.
+ */
+ if (i < __this_cpu_read(actual_nr_irqs))
+ counts_diff = desc->kstat_irqs ?
+ *this_cpu_ptr(desc->kstat_irqs) - counts[i] : 0;
+ else
+ counts_diff = desc->kstat_irqs ?
+ *this_cpu_ptr(desc->kstat_irqs) : 0;
Why do you need to test "kstat_irqs" for 0?
duplicate the math. In other words, I'd expect this (untested):Agree.
if (i < __this_cpu_read(actual_nr_irqs))
count = counts[i];
else
count = 0;
counts_diff = *this_cpu_ptr(desc->kstat_irqs) - count;
I guess I'd also put "__this_cpu_read(actual_nr_irqs)" in a local
variable like you do with counts...