RE: [Linuxarm] Re: [PATCH for-next 00/32] spin lock usage optimization for SCSI drivers

RE: [Linuxarm] Re: [PATCH for-next 00/32] spin lock usage optimization for SCSI drivers

[Finn Thain]

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.

RE: [Linuxarm] Re: [PATCH for-next 00/32] spin lock usage optimization for SCSI drivers

[Song Bao Hua (Barry Song)]

> -----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

RE: [Linuxarm] Re: [PATCH for-next 00/32] spin lock usage optimization for SCSI drivers

[Finn Thain]

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
> 

RE: [Linuxarm] Re: [PATCH for-next 00/32] spin lock usage optimization for SCSI drivers

[Song Bao Hua (Barry Song)]

> -----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

RE: [Linuxarm] Re: [PATCH for-next 00/32] spin lock usage optimization for SCSI drivers

[Finn Thain]

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
> 

RE: [Linuxarm] Re: [PATCH for-next 00/32] spin lock usage optimization for SCSI drivers

[Song Bao Hua (Barry Song)]

> -----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

RE: [Linuxarm] Re: [PATCH for-next 00/32] spin lock usage optimization for SCSI drivers

[Finn Thain]

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
> 
> 

RE: [Linuxarm] Re: [PATCH for-next 00/32] spin lock usage optimization for SCSI drivers

[Song Bao Hua (Barry Song)]

> -----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

RE: [Linuxarm] Re: [PATCH for-next 00/32] spin lock usage optimization for SCSI drivers

[Finn Thain]

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
> 
> 

RE: [Linuxarm] Re: [PATCH for-next 00/32] spin lock usage optimization for SCSI drivers

[Song Bao Hua (Barry Song)]

> -----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

RE: [Linuxarm] Re: [PATCH for-next 00/32] spin lock usage optimization for SCSI drivers

[Finn Thain]

[-- Attachment #1: Type: text/plain, Size: 3934 bytes --]

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
> 

RE: [Linuxarm] Re: [PATCH for-next 00/32] spin lock usage optimization for SCSI drivers

[Song Bao Hua (Barry Song)]

> -----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

RE: [Linuxarm] Re: [PATCH for-next 00/32] spin lock usage optimization for SCSI drivers

[Finn Thain]

[-- Attachment #1: Type: text/plain, Size: 6252 bytes --]

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
> 
> 

RE: [Linuxarm] Re: [PATCH for-next 00/32] spin lock usage optimization for SCSI drivers

[Song Bao Hua (Barry Song)]

> -----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

RE: [Linuxarm] Re: [PATCH for-next 00/32] spin lock usage optimization for SCSI drivers

[Finn Thain]

[-- Attachment #1: Type: text/plain, Size: 11717 bytes --]

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
> 
> 

RE: [Linuxarm] Re: [PATCH for-next 00/32] spin lock usage optimization for SCSI drivers

[Song Bao Hua (Barry Song)]

> -----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

Re: [Linuxarm] Re: [PATCH for-next 00/32] spin lock usage optimization for SCSI drivers

[Xiaofei Tan]

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.

> .
>

Re: [Linuxarm] Re: [PATCH for-next 00/32] spin lock usage optimization for SCSI drivers

[Finn Thain]

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.

RE: [Linuxarm] Re: [PATCH for-next 00/32] spin lock usage optimization for SCSI drivers

[Song Bao Hua (Barry Song)]

> -----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

RE: [Linuxarm] Re: [PATCH for-next 00/32] spin lock usage optimization for SCSI drivers

[Finn Thain]

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
> 
> 

RE: [Linuxarm] Re: [PATCH for-next 00/32] spin lock usage optimization for SCSI drivers

[Song Bao Hua (Barry Song)]

> -----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

RE: [Linuxarm] Re: [PATCH for-next 00/32] spin lock usage optimization for SCSI drivers

[Finn Thain]

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
> 

RE: [Linuxarm] Re: [PATCH for-next 00/32] spin lock usage optimization for SCSI drivers

[Song Bao Hua (Barry Song)]

> -----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

RE: [Linuxarm] Re: [PATCH for-next 00/32] spin lock usage optimization for SCSI drivers

[Finn Thain]

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
> 

Re: [PATCH for-next 00/32] spin lock usage optimization for SCSI drivers

[Finn Thain]

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/
> > 
> > .
> > 
> 
>