[PATCH 0/2] Remove unnecessary NULL check of the kstat_irqs field

[PATCH 0/2] Remove unnecessary NULL check of the kstat_irqs field

[Radu Rendec]

The kstat_irqs field of struct irq_desc is used to store a per-cpu count
of interrupt events. It is initialized in init_desc(), along with all
the other fields in struct irq_desc that need explicit initialization,
and therefore it's always available (non-NULL) for any valid interrupt
descriptor.

This series removes the unnecessary NULL check of the kstat_irqs field.
The first patch addresses the generic IRQ code, and the second patch
addresses one isolated occurrence in the parisc code. The first patch
also includes a more detailed description of why removing the NULL check
is safe.

Radu Rendec (2):
  genirq: Remove unnecessary NULL check of the kstat_irqs field
  parisc: Remove unnecessary NULL check of the kstat_irqs field

 arch/parisc/kernel/smp.c |  2 +-
 include/linux/irqdesc.h  |  2 +-
 kernel/irq/irqdesc.c     | 11 ++++-------
 3 files changed, 6 insertions(+), 9 deletions(-)

-- 
2.54.0

[PATCH 1/2] genirq: Remove unnecessary NULL check of the kstat_irqs field

[Radu Rendec]

The kstat_irqs field of struct irq_desc is used to store a per-cpu count
of interrupt events. It is initialized in init_desc(), along with all
the other fields in struct irq_desc that need explicit initialization,
and therefore it's always available (non-NULL) for any valid interrupt
descriptor (with a caveat - see below).

When CONFIG_SPARSE_IRQ is enabled, all interrupt descriptors are always
allocated dynamically via alloc_desc(), which calls init_desc(), so in
that case kstat_irqs is guaranteed to be non-NULL before a valid struct
irq_desc pointer is even returned.

By contrast, when CONFIG_SPARSE_IRQ is disabled, interrupt descriptors
are allocated statically in the irq_desc[] array, and kstat_irqs is
initialized implicitly to NULL. The per-cpu pointer is initialized only
later, for all descriptors, via start_kernel() -> early_irq_init() ->
init_desc().

The kstat_irqs field is used mostly for printing interrupt statistics
(i.e. reading /proc/interrupts), and that cannot happen until much
later, when user-space is fully initialized. So, there is no concern
with that use case.

The list below includes all functions where the NULL check is removed,
along with a list of all possible call chains and/or a brief explanation
of why it's safe to remove the NULL check in that case.

* irq_desc_kstat_cpu() [include/linux/irqdesc.h]
  - All direct call sites use it for printing IRQ statistics.
  - Indirect call site: per_cpu_count_show() - also used for printing
    IRQ statistics.
* kstat_irqs_cpu() [kernel/irq/irqdesc.c]
  - Called by sun3_int7() and sun3_int5() [arch/m68k/sun3/sun3ints.c]
    These are interrupt handlers and cannot be called until their
    corresponding interrupts are initialized in sun3_init_IRQ(). The
    call chain leading to that is:
      start_kernel() -> init_IRQ() [arch/m68k/kernel/ints.c] ->
      mach_init_IRQ = sun3_init_IRQ()
    The init_IRQ() call happens right *after* the early_irq_init() call,
    which means the descriptors are already fully initialized by the
    time the interrupt handlers are even registered.
  - Called by show_interrupts() [arch/s390/kernel/irq.c] - used for
    printing IRQ statistics.
* kstat_irqs() [kernel/irq/irqdesc.c]
  The only possible call chain is via fs/proc/stat.c:
    stat_open() -> show_stat() -> show_all_irqs() -> kstat_irqs_usr() ->
    kstat_irqs()
  It is used for printing IRQ statistics.
* kstat_snapshot_irqs()
  The only possible call chain is via kernel/watchdog.c:
    watchdog_timer_fn() -> is_softlockup() -> start_counting_irqs() ->
    kstat_snapshot_irqs()
  The watchdog timer cannot fire early, before early_irq_init().
* kstat_get_irq_since_snapshot()
  The only possible call chain is via kernel/watchdog.c:
    watchdog_timer_fn() -> report_cpu_status() -> print_irq_counts() ->
    kstat_get_irq_since_snapshot()
  The watchdog timer cannot fire early, before early_irq_init().

Signed-off-by: Radu Rendec <radu@rendec.net>
---
 include/linux/irqdesc.h |  2 +-
 kernel/irq/irqdesc.c    | 11 ++++-------
 2 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/include/linux/irqdesc.h b/include/linux/irqdesc.h
index 8080db17c1b1..779d6023c5c2 100644
--- a/include/linux/irqdesc.h
+++ b/include/linux/irqdesc.h
@@ -146,7 +146,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(desc->kstat_irqs->cnt, cpu) : 0;
+	return per_cpu(desc->kstat_irqs->cnt, cpu);
 }
 
 static inline struct irq_desc *irq_data_to_desc(struct irq_data *data)
diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c
index 80ef4e27dcf4..3a818f07a101 100644
--- a/kernel/irq/irqdesc.c
+++ b/kernel/irq/irqdesc.c
@@ -1004,7 +1004,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(desc->kstat_irqs->cnt, cpu) : 0;
+	return desc ? irq_desc_kstat_cpu(desc, cpu) : 0;
 }
 
 static unsigned int kstat_irqs_desc(struct irq_desc *desc, const struct cpumask *cpumask)
@@ -1026,7 +1026,7 @@ static unsigned int kstat_irqs(unsigned int irq)
 {
 	struct irq_desc *desc = irq_to_desc(irq);
 
-	if (!desc || !desc->kstat_irqs)
+	if (!desc)
 		return 0;
 	return kstat_irqs_desc(desc, cpu_possible_mask);
 }
@@ -1038,18 +1038,15 @@ void kstat_snapshot_irqs(void)
 	struct irq_desc *desc;
 	unsigned int irq;
 
-	for_each_irq_desc(irq, desc) {
-		if (!desc->kstat_irqs)
-			continue;
+	for_each_irq_desc(irq, desc)
 		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)
+	if (!desc)
 		return 0;
 	return this_cpu_read(desc->kstat_irqs->cnt) - this_cpu_read(desc->kstat_irqs->ref);
 }
-- 
2.54.0

[PATCH 2/2] parisc: Remove unnecessary NULL check of the kstat_irqs field

[Radu Rendec]

The kstat_irqs field of struct irq_desc is used to store a per-cpu count
of interrupt events. It is initialized in init_desc(), along with all
the other fields in struct irq_desc that need explicit initialization,
and therefore it's always available (non-NULL) for any valid interrupt
descriptor.

On parisc, CONFIG_SPARSE_IRQ is disabled, interrupt descriptors are
allocated statically in the irq_desc[] array, and kstat_irqs is
initialized implicitly to NULL. The kstat_irqs field is initialized
later, for all descriptors, via start_kernel() -> early_irq_init() ->
init_desc().

smp_boot_one_cpu() is used only for CPU hotplugging. On a SMP system,
the boot CPU initializes the interrupt descriptors as described above,
and smp_boot_one_cpu() is called later, for the secondary CPU(s).

The previous patch in this series makes a similar change across the
generic IRQ code in kernel/irq/irqdesc.c, and provides a more detailed
explanation of why kstat_irqs is guaranteed to be non-NULL.

Signed-off-by: Radu Rendec <radu@rendec.net>
---
 arch/parisc/kernel/smp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/parisc/kernel/smp.c b/arch/parisc/kernel/smp.c
index b2d12ab728b1..36be17e32948 100644
--- a/arch/parisc/kernel/smp.c
+++ b/arch/parisc/kernel/smp.c
@@ -343,7 +343,7 @@ static int smp_boot_one_cpu(int cpuid, struct task_struct *idle)
 	for (i = 0; i < NR_IRQS; i++) {
 		struct irq_desc *desc = irq_to_desc(i);
 
-		if (desc && desc->kstat_irqs)
+		if (desc)
 			*per_cpu_ptr(desc->kstat_irqs, cpuid) = (struct irqstat) { };
 	}
 #endif
-- 
2.54.0