| Message ID | 1612697823-8073-1-git-send-email-tanxiaofei@huawei.com |
|---|---|
| Headers | show |
| Series | spin lock usage optimization for SCSI drivers | expand |
> -----Original Message----- > From: Finn Thain [mailto:fthain@telegraphics.com.au] > Sent: Monday, February 8, 2021 8:57 PM > To: tanxiaofei <tanxiaofei@huawei.com> > Cc: jejb@linux.ibm.com; martin.petersen@oracle.com; > linux-scsi@vger.kernel.org; linux-kernel@vger.kernel.org; > linuxarm@openeuler.org > Subject: [Linuxarm] Re: [PATCH for-next 00/32] spin lock usage optimization > for SCSI drivers > > On Sun, 7 Feb 2021, Xiaofei Tan wrote: > > > Replace spin_lock_irqsave with spin_lock in hard IRQ of SCSI drivers. > > There are no function changes, but may speed up if interrupt happen too > > often. > > This change doesn't necessarily work on platforms that support nested > interrupts. > > Were you able to measure any benefit from this change on some other > platform? I think the code disabling irq in hardIRQ is simply wrong. Since this commit https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=e58aa3d2d0cc genirq: Run irq handlers with interrupts disabled interrupt handlers are definitely running in a irq-disabled context unless irq handlers enable them explicitly in the handler to permit other interrupts. > > Please see also, > https://lore.kernel.org/linux-scsi/89c5cb05cb844939ae684db0077f675f@h3c.co > m/ > _______________________________________________ > Linuxarm mailing list -- linuxarm@openeuler.org > To unsubscribe Thanks Barry
Hi Finn, Thanks for reviewing the patch set. On 2021/2/8 15:57, Finn Thain wrote: > On Sun, 7 Feb 2021, Xiaofei Tan wrote: > >> Replace spin_lock_irqsave with spin_lock in hard IRQ of SCSI drivers. >> There are no function changes, but may speed up if interrupt happen too >> often. > > This change doesn't necessarily work on platforms that support nested > interrupts. > Linux doesn't support nested interrupts anymore after the following patch, so please don't worry this. https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=e58aa3d2d0cc > Were you able to measure any benefit from this change on some other > platform? > It's hard to measure the benefit of this change. Hmm, you could take this patch set as cleanup. thanks. > Please see also, > https://lore.kernel.org/linux-scsi/89c5cb05cb844939ae684db0077f675f@h3c.com/ > > . >
On Tue, 9 Feb 2021, Song Bao Hua (Barry Song) wrote: > > -----Original Message----- > > From: Finn Thain [mailto:fthain@telegraphics.com.au] > > Sent: Monday, February 8, 2021 8:57 PM > > To: tanxiaofei <tanxiaofei@huawei.com> > > Cc: jejb@linux.ibm.com; martin.petersen@oracle.com; > > linux-scsi@vger.kernel.org; linux-kernel@vger.kernel.org; > > linuxarm@openeuler.org > > Subject: [Linuxarm] Re: [PATCH for-next 00/32] spin lock usage optimization > > for SCSI drivers > > > > On Sun, 7 Feb 2021, Xiaofei Tan wrote: > > > > > Replace spin_lock_irqsave with spin_lock in hard IRQ of SCSI drivers. > > > There are no function changes, but may speed up if interrupt happen too > > > often. > > > > This change doesn't necessarily work on platforms that support nested > > interrupts. > > > > Were you able to measure any benefit from this change on some other > > platform? > > I think the code disabling irq in hardIRQ is simply wrong. > Since this commit > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=e58aa3d2d0cc > genirq: Run irq handlers with interrupts disabled > > interrupt handlers are definitely running in a irq-disabled context > unless irq handlers enable them explicitly in the handler to permit > other interrupts. > Repeating the same claim does not somehow make it true. If you put your claim to the test, you'll see that that interrupts are not disabled on m68k when interrupt handlers execute. The Interrupt Priority Level (IPL) can prevent any given irq handler from being re-entered, but an irq with a higher priority level may be handled during execution of a lower priority irq handler. sonic_interrupt() uses an irq lock within an interrupt handler to avoid issues relating to this. This kind of locking may be needed in the drivers you are trying to patch. Or it might not. Apparently, no-one has looked.
On Tue, 9 Feb 2021, tanxiaofei wrote: > Hi Finn, > Thanks for reviewing the patch set. > > On 2021/2/8 15:57, Finn Thain wrote: > > On Sun, 7 Feb 2021, Xiaofei Tan wrote: > > > > > Replace spin_lock_irqsave with spin_lock in hard IRQ of SCSI drivers. > > > There are no function changes, but may speed up if interrupt happen too > > > often. > > > > This change doesn't necessarily work on platforms that support nested > > interrupts. > > > > Linux doesn't support nested interrupts anymore after the following > patch, so please don't worry this. > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=e58aa3d2d0cc > Clearly that patch did not disable interrupts. It removed a statement that enabled them. > > Were you able to measure any benefit from this change on some other > > platform? > > > > It's hard to measure the benefit of this change. It's hard to see any benefit. But it's easy to see risk, when there's no indication that you've confirmed that the affected drivers do not rely on the irq lock, nor tested them for regressions, nor checked whether the affected platforms meet your assumuptions. > Hmm, you could take this patch set as cleanup. thanks. > A "cleanup" does not change program behaviour. Can you demonstrate that program behaviour is unchanged? > > Please see also, > > https://lore.kernel.org/linux-scsi/89c5cb05cb844939ae684db0077f675f@h3c.com/ > > > > . > > > >
> -----Original Message----- > From: Finn Thain [mailto:fthain@telegraphics.com.au] > Sent: Tuesday, February 9, 2021 6:06 PM > To: Song Bao Hua (Barry Song) <song.bao.hua@hisilicon.com> > Cc: tanxiaofei <tanxiaofei@huawei.com>; jejb@linux.ibm.com; > martin.petersen@oracle.com; linux-scsi@vger.kernel.org; > linux-kernel@vger.kernel.org; linuxarm@openeuler.org; > linux-m68k@vger.kernel.org > Subject: RE: [Linuxarm] Re: [PATCH for-next 00/32] spin lock usage optimization > for SCSI drivers > > On Tue, 9 Feb 2021, Song Bao Hua (Barry Song) wrote: > > > > -----Original Message----- > > > From: Finn Thain [mailto:fthain@telegraphics.com.au] > > > Sent: Monday, February 8, 2021 8:57 PM > > > To: tanxiaofei <tanxiaofei@huawei.com> > > > Cc: jejb@linux.ibm.com; martin.petersen@oracle.com; > > > linux-scsi@vger.kernel.org; linux-kernel@vger.kernel.org; > > > linuxarm@openeuler.org > > > Subject: [Linuxarm] Re: [PATCH for-next 00/32] spin lock usage optimization > > > for SCSI drivers > > > > > > On Sun, 7 Feb 2021, Xiaofei Tan wrote: > > > > > > > Replace spin_lock_irqsave with spin_lock in hard IRQ of SCSI drivers. > > > > There are no function changes, but may speed up if interrupt happen too > > > > often. > > > > > > This change doesn't necessarily work on platforms that support nested > > > interrupts. > > > > > > Were you able to measure any benefit from this change on some other > > > platform? > > > > I think the code disabling irq in hardIRQ is simply wrong. > > Since this commit > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/ > ?id=e58aa3d2d0cc > > genirq: Run irq handlers with interrupts disabled > > > > interrupt handlers are definitely running in a irq-disabled context > > unless irq handlers enable them explicitly in the handler to permit > > other interrupts. > > > > Repeating the same claim does not somehow make it true. If you put your Sorry for I didn't realize xiaofei had replied. > claim to the test, you'll see that that interrupts are not disabled on > m68k when interrupt handlers execute. Sounds like an implementation issue of m68k since IRQF_DISABLED has been totally removed. > > The Interrupt Priority Level (IPL) can prevent any given irq handler from > being re-entered, but an irq with a higher priority level may be handled > during execution of a lower priority irq handler. > We used to have IRQF_DISABLED to support so-called "fast interrupt" to avoid this. But the concept has been totally removed. That is interesting if m68k still has this issue. > sonic_interrupt() uses an irq lock within an interrupt handler to avoid > issues relating to this. This kind of locking may be needed in the drivers > you are trying to patch. Or it might not. Apparently, no-one has looked. Thanks Barry
> -----Original Message----- > From: Song Bao Hua (Barry Song) > Sent: Tuesday, February 9, 2021 6:28 PM > To: 'Finn Thain' <fthain@telegraphics.com.au> > Cc: tanxiaofei <tanxiaofei@huawei.com>; jejb@linux.ibm.com; > martin.petersen@oracle.com; linux-scsi@vger.kernel.org; > linux-kernel@vger.kernel.org; linuxarm@openeuler.org; > linux-m68k@vger.kernel.org > Subject: RE: [Linuxarm] Re: [PATCH for-next 00/32] spin lock usage optimization > for SCSI drivers > > > > > -----Original Message----- > > From: Finn Thain [mailto:fthain@telegraphics.com.au] > > Sent: Tuesday, February 9, 2021 6:06 PM > > To: Song Bao Hua (Barry Song) <song.bao.hua@hisilicon.com> > > Cc: tanxiaofei <tanxiaofei@huawei.com>; jejb@linux.ibm.com; > > martin.petersen@oracle.com; linux-scsi@vger.kernel.org; > > linux-kernel@vger.kernel.org; linuxarm@openeuler.org; > > linux-m68k@vger.kernel.org > > Subject: RE: [Linuxarm] Re: [PATCH for-next 00/32] spin lock usage optimization > > for SCSI drivers > > > > On Tue, 9 Feb 2021, Song Bao Hua (Barry Song) wrote: > > > > > > -----Original Message----- > > > > From: Finn Thain [mailto:fthain@telegraphics.com.au] > > > > Sent: Monday, February 8, 2021 8:57 PM > > > > To: tanxiaofei <tanxiaofei@huawei.com> > > > > Cc: jejb@linux.ibm.com; martin.petersen@oracle.com; > > > > linux-scsi@vger.kernel.org; linux-kernel@vger.kernel.org; > > > > linuxarm@openeuler.org > > > > Subject: [Linuxarm] Re: [PATCH for-next 00/32] spin lock usage optimization > > > > for SCSI drivers > > > > > > > > On Sun, 7 Feb 2021, Xiaofei Tan wrote: > > > > > > > > > Replace spin_lock_irqsave with spin_lock in hard IRQ of SCSI drivers. > > > > > There are no function changes, but may speed up if interrupt happen > too > > > > > often. > > > > > > > > This change doesn't necessarily work on platforms that support nested > > > > interrupts. > > > > > > > > Were you able to measure any benefit from this change on some other > > > > platform? > > > > > > I think the code disabling irq in hardIRQ is simply wrong. > > > Since this commit > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/ > > ?id=e58aa3d2d0cc > > > genirq: Run irq handlers with interrupts disabled > > > > > > interrupt handlers are definitely running in a irq-disabled context > > > unless irq handlers enable them explicitly in the handler to permit > > > other interrupts. > > > > > > > Repeating the same claim does not somehow make it true. If you put your > > Sorry for I didn't realize xiaofei had replied. > > > claim to the test, you'll see that that interrupts are not disabled on > > m68k when interrupt handlers execute. > > Sounds like an implementation issue of m68k since IRQF_DISABLED has > been totally removed. > > > > > The Interrupt Priority Level (IPL) can prevent any given irq handler from > > being re-entered, but an irq with a higher priority level may be handled > > during execution of a lower priority irq handler. > > > > We used to have IRQF_DISABLED to support so-called "fast interrupt" to avoid > this. But the concept has been totally removed. That is interesting if m68k > still has this issue. > > > sonic_interrupt() uses an irq lock within an interrupt handler to avoid > > issues relating to this. This kind of locking may be needed in the drivers > > you are trying to patch. Or it might not. Apparently, no-one has looked. Is the comment in sonic_interrupt() outdated according to this: m68k: irq: Remove IRQF_DISABLED https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=77a4279 http://lkml.iu.edu/hypermail/linux/kernel/1109.2/01687.html and this: genirq: Warn when handler enables interrupts https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=b738a50a wouldn't genirq report a warning on m68k? > > Thanks > Barry Thanks Barry
On Tue, 9 Feb 2021, Song Bao Hua (Barry Song) wrote: > > On Tue, 9 Feb 2021, Song Bao Hua (Barry Song) wrote: > > > > > > On Sun, 7 Feb 2021, Xiaofei Tan wrote: > > > > > > > > > Replace spin_lock_irqsave with spin_lock in hard IRQ of SCSI > > > > > drivers. There are no function changes, but may speed up if > > > > > interrupt happen too often. > > > > > > > > This change doesn't necessarily work on platforms that support > > > > nested interrupts. > > > > > > > > Were you able to measure any benefit from this change on some > > > > other platform? > > > > > > I think the code disabling irq in hardIRQ is simply wrong. Since > > > this commit > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=e58aa3d2d0cc > > > genirq: Run irq handlers with interrupts disabled > > > > > > interrupt handlers are definitely running in a irq-disabled context > > > unless irq handlers enable them explicitly in the handler to permit > > > other interrupts. > > > > > > > Repeating the same claim does not somehow make it true. > > Sorry for I didn't realize xiaofei had replied. > I was referring to the claim in patch 00/32, i.e. that interrupt handlers only run when irqs are disabled. > > If you put your claim to the test, you'll see that that interrupts are > > not disabled on m68k when interrupt handlers execute. > > Sounds like an implementation issue of m68k since IRQF_DISABLED has been > totally removed. > It's true that IRQF_DISABLED could be used to avoid the need for irq locks in interrupt handlers. So, if you want to remove irq locks from interrupt handlers, today you can't use IRQF_DISABLED to help you. So what? > > > > The Interrupt Priority Level (IPL) can prevent any given irq handler > > from being re-entered, but an irq with a higher priority level may be > > handled during execution of a lower priority irq handler. > > > > We used to have IRQF_DISABLED to support so-called "fast interrupt" to > avoid this. > > But the concept has been totally removed. That is interesting if m68k > still has this issue. > Prioritized interrupts are beneficial. Why would you want to avoid them? Moreover, there's no reason to believe that m68k is the only platform that supports nested interrupts. > > sonic_interrupt() uses an irq lock within an interrupt handler to > > avoid issues relating to this. This kind of locking may be needed in > > the drivers you are trying to patch. Or it might not. Apparently, > > no-one has looked. > > Thanks > Barry >
> -----Original Message----- > From: Finn Thain [mailto:fthain@telegraphics.com.au] > Sent: Wednesday, February 10, 2021 1:29 PM > To: Song Bao Hua (Barry Song) <song.bao.hua@hisilicon.com> > Cc: tanxiaofei <tanxiaofei@huawei.com>; jejb@linux.ibm.com; > martin.petersen@oracle.com; linux-scsi@vger.kernel.org; > linux-kernel@vger.kernel.org; linuxarm@openeuler.org; > linux-m68k@vger.kernel.org > Subject: RE: [Linuxarm] Re: [PATCH for-next 00/32] spin lock usage optimization > for SCSI drivers > > On Tue, 9 Feb 2021, Song Bao Hua (Barry Song) wrote: > > > > On Tue, 9 Feb 2021, Song Bao Hua (Barry Song) wrote: > > > > > > > > On Sun, 7 Feb 2021, Xiaofei Tan wrote: > > > > > > > > > > > Replace spin_lock_irqsave with spin_lock in hard IRQ of SCSI > > > > > > drivers. There are no function changes, but may speed up if > > > > > > interrupt happen too often. > > > > > > > > > > This change doesn't necessarily work on platforms that support > > > > > nested interrupts. > > > > > > > > > > Were you able to measure any benefit from this change on some > > > > > other platform? > > > > > > > > I think the code disabling irq in hardIRQ is simply wrong. Since > > > > this commit > > > > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/ > ?id=e58aa3d2d0cc > > > > genirq: Run irq handlers with interrupts disabled > > > > > > > > interrupt handlers are definitely running in a irq-disabled context > > > > unless irq handlers enable them explicitly in the handler to permit > > > > other interrupts. > > > > > > > > > > Repeating the same claim does not somehow make it true. > > > > Sorry for I didn't realize xiaofei had replied. > > > > I was referring to the claim in patch 00/32, i.e. that interrupt handlers > only run when irqs are disabled. > > > > If you put your claim to the test, you'll see that that interrupts are > > > not disabled on m68k when interrupt handlers execute. > > > > Sounds like an implementation issue of m68k since IRQF_DISABLED has been > > totally removed. > > > > It's true that IRQF_DISABLED could be used to avoid the need for irq locks > in interrupt handlers. So, if you want to remove irq locks from interrupt > handlers, today you can't use IRQF_DISABLED to help you. So what? > > > > > > > The Interrupt Priority Level (IPL) can prevent any given irq handler > > > from being re-entered, but an irq with a higher priority level may be > > > handled during execution of a lower priority irq handler. > > > > > > > We used to have IRQF_DISABLED to support so-called "fast interrupt" to > > avoid this. > > > > But the concept has been totally removed. That is interesting if m68k > > still has this issue. > > > > Prioritized interrupts are beneficial. Why would you want to avoid them? > I doubt this is true as it has been already thought as unnecessary in Linux: https://lwn.net/Articles/380931/ > Moreover, there's no reason to believe that m68k is the only platform that > supports nested interrupts. I doubt that is true as genirq is running understand the consumption that hardIRQ is running in irq-disabled context: "We run all handlers with interrupts disabled and expect them not to enable them. Warn when we catch one who does." https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=b738a50a If it is, m68k is against the assumption of genirq. > > > > sonic_interrupt() uses an irq lock within an interrupt handler to > > > avoid issues relating to this. This kind of locking may be needed in > > > the drivers you are trying to patch. Or it might not. Apparently, > > > no-one has looked. > > Thanks Barry
On Wed, 10 Feb 2021, Song Bao Hua (Barry Song) wrote: > > On Tue, 9 Feb 2021, Song Bao Hua (Barry Song) wrote: > > > > > > On Tue, 9 Feb 2021, Song Bao Hua (Barry Song) wrote: > > > > > > > > > > On Sun, 7 Feb 2021, Xiaofei Tan wrote: > > > > > > > > > > > > > Replace spin_lock_irqsave with spin_lock in hard IRQ of SCSI > > > > > > > drivers. There are no function changes, but may speed up if > > > > > > > interrupt happen too often. > > > > > > > > > > > > This change doesn't necessarily work on platforms that support > > > > > > nested interrupts. > > > > > > > > > > > > Were you able to measure any benefit from this change on some > > > > > > other platform? > > > > > > > > > > I think the code disabling irq in hardIRQ is simply wrong. Since > > > > > this commit > > > > > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=e58aa3d2d0cc > > > > > genirq: Run irq handlers with interrupts disabled > > > > > > > > > > interrupt handlers are definitely running in a irq-disabled > > > > > context unless irq handlers enable them explicitly in the > > > > > handler to permit other interrupts. > > > > > > > > > > > > > Repeating the same claim does not somehow make it true. > > > > > > Sorry for I didn't realize xiaofei had replied. > > > > > > > I was referring to the claim in patch 00/32, i.e. that interrupt > > handlers only run when irqs are disabled. > > > > > > If you put your claim to the test, you'll see that that interrupts > > > > are not disabled on m68k when interrupt handlers execute. > > > > > > Sounds like an implementation issue of m68k since IRQF_DISABLED has > > > been totally removed. > > > > > > > It's true that IRQF_DISABLED could be used to avoid the need for irq > > locks in interrupt handlers. So, if you want to remove irq locks from > > interrupt handlers, today you can't use IRQF_DISABLED to help you. So > > what? > > > > > > > > > > The Interrupt Priority Level (IPL) can prevent any given irq > > > > handler from being re-entered, but an irq with a higher priority > > > > level may be handled during execution of a lower priority irq > > > > handler. > > > > > > > > > > We used to have IRQF_DISABLED to support so-called "fast interrupt" > > > to avoid this. > > > > > > But the concept has been totally removed. That is interesting if > > > m68k still has this issue. > > > > > > > Prioritized interrupts are beneficial. Why would you want to avoid > > them? > > > > I doubt this is true as it has been already thought as unnecessary > in Linux: > https://lwn.net/Articles/380931/ > The article you cited does not refute what I said about prioritized interrupts. The article is about eliminating the distinction between fast and slow interrupt handlers. The article says that some developers convinced Linus that, although minimal interrupt latency is desirable, is isn't strictly necessary. The article also warns of stack overflow from arbitrarily deep slow interrupt nesting, but that's not what m68k does. > > Moreover, there's no reason to believe that m68k is the only platform > > that supports nested interrupts. > > I doubt that is true as genirq is running understand the consumption > that hardIRQ is running in irq-disabled context: I'm not going to guess whether other platforms might be affected -- you're supporting this patch so you will have to show that it is correct. > "We run all handlers with interrupts disabled and expect them not to > enable them. Warn when we catch one who does." > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=b738a50a > > If it is, m68k is against the assumption of genirq. > Interrupt handlers on m68k do not enable interrupts. If they did, you would see that warning fire. It doesn't fire. Try it. > > > > > > sonic_interrupt() uses an irq lock within an interrupt handler to > > > > avoid issues relating to this. This kind of locking may be needed in > > > > the drivers you are trying to patch. Or it might not. Apparently, > > > > no-one has looked. > > > > > Thanks > Barry >
On Tue, 9 Feb 2021, Song Bao Hua (Barry Song) wrote: > > > sonic_interrupt() uses an irq lock within an interrupt handler to > > > avoid issues relating to this. This kind of locking may be needed in > > > the drivers you are trying to patch. Or it might not. Apparently, > > > no-one has looked. > > Is the comment in sonic_interrupt() outdated according to this: > m68k: irq: Remove IRQF_DISABLED > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=77a4279 > http://lkml.iu.edu/hypermail/linux/kernel/1109.2/01687.html > The removal of IRQF_DISABLED isn't relevant to this driver. Commit 77a42796786c ("m68k: Remove deprecated IRQF_DISABLED") did not disable interrupts, it just removed some code to enable them. The code and comments in sonic_interrupt() are correct. You can confirm this for yourself quite easily using QEMU and a cross-compiler. > and this: > genirq: Warn when handler enables interrupts > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=b738a50a > > wouldn't genirq report a warning on m68k? > There is no warning from m68k builds. That's because arch_irqs_disabled() returns true when the IPL is non-zero. > > > > Thanks > > Barry > > Thanks > Barry > >
> -----Original Message----- > From: Finn Thain [mailto:fthain@telegraphics.com.au] > Sent: Wednesday, February 10, 2021 5:16 PM > To: Song Bao Hua (Barry Song) <song.bao.hua@hisilicon.com> > Cc: tanxiaofei <tanxiaofei@huawei.com>; jejb@linux.ibm.com; > martin.petersen@oracle.com; linux-scsi@vger.kernel.org; > linux-kernel@vger.kernel.org; linuxarm@openeuler.org; > linux-m68k@vger.kernel.org > Subject: [Linuxarm] Re: [PATCH for-next 00/32] spin lock usage optimization > for SCSI drivers > > On Tue, 9 Feb 2021, Song Bao Hua (Barry Song) wrote: > > > > > sonic_interrupt() uses an irq lock within an interrupt handler to > > > > avoid issues relating to this. This kind of locking may be needed in > > > > the drivers you are trying to patch. Or it might not. Apparently, > > > > no-one has looked. > > > > Is the comment in sonic_interrupt() outdated according to this: > > m68k: irq: Remove IRQF_DISABLED > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/ > ?id=77a4279 > > http://lkml.iu.edu/hypermail/linux/kernel/1109.2/01687.html > > > > The removal of IRQF_DISABLED isn't relevant to this driver. Commit > 77a42796786c ("m68k: Remove deprecated IRQF_DISABLED") did not disable > interrupts, it just removed some code to enable them. > > The code and comments in sonic_interrupt() are correct. You can confirm > this for yourself quite easily using QEMU and a cross-compiler. > > > and this: > > genirq: Warn when handler enables interrupts > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/ > ?id=b738a50a > > > > wouldn't genirq report a warning on m68k? > > > > There is no warning from m68k builds. That's because arch_irqs_disabled() > returns true when the IPL is non-zero. So for m68k, the case is arch_irqs_disabled() is true, but interrupts can still come? Then it seems it is very confusing. If prioritized interrupts can still come while arch_irqs_disabled() is true, how could spin_lock_irqsave() block the prioritized interrupts? Isn't arch_irqs_disabled() a status reflection of irq disable API? Thanks Barry
On Wed, 10 Feb 2021, Song Bao Hua (Barry Song) wrote: > > On Tue, 9 Feb 2021, Song Bao Hua (Barry Song) wrote: > > > > > > > sonic_interrupt() uses an irq lock within an interrupt handler > > > > > to avoid issues relating to this. This kind of locking may be > > > > > needed in the drivers you are trying to patch. Or it might not. > > > > > Apparently, no-one has looked. > > > > > > Is the comment in sonic_interrupt() outdated according to this: > > > m68k: irq: Remove IRQF_DISABLED > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=77a4279 > > > http://lkml.iu.edu/hypermail/linux/kernel/1109.2/01687.html > > > > > > > The removal of IRQF_DISABLED isn't relevant to this driver. Commit > > 77a42796786c ("m68k: Remove deprecated IRQF_DISABLED") did not disable > > interrupts, it just removed some code to enable them. > > > > The code and comments in sonic_interrupt() are correct. You can > > confirm this for yourself quite easily using QEMU and a > > cross-compiler. > > > > > and this: genirq: Warn when handler enables interrupts > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=b738a50a > > > > > > wouldn't genirq report a warning on m68k? > > > > > > > There is no warning from m68k builds. That's because > > arch_irqs_disabled() returns true when the IPL is non-zero. > > > So for m68k, the case is > arch_irqs_disabled() is true, but interrupts can still come? > > Then it seems it is very confusing. If prioritized interrupts can still > come while arch_irqs_disabled() is true, Yes, on m68k CPUs, an IRQ having a priority level higher than the present priority mask will get serviced. Non-Maskable Interrupt (NMI) is not subject to this rule and gets serviced regardless. > how could spin_lock_irqsave() block the prioritized interrupts? It raises the the mask level to 7. Again, please see arch/m68k/include/asm/irqflags.h > Isn't arch_irqs_disabled() a status reflection of irq disable API? > Why not? Are all interrupts (including NMI) masked whenever arch_irqs_disabled() returns true on your platforms? > Thanks > Barry > >
> -----Original Message----- > From: Finn Thain [mailto:fthain@telegraphics.com.au] > Sent: Thursday, February 11, 2021 10:07 AM > To: Song Bao Hua (Barry Song) <song.bao.hua@hisilicon.com> > Cc: tanxiaofei <tanxiaofei@huawei.com>; jejb@linux.ibm.com; > martin.petersen@oracle.com; linux-scsi@vger.kernel.org; > linux-kernel@vger.kernel.org; linuxarm@openeuler.org; > linux-m68k@vger.kernel.org > Subject: RE: [Linuxarm] Re: [PATCH for-next 00/32] spin lock usage optimization > for SCSI drivers > > > On Wed, 10 Feb 2021, Song Bao Hua (Barry Song) wrote: > > > > On Tue, 9 Feb 2021, Song Bao Hua (Barry Song) wrote: > > > > > > > > > sonic_interrupt() uses an irq lock within an interrupt handler > > > > > > to avoid issues relating to this. This kind of locking may be > > > > > > needed in the drivers you are trying to patch. Or it might not. > > > > > > Apparently, no-one has looked. > > > > > > > > Is the comment in sonic_interrupt() outdated according to this: > > > > m68k: irq: Remove IRQF_DISABLED > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/ > ?id=77a4279 > > > > http://lkml.iu.edu/hypermail/linux/kernel/1109.2/01687.html > > > > > > > > > > The removal of IRQF_DISABLED isn't relevant to this driver. Commit > > > 77a42796786c ("m68k: Remove deprecated IRQF_DISABLED") did not disable > > > interrupts, it just removed some code to enable them. > > > > > > The code and comments in sonic_interrupt() are correct. You can > > > confirm this for yourself quite easily using QEMU and a > > > cross-compiler. > > > > > > > and this: genirq: Warn when handler enables interrupts > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/ > ?id=b738a50a > > > > > > > > wouldn't genirq report a warning on m68k? > > > > > > > > > > There is no warning from m68k builds. That's because > > > arch_irqs_disabled() returns true when the IPL is non-zero. > > > > > > So for m68k, the case is > > arch_irqs_disabled() is true, but interrupts can still come? > > > > Then it seems it is very confusing. If prioritized interrupts can still > > come while arch_irqs_disabled() is true, > > Yes, on m68k CPUs, an IRQ having a priority level higher than the present > priority mask will get serviced. > > Non-Maskable Interrupt (NMI) is not subject to this rule and gets serviced > regardless. > > > how could spin_lock_irqsave() block the prioritized interrupts? > > It raises the the mask level to 7. Again, please see > arch/m68k/include/asm/irqflags.h Hi Finn, Thanks for your explanation again. TBH, that is why m68k is so confusing. irqs_disabled() on m68k should just reflect the status of all interrupts have been disabled except NMI. irqs_disabled() should be consistent with the calling of APIs such as local_irq_disable, local_irq_save, spin_lock_irqsave etc. > > > Isn't arch_irqs_disabled() a status reflection of irq disable API? > > > > Why not? If so, arch_irqs_disabled() should mean all interrupts have been masked except NMI as NMI is unmaskable. > > Are all interrupts (including NMI) masked whenever arch_irqs_disabled() > returns true on your platforms? On my platform, once irqs_disabled() is true, all interrupts are masked except NMI. NMI just ignore spin_lock_irqsave or local_irq_disable. On ARM64, we also have high-priority interrupts, but they are running as PESUDO_NMI: https://lwn.net/Articles/755906/ On m68k, it seems you mean： irq_disabled() is true, but high-priority interrupts can still come; local_irq_disable() can disable high-priority interrupts, and at that time, irq_disabled() is also true. TBH, this is wrong and confusing on m68k. > > > Thanks > > Barry > > Thanks Barry
On Wed, 10 Feb 2021, Song Bao Hua (Barry Song) wrote: > > On Wed, 10 Feb 2021, Song Bao Hua (Barry Song) wrote: > > > > > > > > > > There is no warning from m68k builds. That's because > > > > arch_irqs_disabled() returns true when the IPL is non-zero. > > > > > > So for m68k, the case is > > > arch_irqs_disabled() is true, but interrupts can still come? > > > > > > Then it seems it is very confusing. If prioritized interrupts can > > > still come while arch_irqs_disabled() is true, > > > > Yes, on m68k CPUs, an IRQ having a priority level higher than the > > present priority mask will get serviced. > > > > Non-Maskable Interrupt (NMI) is not subject to this rule and gets > > serviced regardless. > > > > > how could spin_lock_irqsave() block the prioritized interrupts? > > > > It raises the the mask level to 7. Again, please see > > arch/m68k/include/asm/irqflags.h > > Hi Finn, > Thanks for your explanation again. > > TBH, that is why m68k is so confusing. irqs_disabled() on m68k should > just reflect the status of all interrupts have been disabled except NMI. > > irqs_disabled() should be consistent with the calling of APIs such as > local_irq_disable, local_irq_save, spin_lock_irqsave etc. > When irqs_disabled() returns true, we cannot infer that arch_local_irq_disable() was called. But I have not yet found driver code or core kernel code attempting that inference. > > > > > Isn't arch_irqs_disabled() a status reflection of irq disable API? > > > > > > > Why not? > > If so, arch_irqs_disabled() should mean all interrupts have been masked > except NMI as NMI is unmaskable. > Can you support that claim with a reference to core kernel code or documentation? (If some arch code agrees with you, that's neither here nor there.) > > > > Are all interrupts (including NMI) masked whenever > > arch_irqs_disabled() returns true on your platforms? > > On my platform, once irqs_disabled() is true, all interrupts are masked > except NMI. NMI just ignore spin_lock_irqsave or local_irq_disable. > > On ARM64, we also have high-priority interrupts, but they are running as > PESUDO_NMI: > https://lwn.net/Articles/755906/ > A glance at the ARM GIC specification suggests that your hardware works much like 68000 hardware. When enabled, a CPU interface takes the highest priority pending interrupt for its connected processor and determines whether the interrupt has sufficient priority for it to signal the interrupt request to the processor. [...] When the processor acknowledges the interrupt at the CPU interface, the Distributor changes the status of the interrupt from pending to either active, or active and pending. At this point the CPU interface can signal another interrupt to the processor, to preempt interrupts that are active on the processor. If there is no pending interrupt with sufficient priority for signaling to the processor, the interface deasserts the interrupt request signal to the processor. https://developer.arm.com/documentation/ihi0048/b/ Have you considered that Linux/arm might benefit if it could fully exploit hardware features already available, such as the interrupt priority masking feature in the GIC in existing arm systems? > On m68k, it seems you mean： > irq_disabled() is true, but high-priority interrupts can still come; > local_irq_disable() can disable high-priority interrupts, and at that > time, irq_disabled() is also true. > > TBH, this is wrong and confusing on m68k. > Like you, I was surprised when I learned about it. But that doesn't mean it's wrong. The fact that it works should tell you something. Things could always be made simpler. But discarding features isn't necessarily an improvement. > > > > > Thanks > > > Barry > > > > > Thanks > Barry >
> -----Original Message----- > From: Finn Thain [mailto:fthain@telegraphics.com.au] > Sent: Thursday, February 11, 2021 11:35 AM > To: Song Bao Hua (Barry Song) <song.bao.hua@hisilicon.com> > Cc: tanxiaofei <tanxiaofei@huawei.com>; jejb@linux.ibm.com; > martin.petersen@oracle.com; linux-scsi@vger.kernel.org; > linux-kernel@vger.kernel.org; linuxarm@openeuler.org; > linux-m68k@vger.kernel.org > Subject: RE: [Linuxarm] Re: [PATCH for-next 00/32] spin lock usage optimization > for SCSI drivers > > On Wed, 10 Feb 2021, Song Bao Hua (Barry Song) wrote: > > > > On Wed, 10 Feb 2021, Song Bao Hua (Barry Song) wrote: > > > > > > > > > > > > > There is no warning from m68k builds. That's because > > > > > arch_irqs_disabled() returns true when the IPL is non-zero. > > > > > > > > So for m68k, the case is > > > > arch_irqs_disabled() is true, but interrupts can still come? > > > > > > > > Then it seems it is very confusing. If prioritized interrupts can > > > > still come while arch_irqs_disabled() is true, > > > > > > Yes, on m68k CPUs, an IRQ having a priority level higher than the > > > present priority mask will get serviced. > > > > > > Non-Maskable Interrupt (NMI) is not subject to this rule and gets > > > serviced regardless. > > > > > > > how could spin_lock_irqsave() block the prioritized interrupts? > > > > > > It raises the the mask level to 7. Again, please see > > > arch/m68k/include/asm/irqflags.h > > > > Hi Finn, > > Thanks for your explanation again. > > > > TBH, that is why m68k is so confusing. irqs_disabled() on m68k should > > just reflect the status of all interrupts have been disabled except NMI. > > > > irqs_disabled() should be consistent with the calling of APIs such as > > local_irq_disable, local_irq_save, spin_lock_irqsave etc. > > > > When irqs_disabled() returns true, we cannot infer that > arch_local_irq_disable() was called. But I have not yet found driver code > or core kernel code attempting that inference. > > > > > > > > Isn't arch_irqs_disabled() a status reflection of irq disable API? > > > > > > > > > > Why not? > > > > If so, arch_irqs_disabled() should mean all interrupts have been masked > > except NMI as NMI is unmaskable. > > > > Can you support that claim with a reference to core kernel code or > documentation? (If some arch code agrees with you, that's neither here nor > there.) I think those links I share you have supported this. Just you don't believe :-) > > > > > > > Are all interrupts (including NMI) masked whenever > > > arch_irqs_disabled() returns true on your platforms? > > > > On my platform, once irqs_disabled() is true, all interrupts are masked > > except NMI. NMI just ignore spin_lock_irqsave or local_irq_disable. > > > > On ARM64, we also have high-priority interrupts, but they are running as > > PESUDO_NMI: > > https://lwn.net/Articles/755906/ > > > > A glance at the ARM GIC specification suggests that your hardware works > much like 68000 hardware. > > When enabled, a CPU interface takes the highest priority pending > interrupt for its connected processor and determines whether the > interrupt has sufficient priority for it to signal the interrupt > request to the processor. [...] > > When the processor acknowledges the interrupt at the CPU interface, the > Distributor changes the status of the interrupt from pending to either > active, or active and pending. At this point the CPU interface can > signal another interrupt to the processor, to preempt interrupts that > are active on the processor. If there is no pending interrupt with > sufficient priority for signaling to the processor, the interface > deasserts the interrupt request signal to the processor. > > https://developer.arm.com/documentation/ihi0048/b/ > > Have you considered that Linux/arm might benefit if it could fully exploit > hardware features already available, such as the interrupt priority > masking feature in the GIC in existing arm systems? I guess no:-) there are only two levels: IRQ and NMI. Injecting a high-prio IRQ level between them makes no sense. To me, arm64's design is quite clear and has no any confusion. > > > On m68k, it seems you mean： > > irq_disabled() is true, but high-priority interrupts can still come; > > local_irq_disable() can disable high-priority interrupts, and at that > > time, irq_disabled() is also true. > > > > TBH, this is wrong and confusing on m68k. > > > > Like you, I was surprised when I learned about it. But that doesn't mean > it's wrong. The fact that it works should tell you something. > The fact is that m68k lets arch_irq_disabled() return true to pretend all IRQs are disabled while high-priority IRQ is still open, thus "pass" all sanitizing check in genirq and kernel core. > Things could always be made simpler. But discarding features isn't > necessarily an improvement. This feature could be used by calling local_irq_enable_in_hardirq() in those IRQ handlers who hope high-priority interrupts to preempt it for a while. It shouldn't hide somewhere and make confusion. On the other hand, those who care about realtime should use threaded IRQ and let IRQ threads preempt each other. Thanks Barry
On Wed, 10 Feb 2021, Song Bao Hua (Barry Song) wrote: > > On Wed, 10 Feb 2021, Song Bao Hua (Barry Song) wrote: > > > > > > On Wed, 10 Feb 2021, Song Bao Hua (Barry Song) wrote: > > > > > > > > > > There is no warning from m68k builds. That's because > > > > > > arch_irqs_disabled() returns true when the IPL is non-zero. > > > > > > > > > > So for m68k, the case is arch_irqs_disabled() is true, but > > > > > interrupts can still come? > > > > > > > > > > Then it seems it is very confusing. If prioritized interrupts > > > > > can still come while arch_irqs_disabled() is true, > > > > > > > > Yes, on m68k CPUs, an IRQ having a priority level higher than the > > > > present priority mask will get serviced. > > > > > > > > Non-Maskable Interrupt (NMI) is not subject to this rule and gets > > > > serviced regardless. > > > > > > > > > how could spin_lock_irqsave() block the prioritized interrupts? > > > > > > > > It raises the the mask level to 7. Again, please see > > > > arch/m68k/include/asm/irqflags.h > > > > > > Hi Finn, > > > Thanks for your explanation again. > > > > > > TBH, that is why m68k is so confusing. irqs_disabled() on m68k > > > should just reflect the status of all interrupts have been disabled > > > except NMI. > > > > > > irqs_disabled() should be consistent with the calling of APIs such > > > as local_irq_disable, local_irq_save, spin_lock_irqsave etc. > > > > > > > When irqs_disabled() returns true, we cannot infer that > > arch_local_irq_disable() was called. But I have not yet found driver > > code or core kernel code attempting that inference. > > > > > > > > > > > Isn't arch_irqs_disabled() a status reflection of irq disable > > > > > API? > > > > > > > > > > > > > Why not? > > > > > > If so, arch_irqs_disabled() should mean all interrupts have been > > > masked except NMI as NMI is unmaskable. > > > > > > > Can you support that claim with a reference to core kernel code or > > documentation? (If some arch code agrees with you, that's neither here > > nor there.) > > I think those links I share you have supported this. Just you don't > believe :-) > Your links show that the distinction between fast and slow handlers was removed. Your links don't support your claim that "arch_irqs_disabled() should mean all interrupts have been masked". Where is the code that makes that inference? Where is the documentation that supports your claim? > > > > > > > > > > Are all interrupts (including NMI) masked whenever > > > > arch_irqs_disabled() returns true on your platforms? > > > > > > On my platform, once irqs_disabled() is true, all interrupts are > > > masked except NMI. NMI just ignore spin_lock_irqsave or > > > local_irq_disable. > > > > > > On ARM64, we also have high-priority interrupts, but they are > > > running as PESUDO_NMI: > > > https://lwn.net/Articles/755906/ > > > > > > > A glance at the ARM GIC specification suggests that your hardware > > works much like 68000 hardware. > > > > When enabled, a CPU interface takes the highest priority pending > > interrupt for its connected processor and determines whether the > > interrupt has sufficient priority for it to signal the interrupt > > request to the processor. [...] > > > > When the processor acknowledges the interrupt at the CPU interface, > > the Distributor changes the status of the interrupt from pending to > > either active, or active and pending. At this point the CPU > > interface can signal another interrupt to the processor, to preempt > > interrupts that are active on the processor. If there is no pending > > interrupt with sufficient priority for signaling to the processor, > > the interface deasserts the interrupt request signal to the > > processor. > > > > https://developer.arm.com/documentation/ihi0048/b/ > > > > Have you considered that Linux/arm might benefit if it could fully > > exploit hardware features already available, such as the interrupt > > priority masking feature in the GIC in existing arm systems? > > I guess no:-) there are only two levels: IRQ and NMI. Injecting a > high-prio IRQ level between them makes no sense. > > To me, arm64's design is quite clear and has no any confusion. > Are you saying that the ARM64 hardware design is confusing because it implements a priority mask, and that's why you had to simplify it with a pseudo-nmi scheme in software? > > > > > On m68k, it seems you mean： > > > irq_disabled() is true, but high-priority interrupts can still come; > > > local_irq_disable() can disable high-priority interrupts, and at that > > > time, irq_disabled() is also true. > > > > > > TBH, this is wrong and confusing on m68k. > > > > > > > Like you, I was surprised when I learned about it. But that doesn't mean > > it's wrong. The fact that it works should tell you something. > > > > The fact is that m68k lets arch_irq_disabled() return true to pretend > all IRQs are disabled while high-priority IRQ is still open, thus "pass" > all sanitizing check in genirq and kernel core. > The fact is that m68k has arch_irq_disabled() return false when all IRQs are enabled. So there is no bug. > > Things could always be made simpler. But discarding features isn't > > necessarily an improvement. > > This feature could be used by calling local_irq_enable_in_hardirq() in > those IRQ handlers who hope high-priority interrupts to preempt it for a > while. > So, if one handler is sensitive to interrupt latency, all other handlers should be modified? I don't think that's workable. In anycase, what you're describing is a completely different nested interrupt scheme that would defeat the priority level mechanism that the hardware provides us with. > It shouldn't hide somewhere and make confusion. > The problem is hiding so well that no-one has found it! I say it doesn't exist. > On the other hand, those who care about realtime should use threaded IRQ > and let IRQ threads preempt each other. > Yes. And those threads also have priority levels. > Thanks > Barry > >
> -----Original Message----- > From: Finn Thain [mailto:fthain@telegraphics.com.au] > Sent: Thursday, February 11, 2021 2:12 PM > To: Song Bao Hua (Barry Song) <song.bao.hua@hisilicon.com> > Cc: tanxiaofei <tanxiaofei@huawei.com>; jejb@linux.ibm.com; > martin.petersen@oracle.com; linux-scsi@vger.kernel.org; > linux-kernel@vger.kernel.org; linuxarm@openeuler.org; > linux-m68k@vger.kernel.org > Subject: RE: [Linuxarm] Re: [PATCH for-next 00/32] spin lock usage optimization > for SCSI drivers > > On Wed, 10 Feb 2021, Song Bao Hua (Barry Song) wrote: > > > > On Wed, 10 Feb 2021, Song Bao Hua (Barry Song) wrote: > > > > > > > > On Wed, 10 Feb 2021, Song Bao Hua (Barry Song) wrote: > > > > > > > > > > > > There is no warning from m68k builds. That's because > > > > > > > arch_irqs_disabled() returns true when the IPL is non-zero. > > > > > > > > > > > > So for m68k, the case is arch_irqs_disabled() is true, but > > > > > > interrupts can still come? > > > > > > > > > > > > Then it seems it is very confusing. If prioritized interrupts > > > > > > can still come while arch_irqs_disabled() is true, > > > > > > > > > > Yes, on m68k CPUs, an IRQ having a priority level higher than the > > > > > present priority mask will get serviced. > > > > > > > > > > Non-Maskable Interrupt (NMI) is not subject to this rule and gets > > > > > serviced regardless. > > > > > > > > > > > how could spin_lock_irqsave() block the prioritized interrupts? > > > > > > > > > > It raises the the mask level to 7. Again, please see > > > > > arch/m68k/include/asm/irqflags.h > > > > > > > > Hi Finn, > > > > Thanks for your explanation again. > > > > > > > > TBH, that is why m68k is so confusing. irqs_disabled() on m68k > > > > should just reflect the status of all interrupts have been disabled > > > > except NMI. > > > > > > > > irqs_disabled() should be consistent with the calling of APIs such > > > > as local_irq_disable, local_irq_save, spin_lock_irqsave etc. > > > > > > > > > > When irqs_disabled() returns true, we cannot infer that > > > arch_local_irq_disable() was called. But I have not yet found driver > > > code or core kernel code attempting that inference. > > > > > > > > > > > > > > Isn't arch_irqs_disabled() a status reflection of irq disable > > > > > > API? > > > > > > > > > > > > > > > > Why not? > > > > > > > > If so, arch_irqs_disabled() should mean all interrupts have been > > > > masked except NMI as NMI is unmaskable. > > > > > > > > > > Can you support that claim with a reference to core kernel code or > > > documentation? (If some arch code agrees with you, that's neither here > > > nor there.) > > > > I think those links I share you have supported this. Just you don't > > believe :-) > > > > Your links show that the distinction between fast and slow handlers was > removed. Your links don't support your claim that "arch_irqs_disabled() > should mean all interrupts have been masked". Where is the code that makes > that inference? Where is the documentation that supports your claim? (1) https://lwn.net/Articles/380931/ Looking at all these worries, one might well wonder if a system which *disabled interrupts for all handlers* would function well at all. So it is interesting to note one thing: any system which has the lockdep locking checker enabled has been running all handlers that way for some years now. Many developers and testers run lockdep-enabled kernels, and they are available for some of the more adventurous distributions (Rawhide, for example) as well. So we have quite a bit of test coverage for this mode of operation already. (2) https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=b738a50a "We run all handlers *with interrupts disabled* and expect them not to enable them. Warn when we catch one who does." (3) https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=e58aa3d2d0cc genirq: Run irq handlers *with interrupts disabled* Running interrupt handlers with interrupts enabled can cause stack overflows. That has been observed with multiqueue NICs delivering all their interrupts to a single core. We might band aid that somehow by checking the interrupt stacks, but the real safe fix is to *run the irq handlers with interrupts disabled*. All these documents say we are running irq handler with interrupts disabled. but it seems you think high-prio interrupts don't belong to "interrupts" in those documents :-) that is why we can't get agreement. I think "interrupts" mean all except NMI in these documents, but you insist high-prio IRQ is an exception. > > > > > > > > > > > > > > Are all interrupts (including NMI) masked whenever > > > > > arch_irqs_disabled() returns true on your platforms? > > > > > > > > On my platform, once irqs_disabled() is true, all interrupts are > > > > masked except NMI. NMI just ignore spin_lock_irqsave or > > > > local_irq_disable. > > > > > > > > On ARM64, we also have high-priority interrupts, but they are > > > > running as PESUDO_NMI: > > > > https://lwn.net/Articles/755906/ > > > > > > > > > > A glance at the ARM GIC specification suggests that your hardware > > > works much like 68000 hardware. > > > > > > When enabled, a CPU interface takes the highest priority pending > > > interrupt for its connected processor and determines whether the > > > interrupt has sufficient priority for it to signal the interrupt > > > request to the processor. [...] > > > > > > When the processor acknowledges the interrupt at the CPU interface, > > > the Distributor changes the status of the interrupt from pending to > > > either active, or active and pending. At this point the CPU > > > interface can signal another interrupt to the processor, to preempt > > > interrupts that are active on the processor. If there is no pending > > > interrupt with sufficient priority for signaling to the processor, > > > the interface deasserts the interrupt request signal to the > > > processor. > > > > > > https://developer.arm.com/documentation/ihi0048/b/ > > > > > > Have you considered that Linux/arm might benefit if it could fully > > > exploit hardware features already available, such as the interrupt > > > priority masking feature in the GIC in existing arm systems? > > > > I guess no:-) there are only two levels: IRQ and NMI. Injecting a > > high-prio IRQ level between them makes no sense. > > > > To me, arm64's design is quite clear and has no any confusion. > > > > Are you saying that the ARM64 hardware design is confusing because it > implements a priority mask, and that's why you had to simplify it with a > pseudo-nmi scheme in software? No, I was not saying this. I think both m68k and arm64 have good hardware design. Just Linux's implementation is running irq-handlers with interrupts disabled. So ARM64's pseudo-nmi is adapted to Linux better. > > > > > > > > On m68k, it seems you mean： > > > > irq_disabled() is true, but high-priority interrupts can still come; > > > > local_irq_disable() can disable high-priority interrupts, and at that > > > > time, irq_disabled() is also true. > > > > > > > > TBH, this is wrong and confusing on m68k. > > > > > > > > > > Like you, I was surprised when I learned about it. But that doesn't mean > > > it's wrong. The fact that it works should tell you something. > > > > > > > The fact is that m68k lets arch_irq_disabled() return true to pretend > > all IRQs are disabled while high-priority IRQ is still open, thus "pass" > > all sanitizing check in genirq and kernel core. > > > > The fact is that m68k has arch_irq_disabled() return false when all IRQs > are enabled. So there is no bug. But it has arch_irq_disabled() return true while some interrupts(not NMI) are still open. > > > > Things could always be made simpler. But discarding features isn't > > > necessarily an improvement. > > > > This feature could be used by calling local_irq_enable_in_hardirq() in > > those IRQ handlers who hope high-priority interrupts to preempt it for a > > while. > > > > So, if one handler is sensitive to interrupt latency, all other handlers > should be modified? I don't think that's workable. I think we just enable preempt_rt or force threaded_irq, and then improve the priority of the irq thread who is sensitive to latency. No need to touch all threads. I also understand your point, we let one high-prio interrupt preempt low priority interrupt, then we don't need to change the whole system. But I think Linux prefers the method of threaded_irq or preempt_rt for this kind of problems. > > In anycase, what you're describing is a completely different nested > interrupt scheme that would defeat the priority level mechanism that the > hardware provides us with. Yes. Indeed. > > > It shouldn't hide somewhere and make confusion. > > > > The problem is hiding so well that no-one has found it! I say it doesn't > exist. Long long ago(before 2.6.38), we had a kernel supporting IRQF_DISABLED and nested interrupts were widely supported, but system also ran well in most cases. That means nested interrupts don't really matter in most cases. That is why m68k is also running well even though it is still nesting. > > > On the other hand, those who care about realtime should use threaded IRQ > > and let IRQ threads preempt each other. > > > > Yes. And those threads also have priority levels. Finn, I am not a m68k guy, would you help check if this could activate a warning on m68k. maybe we can discuss this question in genirq maillist from this warning if you are happy. Thanks very much. diff --git a/include/linux/hardirq.h b/include/linux/hardirq.h index 7c9d6a2d7e90..b8ca27555c76 100644 --- a/include/linux/hardirq.h +++ b/include/linux/hardirq.h @@ -32,6 +32,7 @@ static __always_inline void rcu_irq_enter_check_tick(void) */ #define __irq_enter() \ do { \ + WARN_ONCE(in_hardirq() && irqs_disabled(), "nested interrupts\n"); \ preempt_count_add(HARDIRQ_OFFSET); \ lockdep_hardirq_enter(); \ account_hardirq_enter(current); \ @@ -44,6 +45,7 @@ static __always_inline void rcu_irq_enter_check_tick(void) */ #define __irq_enter_raw() \ do { \ + WARN_ONCE(in_hardirq() && irqs_disabled(), "nested interrupts\n"); \ preempt_count_add(HARDIRQ_OFFSET); \ lockdep_hardirq_enter(); \ } while (0) Best Regards Barry
On Thu, 11 Feb 2021, Song Bao Hua (Barry Song) wrote: > > On Wed, 10 Feb 2021, Song Bao Hua (Barry Song) wrote: > > > > > > On Wed, 10 Feb 2021, Song Bao Hua (Barry Song) wrote: > > > > > > > > > TBH, that is why m68k is so confusing. irqs_disabled() on m68k > > > > > should just reflect the status of all interrupts have been > > > > > disabled except NMI. > > > > > > > > > > irqs_disabled() should be consistent with the calling of APIs > > > > > such as local_irq_disable, local_irq_save, spin_lock_irqsave > > > > > etc. > > > > > > > > > > > > > When irqs_disabled() returns true, we cannot infer that > > > > arch_local_irq_disable() was called. But I have not yet found > > > > driver code or core kernel code attempting that inference. > > > > > > > > > > > > > > > > > Isn't arch_irqs_disabled() a status reflection of irq > > > > > > > disable API? > > > > > > > > > > > > > > > > > > > Why not? > > > > > > > > > > If so, arch_irqs_disabled() should mean all interrupts have been > > > > > masked except NMI as NMI is unmaskable. > > > > > > > > > > > > > Can you support that claim with a reference to core kernel code or > > > > documentation? (If some arch code agrees with you, that's neither > > > > here nor there.) > > > > > > I think those links I share you have supported this. Just you don't > > > believe :-) > > > > > > > Your links show that the distinction between fast and slow handlers > > was removed. Your links don't support your claim that > > "arch_irqs_disabled() should mean all interrupts have been masked". > > Where is the code that makes that inference? Where is the > > documentation that supports your claim? > > (1) > https://lwn.net/Articles/380931/ > Looking at all these worries, one might well wonder if a system which > *disabled interrupts for all handlers* would function well at all. So it > is interesting to note one thing: any system which has the lockdep > locking checker enabled has been running all handlers that way for some > years now. Many developers and testers run lockdep-enabled kernels, and > they are available for some of the more adventurous distributions > (Rawhide, for example) as well. So we have quite a bit of test coverage > for this mode of operation already. > IIUC, your claim is that CONFIG_LOCKDEP involves code that contains the inference, "arch_irqs_disabled() means all interrupts have been masked". Unfortunately, m68k lacks CONFIG_LOCKDEP support so I can't easily confirm this. I suppose there may be other architectures that support both LOCKDEP and nested interrupts (?) Anyway, if you would point to the code that contains said inference, that would help a lot. > (2) > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=b738a50a > > "We run all handlers *with interrupts disabled* and expect them not to > enable them. Warn when we catch one who does." > Again, you don't see that warning because irqs_disabled() correctly returns true. You can confirm this fact in QEMU or Aranym if you are interested. > (3) > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=e58aa3d2d0cc > genirq: Run irq handlers *with interrupts disabled* > > Running interrupt handlers with interrupts enabled can cause stack > overflows. That has been observed with multiqueue NICs delivering all > their interrupts to a single core. We might band aid that somehow by > checking the interrupt stacks, but the real safe fix is to *run the irq > handlers with interrupts disabled*. > Again, the stack overflow issue is not applicable. 68000 uses a priority mask, like ARM GIC. So there's no arbitrary nesting of interrupt handlers. In practice stack overflows simply don't occur on m68k. Please do try it. > > All these documents say we are running irq handler with interrupts > disabled. but it seems you think high-prio interrupts don't belong > to "interrupts" in those documents :-) > > that is why we can't get agreement. I think "interrupts" mean all except > NMI in these documents, but you insist high-prio IRQ is an exception. > We can't get agreement because you seek to remove functionality without justification. > > > > > > > > > > > > > > > > Are all interrupts (including NMI) masked whenever > > > > > > arch_irqs_disabled() returns true on your platforms? > > > > > > > > > > On my platform, once irqs_disabled() is true, all interrupts are > > > > > masked except NMI. NMI just ignore spin_lock_irqsave or > > > > > local_irq_disable. > > > > > > > > > > On ARM64, we also have high-priority interrupts, but they are > > > > > running as PESUDO_NMI: > > > > > https://lwn.net/Articles/755906/ > > > > > > > > > > > > > A glance at the ARM GIC specification suggests that your hardware > > > > works much like 68000 hardware. > > > > > > > > When enabled, a CPU interface takes the highest priority > > > > pending interrupt for its connected processor and determines > > > > whether the interrupt has sufficient priority for it to signal > > > > the interrupt request to the processor. [...] > > > > > > > > When the processor acknowledges the interrupt at the CPU > > > > interface, the Distributor changes the status of the interrupt > > > > from pending to either active, or active and pending. At this > > > > point the CPU interface can signal another interrupt to the > > > > processor, to preempt interrupts that are active on the > > > > processor. If there is no pending interrupt with sufficient > > > > priority for signaling to the processor, the interface > > > > deasserts the interrupt request signal to the processor. > > > > > > > > https://developer.arm.com/documentation/ihi0048/b/ > > > > > > > > Have you considered that Linux/arm might benefit if it could fully > > > > exploit hardware features already available, such as the interrupt > > > > priority masking feature in the GIC in existing arm systems? > > > > > > I guess no:-) there are only two levels: IRQ and NMI. Injecting a > > > high-prio IRQ level between them makes no sense. > > > > > > To me, arm64's design is quite clear and has no any confusion. > > > > > > > Are you saying that the ARM64 hardware design is confusing because it > > implements a priority mask, and that's why you had to simplify it with > > a pseudo-nmi scheme in software? > > No, I was not saying this. I think both m68k and arm64 have good > hardware design. Just Linux's implementation is running irq-handlers > with interrupts disabled. So ARM64's pseudo-nmi is adapted to Linux > better. > So, a platform should do what all the other platforms do because to deviate would be too dangerous? > > > > > > > > > On m68k, it seems you mean： > > > > > irq_disabled() is true, but high-priority interrupts can still > > > > > come; local_irq_disable() can disable high-priority interrupts, > > > > > and at that time, irq_disabled() is also true. > > > > > > > > > > TBH, this is wrong and confusing on m68k. > > > > > > > > > > > > > Like you, I was surprised when I learned about it. But that > > > > doesn't mean it's wrong. The fact that it works should tell you > > > > something. > > > > > > > > > > The fact is that m68k lets arch_irq_disabled() return true to > > > pretend all IRQs are disabled while high-priority IRQ is still open, > > > thus "pass" all sanitizing check in genirq and kernel core. > > > > > > > The fact is that m68k has arch_irq_disabled() return false when all > > IRQs are enabled. So there is no bug. > > But it has arch_irq_disabled() return true while some interrupts(not > NMI) are still open. > > > > > > > Things could always be made simpler. But discarding features isn't > > > > necessarily an improvement. > > > > > > This feature could be used by calling local_irq_enable_in_hardirq() > > > in those IRQ handlers who hope high-priority interrupts to preempt > > > it for a while. > > > > > > > So, if one handler is sensitive to interrupt latency, all other > > handlers should be modified? I don't think that's workable. > > I think we just enable preempt_rt or force threaded_irq, and then > improve the priority of the irq thread who is sensitive to latency. No > need to touch all threads. > > I also understand your point, we let one high-prio interrupt preempt low > priority interrupt, then we don't need to change the whole system. But I > think Linux prefers the method of threaded_irq or preempt_rt for this > kind of problems. > So, some interrupt (or exception) processing happens atomically and the rest is deferred to a different execution context. (Not a new idea.) If you introduce latency in the former context you can't win it back in the latter. Your solution fails because it adds latency to high priority handlers. > > > > In anycase, what you're describing is a completely different nested > > interrupt scheme that would defeat the priority level mechanism that > > the hardware provides us with. > > Yes. Indeed. > > > > > > It shouldn't hide somewhere and make confusion. > > > > > > > The problem is hiding so well that no-one has found it! I say it > > doesn't exist. > > Long long ago(before 2.6.38), we had a kernel supporting IRQF_DISABLED > and nested interrupts were widely supported, but system also ran well in > most cases. That means nested interrupts don't really matter in most > cases. That is why m68k is also running well even though it is still > nesting. > No, m68k runs well because it uses priority masking. It is not because some cases are untested. Your hardware may not have been around for 4 decades but it implements the same capability because the design is known to work. > > > > > On the other hand, those who care about realtime should use threaded > > > IRQ and let IRQ threads preempt each other. > > > > > > > Yes. And those threads also have priority levels. > > Finn, I am not a m68k guy, would you help check if this could activate a > warning on m68k. maybe we can discuss this question in genirq maillist from > this warning if you are happy. Thanks very much. > > diff --git a/include/linux/hardirq.h b/include/linux/hardirq.h > index 7c9d6a2d7e90..b8ca27555c76 100644 > --- a/include/linux/hardirq.h > +++ b/include/linux/hardirq.h > @@ -32,6 +32,7 @@ static __always_inline void rcu_irq_enter_check_tick(void) > */ > #define __irq_enter() \ > do { \ > + WARN_ONCE(in_hardirq() && irqs_disabled(), "nested interrupts\n"); \ > preempt_count_add(HARDIRQ_OFFSET); \ > lockdep_hardirq_enter(); \ > account_hardirq_enter(current); \ > @@ -44,6 +45,7 @@ static __always_inline void rcu_irq_enter_check_tick(void) > */ > #define __irq_enter_raw() \ > do { \ > + WARN_ONCE(in_hardirq() && irqs_disabled(), "nested interrupts\n"); \ > preempt_count_add(HARDIRQ_OFFSET); \ > lockdep_hardirq_enter(); \ > } while (0) > If you think that lockdep or some other code somewhere should be protected in this way, perhaps you can point to that code. Otherwise, your patch seems to lack any justification. > Best Regards > Barry > >
> -----Original Message----- > From: Finn Thain [mailto:fthain@telegraphics.com.au] > Sent: Friday, February 12, 2021 12:58 PM > To: Song Bao Hua (Barry Song) <song.bao.hua@hisilicon.com> > Cc: tanxiaofei <tanxiaofei@huawei.com>; jejb@linux.ibm.com; > martin.petersen@oracle.com; linux-scsi@vger.kernel.org; > linux-kernel@vger.kernel.org; linuxarm@openeuler.org; > linux-m68k@vger.kernel.org > Subject: RE: [Linuxarm] Re: [PATCH for-next 00/32] spin lock usage optimization > for SCSI drivers > > On Thu, 11 Feb 2021, Song Bao Hua (Barry Song) wrote: > > > > On Wed, 10 Feb 2021, Song Bao Hua (Barry Song) wrote: > > > > > > > > On Wed, 10 Feb 2021, Song Bao Hua (Barry Song) wrote: > > > > > > > > > > > TBH, that is why m68k is so confusing. irqs_disabled() on m68k > > > > > > should just reflect the status of all interrupts have been > > > > > > disabled except NMI. > > > > > > > > > > > > irqs_disabled() should be consistent with the calling of APIs > > > > > > such as local_irq_disable, local_irq_save, spin_lock_irqsave > > > > > > etc. > > > > > > > > > > > > > > > > When irqs_disabled() returns true, we cannot infer that > > > > > arch_local_irq_disable() was called. But I have not yet found > > > > > driver code or core kernel code attempting that inference. > > > > > > > > > > > > > > > > > > > > Isn't arch_irqs_disabled() a status reflection of irq > > > > > > > > disable API? > > > > > > > > > > > > > > > > > > > > > > Why not? > > > > > > > > > > > > If so, arch_irqs_disabled() should mean all interrupts have been > > > > > > masked except NMI as NMI is unmaskable. > > > > > > > > > > > > > > > > Can you support that claim with a reference to core kernel code or > > > > > documentation? (If some arch code agrees with you, that's neither > > > > > here nor there.) > > > > > > > > I think those links I share you have supported this. Just you don't > > > > believe :-) > > > > > > > > > > Your links show that the distinction between fast and slow handlers > > > was removed. Your links don't support your claim that > > > "arch_irqs_disabled() should mean all interrupts have been masked". > > > Where is the code that makes that inference? Where is the > > > documentation that supports your claim? > > > > (1) > > https://lwn.net/Articles/380931/ > > Looking at all these worries, one might well wonder if a system which > > *disabled interrupts for all handlers* would function well at all. So it > > is interesting to note one thing: any system which has the lockdep > > locking checker enabled has been running all handlers that way for some > > years now. Many developers and testers run lockdep-enabled kernels, and > > they are available for some of the more adventurous distributions > > (Rawhide, for example) as well. So we have quite a bit of test coverage > > for this mode of operation already. > > > > IIUC, your claim is that CONFIG_LOCKDEP involves code that contains the > inference, "arch_irqs_disabled() means all interrupts have been masked". > > Unfortunately, m68k lacks CONFIG_LOCKDEP support so I can't easily confirm > this. I suppose there may be other architectures that support both LOCKDEP > and nested interrupts (?) > > Anyway, if you would point to the code that contains said inference, that > would help a lot. > > > (2) > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/ > ?id=b738a50a > > > > "We run all handlers *with interrupts disabled* and expect them not to > > enable them. Warn when we catch one who does." > > > > Again, you don't see that warning because irqs_disabled() correctly > returns true. You can confirm this fact in QEMU or Aranym if you are > interested. > > > (3) > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/ > ?id=e58aa3d2d0cc > > genirq: Run irq handlers *with interrupts disabled* > > > > Running interrupt handlers with interrupts enabled can cause stack > > overflows. That has been observed with multiqueue NICs delivering all > > their interrupts to a single core. We might band aid that somehow by > > checking the interrupt stacks, but the real safe fix is to *run the irq > > handlers with interrupts disabled*. > > > > Again, the stack overflow issue is not applicable. 68000 uses a priority > mask, like ARM GIC. So there's no arbitrary nesting of interrupt handlers. > > In practice stack overflows simply don't occur on m68k. Please do try it. > > > > > All these documents say we are running irq handler with interrupts > > disabled. but it seems you think high-prio interrupts don't belong > > to "interrupts" in those documents :-) > > > > that is why we can't get agreement. I think "interrupts" mean all except > > NMI in these documents, but you insist high-prio IRQ is an exception. > > > > We can't get agreement because you seek to remove functionality without > justification. > > > > > > > > > > > > > > > > > > > > Are all interrupts (including NMI) masked whenever > > > > > > > arch_irqs_disabled() returns true on your platforms? > > > > > > > > > > > > On my platform, once irqs_disabled() is true, all interrupts are > > > > > > masked except NMI. NMI just ignore spin_lock_irqsave or > > > > > > local_irq_disable. > > > > > > > > > > > > On ARM64, we also have high-priority interrupts, but they are > > > > > > running as PESUDO_NMI: > > > > > > https://lwn.net/Articles/755906/ > > > > > > > > > > > > > > > > A glance at the ARM GIC specification suggests that your hardware > > > > > works much like 68000 hardware. > > > > > > > > > > When enabled, a CPU interface takes the highest priority > > > > > pending interrupt for its connected processor and determines > > > > > whether the interrupt has sufficient priority for it to signal > > > > > the interrupt request to the processor. [...] > > > > > > > > > > When the processor acknowledges the interrupt at the CPU > > > > > interface, the Distributor changes the status of the interrupt > > > > > from pending to either active, or active and pending. At this > > > > > point the CPU interface can signal another interrupt to the > > > > > processor, to preempt interrupts that are active on the > > > > > processor. If there is no pending interrupt with sufficient > > > > > priority for signaling to the processor, the interface > > > > > deasserts the interrupt request signal to the processor. > > > > > > > > > > https://developer.arm.com/documentation/ihi0048/b/ > > > > > > > > > > Have you considered that Linux/arm might benefit if it could fully > > > > > exploit hardware features already available, such as the interrupt > > > > > priority masking feature in the GIC in existing arm systems? > > > > > > > > I guess no:-) there are only two levels: IRQ and NMI. Injecting a > > > > high-prio IRQ level between them makes no sense. > > > > > > > > To me, arm64's design is quite clear and has no any confusion. > > > > > > > > > > Are you saying that the ARM64 hardware design is confusing because it > > > implements a priority mask, and that's why you had to simplify it with > > > a pseudo-nmi scheme in software? > > > > No, I was not saying this. I think both m68k and arm64 have good > > hardware design. Just Linux's implementation is running irq-handlers > > with interrupts disabled. So ARM64's pseudo-nmi is adapted to Linux > > better. > > > > So, a platform should do what all the other platforms do because to > deviate would be too dangerous? > > > > > > > > > > > > On m68k, it seems you mean： > > > > > > irq_disabled() is true, but high-priority interrupts can still > > > > > > come; local_irq_disable() can disable high-priority interrupts, > > > > > > and at that time, irq_disabled() is also true. > > > > > > > > > > > > TBH, this is wrong and confusing on m68k. > > > > > > > > > > > > > > > > Like you, I was surprised when I learned about it. But that > > > > > doesn't mean it's wrong. The fact that it works should tell you > > > > > something. > > > > > > > > > > > > > The fact is that m68k lets arch_irq_disabled() return true to > > > > pretend all IRQs are disabled while high-priority IRQ is still open, > > > > thus "pass" all sanitizing check in genirq and kernel core. > > > > > > > > > > The fact is that m68k has arch_irq_disabled() return false when all > > > IRQs are enabled. So there is no bug. > > > > But it has arch_irq_disabled() return true while some interrupts(not > > NMI) are still open. > > > > > > > > > > Things could always be made simpler. But discarding features isn't > > > > > necessarily an improvement. > > > > > > > > This feature could be used by calling local_irq_enable_in_hardirq() > > > > in those IRQ handlers who hope high-priority interrupts to preempt > > > > it for a while. > > > > > > > > > > So, if one handler is sensitive to interrupt latency, all other > > > handlers should be modified? I don't think that's workable. > > > > I think we just enable preempt_rt or force threaded_irq, and then > > improve the priority of the irq thread who is sensitive to latency. No > > need to touch all threads. > > > > I also understand your point, we let one high-prio interrupt preempt low > > priority interrupt, then we don't need to change the whole system. But I > > think Linux prefers the method of threaded_irq or preempt_rt for this > > kind of problems. > > > > So, some interrupt (or exception) processing happens atomically and the > rest is deferred to a different execution context. (Not a new idea.) > > If you introduce latency in the former context you can't win it back in > the latter. Your solution fails because it adds latency to high priority > handlers. > > > > > > > In anycase, what you're describing is a completely different nested > > > interrupt scheme that would defeat the priority level mechanism that > > > the hardware provides us with. > > > > Yes. Indeed. > > > > > > > > > It shouldn't hide somewhere and make confusion. > > > > > > > > > > The problem is hiding so well that no-one has found it! I say it > > > doesn't exist. > > > > Long long ago(before 2.6.38), we had a kernel supporting IRQF_DISABLED > > and nested interrupts were widely supported, but system also ran well in > > most cases. That means nested interrupts don't really matter in most > > cases. That is why m68k is also running well even though it is still > > nesting. > > > > No, m68k runs well because it uses priority masking. It is not because > some cases are untested. > > Your hardware may not have been around for 4 decades but it implements the > same capability because the design is known to work. > > > > > > > > On the other hand, those who care about realtime should use threaded > > > > IRQ and let IRQ threads preempt each other. > > > > > > > > > > Yes. And those threads also have priority levels. > > > > Finn, I am not a m68k guy, would you help check if this could activate a > > warning on m68k. maybe we can discuss this question in genirq maillist from > > this warning if you are happy. Thanks very much. > > > > diff --git a/include/linux/hardirq.h b/include/linux/hardirq.h > > index 7c9d6a2d7e90..b8ca27555c76 100644 > > --- a/include/linux/hardirq.h > > +++ b/include/linux/hardirq.h > > @@ -32,6 +32,7 @@ static __always_inline void rcu_irq_enter_check_tick(void) > > */ > > #define __irq_enter() \ > > do { \ > > + WARN_ONCE(in_hardirq() && irqs_disabled(), "nested > interrupts\n"); \ > > preempt_count_add(HARDIRQ_OFFSET); \ > > lockdep_hardirq_enter(); \ > > account_hardirq_enter(current); \ > > @@ -44,6 +45,7 @@ static __always_inline void rcu_irq_enter_check_tick(void) > > */ > > #define __irq_enter_raw() \ > > do { \ > > + WARN_ONCE(in_hardirq() && irqs_disabled(), "nested > interrupts\n"); \ > > preempt_count_add(HARDIRQ_OFFSET); \ > > lockdep_hardirq_enter(); \ > > } while (0) > > > > If you think that lockdep or some other code somewhere should be protected > in this way, perhaps you can point to that code. Otherwise, your patch > seems to lack any justification. Ok. I am not say this patch is right. I was actually expecting a log to start the discussion in genirq. Anyway, I think this is a very important problem we need to clarify in genirq. If irq handlers are able to run with some high-prio interrupts enabled on some platform like m68k, we need somewhere to document this is a case somewhere. Anyway, I think it is important to clarify this thoroughly in genirq by starting a discussion with related guys. > > > Best Regards > > Barry > > > > Thanks Barry
Hi Finn, On 2021/2/9 13:06, Finn Thain wrote: > On Tue, 9 Feb 2021, Song Bao Hua (Barry Song) wrote: > >>> -----Original Message----- >>> From: Finn Thain [mailto:fthain@telegraphics.com.au] >>> Sent: Monday, February 8, 2021 8:57 PM >>> To: tanxiaofei <tanxiaofei@huawei.com> >>> Cc: jejb@linux.ibm.com; martin.petersen@oracle.com; >>> linux-scsi@vger.kernel.org; linux-kernel@vger.kernel.org; >>> linuxarm@openeuler.org >>> Subject: [Linuxarm] Re: [PATCH for-next 00/32] spin lock usage optimization >>> for SCSI drivers >>> >>> On Sun, 7 Feb 2021, Xiaofei Tan wrote: >>> >>>> Replace spin_lock_irqsave with spin_lock in hard IRQ of SCSI drivers. >>>> There are no function changes, but may speed up if interrupt happen too >>>> often. >>> >>> This change doesn't necessarily work on platforms that support nested >>> interrupts. >>> >>> Were you able to measure any benefit from this change on some other >>> platform? >> >> I think the code disabling irq in hardIRQ is simply wrong. >> Since this commit >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=e58aa3d2d0cc >> genirq: Run irq handlers with interrupts disabled >> >> interrupt handlers are definitely running in a irq-disabled context >> unless irq handlers enable them explicitly in the handler to permit >> other interrupts. >> > > Repeating the same claim does not somehow make it true. If you put your > claim to the test, you'll see that that interrupts are not disabled on > m68k when interrupt handlers execute. > > The Interrupt Priority Level (IPL) can prevent any given irq handler from > being re-entered, but an irq with a higher priority level may be handled > during execution of a lower priority irq handler. > > sonic_interrupt() uses an irq lock within an interrupt handler to avoid > issues relating to this. This kind of locking may be needed in the drivers > you are trying to patch. Or it might not. Apparently, no-one has looked. > According to your discussion with Barry, it seems that m68k is a little different from other architecture, and this kind of modification of this patch cannot be applied to m68k. So, could help to point out which driver belong to m68k architecture in this patch set of SCSI? I can remove them. BTW, sonic_interrupt() is from net driver natsemi, right? It would be appreciative if only discuss SCSI drivers in this patch set. thanks. > . >
On Thu, 18 Feb 2021, Xiaofei Tan wrote: > On 2021/2/9 13:06, Finn Thain wrote: > > On Tue, 9 Feb 2021, Song Bao Hua (Barry Song) wrote: > > > > > > On Sun, 7 Feb 2021, Xiaofei Tan wrote: > > > > > > > > > Replace spin_lock_irqsave with spin_lock in hard IRQ of SCSI > > > > > drivers. There are no function changes, but may speed up if > > > > > interrupt happen too often. > > > > > > > > This change doesn't necessarily work on platforms that support > > > > nested interrupts. > > > > > > > > Were you able to measure any benefit from this change on some > > > > other platform? > > > > > > I think the code disabling irq in hardIRQ is simply wrong. > > > Since this commit > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=e58aa3d2d0cc > > > genirq: Run irq handlers with interrupts disabled > > > > > > interrupt handlers are definitely running in a irq-disabled context > > > unless irq handlers enable them explicitly in the handler to permit > > > other interrupts. > > > > > > > Repeating the same claim does not somehow make it true. If you put > > your claim to the test, you'll see that that interrupts are not > > disabled on m68k when interrupt handlers execute. > > > > The Interrupt Priority Level (IPL) can prevent any given irq handler > > from being re-entered, but an irq with a higher priority level may be > > handled during execution of a lower priority irq handler. > > > > sonic_interrupt() uses an irq lock within an interrupt handler to > > avoid issues relating to this. This kind of locking may be needed in > > the drivers you are trying to patch. Or it might not. Apparently, > > no-one has looked. > > > > According to your discussion with Barry, it seems that m68k is a little > different from other architecture, and this kind of modification of this > patch cannot be applied to m68k. So, could help to point out which > driver belong to m68k architecture in this patch set of SCSI? I can > remove them. > If you would claim that "there are no function changes" in your patches (as above) then the onus is on you to support that claim. I assume that there are some platforms on which your assumptions hold. With regard to drivers for those platforms, you might want to explain why your patches should be applied there, given that the existing code is superior for being more portable. > BTW, sonic_interrupt() is from net driver natsemi, right? It would be > appreciative if only discuss SCSI drivers in this patch set. thanks. > The 'net' subsystem does have some different requirements than the 'scsi' subsystem. But I don't see how that's relevant. Perhaps you can explain it. Thanks.
> -----Original Message----- > From: Finn Thain [mailto:fthain@telegraphics.com.au] > Sent: Saturday, February 20, 2021 6:18 PM > To: tanxiaofei <tanxiaofei@huawei.com> > Cc: Song Bao Hua (Barry Song) <song.bao.hua@hisilicon.com>; jejb@linux.ibm.com; > martin.petersen@oracle.com; linux-scsi@vger.kernel.org; > linux-kernel@vger.kernel.org; linuxarm@openeuler.org; > linux-m68k@vger.kernel.org > Subject: Re: [Linuxarm] Re: [PATCH for-next 00/32] spin lock usage optimization > for SCSI drivers > > On Thu, 18 Feb 2021, Xiaofei Tan wrote: > > > On 2021/2/9 13:06, Finn Thain wrote: > > > On Tue, 9 Feb 2021, Song Bao Hua (Barry Song) wrote: > > > > > > > > On Sun, 7 Feb 2021, Xiaofei Tan wrote: > > > > > > > > > > > Replace spin_lock_irqsave with spin_lock in hard IRQ of SCSI > > > > > > drivers. There are no function changes, but may speed up if > > > > > > interrupt happen too often. > > > > > > > > > > This change doesn't necessarily work on platforms that support > > > > > nested interrupts. > > > > > > > > > > Were you able to measure any benefit from this change on some > > > > > other platform? > > > > > > > > I think the code disabling irq in hardIRQ is simply wrong. > > > > Since this commit > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/ > ?id=e58aa3d2d0cc > > > > genirq: Run irq handlers with interrupts disabled > > > > > > > > interrupt handlers are definitely running in a irq-disabled context > > > > unless irq handlers enable them explicitly in the handler to permit > > > > other interrupts. > > > > > > > > > > Repeating the same claim does not somehow make it true. If you put > > > your claim to the test, you'll see that that interrupts are not > > > disabled on m68k when interrupt handlers execute. > > > > > > The Interrupt Priority Level (IPL) can prevent any given irq handler > > > from being re-entered, but an irq with a higher priority level may be > > > handled during execution of a lower priority irq handler. > > > > > > sonic_interrupt() uses an irq lock within an interrupt handler to > > > avoid issues relating to this. This kind of locking may be needed in > > > the drivers you are trying to patch. Or it might not. Apparently, > > > no-one has looked. > > > > > > > According to your discussion with Barry, it seems that m68k is a little > > different from other architecture, and this kind of modification of this > > patch cannot be applied to m68k. So, could help to point out which > > driver belong to m68k architecture in this patch set of SCSI? I can > > remove them. > > > > If you would claim that "there are no function changes" in your patches > (as above) then the onus is on you to support that claim. > > I assume that there are some platforms on which your assumptions hold. > > With regard to drivers for those platforms, you might want to explain why > your patches should be applied there, given that the existing code is > superior for being more portable. I don't think it has nothing to do with portability. In the case of sonic_interrupt() you pointed out, on m68k, there is a high-priority interrupt can preempt low-priority interrupt, they will result in access the same critical data. M68K's spin_lock_irqsave() can disable the high-priority interrupt and avoid the race condition of the data. So the case should not be touched. I'd like to accept the reality and leave sonic_interrupt() alone. However, even on m68k, spin_lock_irqsave is not needed for other ordinary cases. If there is no other irq handler coming to access same critical data, it is pointless to hold a redundant irqsave lock in irqhandler even on m68k. In thread contexts, we always need that if an irqhandler can preempt those threads and access the same data. In hardirq, if there is an high-priority which can jump out on m68k to access the critical data which needs protection, we use the spin_lock_irqsave as you have used in sonic_interrupt(). Otherwise, the irqsave is also redundant for m68k. > > > BTW, sonic_interrupt() is from net driver natsemi, right? It would be > > appreciative if only discuss SCSI drivers in this patch set. thanks. > > > > The 'net' subsystem does have some different requirements than the 'scsi' > subsystem. But I don't see how that's relevant. Perhaps you can explain > it. Thanks. The difference is that if there are two co-existing interrupts which can access the same critical data on m68k. I don't think net and scsi matter. What really matters is the specific driver. Thanks Barry
On Mon, 22 Feb 2021, Song Bao Hua (Barry Song) wrote: > > On Thu, 18 Feb 2021, Xiaofei Tan wrote: > > > > > On 2021/2/9 13:06, Finn Thain wrote: > > > > On Tue, 9 Feb 2021, Song Bao Hua (Barry Song) wrote: > > > > > > > > > > On Sun, 7 Feb 2021, Xiaofei Tan wrote: > > > > > > > > > > > > > Replace spin_lock_irqsave with spin_lock in hard IRQ of SCSI > > > > > > > drivers. There are no function changes, but may speed up if > > > > > > > interrupt happen too often. > > > > > > > > > > > > This change doesn't necessarily work on platforms that support > > > > > > nested interrupts. > > > > > > > > > > > > Were you able to measure any benefit from this change on some > > > > > > other platform? > > > > > > > > > > I think the code disabling irq in hardIRQ is simply wrong. Since > > > > > this commit > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=e58aa3d2d0cc > > > > > genirq: Run irq handlers with interrupts disabled > > > > > > > > > > interrupt handlers are definitely running in a irq-disabled > > > > > context unless irq handlers enable them explicitly in the > > > > > handler to permit other interrupts. > > > > > > > > > > > > > Repeating the same claim does not somehow make it true. If you put > > > > your claim to the test, you'll see that that interrupts are not > > > > disabled on m68k when interrupt handlers execute. > > > > > > > > The Interrupt Priority Level (IPL) can prevent any given irq > > > > handler from being re-entered, but an irq with a higher priority > > > > level may be handled during execution of a lower priority irq > > > > handler. > > > > > > > > sonic_interrupt() uses an irq lock within an interrupt handler to > > > > avoid issues relating to this. This kind of locking may be needed > > > > in the drivers you are trying to patch. Or it might not. > > > > Apparently, no-one has looked. > > > > > > > > > > According to your discussion with Barry, it seems that m68k is a > > > little different from other architecture, and this kind of > > > modification of this patch cannot be applied to m68k. So, could help > > > to point out which driver belong to m68k architecture in this patch > > > set of SCSI? I can remove them. > > > > > > > If you would claim that "there are no function changes" in your > > patches (as above) then the onus is on you to support that claim. > > > > I assume that there are some platforms on which your assumptions hold. > > > > With regard to drivers for those platforms, you might want to explain > > why your patches should be applied there, given that the existing code > > is superior for being more portable. > > I don't think it has nothing to do with portability. In the case of > sonic_interrupt() you pointed out, on m68k, there is a high-priority > interrupt can preempt low-priority interrupt, they will result in access > the same critical data. M68K's spin_lock_irqsave() can disable the > high-priority interrupt and avoid the race condition of the data. So the > case should not be touched. I'd like to accept the reality and leave > sonic_interrupt() alone. > > However, even on m68k, spin_lock_irqsave is not needed for other > ordinary cases. > If there is no other irq handler coming to access same critical data, > it is pointless to hold a redundant irqsave lock in irqhandler even > on m68k. > > In thread contexts, we always need that if an irqhandler can preempt > those threads and access the same data. In hardirq, if there is an > high-priority which can jump out on m68k to access the critical data > which needs protection, we use the spin_lock_irqsave as you have used in > sonic_interrupt(). Otherwise, the irqsave is also redundant for m68k. > > > > > > BTW, sonic_interrupt() is from net driver natsemi, right? It would > > > be appreciative if only discuss SCSI drivers in this patch set. > > > thanks. > > > > > > > The 'net' subsystem does have some different requirements than the > > 'scsi' subsystem. But I don't see how that's relevant. Perhaps you can > > explain it. Thanks. > > The difference is that if there are two co-existing interrupts which can > access the same critical data on m68k. I don't think net and scsi > matter. What really matters is the specific driver. > Regarding m68k, your analysis overlooks the timing issue. E.g. patch 11/32 could be a problem because removing the irqsave would allow PDMA transfers to be interrupted. Aside from the timing issues, I agree with your analysis above regarding m68k. With regard to other architectures and platforms, in specific cases, e.g. where there's never more than one IRQ involved, then I could agree that your assumptions probably hold and an irqsave would be probably redundant. When you find a redundant irqsave, to actually patch it would bring a risk of regression with little or no reward. It's not my place to veto this entire patch series on that basis but IMO this kind of churn is misguided. > Thanks > Barry > >
> -----Original Message----- > From: Finn Thain [mailto:fthain@telegraphics.com.au] > Sent: Tuesday, February 23, 2021 6:25 PM > To: Song Bao Hua (Barry Song) <song.bao.hua@hisilicon.com> > Cc: tanxiaofei <tanxiaofei@huawei.com>; jejb@linux.ibm.com; > martin.petersen@oracle.com; linux-scsi@vger.kernel.org; > linux-kernel@vger.kernel.org; linuxarm@openeuler.org; > linux-m68k@vger.kernel.org > Subject: RE: [Linuxarm] Re: [PATCH for-next 00/32] spin lock usage optimization > for SCSI drivers > > On Mon, 22 Feb 2021, Song Bao Hua (Barry Song) wrote: > > > > On Thu, 18 Feb 2021, Xiaofei Tan wrote: > > > > > > > On 2021/2/9 13:06, Finn Thain wrote: > > > > > On Tue, 9 Feb 2021, Song Bao Hua (Barry Song) wrote: > > > > > > > > > > > > On Sun, 7 Feb 2021, Xiaofei Tan wrote: > > > > > > > > > > > > > > > Replace spin_lock_irqsave with spin_lock in hard IRQ of SCSI > > > > > > > > drivers. There are no function changes, but may speed up if > > > > > > > > interrupt happen too often. > > > > > > > > > > > > > > This change doesn't necessarily work on platforms that support > > > > > > > nested interrupts. > > > > > > > > > > > > > > Were you able to measure any benefit from this change on some > > > > > > > other platform? > > > > > > > > > > > > I think the code disabling irq in hardIRQ is simply wrong. Since > > > > > > this commit > > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/ > ?id=e58aa3d2d0cc > > > > > > genirq: Run irq handlers with interrupts disabled > > > > > > > > > > > > interrupt handlers are definitely running in a irq-disabled > > > > > > context unless irq handlers enable them explicitly in the > > > > > > handler to permit other interrupts. > > > > > > > > > > > > > > > > Repeating the same claim does not somehow make it true. If you put > > > > > your claim to the test, you'll see that that interrupts are not > > > > > disabled on m68k when interrupt handlers execute. > > > > > > > > > > The Interrupt Priority Level (IPL) can prevent any given irq > > > > > handler from being re-entered, but an irq with a higher priority > > > > > level may be handled during execution of a lower priority irq > > > > > handler. > > > > > > > > > > sonic_interrupt() uses an irq lock within an interrupt handler to > > > > > avoid issues relating to this. This kind of locking may be needed > > > > > in the drivers you are trying to patch. Or it might not. > > > > > Apparently, no-one has looked. > > > > > > > > > > > > > According to your discussion with Barry, it seems that m68k is a > > > > little different from other architecture, and this kind of > > > > modification of this patch cannot be applied to m68k. So, could help > > > > to point out which driver belong to m68k architecture in this patch > > > > set of SCSI? I can remove them. > > > > > > > > > > If you would claim that "there are no function changes" in your > > > patches (as above) then the onus is on you to support that claim. > > > > > > I assume that there are some platforms on which your assumptions hold. > > > > > > With regard to drivers for those platforms, you might want to explain > > > why your patches should be applied there, given that the existing code > > > is superior for being more portable. > > > > I don't think it has nothing to do with portability. In the case of > > sonic_interrupt() you pointed out, on m68k, there is a high-priority > > interrupt can preempt low-priority interrupt, they will result in access > > the same critical data. M68K's spin_lock_irqsave() can disable the > > high-priority interrupt and avoid the race condition of the data. So the > > case should not be touched. I'd like to accept the reality and leave > > sonic_interrupt() alone. > > > > However, even on m68k, spin_lock_irqsave is not needed for other > > ordinary cases. > > If there is no other irq handler coming to access same critical data, > > it is pointless to hold a redundant irqsave lock in irqhandler even > > on m68k. > > > > In thread contexts, we always need that if an irqhandler can preempt > > those threads and access the same data. In hardirq, if there is an > > high-priority which can jump out on m68k to access the critical data > > which needs protection, we use the spin_lock_irqsave as you have used in > > sonic_interrupt(). Otherwise, the irqsave is also redundant for m68k. > > > > > > > > > BTW, sonic_interrupt() is from net driver natsemi, right? It would > > > > be appreciative if only discuss SCSI drivers in this patch set. > > > > thanks. > > > > > > > > > > The 'net' subsystem does have some different requirements than the > > > 'scsi' subsystem. But I don't see how that's relevant. Perhaps you can > > > explain it. Thanks. > > > > The difference is that if there are two co-existing interrupts which can > > access the same critical data on m68k. I don't think net and scsi > > matter. What really matters is the specific driver. > > > > Regarding m68k, your analysis overlooks the timing issue. E.g. patch 11/32 > could be a problem because removing the irqsave would allow PDMA transfers > to be interrupted. Aside from the timing issues, I agree with your > analysis above regarding m68k. You mentioned you need realtime so you want an interrupt to be able to preempt another one. Now you said you want an interrupt not to be preempted as it will make a timing issue. If this PDMA transfer will have some problem when it is preempted, I believe we need some enhanced ways to handle this, otherwise, once we enable preempt_rt or threaded_irq, it will get the timing issue. so here it needs a clear comment and IRQF_NO_THREAD if this is the case. > > With regard to other architectures and platforms, in specific cases, e.g. > where there's never more than one IRQ involved, then I could agree that > your assumptions probably hold and an irqsave would be probably redundant. > > When you find a redundant irqsave, to actually patch it would bring a risk > of regression with little or no reward. It's not my place to veto this > entire patch series on that basis but IMO this kind of churn is misguided. Nope. I would say the real misguidance is that the code adds one lock while it doesn't need the lock. Easily we can add redundant locks or exaggerate the coverage range of locks, but the smarter way is that people add locks only when they really need the lock by considering concurrency and realtime performance. Thanks Barry
On Tue, 23 Feb 2021, Song Bao Hua (Barry Song) wrote: > > > > Regarding m68k, your analysis overlooks the timing issue. E.g. patch > > 11/32 could be a problem because removing the irqsave would allow PDMA > > transfers to be interrupted. Aside from the timing issues, I agree > > with your analysis above regarding m68k. > > You mentioned you need realtime so you want an interrupt to be able to > preempt another one. That's not what I said. But for the sake of discussion, yes, I do know people who run Linux on ARM hardware (if Android vendor kernels can be called "Linux") and who would benefit from realtime support on those devices. > Now you said you want an interrupt not to be preempted as it will make a > timing issue. mac_esp deliberately constrains segment sizes so that it can harmlessly disable interrupts for the duration of the transfer. Maybe the irqsave in this driver is over-cautious. Who knows? The PDMA timing problem relates to SCSI bus signalling and the tolerance of real- world SCSI devices to same. The other problem is that the PDMA logic circuit is undocumented hardware. So there may be further timing requirements lurking there. Therefore, patch 11/32 is too risky. > If this PDMA transfer will have some problem when it is preempted, I > believe we need some enhanced ways to handle this, otherwise, once we > enable preempt_rt or threaded_irq, it will get the timing issue. so here > it needs a clear comment and IRQF_NO_THREAD if this is the case. > People who require fast response times cannot expect random drivers or platforms to meet such requirements. I fear you may be asking too much from Mac Quadra machines. > > > > With regard to other architectures and platforms, in specific cases, > > e.g. where there's never more than one IRQ involved, then I could > > agree that your assumptions probably hold and an irqsave would be > > probably redundant. > > > > When you find a redundant irqsave, to actually patch it would bring a > > risk of regression with little or no reward. It's not my place to veto > > this entire patch series on that basis but IMO this kind of churn is > > misguided. > > Nope. > > I would say the real misguidance is that the code adds one lock while it > doesn't need the lock. Easily we can add redundant locks or exaggerate > the coverage range of locks, but the smarter way is that people add > locks only when they really need the lock by considering concurrency and > realtime performance. > You appear to be debating a strawman. No-one is advocating excessive locking in new code. > Thanks > Barry >
Hi Xiaofei, On Sun, Feb 7, 2021 at 12:46 PM Xiaofei Tan <tanxiaofei@huawei.com> wrote: > Replace spin_lock_irqsave with spin_lock in hard IRQ of SCSI drivers. > There are no function changes, but may speed up if interrupt happen > too often. I'll bite: how much does this speed up interrupt processing? What's the typical cost of saving/disabling, and restoring interrupt state? Is removing this cost worth the risk of introducing subtle regressions on platforms you cannot test yourself? BTW, how many of these legacy SCSI controllers do you have access to? Thanks for your answers! Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
> -----Original Message----- > From: Finn Thain [mailto:fthain@telegraphics.com.au] > Sent: Wednesday, February 24, 2021 6:21 PM > To: Song Bao Hua (Barry Song) <song.bao.hua@hisilicon.com> > Cc: tanxiaofei <tanxiaofei@huawei.com>; jejb@linux.ibm.com; > martin.petersen@oracle.com; linux-scsi@vger.kernel.org; > linux-kernel@vger.kernel.org; linuxarm@openeuler.org; > linux-m68k@vger.kernel.org > Subject: [Linuxarm] Re: [PATCH for-next 00/32] spin lock usage optimization > for SCSI drivers > > On Tue, 23 Feb 2021, Song Bao Hua (Barry Song) wrote: > > > > > > > Regarding m68k, your analysis overlooks the timing issue. E.g. patch > > > 11/32 could be a problem because removing the irqsave would allow PDMA > > > transfers to be interrupted. Aside from the timing issues, I agree > > > with your analysis above regarding m68k. > > > > You mentioned you need realtime so you want an interrupt to be able to > > preempt another one. > > That's not what I said. But for the sake of discussion, yes, I do know > people who run Linux on ARM hardware (if Android vendor kernels can be > called "Linux") and who would benefit from realtime support on those > devices. Realtime requirement is definitely a true requirement on ARM Linux. I once talked/worked with some guys who were using ARM for realtime system. The feasible approaches include: 1. Dual OS(RTOS + Linux): e.g. QNX+Linux XENOMAI+Linux L4+Linux 2. preempt-rt Which is continuously maintained like: https://lore.kernel.org/lkml/20210218201041.65fknr7bdplwqbez@linutronix.de/ 3. bootargs isolcpus= to isolate a cpu for a specific realtime task or interrupt https://access.redhat.com/documentation/en-us/red_hat_enterprise_linux_for_real_time/7/html/tuning_guide/isolating_cpus_using_tuned-profiles-realtime 4. ARM FIQ which has separate fiq API, an example in fsl sound: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/sound/soc/fsl/imx-pcm-fiq.c 5. Let one core invisible to Linux Running non-os system and rtos on the core Honestly, I've never seen anyone who depends on irq priority to support realtime in ARM Linux though ARM's RTOS-es use it quite commonly. > > > Now you said you want an interrupt not to be preempted as it will make a > > timing issue. > > mac_esp deliberately constrains segment sizes so that it can harmlessly > disable interrupts for the duration of the transfer. > > Maybe the irqsave in this driver is over-cautious. Who knows? The PDMA > timing problem relates to SCSI bus signalling and the tolerance of real- > world SCSI devices to same. The other problem is that the PDMA logic > circuit is undocumented hardware. So there may be further timing > requirements lurking there. Therefore, patch 11/32 is too risky. > > > If this PDMA transfer will have some problem when it is preempted, I > > believe we need some enhanced ways to handle this, otherwise, once we > > enable preempt_rt or threaded_irq, it will get the timing issue. so here > > it needs a clear comment and IRQF_NO_THREAD if this is the case. > > > > People who require fast response times cannot expect random drivers or > platforms to meet such requirements. I fear you may be asking too much > from Mac Quadra machines. Once preempt_rt is enabled, those who want a fast irq environment need a no_thread flag, or need to set its irq thread to higher sched_fifo/rr priority. > > > > > > > With regard to other architectures and platforms, in specific cases, > > > e.g. where there's never more than one IRQ involved, then I could > > > agree that your assumptions probably hold and an irqsave would be > > > probably redundant. > > > > > > When you find a redundant irqsave, to actually patch it would bring a > > > risk of regression with little or no reward. It's not my place to veto > > > this entire patch series on that basis but IMO this kind of churn is > > > misguided. > > > > Nope. > > > > I would say the real misguidance is that the code adds one lock while it > > doesn't need the lock. Easily we can add redundant locks or exaggerate > > the coverage range of locks, but the smarter way is that people add > > locks only when they really need the lock by considering concurrency and > > realtime performance. > > > > You appear to be debating a strawman. No-one is advocating excessive > locking in new code. > I actually meant most irqsave(s) in hardirq were added carelessly. When irq and threads could access same data, people added irqsave in threads, that is perfectly good as it could block irq. But people were likely to put an irqsave in irq without any thinking. We do have some drivers which are doing that with a clear intention as your sonic_interrupt(), but I bet most were done aimlessly. Anyway, the debate is long enough, let's move to some more important things. I appreciate that you shared a lot of knowledge of m68k. Thanks Barry
Hi Geert, On 2021/2/24 17:41, Geert Uytterhoeven wrote: > Hi Xiaofei, > > On Sun, Feb 7, 2021 at 12:46 PM Xiaofei Tan <tanxiaofei@huawei.com> wrote: >> Replace spin_lock_irqsave with spin_lock in hard IRQ of SCSI drivers. >> There are no function changes, but may speed up if interrupt happen >> too often. > > I'll bite: how much does this speed up interrupt processing? > What's the typical cost of saving/disabling, and restoring interrupt > state? It could only take a few CPU cycles. So there is little benefit for speeding up interrupt processing.You could take them as cleanup. Is removing this cost worth the risk of introducing subtle > regressions on platforms you cannot test yourself? > Currently, only found M68K platform support that high-priority interrupt preempts low-priority. No other platform has such services. Therefore, these changes do not affect non-M68K platforms. For M68K platform, no one report such interrupt preemption case in these SCSI drivers. > BTW, how many of these legacy SCSI controllers do you have access to? > Actually, no. > Thanks for your answers! > > Gr{oetje,eeting}s, > > Geert >
On Wed, 24 Feb 2021, Song Bao Hua (Barry Song) wrote: > > Realtime requirement is definitely a true requirement on ARM Linux. > > I once talked/worked with some guys who were using ARM for realtime > system. > The feasible approaches include: > 1. Dual OS(RTOS + Linux): e.g. QNX+Linux XENOMAI+Linux L4+Linux > 2. preempt-rt > Which is continuously maintained like: > https://lore.kernel.org/lkml/20210218201041.65fknr7bdplwqbez@linutronix.de/ > 3. bootargs isolcpus= > to isolate a cpu for a specific realtime task or interrupt > https://access.redhat.com/documentation/en-us/red_hat_enterprise_linux_for_real_time/7/html/tuning_guide/isolating_cpus_using_tuned-profiles-realtime > 4. ARM FIQ which has separate fiq API, an example in fsl sound: > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/sound/soc/fsl/imx-pcm-fiq.c > 5. Let one core invisible to Linux > Running non-os system and rtos on the core > Regarding Linux systems, it appears that approach 3 could theoretically achieve minimal interrupt latency for a given device without requiring any interrupt nesting. But the price is one CPU core which is not going to work on a uniprocessor system. > Honestly, I've never seen anyone who depends on irq priority to support > realtime in ARM Linux though ARM's RTOS-es use it quite commonly. > Perhaps you don't work with uniprocessor ARM Linux systems? > Once preempt_rt is enabled, those who want a fast irq environment need a > no_thread flag, or need to set its irq thread to higher sched_fifo/rr > priority. > Thanks for the tip. > [...] > > Anyway, the debate is long enough, let's move to some more important > things. I appreciate that you shared a lot of knowledge of m68k. > No problem. > Thanks > Barry >
Replace spin_lock_irqsave with spin_lock in hard IRQ of SCSI drivers. There are no function changes, but may speed up if interrupt happen too often. Xiaofei Tan (32): scsi: 53c700: Replace spin_lock_irqsave with spin_lock in hard IRQ scsi: ipr: Replace spin_lock_irqsave with spin_lock in hard IRQ scsi: lpfc: Replace spin_lock_irqsave with spin_lock in hard IRQ scsi: qla4xxx: Replace spin_lock_irqsave with spin_lock in hard IRQ scsi: BusLogic: Replace spin_lock_irqsave with spin_lock in hard IRQ scsi: a100u2w: Replace spin_lock_irqsave with spin_lock in hard IRQ scsi: a2091: Replace spin_lock_irqsave with spin_lock in hard IRQ scsi: a3000: Replace spin_lock_irqsave with spin_lock in hard IRQ scsi: aha1740: Replace spin_lock_irqsave with spin_lock in hard IRQ scsi: bfa: Replace spin_lock_irqsave with spin_lock in hard IRQ scsi: esp_scsi: Replace spin_lock_irqsave with spin_lock in hard IRQ scsi: gvp11: Replace spin_lock_irqsave with spin_lock in hard IRQ scsi: hptiop: Replace spin_lock_irqsave with spin_lock in hard IRQ scsi: ibmvscsi: Replace spin_lock_irqsave with spin_lock in hard IRQ scsi: initio: Replace spin_lock_irqsave with spin_lock in hard IRQ scsi: megaraid: Replace spin_lock_irqsave with spin_lock in hard IRQ scsi: mac53c94: Replace spin_lock_irqsave with spin_lock in hard IRQ scsi: mesh: Replace spin_lock_irqsave with spin_lock in hard IRQ scsi: mvumi: Replace spin_lock_irqsave with spin_lock in hard IRQ scsi: myrb: Replace spin_lock_irqsave with spin_lock in hard IRQ scsi: myrs: Replace spin_lock_irqsave with spin_lock in hard IRQ scsi: ncr53c8xx: Replace spin_lock_irqsave with spin_lock in hard IRQ scsi: nsp32: Replace spin_lock_irqsave with spin_lock in hard IRQ scsi: pmcraid: Replace spin_lock_irqsave with spin_lock in hard IRQ scsi: pcmcia: Replace spin_lock_irqsave with spin_lock in hard IRQ scsi: qlogicfas408: Replace spin_lock_irqsave with spin_lock in hard IRQ scsi: qlogicpti: Replace spin_lock_irqsave with spin_lock in hard IRQ scsi: sgiwd93: Replace spin_lock_irqsave with spin_lock in hard IRQ scsi: stex: Replace spin_lock_irqsave with spin_lock in hard IRQ scsi: vmw_pvscsi: Replace spin_lock_irqsave with spin_lock in hard IRQ scsi: wd719x: Replace spin_lock_irqsave with spin_lock in hard IRQ scsi: advansys: Replace spin_lock_irqsave with spin_lock in hard IRQ drivers/scsi/53c700.c | 5 ++-- drivers/scsi/BusLogic.c | 5 ++-- drivers/scsi/a100u2w.c | 5 ++-- drivers/scsi/a2091.c | 5 ++-- drivers/scsi/a3000.c | 5 ++-- drivers/scsi/advansys.c | 5 ++-- drivers/scsi/aha1740.c | 5 ++-- drivers/scsi/bfa/bfad.c | 20 ++++++------- drivers/scsi/esp_scsi.c | 5 ++-- drivers/scsi/gvp11.c | 5 ++-- drivers/scsi/hptiop.c | 5 ++-- drivers/scsi/ibmvscsi/ibmvfc.c | 5 ++-- drivers/scsi/initio.c | 5 ++-- drivers/scsi/ipr.c | 21 ++++++------- drivers/scsi/lpfc/lpfc_sli.c | 49 +++++++++++++------------------ drivers/scsi/mac53c94.c | 5 ++-- drivers/scsi/megaraid.c | 10 +++---- drivers/scsi/megaraid/megaraid_sas_base.c | 5 ++-- drivers/scsi/mesh.c | 5 ++-- drivers/scsi/mvumi.c | 7 ++--- drivers/scsi/myrb.c | 20 +++++-------- drivers/scsi/myrs.c | 15 ++++------ drivers/scsi/ncr53c8xx.c | 5 ++-- drivers/scsi/nsp32.c | 5 ++-- drivers/scsi/pcmcia/sym53c500_cs.c | 5 ++-- drivers/scsi/pmcraid.c | 8 ++--- drivers/scsi/qla4xxx/ql4_isr.c | 15 ++++------ drivers/scsi/qlogicfas408.c | 5 ++-- drivers/scsi/qlogicpti.c | 5 ++-- drivers/scsi/sgiwd93.c | 5 ++-- drivers/scsi/stex.c | 16 +++++----- drivers/scsi/vmw_pvscsi.c | 4 +-- drivers/scsi/wd719x.c | 7 ++--- 33 files changed, 122 insertions(+), 175 deletions(-)