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;
 		}

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)

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.

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

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)

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