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

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

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