linux-kernel - Re: [PATCH 07/21] hpet: Switch to number_of

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <874j5oun7x.ffs@tglx>
Date: Mon, 07 Oct 2024 14:11:30 +0200
From: Thomas Gleixner <tglx@...utronix.de>
To: Bart Van Assche <bvanassche@....org>, David Laight
 <David.Laight@...LAB.COM>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>, Russell King
 <linux@...linux.org.uk>, "linux-kernel@...r.kernel.org"
 <linux-kernel@...r.kernel.org>, Clemens Ladisch <clemens@...isch.de>
Subject: Re: [PATCH 07/21] hpet: Switch to number_of_interrupts()

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