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@xxxxxxxxxxxxxxxxxxx]
> Sent: Thursday, February 11, 2021 11:35 AM
> To: Song Bao Hua (Barry Song) <song.bao.hua@xxxxxxxxxxxxx>
> Cc: tanxiaofei <tanxiaofei@xxxxxxxxxx>; jejb@xxxxxxxxxxxxx;
> martin.petersen@xxxxxxxxxx; linux-scsi@xxxxxxxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx; linuxarm@xxxxxxxxxxxxx;
> linux-m68k@xxxxxxxxxxxxxxx
> 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