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

[Finn Thain <fthain@telegraphics.com.au>]

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