[PATCH] rtc: cmos: Don't enable shared interrupts if using HPET-based IRQ handler

[PATCH] rtc: cmos: Don't enable shared interrupts if using HPET-based IRQ handler

[Guilherme G. Piccoli]

The rtc-cmos interrupt setting was changed in commit 079062b28fb4
("rtc: cmos: prevent kernel warning on IRQ flags mismatch") in order to
allow shared interrupts; according to that commit's description, some
machine got kernel warnings due to the interrupt line being shared
between rtc-cmos and other hardware, and rtc-cmos didn't allow IRQ
sharing that time.

After the aforementioned commit though it was observed a huge increase
in lost HPET interrupts in some systems, observed through the following
kernel message:

[...] hpet1: lost 35 rtc interrupts

After investigation, it was narrowed down to the shared interrupts
usage when having the kernel option "irqpoll" enabled. In this case,
all IRQ handlers are called for non-timer interrupts, if such handlers
are setup in shared IRQ lines. The rtc-cmos IRQ handler could be set to
hpet_rtc_interrupt(), which will produce the kernel "lost interrupts"
message after doing work - lots of readl/writel to HPET registers, which
are known to be slow.

This patch changes this behavior by preventing shared interrupts if the
HPET-based IRQ handler is used instead of the regular cmos_interrupt()
one. Although "irqpoll" is not a default kernel option, it's used in
some contexts, one being the kdump kernel (which is an already "impaired"
kernel usually running with 1 CPU available), so the performance burden
could be considerable. Also, the same issue would happen (in a shorter
extent though) when using "irqfixup" kernel option.

In a quick experiment, a virtual machine with uptime of 2 minutes produced
>300 calls to hpet_rtc_interrupt() when "irqpoll" was set, whereas without
sharing interrupts this number reduced to 1 interrupt. Machines with more
hardware than a VM should generate even more unnecessary HPET interrupts
in this scenario.

Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Guilherme G. Piccoli <gpiccoli@canonical.com>
---


Hi all, thanks for reading/reviewing this patch! One of the
alternatives I considered in case sharing interrupts are really
desirable is a new kernel parameter to rtc-cmos to allow
sharing interrupts, and default the IRQ setup to non-shared.

We could also disable sharing if "irqpoll" or "irqfixup" is set,
but this would somewhat "bypass" IRQ code API which I think would
be a bit ugly.

Please let me know your thoughts, and thanks in advance.

Guilherme


 drivers/rtc/rtc-cmos.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/rtc/rtc-cmos.c b/drivers/rtc/rtc-cmos.c
index 033303708c8b..16416154eb00 100644
--- a/drivers/rtc/rtc-cmos.c
+++ b/drivers/rtc/rtc-cmos.c
@@ -710,6 +710,7 @@ cmos_do_probe(struct device *dev, struct resource *ports, int rtc_irq)
 	unsigned char			rtc_control;
 	unsigned			address_space;
 	u32				flags = 0;
+	unsigned long			irq_flags;
 	struct nvmem_config nvmem_cfg = {
 		.name = "cmos_nvram",
 		.word_size = 1,
@@ -839,6 +840,7 @@ cmos_do_probe(struct device *dev, struct resource *ports, int rtc_irq)
 
 		if (use_hpet_alarm()) {
 			rtc_cmos_int_handler = hpet_rtc_interrupt;
+			irq_flags = 0;
 			retval = hpet_register_irq_handler(cmos_interrupt);
 			if (retval) {
 				hpet_mask_rtc_irq_bit(RTC_IRQMASK);
@@ -846,11 +848,13 @@ cmos_do_probe(struct device *dev, struct resource *ports, int rtc_irq)
 						" failed in rtc_init().");
 				goto cleanup1;
 			}
-		} else
+		} else {
 			rtc_cmos_int_handler = cmos_interrupt;
+			irq_flags = IRQF_SHARED;
+		}
 
 		retval = request_irq(rtc_irq, rtc_cmos_int_handler,
-				IRQF_SHARED, dev_name(&cmos_rtc.rtc->dev),
+				irq_flags, dev_name(&cmos_rtc.rtc->dev),
 				cmos_rtc.rtc);
 		if (retval < 0) {
 			dev_dbg(dev, "IRQ %d is already in use\n", rtc_irq);
-- 
2.24.0

Re: [PATCH] rtc: cmos: Don't enable shared interrupts if using HPET-based IRQ handler

[Andy Shevchenko]

On Fri, Jan 03, 2020 at 11:02:40AM -0300, Guilherme G. Piccoli wrote:
> The rtc-cmos interrupt setting was changed in commit 079062b28fb4
> ("rtc: cmos: prevent kernel warning on IRQ flags mismatch") in order to
> allow shared interrupts; according to that commit's description, some
> machine got kernel warnings due to the interrupt line being shared
> between rtc-cmos and other hardware, and rtc-cmos didn't allow IRQ
> sharing that time.
> 
> After the aforementioned commit though it was observed a huge increase
> in lost HPET interrupts in some systems, observed through the following
> kernel message:
> 
> [...] hpet1: lost 35 rtc interrupts
> 
> After investigation, it was narrowed down to the shared interrupts
> usage when having the kernel option "irqpoll" enabled. In this case,
> all IRQ handlers are called for non-timer interrupts, if such handlers
> are setup in shared IRQ lines. The rtc-cmos IRQ handler could be set to
> hpet_rtc_interrupt(), which will produce the kernel "lost interrupts"
> message after doing work - lots of readl/writel to HPET registers, which
> are known to be slow.
> 
> This patch changes this behavior by preventing shared interrupts if the
> HPET-based IRQ handler is used instead of the regular cmos_interrupt()
> one. Although "irqpoll" is not a default kernel option, it's used in
> some contexts, one being the kdump kernel (which is an already "impaired"
> kernel usually running with 1 CPU available), so the performance burden
> could be considerable. Also, the same issue would happen (in a shorter
> extent though) when using "irqfixup" kernel option.
> 
> In a quick experiment, a virtual machine with uptime of 2 minutes produced
> >300 calls to hpet_rtc_interrupt() when "irqpoll" was set, whereas without
> sharing interrupts this number reduced to 1 interrupt. Machines with more
> hardware than a VM should generate even more unnecessary HPET interrupts
> in this scenario.
> 
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Signed-off-by: Guilherme G. Piccoli <gpiccoli@canonical.com>
> ---
> 
> 
> Hi all, thanks for reading/reviewing this patch! One of the
> alternatives I considered in case sharing interrupts are really
> desirable is a new kernel parameter to rtc-cmos to allow
> sharing interrupts, and default the IRQ setup to non-shared.
> 
> We could also disable sharing if "irqpoll" or "irqfixup" is set,
> but this would somewhat "bypass" IRQ code API which I think would
> be a bit ugly.
> 
> Please let me know your thoughts, and thanks in advance.

I think you may ask for Thomas' opinion (Cc'ed now).

> 
> Guilherme
> 
> 
>  drivers/rtc/rtc-cmos.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-cmos.c b/drivers/rtc/rtc-cmos.c
> index 033303708c8b..16416154eb00 100644
> --- a/drivers/rtc/rtc-cmos.c
> +++ b/drivers/rtc/rtc-cmos.c
> @@ -710,6 +710,7 @@ cmos_do_probe(struct device *dev, struct resource *ports, int rtc_irq)
>  	unsigned char			rtc_control;
>  	unsigned			address_space;
>  	u32				flags = 0;
> +	unsigned long			irq_flags;
>  	struct nvmem_config nvmem_cfg = {
>  		.name = "cmos_nvram",
>  		.word_size = 1,
> @@ -839,6 +840,7 @@ cmos_do_probe(struct device *dev, struct resource *ports, int rtc_irq)
>  
>  		if (use_hpet_alarm()) {
>  			rtc_cmos_int_handler = hpet_rtc_interrupt;
> +			irq_flags = 0;
>  			retval = hpet_register_irq_handler(cmos_interrupt);
>  			if (retval) {
>  				hpet_mask_rtc_irq_bit(RTC_IRQMASK);
> @@ -846,11 +848,13 @@ cmos_do_probe(struct device *dev, struct resource *ports, int rtc_irq)
>  						" failed in rtc_init().");
>  				goto cleanup1;
>  			}
> -		} else
> +		} else {
>  			rtc_cmos_int_handler = cmos_interrupt;
> +			irq_flags = IRQF_SHARED;
> +		}
>  
>  		retval = request_irq(rtc_irq, rtc_cmos_int_handler,
> -				IRQF_SHARED, dev_name(&cmos_rtc.rtc->dev),
> +				irq_flags, dev_name(&cmos_rtc.rtc->dev),
>  				cmos_rtc.rtc);
>  		if (retval < 0) {
>  			dev_dbg(dev, "IRQ %d is already in use\n", rtc_irq);
> -- 
> 2.24.0
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] rtc: cmos: Don't enable shared interrupts if using HPET-based IRQ handler

[Thomas Gleixner]

Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:
> On Fri, Jan 03, 2020 at 11:02:40AM -0300, Guilherme G. Piccoli wrote:
 
>> This patch changes this behavior by preventing shared interrupts if the
>> HPET-based IRQ handler is used instead of the regular cmos_interrupt()
>> one. Although "irqpoll" is not a default kernel option, it's used in
>> some contexts, one being the kdump kernel (which is an already "impaired"
>> kernel usually running with 1 CPU available), so the performance burden
>> could be considerable. Also, the same issue would happen (in a shorter
>> extent though) when using "irqfixup" kernel option.
...
>> Hi all, thanks for reading/reviewing this patch! One of the
>> alternatives I considered in case sharing interrupts are really
>> desirable is a new kernel parameter to rtc-cmos to allow
>> sharing interrupts, and default the IRQ setup to non-shared.
>> 
>> We could also disable sharing if "irqpoll" or "irqfixup" is set,
>> but this would somewhat "bypass" IRQ code API which I think would
>> be a bit ugly.
>> 
>> Please let me know your thoughts, and thanks in advance.
>
> I think you may ask for Thomas' opinion (Cc'ed now).

I'm a bit wary with binding this to the fact that the HPET is
involved.

Especially I don't know whether the Surface3 which is Intel based
exposes the HPET via ACPI which would pretty much revert the effect of
the mentioned commit which introduced the IRQF_SHARED magic especially
for Surface3.

As this is a Surface3 specific misfeature, it might be trivial enough to
set IRQF_SHARED based on a DMI quirk for the Surface3.

Thanks,

        tglx

Re: [PATCH] rtc: cmos: Don't enable shared interrupts if using HPET-based IRQ handler

[Guilherme G. Piccoli]

On 08/01/2020 16:40, Thomas Gleixner wrote:
>> [...]
>> I think you may ask for Thomas' opinion (Cc'ed now).
> 
> I'm a bit wary with binding this to the fact that the HPET is
> involved.
> 
> Especially I don't know whether the Surface3 which is Intel based
> exposes the HPET via ACPI which would pretty much revert the effect of
> the mentioned commit which introduced the IRQF_SHARED magic especially
> for Surface3.
> 
> As this is a Surface3 specific misfeature, it might be trivial enough to
> set IRQF_SHARED based on a DMI quirk for the Surface3.
> 
> Thanks,
> 
>         tglx

Thanks Andy and Thomas for the discussion.
If I understood correctly, Thomas' opinion is that we shouldn't keep the
IRQ flag as shared and instead pursue a device's quirk for Surface3 in
order of making IRQs shared only in such laptop - correct?

In that case, shall we revert that commit entirely (I can respin the
patch fixing the description and reverting, if that's the case), or this
version (HPET-restricted) is enough?

I don't have a Surface 3 on hands, if anybody can provide SMBIOS data
from it, I can also take a look on quirking it to keep the shared IRQs.

Cheers,


Guilherme

Re: [PATCH] rtc: cmos: Don't enable shared interrupts if using HPET-based IRQ handler

[Guilherme G. Piccoli]

Hi all, do we have any news about this one?
Thanks in advance,


Guilherme

Re: [PATCH] rtc: cmos: Don't enable shared interrupts if using HPET-based IRQ handler

[Andy Shevchenko]

On Fri, Jan 17, 2020 at 07:09:13AM -0300, Guilherme G. Piccoli wrote:
> Hi all, do we have any news about this one?
> Thanks in advance,

Let me come out with something such as Thomas suggested.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] rtc: cmos: Don't enable shared interrupts if using HPET-based IRQ handler

[Guilherme Piccoli]

On Fri, Jan 17, 2020 at 11:42 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> Let me come out with something such as Thomas suggested.
>

Thank you Andy! In case it's gonna take a while (like more than 1
kernel release), could we get this one accepted to mitigate the issue
meanwhile?
Cheers,


Guilherme

Re: [PATCH] rtc: cmos: Don't enable shared interrupts if using HPET-based IRQ handler

[Andy Shevchenko]

On Fri, Jan 17, 2020 at 11:57:08AM -0300, Guilherme Piccoli wrote:
> On Fri, Jan 17, 2020 at 11:42 AM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > Let me come out with something such as Thomas suggested.
> >
> 
> Thank you Andy! In case it's gonna take a while (like more than 1
> kernel release), could we get this one accepted to mitigate the issue
> meanwhile?

I have just sent a series where first patch made to be fix and the rest is
some clean ups.

It would be nice if you can test first patch and series as a whole and
give your Tested-by tags.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] rtc: cmos: Don't enable shared interrupts if using HPET-based IRQ handler

[Guilherme Piccoli]

On Fri, Jan 17, 2020 at 2:57 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> I have just sent a series where first patch made to be fix and the rest is
> some clean ups.
>
> It would be nice if you can test first patch and series as a whole and
> give your Tested-by tags.

Sure Andy, thanks for the series! I'll give it a try next week and let you know.
Cheers,


Guilherme