[PATCH 00/21] Reduce the scope of 'nr_irqs'

[PATCH 00/21] Reduce the scope of 'nr_irqs'

[Bart Van Assche]

Hi Thomas,

In addition to the global 'nr_irqs' variable, there are plenty of local
variables with the same name. This makes reviewing kernel patches and
auditing kernel source code harder than necessary. Hence this patch series
that reduces the scope of the global 'nr_irqs' variable to file scope and
that introduces functions for retrieving and setting the value of this
variable. Please consider this patch series for the next merge window.

All patches in this series except the first and the last two have been
generated by applying the following Coccinelle semantic patch on the
kernel tree:

// git grep -lw nr_irqs | grep -Ev 'kernel/irq/irqdesc.c|include/linux/irqnr.h' | while read -r f; do spatch --sp-file replace-nr_irqs.spatch --in-place "$f"; done
@@
global idexpression nr_irqs =~ "^nr_irqs$";
expression E;
@@
-nr_irqs = E;
-return nr_irqs;
+return set_number_of_interrupts(E);

@@
global idexpression nr_irqs =~ "^nr_irqs$";
expression E;
@@
-nr_irqs = (E);
+set_number_of_interrupts(E);

@@
global idexpression nr_irqs =~ "^nr_irqs$";
expression E;
@@
-nr_irqs = E;
+set_number_of_interrupts(E);

@@
global idexpression nr_irqs =~ "^nr_irqs$";
@@
-nr_irqs
+number_of_interrupts()

Thanks,

Bart.

Bart Van Assche (21):
  genirq: Introduce number_of_interrupts() and
    set_number_of_interrupts()
  ARM: Switch to number_of_interrupts() / set_number_of_interrupts()
  LoongArch: Switch to set_number_of_interrupts()
  powerpc/cell: Switch to number_of_interrupts()
  s390/irq: Switch to number_of_interrupts()
  x86/acpi: Switch to number_of_interrupts() /
    set_number_of_interrupts()
  hpet: Switch to number_of_interrupts()
  net: 3com: 3c59x: Switch to number_of_interrupts()
  net: hamradio: baycom_ser_fdx: Switch to number_of_interrupts()
  scsi: aha152x: Switch to number_of_interrupts()
  serial: core: Switch to number_of_interrupts()
  serial: 8250: Switch to number_of_interrupts()
  serial: amba-pl010: Switch to number_of_interrupts()
  serial: amba-pl011: Switch to number_of_interrupts()
  serial: cpm_uart: Switch to number_of_interrupts()
  serial: ucc_uart: Switch to number_of_interrupts()
  sh: intc: Switch to number_of_interrupts()
  xen/events: Switch to number_of_interrupts()
  fs/procfs: Switch to number_of_interrupts()
  genirq: Switch to number_of_interrupts()
  genirq: Unexport nr_irqs

 arch/arm/kernel/irq.c                  |  5 ++---
 arch/loongarch/kernel/irq.c            |  4 ++--
 arch/powerpc/platforms/cell/axon_msi.c |  2 +-
 arch/s390/kernel/irq.c                 |  2 +-
 arch/x86/kernel/acpi/boot.c            |  6 ++++--
 arch/x86/kernel/apic/vector.c          |  8 ++++----
 drivers/char/hpet.c                    |  2 +-
 drivers/net/ethernet/3com/3c59x.c      |  2 +-
 drivers/net/hamradio/baycom_ser_fdx.c  |  4 ++--
 drivers/net/hamradio/scc.c             |  6 +++---
 drivers/scsi/aha152x.c                 |  2 +-
 drivers/sh/intc/virq-debugfs.c         |  2 +-
 drivers/tty/serial/8250/8250_port.c    |  2 +-
 drivers/tty/serial/amba-pl010.c        |  2 +-
 drivers/tty/serial/amba-pl011.c        |  2 +-
 drivers/tty/serial/cpm_uart.c          |  2 +-
 drivers/tty/serial/serial_core.c       |  2 +-
 drivers/tty/serial/ucc_uart.c          |  2 +-
 drivers/xen/events/events_base.c       |  2 +-
 fs/proc/interrupts.c                   |  4 ++--
 fs/proc/stat.c                         |  4 ++--
 include/linux/irqnr.h                  | 18 ++++++++++--------
 kernel/irq/irqdesc.c                   | 17 +++++++++++++++--
 kernel/irq/irqdomain.c                 |  2 +-
 kernel/irq/proc.c                      |  5 +++--
 25 files changed, 63 insertions(+), 46 deletions(-)

[PATCH 01/21] genirq: Introduce number_of_interrupts() and set_number_of_interrupts()

[Bart Van Assche]

This patch prepares for changing 'nr_irqs' from an exported global variable
into a variable with file scope.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 include/linux/irqnr.h |  2 ++
 kernel/irq/irqdesc.c  | 14 ++++++++++++++
 2 files changed, 16 insertions(+)

diff --git a/include/linux/irqnr.h b/include/linux/irqnr.h
index 3496baa0b07f..a8b2cb6146e8 100644
--- a/include/linux/irqnr.h
+++ b/include/linux/irqnr.h
@@ -6,6 +6,8 @@
 
 
 extern int nr_irqs;
+int number_of_interrupts(void) __pure;
+int set_number_of_interrupts(int nr);
 extern struct irq_desc *irq_to_desc(unsigned int irq);
 unsigned int irq_get_next_irq(unsigned int offset);
 
diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c
index 1dee88ba0ae4..8c6280843964 100644
--- a/kernel/irq/irqdesc.c
+++ b/kernel/irq/irqdesc.c
@@ -141,6 +141,20 @@ static void desc_set_defaults(unsigned int irq, struct irq_desc *desc, int node,
 int nr_irqs = NR_IRQS;
 EXPORT_SYMBOL_GPL(nr_irqs);
 
+int number_of_interrupts(void)
+{
+	return nr_irqs;
+}
+EXPORT_SYMBOL_GPL(number_of_interrupts);
+
+int set_number_of_interrupts(int nr)
+{
+	nr_irqs = nr;
+
+	return nr;
+}
+EXPORT_SYMBOL_GPL(set_number_of_interrupts);
+
 static DEFINE_MUTEX(sparse_irq_lock);
 static struct maple_tree sparse_irqs = MTREE_INIT_EXT(sparse_irqs,
 					MT_FLAGS_ALLOC_RANGE |

Re: [PATCH 01/21] genirq: Introduce number_of_interrupts() and set_number_of_interrupts()

[Thomas Gleixner]

On Mon, Sep 30 2024 at 11:15, Bart Van Assche wrote:
> This patch prepares for changing 'nr_irqs' from an exported global
> variable

git grep 'This patch' Documentation/process/

> into a variable with file scope.

Also what's the rationale for this?

>  
>  extern int nr_irqs;
> +int number_of_interrupts(void) __pure;
> +int set_number_of_interrupts(int nr);

Please use a proper name space prefix for the functions
irq_.....(). These random names are horrible.

>  extern struct irq_desc *irq_to_desc(unsigned int irq);
>  unsigned int irq_get_next_irq(unsigned int offset);
>  
> diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c
> index 1dee88ba0ae4..8c6280843964 100644
> --- a/kernel/irq/irqdesc.c
> +++ b/kernel/irq/irqdesc.c
> @@ -141,6 +141,20 @@ static void desc_set_defaults(unsigned int irq, struct irq_desc *desc, int node,
>  int nr_irqs = NR_IRQS;
>  EXPORT_SYMBOL_GPL(nr_irqs);
>  
> +int number_of_interrupts(void)
> +{
> +	return nr_irqs;

Why is this int? The number of interrupts is strictly positive, no?

Re: [PATCH 01/21] genirq: Introduce number_of_interrupts() and set_number_of_interrupts()

[Bart Van Assche]

On 10/1/24 5:33 AM, Thomas Gleixner wrote:
> On Mon, Sep 30 2024 at 11:15, Bart Van Assche wrote:
>> This patch prepares for changing 'nr_irqs' from an exported global
>> variable
> 
> git grep 'This patch' Documentation/process/

Is this the documentation that you are referring to? Anyway, I will 
change the patch description into the imperative mood. <quote>Describe
your changes in imperative mood, e.g. "make xyzzy do frotz"
instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy
to do frotz", as if you are giving orders to the codebase to change
its behaviour.</quote>

>> into a variable with file scope.
> 
> Also what's the rationale for this?

Suppose that a patch would be submitted for review that removes a
declaration of a local variable with the name 'nr_irqs' and that does
not remove all assignments to that local variable. Such a patch converts
an assignment to a local variable into an assignment into a global
variable. If the 'nr_irqs' assignment is more than three lines away from 
other changes, the assignment won't be included in the diff context 
lines and hence won't be visible without inspecting the modified file.
This is why I mentioned in the cover letter that this change makes
patches easier to review. With this patch series applied, such
accidental conversions from assignments to a local variable into an
assignment to a global variable are converted into a compilation error.

>>   extern int nr_irqs;
>> +int number_of_interrupts(void) __pure;
>> +int set_number_of_interrupts(int nr);
> 
> Please use a proper name space prefix for the functions
> irq_.....(). These random names are horrible.

How about irq_count() and irq_set_count()?

>> +int number_of_interrupts(void)
>> +{
>> +	return nr_irqs;
> 
> Why is this int? The number of interrupts is strictly positive, no?

Yes, the number of interrupts is strictly positive. The return type
comes from the type of 'nr_irqs' and been chosen to minimize the risk of
the changes in this patch series. Anyway, I will audit the code that
reads and sets the global 'nr_irqs' variable to see whether its type can
be changed safely into 'unsigned int'.

Thanks,

Bart.

Re: [PATCH 01/21] genirq: Introduce number_of_interrupts() and set_number_of_interrupts()

[Thomas Gleixner]

On Tue, Oct 01 2024 at 13:12, Bart Van Assche wrote:
> On 10/1/24 5:33 AM, Thomas Gleixner wrote:
>> On Mon, Sep 30 2024 at 11:15, Bart Van Assche wrote:
>>> This patch prepares for changing 'nr_irqs' from an exported global
>>> variable
>> 
>> git grep 'This patch' Documentation/process/
>
> Is this the documentation that you are referring to? Anyway, I will 
> change the patch description into the imperative mood. <quote>Describe
> your changes in imperative mood, e.g. "make xyzzy do frotz"
> instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy
> to do frotz", as if you are giving orders to the codebase to change
> its behaviour.</quote>

Yes.

>>> into a variable with file scope.
>> 
>> Also what's the rationale for this?
>
> Suppose that a patch would be submitted for review that removes a
> declaration of a local variable with the name 'nr_irqs' and that does
> not remove all assignments to that local variable. Such a patch converts
> an assignment to a local variable into an assignment into a global
> variable. If the 'nr_irqs' assignment is more than three lines away from 
> other changes, the assignment won't be included in the diff context 
> lines and hence won't be visible without inspecting the modified file.
> This is why I mentioned in the cover letter that this change makes
> patches easier to review. With this patch series applied, such
> accidental conversions from assignments to a local variable into an
> assignment to a global variable are converted into a compilation
> error.

Can you please add that to the change log?

>>>   extern int nr_irqs;
>>> +int number_of_interrupts(void) __pure;
>>> +int set_number_of_interrupts(int nr);
>> 
>> Please use a proper name space prefix for the functions
>> irq_.....(). These random names are horrible.
>
> How about irq_count() and irq_set_count()?

Sure.

>>> +int number_of_interrupts(void)
>>> +{
>>> +	return nr_irqs;
>> 
>> Why is this int? The number of interrupts is strictly positive, no?
>
> Yes, the number of interrupts is strictly positive. The return type
> comes from the type of 'nr_irqs' and been chosen to minimize the risk of
> the changes in this patch series. Anyway, I will audit the code that
> reads and sets the global 'nr_irqs' variable to see whether its type can
> be changed safely into 'unsigned int'.

Thank you!

      tglx

Re: [PATCH 01/21] genirq: Introduce number_of_interrupts() and set_number_of_interrupts()

[Bart Van Assche]

On 10/2/24 8:49 AM, Thomas Gleixner wrote:
> On Tue, Oct 01 2024 at 13:12, Bart Van Assche wrote:
>> On 10/1/24 5:33 AM, Thomas Gleixner wrote:
>>> On Mon, Sep 30 2024 at 11:15, Bart Van Assche wrote:
>>>> into a variable with file scope.
>>>
>>> Also what's the rationale for this?
>>
>> Suppose that a patch would be submitted for review that removes a
>> declaration of a local variable with the name 'nr_irqs' and that does
>> not remove all assignments to that local variable. Such a patch converts
>> an assignment to a local variable into an assignment into a global
>> variable. If the 'nr_irqs' assignment is more than three lines away from
>> other changes, the assignment won't be included in the diff context
>> lines and hence won't be visible without inspecting the modified file.
>> This is why I mentioned in the cover letter that this change makes
>> patches easier to review. With this patch series applied, such
>> accidental conversions from assignments to a local variable into an
>> assignment to a global variable are converted into a compilation
>> error.
> 
> Can you please add that to the change log?

I will do that.

>>>>    extern int nr_irqs;
>>>> +int number_of_interrupts(void) __pure;
>>>> +int set_number_of_interrupts(int nr);
>>>
>>> Please use a proper name space prefix for the functions
>>> irq_.....(). These random names are horrible.
>>
>> How about irq_count() and irq_set_count()?
> 
> Sure.

I just noticed that a macro with the name irq_count() already exists.
How about the names irq_get_nr_irqs() and irq_set_nr_irqs() instead?

Thanks,

Bart.

Re: [PATCH 01/21] genirq: Introduce number_of_interrupts() and set_number_of_interrupts()

[Thomas Gleixner]

On Thu, Oct 03 2024 at 14:01, Bart Van Assche wrote:
> On 10/2/24 8:49 AM, Thomas Gleixner wrote:
>>> How about irq_count() and irq_set_count()?
>> 
>> Sure.
>
> I just noticed that a macro with the name irq_count() already exists.
> How about the names irq_get_nr_irqs() and irq_set_nr_irqs() instead?

Sounds good to me.

Thanks,

        tglx

[PATCH 02/21] ARM: Switch to number_of_interrupts() / set_number_of_interrupts()

[Bart Van Assche]

Use the number_of_interrupts() and set_number_of_interrupts() functions
instead of the global variable 'nr_irqs'. This patch prepares for changing
'nr_irqs' from an exported global variable into a variable with file scope.

Cc: Russell King <linux@armlinux.org.uk>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 arch/arm/kernel/irq.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/arch/arm/kernel/irq.c b/arch/arm/kernel/irq.c
index dab42d066d06..cac062d5c8d5 100644
--- a/arch/arm/kernel/irq.c
+++ b/arch/arm/kernel/irq.c
@@ -111,7 +111,7 @@ void handle_IRQ(unsigned int irq, struct pt_regs *regs)
 	 * Some hardware gives randomly wrong interrupts.  Rather
 	 * than crashing, do something sensible.
 	 */
-	if (unlikely(!irq || irq >= nr_irqs))
+	if (unlikely(!irq || irq >= number_of_interrupts()))
 		desc = NULL;
 	else
 		desc = irq_to_desc(irq);
@@ -151,7 +151,6 @@ void __init init_IRQ(void)
 #ifdef CONFIG_SPARSE_IRQ
 int __init arch_probe_nr_irqs(void)
 {
-	nr_irqs = machine_desc->nr_irqs ? machine_desc->nr_irqs : NR_IRQS;
-	return nr_irqs;
+	return set_number_of_interrupts(machine_desc->nr_irqs ? : NR_IRQS);
 }
 #endif

[PATCH 03/21] LoongArch: Switch to set_number_of_interrupts()

[Bart Van Assche]

Use the set_number_of_interrupts() function instead of the global variable
'nr_irqs'. This patch prepares for changing 'nr_irqs' from an exported
global variable into a variable with file scope.

Cc: Huacai Chen <chenhuacai@kernel.org>
Cc: WANG Xuerui <kernel@xen0n.name>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 arch/loongarch/kernel/irq.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/loongarch/kernel/irq.c b/arch/loongarch/kernel/irq.c
index d129039b368b..8eae704ab5e4 100644
--- a/arch/loongarch/kernel/irq.c
+++ b/arch/loongarch/kernel/irq.c
@@ -92,9 +92,9 @@ int __init arch_probe_nr_irqs(void)
 	int nr_io_pics = bitmap_weight(loongson_sysconf.cores_io_master, NR_CPUS);
 
 	if (!cpu_has_avecint)
-		nr_irqs = (64 + NR_VECTORS * nr_io_pics);
+		set_number_of_interrupts(64 + NR_VECTORS * nr_io_pics);
 	else
-		nr_irqs = (64 + NR_VECTORS * (nr_cpu_ids + nr_io_pics));
+		set_number_of_interrupts(64 + NR_VECTORS * (nr_cpu_ids + nr_io_pics));
 
 	return NR_IRQS_LEGACY;
 }

[PATCH 04/21] powerpc/cell: Switch to number_of_interrupts()

[Bart Van Assche]

Use the number_of_interrupts() function instead of the global variable
'nr_irqs'. This patch prepares for changing 'nr_irqs' from an exported
global variable into a variable with file scope.

Cc: linuxppc-dev@lists.ozlabs.org
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 arch/powerpc/platforms/cell/axon_msi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/powerpc/platforms/cell/axon_msi.c b/arch/powerpc/platforms/cell/axon_msi.c
index 28dc86744cac..b7996bc091e6 100644
--- a/arch/powerpc/platforms/cell/axon_msi.c
+++ b/arch/powerpc/platforms/cell/axon_msi.c
@@ -112,7 +112,7 @@ static void axon_msi_cascade(struct irq_desc *desc)
 		pr_devel("axon_msi: woff %x roff %x msi %x\n",
 			  write_offset, msic->read_offset, msi);
 
-		if (msi < nr_irqs && irq_get_chip_data(msi) == msic) {
+		if (msi < number_of_interrupts() && irq_get_chip_data(msi) == msic) {
 			generic_handle_irq(msi);
 			msic->fifo_virt[idx] = cpu_to_le32(0xffffffff);
 		} else {

[PATCH 05/21] s390/irq: Switch to number_of_interrupts()

[Bart Van Assche]

Use the number_of_interrupts() function instead of the global variable
'nr_irqs'. This patch prepares for changing 'nr_irqs' from an exported
global variable into a variable with file scope.

Cc: Heiko Carstens <hca@linux.ibm.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 arch/s390/kernel/irq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/s390/kernel/irq.c b/arch/s390/kernel/irq.c
index 2639a3d12736..f3842d5eb2c3 100644
--- a/arch/s390/kernel/irq.c
+++ b/arch/s390/kernel/irq.c
@@ -253,7 +253,7 @@ int show_interrupts(struct seq_file *p, void *v)
 		seq_putc(p, '\n');
 		goto out;
 	}
-	if (index < nr_irqs) {
+	if (index < number_of_interrupts()) {
 		show_msi_interrupt(p, index);
 		goto out;
 	}

[PATCH 06/21] x86/acpi: Switch to number_of_interrupts() / set_number_of_interrupts()

[Bart Van Assche]

Use the number_of_interrupts() and set_number_of_interrupts() functions
instead of the global variable 'nr_irqs'. This patch prepares for changing
'nr_irqs' from an exported global variable into a variable with file scope.

Cc: Ingo Molnar <mingo@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Rafael J. Wysocki <rafael@kernel.org>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 arch/x86/kernel/acpi/boot.c   | 6 ++++--
 arch/x86/kernel/apic/vector.c | 8 ++++----
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
index 4efecac49863..cba2c735dd03 100644
--- a/arch/x86/kernel/acpi/boot.c
+++ b/arch/x86/kernel/acpi/boot.c
@@ -1171,7 +1171,8 @@ static int __init acpi_parse_madt_ioapic_entries(void)
 	}
 
 	count = acpi_table_parse_madt(ACPI_MADT_TYPE_INTERRUPT_OVERRIDE,
-				      acpi_parse_int_src_ovr, nr_irqs);
+				      acpi_parse_int_src_ovr,
+				      number_of_interrupts());
 	if (count < 0) {
 		pr_err("Error parsing interrupt source overrides entry\n");
 		/* TBD: Cleanup to allow fallback to MPS */
@@ -1191,7 +1192,8 @@ static int __init acpi_parse_madt_ioapic_entries(void)
 	mp_config_acpi_legacy_irqs();
 
 	count = acpi_table_parse_madt(ACPI_MADT_TYPE_NMI_SOURCE,
-				      acpi_parse_nmi_src, nr_irqs);
+				      acpi_parse_nmi_src,
+				      number_of_interrupts());
 	if (count < 0) {
 		pr_err("Error parsing NMI SRC entry\n");
 		/* TBD: Cleanup to allow fallback to MPS */
diff --git a/arch/x86/kernel/apic/vector.c b/arch/x86/kernel/apic/vector.c
index 557318145038..cbc68ffadb7a 100644
--- a/arch/x86/kernel/apic/vector.c
+++ b/arch/x86/kernel/apic/vector.c
@@ -712,8 +712,8 @@ int __init arch_probe_nr_irqs(void)
 {
 	int nr;
 
-	if (nr_irqs > (NR_VECTORS * nr_cpu_ids))
-		nr_irqs = NR_VECTORS * nr_cpu_ids;
+	if (number_of_interrupts() > NR_VECTORS * nr_cpu_ids)
+		set_number_of_interrupts(NR_VECTORS * nr_cpu_ids);
 
 	nr = (gsi_top + nr_legacy_irqs()) + 8 * nr_cpu_ids;
 #if defined(CONFIG_PCI_MSI)
@@ -725,8 +725,8 @@ int __init arch_probe_nr_irqs(void)
 	else
 		nr += gsi_top * 16;
 #endif
-	if (nr < nr_irqs)
-		nr_irqs = nr;
+	if (nr < number_of_interrupts())
+		set_number_of_interrupts(nr);
 
 	/*
 	 * We don't know if PIC is present at this point so we need to do

[PATCH 07/21] hpet: Switch to number_of_interrupts()

[Bart Van Assche]

Use the number_of_interrupts() function instead of the global variable
'nr_irqs'. This patch prepares for changing 'nr_irqs' from an exported
global variable into a variable with file scope.

Cc: Clemens Ladisch <clemens@ladisch.de>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/char/hpet.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/char/hpet.c b/drivers/char/hpet.c
index e904e476e49a..e618ae66587d 100644
--- a/drivers/char/hpet.c
+++ b/drivers/char/hpet.c
@@ -195,7 +195,7 @@ static void hpet_timer_set_irq(struct hpet_dev *devp)
 		v &= ~0xffff;
 
 	for_each_set_bit(irq, &v, HPET_MAX_IRQ) {
-		if (irq >= nr_irqs) {
+		if (irq >= number_of_interrupts()) {
 			irq = HPET_MAX_IRQ;
 			break;
 		}

RE: [PATCH 07/21] hpet: Switch to number_of_interrupts()

[David Laight]

From: Bart Van Assche
> Sent: 30 September 2024 19:16
> 
> Use the number_of_interrupts() function instead of the global variable
> 'nr_irqs'. This patch prepares for changing 'nr_irqs' from an exported
> global variable into a variable with file scope.
> 
> Cc: Clemens Ladisch <clemens@ladisch.de>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  drivers/char/hpet.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/char/hpet.c b/drivers/char/hpet.c
> index e904e476e49a..e618ae66587d 100644
> --- a/drivers/char/hpet.c
> +++ b/drivers/char/hpet.c
> @@ -195,7 +195,7 @@ static void hpet_timer_set_irq(struct hpet_dev *devp)
>  		v &= ~0xffff;
> 
>  	for_each_set_bit(irq, &v, HPET_MAX_IRQ) {
> -		if (irq >= nr_irqs) {
> +		if (irq >= number_of_interrupts()) {
>  			irq = HPET_MAX_IRQ;
>  			break;
>  		}

This is horrid.
You've replaced the read of a global variable (which, in some cases the
compiler might be able to pull outside the loop) with a real function
call in every loop iteration.

With all the mitigations for cpu speculative execution 'issues' you
pretty much don't want trivial function calls.

If you are worried about locals shadowing globals just change one of the names.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH 07/21] hpet: Switch to number_of_interrupts()

[Bart Van Assche]

On 10/6/24 10:13 AM, David Laight wrote:
> From: Bart Van Assche
>> Sent: 30 September 2024 19:16
>> --- a/drivers/char/hpet.c
>> +++ b/drivers/char/hpet.c
>> @@ -195,7 +195,7 @@ static void hpet_timer_set_irq(struct hpet_dev *devp)
>>   		v &= ~0xffff;
>>
>>   	for_each_set_bit(irq, &v, HPET_MAX_IRQ) {
>> -		if (irq >= nr_irqs) {
>> +		if (irq >= number_of_interrupts()) {
>>   			irq = HPET_MAX_IRQ;
>>   			break;
>>   		}
> 
> This is horrid.
> You've replaced the read of a global variable (which, in some cases the
> compiler might be able to pull outside the loop) with a real function
> call in every loop iteration.
> 
> With all the mitigations for cpu speculative execution 'issues' you
> pretty much don't want trivial function calls.
> 
> If you are worried about locals shadowing globals just change one of the names.

Since HPET_MAX_IRQ == 32 and since the lower 16 bits of 'v' are cleared
on modern systems, would it be such a big deal if number_of_interrupts()
is called 16 times?

Since number_of_interrupts() has been marked as __pure, and since the
kernel is built with -O2, do you agree that this should be sufficient to
let the compiler CSE optimization step move function calls like the
above from inside a loop out of the loop?

Thanks,

Bart.

Re: [PATCH 07/21] hpet: Switch to number_of_interrupts()

[Thomas Gleixner]

On Sun, Oct 06 2024 at 17:45, Bart Van Assche wrote:
> On 10/6/24 10:13 AM, David Laight wrote:
>> From: Bart Van Assche
>>> Sent: 30 September 2024 19:16
>>> --- a/drivers/char/hpet.c
>>> +++ b/drivers/char/hpet.c
>>> @@ -195,7 +195,7 @@ static void hpet_timer_set_irq(struct hpet_dev *devp)
>>>   		v &= ~0xffff;
>>>
>>>   	for_each_set_bit(irq, &v, HPET_MAX_IRQ) {
>>> -		if (irq >= nr_irqs) {
>>> +		if (irq >= number_of_interrupts()) {
>>>   			irq = HPET_MAX_IRQ;
>>>   			break;
>>>   		}
>> 
>> This is horrid.
>> You've replaced the read of a global variable (which, in some cases the
>> compiler might be able to pull outside the loop) with a real function
>> call in every loop iteration.
>> 
>> With all the mitigations for cpu speculative execution 'issues' you
>> pretty much don't want trivial function calls.
>> 
>> If you are worried about locals shadowing globals just change one of the names.
>
> Since HPET_MAX_IRQ == 32 and since the lower 16 bits of 'v' are cleared
> on modern systems, would it be such a big deal if number_of_interrupts()
> is called 16 times?

No. The context is open() which is a slow path operation.

> Since number_of_interrupts() has been marked as __pure, and since the
> kernel is built with -O2, do you agree that this should be sufficient to
> let the compiler CSE optimization step move function calls like the
> above from inside a loop out of the loop?

It could do so.

Thanks,

        tglx

RE: [PATCH 07/21] hpet: Switch to number_of_interrupts()

[David Laight]

From: Thomas Gleixner <tglx@linutronix.de>
> Sent: 07 October 2024 13:12
> 
> On Sun, Oct 06 2024 at 17:45, Bart Van Assche wrote:
> > On 10/6/24 10:13 AM, David Laight wrote:
> >> From: Bart Van Assche
> >>> Sent: 30 September 2024 19:16
> >>> --- a/drivers/char/hpet.c
> >>> +++ b/drivers/char/hpet.c
> >>> @@ -195,7 +195,7 @@ static void hpet_timer_set_irq(struct hpet_dev *devp)
> >>>   		v &= ~0xffff;
> >>>
> >>>   	for_each_set_bit(irq, &v, HPET_MAX_IRQ) {
> >>> -		if (irq >= nr_irqs) {
> >>> +		if (irq >= number_of_interrupts()) {
> >>>   			irq = HPET_MAX_IRQ;
> >>>   			break;
> >>>   		}
> >>
> >> This is horrid.
> >> You've replaced the read of a global variable (which, in some cases the
> >> compiler might be able to pull outside the loop) with a real function
> >> call in every loop iteration.
> >>
> >> With all the mitigations for cpu speculative execution 'issues' you
> >> pretty much don't want trivial function calls.
> >>
> >> If you are worried about locals shadowing globals just change one of the names.
> >
> > Since HPET_MAX_IRQ == 32 and since the lower 16 bits of 'v' are cleared
> > on modern systems, would it be such a big deal if number_of_interrupts()
> > is called 16 times?
> 
> No. The context is open() which is a slow path operation.

This was one example of that code loop - and probably not the best
to have picked.
Indeed, it is using a long[] bitmap designed for 'big' bitmaps
for one that fits in 32 bits and (probably) coding a 'find first bit'
function using the impossible HPET_NAX_IRQ value as an error exit.

In any case 'accessor functions' just move the global symbol from
being a data symbol to a code symbol while obfuscating the
code and making it harder to find where values are set and used.

	David

> 
> > Since number_of_interrupts() has been marked as __pure, and since the
> > kernel is built with -O2, do you agree that this should be sufficient to
> > let the compiler CSE optimization step move function calls like the
> > above from inside a loop out of the loop?
> 
> It could do so.
> 
> Thanks,
> 
>         tglx

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

RE: [PATCH 07/21] hpet: Switch to number_of_interrupts()

[Thomas Gleixner]

On Mon, Oct 07 2024 at 13:00, David Laight wrote:
> From: Thomas Gleixner <tglx@linutronix.de>
> In any case 'accessor functions' just move the global symbol from
> being a data symbol to a code symbol while obfuscating the
> code and making it harder to find where values are set and used.

That's nonsense. The accessor functions can be as easily grepped for as
the variable. So that's not making it harder at all, but it encapsulates
stuff better. That's the whole point of the exercise.

Thanks,

        tglx

[PATCH 08/21] net: 3com: 3c59x: Switch to number_of_interrupts()

[Bart Van Assche]

Use the number_of_interrupts() function instead of the global variable
'nr_irqs'. This patch prepares for changing 'nr_irqs' from an exported
global variable into a variable with file scope.

Cc: Steffen Klassert <klassert@kernel.org>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/net/ethernet/3com/3c59x.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/3com/3c59x.c b/drivers/net/ethernet/3com/3c59x.c
index 082388bb6169..f7f2baf33f50 100644
--- a/drivers/net/ethernet/3com/3c59x.c
+++ b/drivers/net/ethernet/3com/3c59x.c
@@ -1302,7 +1302,7 @@ static int vortex_probe1(struct device *gendev, void __iomem *ioaddr, int irq,
 	if (print_info)
 		pr_cont(", IRQ %d\n", dev->irq);
 	/* Tell them about an invalid IRQ. */
-	if (dev->irq <= 0 || dev->irq >= nr_irqs)
+	if (dev->irq <= 0 || dev->irq >= number_of_interrupts())
 		pr_warn(" *** Warning: IRQ %d is unlikely to work! ***\n",
 			dev->irq);
 

[PATCH 09/21] net: hamradio: baycom_ser_fdx: Switch to number_of_interrupts()

[Bart Van Assche]

Use the number_of_interrupts() function instead of the global variable
'nr_irqs'. This patch prepares for changing 'nr_irqs' from an exported
global variable into a variable with file scope.

Cc: Thomas Sailer <t.sailer@alumni.ethz.ch>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/net/hamradio/baycom_ser_fdx.c | 4 ++--
 drivers/net/hamradio/scc.c            | 6 +++---
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/net/hamradio/baycom_ser_fdx.c b/drivers/net/hamradio/baycom_ser_fdx.c
index 646f605e358f..8c6ca1c001d5 100644
--- a/drivers/net/hamradio/baycom_ser_fdx.c
+++ b/drivers/net/hamradio/baycom_ser_fdx.c
@@ -379,10 +379,10 @@ static int ser12_open(struct net_device *dev)
 	if (!dev || !bc)
 		return -ENXIO;
 	if (!dev->base_addr || dev->base_addr > 0xffff-SER12_EXTENT ||
-	    dev->irq < 2 || dev->irq > nr_irqs) {
+	    dev->irq < 2 || dev->irq > number_of_interrupts()) {
 		printk(KERN_INFO "baycom_ser_fdx: invalid portnumber (max %u) "
 				"or irq (2 <= irq <= %d)\n",
-				0xffff-SER12_EXTENT, nr_irqs);
+				0xffff-SER12_EXTENT, number_of_interrupts());
 		return -ENXIO;
 	}
 	if (bc->baud < 300 || bc->baud > 4800) {
diff --git a/drivers/net/hamradio/scc.c b/drivers/net/hamradio/scc.c
index a9184a78650b..e00cd21422a3 100644
--- a/drivers/net/hamradio/scc.c
+++ b/drivers/net/hamradio/scc.c
@@ -1469,7 +1469,7 @@ static void z8530_init(void)
 	printk(KERN_INFO "Init Z8530 driver: %u channels, IRQ", Nchips*2);
 	
 	flag=" ";
-	for (k = 0; k < nr_irqs; k++)
+	for (k = 0; k < number_of_interrupts(); k++)
 		if (Ivec[k].used) 
 		{
 			printk("%s%d", flag, k);
@@ -1735,7 +1735,7 @@ static int scc_net_siocdevprivate(struct net_device *dev,
 
 			if (hwcfg.irq == 2) hwcfg.irq = 9;
 
-			if (hwcfg.irq < 0 || hwcfg.irq >= nr_irqs)
+			if (hwcfg.irq < 0 || hwcfg.irq >= number_of_interrupts())
 				return -EINVAL;
 				
 			if (!Ivec[hwcfg.irq].used && hwcfg.irq)
@@ -2140,7 +2140,7 @@ static void __exit scc_cleanup_driver(void)
 		}
 		
 	/* To unload the port must be closed so no real IRQ pending */
-	for (k = 0; k < nr_irqs ; k++)
+	for (k = 0; k < number_of_interrupts() ; k++)
 		if (Ivec[k].used) free_irq(k, NULL);
 		
 	local_irq_enable();

[PATCH 10/21] scsi: aha152x: Switch to number_of_interrupts()

[Bart Van Assche]

Use the number_of_interrupts() function instead of the global variable
'nr_irqs'. This patch prepares for changing 'nr_irqs' from an exported
global variable into a variable with file scope.

Cc: Juergen E. Fischer <fischer@norbit.de>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/aha152x.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/aha152x.c b/drivers/scsi/aha152x.c
index a0fb330b8df5..cc1456e049c4 100644
--- a/drivers/scsi/aha152x.c
+++ b/drivers/scsi/aha152x.c
@@ -295,7 +295,7 @@ CMD_INC_RESID(struct scsi_cmnd *cmd, int inc)
 #else
 #define IRQ_MIN 9
 #if defined(__PPC)
-#define IRQ_MAX (nr_irqs-1)
+#define IRQ_MAX (number_of_interrupts()-1)
 #else
 #define IRQ_MAX 12
 #endif

[PATCH 11/21] serial: core: Switch to number_of_interrupts()

[Bart Van Assche]

Use the number_of_interrupts() function instead of the global variable
'nr_irqs'. This patch prepares for changing 'nr_irqs' from an exported
global variable into a variable with file scope.

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/tty/serial/serial_core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index d94d73e45fb6..fb3915065ecd 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -919,7 +919,7 @@ static int uart_set_info(struct tty_struct *tty, struct tty_port *port,
 	if (uport->ops->verify_port)
 		retval = uport->ops->verify_port(uport, new_info);
 
-	if ((new_info->irq >= nr_irqs) || (new_info->irq < 0) ||
+	if ((new_info->irq >= number_of_interrupts()) || (new_info->irq < 0) ||
 	    (new_info->baud_base < 9600))
 		retval = -EINVAL;
 

[PATCH 12/21] serial: 8250: Switch to number_of_interrupts()

[Bart Van Assche]

Use the number_of_interrupts() function instead of the global variable
'nr_irqs'. This patch prepares for changing 'nr_irqs' from an exported
global variable into a variable with file scope.

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/tty/serial/8250/8250_port.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 3509af7dc52b..7983526539fb 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -3176,7 +3176,7 @@ static void serial8250_config_port(struct uart_port *port, int flags)
 static int
 serial8250_verify_port(struct uart_port *port, struct serial_struct *ser)
 {
-	if (ser->irq >= nr_irqs || ser->irq < 0 ||
+	if (ser->irq >= number_of_interrupts() || ser->irq < 0 ||
 	    ser->baud_base < 9600 || ser->type < PORT_UNKNOWN ||
 	    ser->type >= ARRAY_SIZE(uart_config) || ser->type == PORT_CIRRUS ||
 	    ser->type == PORT_STARTECH)

[PATCH 13/21] serial: amba-pl010: Switch to number_of_interrupts()

[Bart Van Assche]

Use the number_of_interrupts() function instead of the global variable
'nr_irqs'. This patch prepares for changing 'nr_irqs' from an exported
global variable into a variable with file scope.

Cc: Russell King <linux@armlinux.org.uk>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/tty/serial/amba-pl010.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/serial/amba-pl010.c b/drivers/tty/serial/amba-pl010.c
index eabbf8afc9b5..cd7be4453dd5 100644
--- a/drivers/tty/serial/amba-pl010.c
+++ b/drivers/tty/serial/amba-pl010.c
@@ -499,7 +499,7 @@ static int pl010_verify_port(struct uart_port *port, struct serial_struct *ser)
 	int ret = 0;
 	if (ser->type != PORT_UNKNOWN && ser->type != PORT_AMBA)
 		ret = -EINVAL;
-	if (ser->irq < 0 || ser->irq >= nr_irqs)
+	if (ser->irq < 0 || ser->irq >= number_of_interrupts())
 		ret = -EINVAL;
 	if (ser->baud_base < 9600)
 		ret = -EINVAL;

[PATCH 14/21] serial: amba-pl011: Switch to number_of_interrupts()

[Bart Van Assche]

Use the number_of_interrupts() function instead of the global variable
'nr_irqs'. This patch prepares for changing 'nr_irqs' from an exported
global variable into a variable with file scope.

Cc: Russell King <linux@armlinux.org.uk>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/tty/serial/amba-pl011.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index 7d0134ecd82f..6f39323cb652 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -2202,7 +2202,7 @@ static int pl011_verify_port(struct uart_port *port, struct serial_struct *ser)
 
 	if (ser->type != PORT_UNKNOWN && ser->type != PORT_AMBA)
 		ret = -EINVAL;
-	if (ser->irq < 0 || ser->irq >= nr_irqs)
+	if (ser->irq < 0 || ser->irq >= number_of_interrupts())
 		ret = -EINVAL;
 	if (ser->baud_base < 9600)
 		ret = -EINVAL;

[PATCH 15/21] serial: cpm_uart: Switch to number_of_interrupts()

[Bart Van Assche]

Use the number_of_interrupts() function instead of the global variable
'nr_irqs'. This patch prepares for changing 'nr_irqs' from an exported
global variable into a variable with file scope.

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/tty/serial/cpm_uart.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/serial/cpm_uart.c b/drivers/tty/serial/cpm_uart.c
index a927478f581d..7ead7277e817 100644
--- a/drivers/tty/serial/cpm_uart.c
+++ b/drivers/tty/serial/cpm_uart.c
@@ -631,7 +631,7 @@ static int cpm_uart_verify_port(struct uart_port *port,
 
 	if (ser->type != PORT_UNKNOWN && ser->type != PORT_CPM)
 		ret = -EINVAL;
-	if (ser->irq < 0 || ser->irq >= nr_irqs)
+	if (ser->irq < 0 || ser->irq >= number_of_interrupts())
 		ret = -EINVAL;
 	if (ser->baud_base < 9600)
 		ret = -EINVAL;

[PATCH 16/21] serial: ucc_uart: Switch to number_of_interrupts()

[Bart Van Assche]

Use the number_of_interrupts() function instead of the global variable
'nr_irqs'. This patch prepares for changing 'nr_irqs' from an exported
global variable into a variable with file scope.

Cc: Timur Tabi <timur@kernel.org>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/tty/serial/ucc_uart.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/serial/ucc_uart.c b/drivers/tty/serial/ucc_uart.c
index 53bb8c5ef499..73bdb306a160 100644
--- a/drivers/tty/serial/ucc_uart.c
+++ b/drivers/tty/serial/ucc_uart.c
@@ -1045,7 +1045,7 @@ static int qe_uart_verify_port(struct uart_port *port,
 	if (ser->type != PORT_UNKNOWN && ser->type != PORT_CPM)
 		return -EINVAL;
 
-	if (ser->irq < 0 || ser->irq >= nr_irqs)
+	if (ser->irq < 0 || ser->irq >= number_of_interrupts())
 		return -EINVAL;
 
 	if (ser->baud_base < 9600)

[PATCH 17/21] sh: intc: Switch to number_of_interrupts()

[Bart Van Assche]

Use the number_of_interrupts() function instead of the global variable
'nr_irqs'. This patch prepares for changing 'nr_irqs' from an exported
global variable into a variable with file scope.

Cc: Yoshinori Sato <ysato@users.sourceforge.jp>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/sh/intc/virq-debugfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/sh/intc/virq-debugfs.c b/drivers/sh/intc/virq-debugfs.c
index 939915a07d99..09d685cda79e 100644
--- a/drivers/sh/intc/virq-debugfs.c
+++ b/drivers/sh/intc/virq-debugfs.c
@@ -22,7 +22,7 @@ static int intc_irq_xlate_show(struct seq_file *m, void *priv)
 
 	seq_printf(m, "%-5s  %-7s  %-15s\n", "irq", "enum", "chip name");
 
-	for (i = 1; i < nr_irqs; i++) {
+	for (i = 1; i < number_of_interrupts(); i++) {
 		struct intc_map_entry *entry = intc_irq_xlate_get(i);
 		struct intc_desc_int *desc = entry->desc;
 

[PATCH 18/21] xen/events: Switch to number_of_interrupts()

[Bart Van Assche]

Use the number_of_interrupts() function instead of the global variable
'nr_irqs'. This patch prepares for changing 'nr_irqs' from an exported
global variable into a variable with file scope.

Cc: Juergen Gross <jgross@suse.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/xen/events/events_base.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/xen/events/events_base.c b/drivers/xen/events/events_base.c
index 81effbd53dc5..3c020b3f583d 100644
--- a/drivers/xen/events/events_base.c
+++ b/drivers/xen/events/events_base.c
@@ -411,7 +411,7 @@ static evtchn_port_t evtchn_from_irq(unsigned int irq)
 {
 	const struct irq_info *info = NULL;
 
-	if (likely(irq < nr_irqs))
+	if (likely(irq < number_of_interrupts()))
 		info = info_for_irq(irq);
 	if (!info)
 		return 0;

[PATCH 19/21] fs/procfs: Switch to number_of_interrupts()

[Bart Van Assche]

Use the number_of_interrupts() function instead of the global variable
'nr_irqs'. This patch prepares for changing 'nr_irqs' from an exported
global variable into a variable with file scope.

Cc: Alexey Dobriyan <adobriyan@gmail.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 fs/proc/interrupts.c | 4 ++--
 fs/proc/stat.c       | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/proc/interrupts.c b/fs/proc/interrupts.c
index cb0edc7cbf09..90cc6b9a30f5 100644
--- a/fs/proc/interrupts.c
+++ b/fs/proc/interrupts.c
@@ -11,13 +11,13 @@
  */
 static void *int_seq_start(struct seq_file *f, loff_t *pos)
 {
-	return (*pos <= nr_irqs) ? pos : NULL;
+	return *pos <= number_of_interrupts() ? pos : NULL;
 }
 
 static void *int_seq_next(struct seq_file *f, void *v, loff_t *pos)
 {
 	(*pos)++;
-	if (*pos > nr_irqs)
+	if (*pos > number_of_interrupts())
 		return NULL;
 	return pos;
 }
diff --git a/fs/proc/stat.c b/fs/proc/stat.c
index da60956b2915..6e22a7bca973 100644
--- a/fs/proc/stat.c
+++ b/fs/proc/stat.c
@@ -76,7 +76,7 @@ static void show_all_irqs(struct seq_file *p)
 		seq_put_decimal_ull(p, " ", kstat_irqs_usr(i));
 		next = i + 1;
 	}
-	show_irq_gap(p, nr_irqs - next);
+	show_irq_gap(p, number_of_interrupts() - next);
 }
 
 static int show_stat(struct seq_file *p, void *v)
@@ -196,7 +196,7 @@ static int stat_open(struct inode *inode, struct file *file)
 	unsigned int size = 1024 + 128 * num_online_cpus();
 
 	/* minimum size to display an interrupt count : 2 bytes */
-	size += 2 * nr_irqs;
+	size += 2 * number_of_interrupts();
 	return single_open_size(file, show_stat, NULL, size);
 }
 

[PATCH 20/21] genirq: Switch to number_of_interrupts()

[Bart Van Assche]

Use the number_of_interrupts() function instead of the global variable
'nr_irqs'. This patch prepares for changing 'nr_irqs' from an exported
global variable into a variable with file scope.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 include/linux/irqnr.h  | 15 ++++++++-------
 kernel/irq/irqdomain.c |  2 +-
 kernel/irq/proc.c      |  5 +++--
 3 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/include/linux/irqnr.h b/include/linux/irqnr.h
index a8b2cb6146e8..81b76949c52e 100644
--- a/include/linux/irqnr.h
+++ b/include/linux/irqnr.h
@@ -12,7 +12,8 @@ extern struct irq_desc *irq_to_desc(unsigned int irq);
 unsigned int irq_get_next_irq(unsigned int offset);
 
 # define for_each_irq_desc(irq, desc)					\
-	for (irq = 0, desc = irq_to_desc(irq); irq < nr_irqs;		\
+	for (irq = 0, desc = irq_to_desc(irq);				\
+	     irq < number_of_interrupts();				\
 	     irq++, desc = irq_to_desc(irq))				\
 		if (!desc)						\
 			;						\
@@ -20,17 +21,17 @@ unsigned int irq_get_next_irq(unsigned int offset);
 
 
 # define for_each_irq_desc_reverse(irq, desc)				\
-	for (irq = nr_irqs - 1, desc = irq_to_desc(irq); irq >= 0;	\
-	     irq--, desc = irq_to_desc(irq))				\
+	for (irq = number_of_interrupts() - 1, desc = irq_to_desc(irq);	\
+	     irq >= 0; irq--, desc = irq_to_desc(irq))			\
 		if (!desc)						\
 			;						\
 		else
 
-# define for_each_active_irq(irq)			\
-	for (irq = irq_get_next_irq(0); irq < nr_irqs;	\
+# define for_each_active_irq(irq)					\
+	for (irq = irq_get_next_irq(0); irq < number_of_interrupts();	\
 	     irq = irq_get_next_irq(irq + 1))
 
-#define for_each_irq_nr(irq)                   \
-       for (irq = 0; irq < nr_irqs; irq++)
+#define for_each_irq_nr(irq)					\
+       for (irq = 0; irq < number_of_interrupts(); irq++)
 
 #endif
diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index e0bff21f30e0..298930f2525a 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -1225,7 +1225,7 @@ int irq_domain_alloc_descs(int virq, unsigned int cnt, irq_hw_number_t hwirq,
 		virq = __irq_alloc_descs(virq, virq, cnt, node, THIS_MODULE,
 					 affinity);
 	} else {
-		hint = hwirq % nr_irqs;
+		hint = hwirq % number_of_interrupts();
 		if (hint == 0)
 			hint++;
 		virq = __irq_alloc_descs(-1, hint, cnt, node, THIS_MODULE,
diff --git a/kernel/irq/proc.c b/kernel/irq/proc.c
index 9081ada81c3d..c301dfd7d9d0 100644
--- a/kernel/irq/proc.c
+++ b/kernel/irq/proc.c
@@ -457,7 +457,7 @@ int __weak arch_show_interrupts(struct seq_file *p, int prec)
 }
 
 #ifndef ACTUAL_NR_IRQS
-# define ACTUAL_NR_IRQS nr_irqs
+# define ACTUAL_NR_IRQS number_of_interrupts()
 #endif
 
 int show_interrupts(struct seq_file *p, void *v)
@@ -477,7 +477,8 @@ int show_interrupts(struct seq_file *p, void *v)
 
 	/* print header and calculate the width of the first column */
 	if (i == 0) {
-		for (prec = 3, j = 1000; prec < 10 && j <= nr_irqs; ++prec)
+		for (prec = 3, j = 1000;
+		     prec < 10 && j <= number_of_interrupts(); ++prec)
 			j *= 10;
 
 		seq_printf(p, "%*s", prec + 8, "");

Re: [PATCH 20/21] genirq: Switch to number_of_interrupts()

[Bart Van Assche]

On 9/30/24 11:15 AM, Bart Van Assche wrote:
> Use the number_of_interrupts() function instead of the global variable
> 'nr_irqs'. This patch prepares for changing 'nr_irqs' from an exported
> global variable into a variable with file scope.

(replying to my own email)

Although no code should loop over all interrupts in the hot path, if 
nobody objects, I will replace this patch with the patch below. The 
patch below does not rely on CSE (common subexpression eliminiation) to
reduce the number of irq_get_nr_irqs() calls. The patch below should
support code that uses 'break' or 'continue' inside for_each_irq* loops.

Thanks,

Bart.


Subject: [PATCH] genirq: Switch to irq_get_nr_irqs()

Use the irq_get_nr_irqs() function instead of the global variable
'nr_irqs'. This patch prepares for changing 'nr_irqs' from an exported
global variable into a variable with file scope.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
  include/linux/irqnr.h  | 33 +++++++++++++++++++--------------
  kernel/irq/irqdomain.c |  2 +-
  kernel/irq/proc.c      |  5 +++--
  3 files changed, 23 insertions(+), 17 deletions(-)

diff --git a/include/linux/irqnr.h b/include/linux/irqnr.h
index 7419b807b71b..a33088d27c54 100644
--- a/include/linux/irqnr.h
+++ b/include/linux/irqnr.h
@@ -11,26 +11,31 @@ unsigned int irq_set_nr_irqs(unsigned int nr);
  extern struct irq_desc *irq_to_desc(unsigned int irq);
  unsigned int irq_get_next_irq(unsigned int offset);

-# define for_each_irq_desc(irq, desc)					\
-	for (irq = 0, desc = irq_to_desc(irq); irq < nr_irqs;		\
-	     irq++, desc = irq_to_desc(irq))				\
-		if (!desc)						\
-			;						\
-		else
-
+#define for_each_irq_desc(irq, desc)                                      \
+	for (unsigned int __nr_irqs__ = irq_get_nr_irqs(); __nr_irqs__;   \
+	     __nr_irqs__ = 0)                                             \
+		for (irq = 0, desc = irq_to_desc(irq); irq < __nr_irqs__; \
+		     irq++, desc = irq_to_desc(irq))                      \
+			if (!desc)                                        \
+				;                                         \
+			else

  # define for_each_irq_desc_reverse(irq, desc)				\
-	for (irq = nr_irqs - 1, desc = irq_to_desc(irq); irq >= 0;	\
-	     irq--, desc = irq_to_desc(irq))				\
+	for (irq = irq_get_nr_irqs() - 1, desc = irq_to_desc(irq);	\
+	     irq >= 0; irq--, desc = irq_to_desc(irq))			\
  		if (!desc)						\
  			;						\
  		else

-# define for_each_active_irq(irq)			\
-	for (irq = irq_get_next_irq(0); irq < nr_irqs;	\
-	     irq = irq_get_next_irq(irq + 1))
+#define for_each_active_irq(irq)                                        \
+	for (unsigned int __nr_irqs__ = irq_get_nr_irqs(); __nr_irqs__; \
+	     __nr_irqs__ = 0)                                           \
+		for (irq = irq_get_next_irq(0); irq < __nr_irqs__;      \
+		     irq = irq_get_next_irq(irq + 1))

-#define for_each_irq_nr(irq)                   \
-       for (irq = 0; irq < nr_irqs; irq++)
+#define for_each_irq_nr(irq)                                            \
+	for (unsigned int __nr_irqs__ = irq_get_nr_irqs(); __nr_irqs__; \
+	     __nr_irqs__ = 0)                                           \
+		for (irq = 0; irq < __nr_irqs__; irq++)

  #endif
diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index e0bff21f30e0..ec6d8e72d980 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -1225,7 +1225,7 @@ int irq_domain_alloc_descs(int virq, unsigned int 
cnt, irq_hw_number_t hwirq,
  		virq = __irq_alloc_descs(virq, virq, cnt, node, THIS_MODULE,
  					 affinity);
  	} else {
-		hint = hwirq % nr_irqs;
+		hint = hwirq % irq_get_nr_irqs();
  		if (hint == 0)
  			hint++;
  		virq = __irq_alloc_descs(-1, hint, cnt, node, THIS_MODULE,
diff --git a/kernel/irq/proc.c b/kernel/irq/proc.c
index 9081ada81c3d..36ffff4eb352 100644
--- a/kernel/irq/proc.c
+++ b/kernel/irq/proc.c
@@ -457,7 +457,7 @@ int __weak arch_show_interrupts(struct seq_file *p, 
int prec)
  }

  #ifndef ACTUAL_NR_IRQS
-# define ACTUAL_NR_IRQS nr_irqs
+# define ACTUAL_NR_IRQS irq_get_nr_irqs()
  #endif

  int show_interrupts(struct seq_file *p, void *v)
@@ -477,7 +477,8 @@ int show_interrupts(struct seq_file *p, void *v)

  	/* print header and calculate the width of the first column */
  	if (i == 0) {
-		for (prec = 3, j = 1000; prec < 10 && j <= nr_irqs; ++prec)
+		for (prec = 3, j = 1000;
+		     prec < 10 && j <= irq_get_nr_irqs(); ++prec)
  			j *= 10;

  		seq_printf(p, "%*s", prec + 8, "");

[PATCH 21/21] genirq: Unexport nr_irqs

[Bart Van Assche]

Unexport nr_irqs and declare it static now that all code that reads or
modifies nr_irqs has been converted to number_of_interrupts() /
set_number_of_interrupts().

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 include/linux/irqnr.h | 1 -
 kernel/irq/irqdesc.c  | 3 +--
 2 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/include/linux/irqnr.h b/include/linux/irqnr.h
index 81b76949c52e..82060d75642c 100644
--- a/include/linux/irqnr.h
+++ b/include/linux/irqnr.h
@@ -5,7 +5,6 @@
 #include <uapi/linux/irqnr.h>
 
 
-extern int nr_irqs;
 int number_of_interrupts(void) __pure;
 int set_number_of_interrupts(int nr);
 extern struct irq_desc *irq_to_desc(unsigned int irq);
diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c
index 8c6280843964..dea51c641f87 100644
--- a/kernel/irq/irqdesc.c
+++ b/kernel/irq/irqdesc.c
@@ -138,8 +138,7 @@ static void desc_set_defaults(unsigned int irq, struct irq_desc *desc, int node,
 	desc_smp_init(desc, node, affinity);
 }
 
-int nr_irqs = NR_IRQS;
-EXPORT_SYMBOL_GPL(nr_irqs);
+static int nr_irqs = NR_IRQS;
 
 int number_of_interrupts(void)
 {