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

[David Laight <David.Laight@ACULAB.COM>]

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)