| Message ID | 20200122144529.30307-1-andriy.shevchenko@linux.intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v1,1/3] rtc: cmos: Stop using shared IRQ | expand |
Hi, On 22-01-2020 15:45, Andy Shevchenko wrote: > As reported by Guilherme G. Piccoli: > > --- 8< --- 8< --- > > The rtc-cmos interrupt setting was changed in the 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. > > 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. > > --- 8< --- 8< --- > > After looking into the rtc-cmos driver history and DSDT table from > the Microsoft Surface 3, we may notice that Hans de Goede submitted > a correct fix (see dependency below). Thus, we simply revert > the culprit commit. > > Fixes: 079062b28fb4 ("rtc: cmos: prevent kernel warning on IRQ flags mismatch") > Depends-on: a1e23a42f1bd ("rtc: cmos: Do not assume irq 8 for rtc when there are no legacy irqs") > Reported-by: Guilherme G. Piccoli <gpiccoli@canonical.com> > Cc: Hans de Goede <hdegoede@redhat.com> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Entire series looks good to me: Signed-off-by: Hans de Goede <hdegoede@redhat.com> Regards, Hans > --- > drivers/rtc/rtc-cmos.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/rtc/rtc-cmos.c b/drivers/rtc/rtc-cmos.c > index 033303708c8b..cb28bbdc9e17 100644 > --- a/drivers/rtc/rtc-cmos.c > +++ b/drivers/rtc/rtc-cmos.c > @@ -850,7 +850,7 @@ cmos_do_probe(struct device *dev, struct resource *ports, int rtc_irq) > rtc_cmos_int_handler = cmos_interrupt; > > retval = request_irq(rtc_irq, rtc_cmos_int_handler, > - IRQF_SHARED, dev_name(&cmos_rtc.rtc->dev), > + 0, dev_name(&cmos_rtc.rtc->dev), > cmos_rtc.rtc); > if (retval < 0) { > dev_dbg(dev, "IRQ %d is already in use\n", rtc_irq); >
Hi, On 22-01-2020 17:57, Hans de Goede wrote: > Hi, > > On 22-01-2020 15:45, Andy Shevchenko wrote: >> As reported by Guilherme G. Piccoli: >> >> --- 8< --- 8< --- >> >> The rtc-cmos interrupt setting was changed in the 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. >> >> 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. >> >> --- 8< --- 8< --- >> >> After looking into the rtc-cmos driver history and DSDT table from >> the Microsoft Surface 3, we may notice that Hans de Goede submitted >> a correct fix (see dependency below). Thus, we simply revert >> the culprit commit. >> >> Fixes: 079062b28fb4 ("rtc: cmos: prevent kernel warning on IRQ flags mismatch") >> Depends-on: a1e23a42f1bd ("rtc: cmos: Do not assume irq 8 for rtc when there are no legacy irqs") >> Reported-by: Guilherme G. Piccoli <gpiccoli@canonical.com> >> Cc: Hans de Goede <hdegoede@redhat.com> >> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > Entire series looks good to me: > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> That should have been: Reviewed-by: Hans de Goede <hdegoede@redhat.com> Of course, sorry. Regards, Hans >> --- >> drivers/rtc/rtc-cmos.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/rtc/rtc-cmos.c b/drivers/rtc/rtc-cmos.c >> index 033303708c8b..cb28bbdc9e17 100644 >> --- a/drivers/rtc/rtc-cmos.c >> +++ b/drivers/rtc/rtc-cmos.c >> @@ -850,7 +850,7 @@ cmos_do_probe(struct device *dev, struct resource *ports, int rtc_irq) >> rtc_cmos_int_handler = cmos_interrupt; >> retval = request_irq(rtc_irq, rtc_cmos_int_handler, >> - IRQF_SHARED, dev_name(&cmos_rtc.rtc->dev), >> + 0, dev_name(&cmos_rtc.rtc->dev), >> cmos_rtc.rtc); >> if (retval < 0) { >> dev_dbg(dev, "IRQ %d is already in use\n", rtc_irq); >>
On Wed, Jan 22, 2020 at 11:45 AM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > [...] Thank you Andy, great and simple series! I've tested that on top of 5.5-rc7 and it's working fine with "irqpoll" enabled. Feel free to add my: Tested-by: Guilherme G. Piccoli <gpiccoli@canonical.com> The only oddity here is about the scissors, I'm not sure how it is supposed to work on git, but when I git am'ed the patch, the commit message was only "As reported by Guilherme G. Piccoli:", everything else was dropped. Cheers, Guilherme
On Wed, Jan 22, 2020 at 02:32:01PM -0300, Guilherme Piccoli wrote: > On Wed, Jan 22, 2020 at 11:45 AM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > [...] > > Thank you Andy, great and simple series! I've tested that on top of > 5.5-rc7 and it's working fine with "irqpoll" enabled. > Feel free to add my: > > Tested-by: Guilherme G. Piccoli <gpiccoli@canonical.com> Thanks! > The only oddity here is about the scissors, I'm not sure how it is > supposed to work on git, but when I git am'ed the patch, the commit > message was only "As reported by Guilherme G. Piccoli:", everything > else was dropped. I didn't read any RFC or document about '--- ' line. But seems either bug in git-am, or I have to put something else at the beginning of those lines.
On Wed, Jan 22, 2020 at 4:58 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > [...] > > The only oddity here is about the scissors, I'm not sure how it is > > supposed to work on git, but when I git am'ed the patch, the commit > > message was only "As reported by Guilherme G. Piccoli:", everything > > else was dropped. > > I didn't read any RFC or document about '--- ' line. But seems either bug in > git-am, or I have to put something else at the beginning of those lines. > Interesting! What I usually do is to add comments to-be-disregarded-on-merge right below "---", before the diff starts. Git will discard this full block. I guess probably having the "---" on top makes Git discard everything below it until the diff. Anyway, hope your commit message don't get messed in the merge heh Cheers, Guilherme
On Wed, Jan 22, 2020 at 05:05:15PM -0300, Guilherme Piccoli wrote: > On Wed, Jan 22, 2020 at 4:58 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > [...] > > > The only oddity here is about the scissors, I'm not sure how it is > > > supposed to work on git, but when I git am'ed the patch, the commit > > > message was only "As reported by Guilherme G. Piccoli:", everything > > > else was dropped. > > > > I didn't read any RFC or document about '--- ' line. But seems either bug in > > git-am, or I have to put something else at the beginning of those lines. > > > > Interesting! What I usually do is to add comments > to-be-disregarded-on-merge right below "---", before the diff starts. > Git will discard this full block. I guess probably having the "---" on > top makes Git discard everything below it until the diff. For the record it's '--- ' (mind the whitespace at the end). > Anyway, hope your commit message don't get messed in the merge heh I'll send a new version.
diff --git a/drivers/rtc/rtc-cmos.c b/drivers/rtc/rtc-cmos.c index 033303708c8b..cb28bbdc9e17 100644 --- a/drivers/rtc/rtc-cmos.c +++ b/drivers/rtc/rtc-cmos.c @@ -850,7 +850,7 @@ cmos_do_probe(struct device *dev, struct resource *ports, int rtc_irq) rtc_cmos_int_handler = cmos_interrupt; retval = request_irq(rtc_irq, rtc_cmos_int_handler, - IRQF_SHARED, dev_name(&cmos_rtc.rtc->dev), + 0, dev_name(&cmos_rtc.rtc->dev), cmos_rtc.rtc); if (retval < 0) { dev_dbg(dev, "IRQ %d is already in use\n", rtc_irq);
As reported by Guilherme G. Piccoli: --- 8< --- 8< --- The rtc-cmos interrupt setting was changed in the 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. 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. --- 8< --- 8< --- After looking into the rtc-cmos driver history and DSDT table from the Microsoft Surface 3, we may notice that Hans de Goede submitted a correct fix (see dependency below). Thus, we simply revert the culprit commit. Fixes: 079062b28fb4 ("rtc: cmos: prevent kernel warning on IRQ flags mismatch") Depends-on: a1e23a42f1bd ("rtc: cmos: Do not assume irq 8 for rtc when there are no legacy irqs") Reported-by: Guilherme G. Piccoli <gpiccoli@canonical.com> Cc: Hans de Goede <hdegoede@redhat.com> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/rtc/rtc-cmos.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)