Linux-RTC Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] rtc: cmos: Don't enable shared interrupts if using HPET-based IRQ handler
@ 2020-01-03 14:02 Guilherme G. Piccoli
  2020-01-08 17:41 ` Andy Shevchenko
  0 siblings, 1 reply; 9+ messages in thread
From: Guilherme G. Piccoli @ 2020-01-03 14:02 UTC (permalink / raw)
  To: linux-rtc; +Cc: a.zummo, alexandre.belloni, gpiccoli, kernel, Andy Shevchenko

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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] rtc: cmos: Don't enable shared interrupts if using HPET-based IRQ handler
  2020-01-03 14:02 [PATCH] rtc: cmos: Don't enable shared interrupts if using HPET-based IRQ handler Guilherme G. Piccoli
@ 2020-01-08 17:41 ` Andy Shevchenko
  2020-01-08 19:40   ` Thomas Gleixner
  0 siblings, 1 reply; 9+ messages in thread
From: Andy Shevchenko @ 2020-01-08 17:41 UTC (permalink / raw)
  To: Guilherme G. Piccoli
  Cc: linux-rtc, a.zummo, alexandre.belloni, kernel, Thomas Gleixner

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



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] rtc: cmos: Don't enable shared interrupts if using HPET-based IRQ handler
  2020-01-08 17:41 ` Andy Shevchenko
@ 2020-01-08 19:40   ` Thomas Gleixner
  2020-01-09 18:27     ` Guilherme G. Piccoli
  0 siblings, 1 reply; 9+ messages in thread
From: Thomas Gleixner @ 2020-01-08 19:40 UTC (permalink / raw)
  To: Andy Shevchenko, Guilherme G. Piccoli
  Cc: linux-rtc, a.zummo, alexandre.belloni, kernel

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



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] rtc: cmos: Don't enable shared interrupts if using HPET-based IRQ handler
  2020-01-08 19:40   ` Thomas Gleixner
@ 2020-01-09 18:27     ` Guilherme G. Piccoli
  2020-01-17 10:09       ` Guilherme G. Piccoli
  0 siblings, 1 reply; 9+ messages in thread
From: Guilherme G. Piccoli @ 2020-01-09 18:27 UTC (permalink / raw)
  To: Thomas Gleixner, Andy Shevchenko
  Cc: linux-rtc, a.zummo, alexandre.belloni, kernel, gpiccoli

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] rtc: cmos: Don't enable shared interrupts if using HPET-based IRQ handler
  2020-01-09 18:27     ` Guilherme G. Piccoli
@ 2020-01-17 10:09       ` Guilherme G. Piccoli
  2020-01-17 14:42         ` Andy Shevchenko
  0 siblings, 1 reply; 9+ messages in thread
From: Guilherme G. Piccoli @ 2020-01-17 10:09 UTC (permalink / raw)
  To: Thomas Gleixner, Andy Shevchenko, a.zummo, alexandre.belloni
  Cc: linux-rtc, kernel, gpiccoli

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


Guilherme

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] rtc: cmos: Don't enable shared interrupts if using HPET-based IRQ handler
  2020-01-17 10:09       ` Guilherme G. Piccoli
@ 2020-01-17 14:42         ` Andy Shevchenko
  2020-01-17 14:57           ` Guilherme Piccoli
  0 siblings, 1 reply; 9+ messages in thread
From: Andy Shevchenko @ 2020-01-17 14:42 UTC (permalink / raw)
  To: Guilherme G. Piccoli
  Cc: Thomas Gleixner, a.zummo, alexandre.belloni, linux-rtc, kernel

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



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] rtc: cmos: Don't enable shared interrupts if using HPET-based IRQ handler
  2020-01-17 14:42         ` Andy Shevchenko
@ 2020-01-17 14:57           ` Guilherme Piccoli
  2020-01-17 17:57             ` Andy Shevchenko
  0 siblings, 1 reply; 9+ messages in thread
From: Guilherme Piccoli @ 2020-01-17 14:57 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Thomas Gleixner, a.zummo, alexandre.belloni, linux-rtc,
	Guilherme G. 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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] rtc: cmos: Don't enable shared interrupts if using HPET-based IRQ handler
  2020-01-17 14:57           ` Guilherme Piccoli
@ 2020-01-17 17:57             ` Andy Shevchenko
  2020-01-17 18:01               ` Guilherme Piccoli
  0 siblings, 1 reply; 9+ messages in thread
From: Andy Shevchenko @ 2020-01-17 17:57 UTC (permalink / raw)
  To: Guilherme Piccoli
  Cc: Thomas Gleixner, a.zummo, alexandre.belloni, linux-rtc,
	Guilherme G. Piccoli

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



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] rtc: cmos: Don't enable shared interrupts if using HPET-based IRQ handler
  2020-01-17 17:57             ` Andy Shevchenko
@ 2020-01-17 18:01               ` Guilherme Piccoli
  0 siblings, 0 replies; 9+ messages in thread
From: Guilherme Piccoli @ 2020-01-17 18:01 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Thomas Gleixner, a.zummo, alexandre.belloni, linux-rtc,
	Guilherme G. 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

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, back to index

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-03 14:02 [PATCH] rtc: cmos: Don't enable shared interrupts if using HPET-based IRQ handler Guilherme G. Piccoli
2020-01-08 17:41 ` Andy Shevchenko
2020-01-08 19:40   ` Thomas Gleixner
2020-01-09 18:27     ` Guilherme G. Piccoli
2020-01-17 10:09       ` Guilherme G. Piccoli
2020-01-17 14:42         ` Andy Shevchenko
2020-01-17 14:57           ` Guilherme Piccoli
2020-01-17 17:57             ` Andy Shevchenko
2020-01-17 18:01               ` Guilherme Piccoli

Linux-RTC Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-rtc/0 linux-rtc/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-rtc linux-rtc/ https://lore.kernel.org/linux-rtc \
		linux-rtc@vger.kernel.org
	public-inbox-index linux-rtc

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-rtc


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git