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: Wednesday, February 24, 2021 6:21 PM
> 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: [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@xxxxxxxxxxxxx/
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