On Thu, 18 Feb 2021, Arnd Bergmann wrote:
On Thu, Feb 18, 2021 at 6:30 AM Finn Thain <fthain@xxxxxxxxxxxxxxxxxxx> wrote:
On Wed, 17 Feb 2021, Song Bao Hua (Barry Song) wrote:
On Sat, 13 Feb 2021, Song Bao Hua (Barry Song) wrote:
That scenario seems a little contrived to me (drivers for two or
more devices sharing state through their interrupt handlers). Is
it real? I suppose every platform has its quirks. The irq lock in
sonic_interrupt() is only there because of a platform quirk (the
same device can trigger either of two IRQs). Anyway, no-one
expects all drivers to work on all platforms; I don't know why it
bothers you so much when platforms differ.
Basically, we wrote drivers with the assumption that this driver
will be cross-platform. (Of course there are some drivers which can
only work on one platform, for example, if the IP of the device is
only used in one platform as an internal component of a specific
SoC.)
So once a device has two or more interrupts, we need to consider one
interrupt might preempt another one on m68k on the same cpu if we
also want to support this driver on m68k. this usually doesn't
matter on other platforms.
When users show up who desire to run your drivers on their platform,
you can expect them to bring patches and a MAINTAINERS file entry.
AFAIK, Linux development has always worked that way.
This is only part of the picture though. We also also constantly trying
to generalize the internal interfaces, to make sure that platforms work
the same way and that it's possible to write drivers in a portable way
without having to rely on platform maintainers to point out the
differences.
I think it would make a lot of sense to remove the architecture
differences here by making m68k work the same way as the others and
documenting that as the expected behavior.
If you had some great new feature that was incompatible with priority
masking, or incompatible with existing drivers portable enough to support
such features, then I would be more amenable to your plan to remove
functionality.
But there's no real justification here. You say platform maintainers
should not have to "point out the differences". But is that not their job?
You are probably right that there are no specific bugs on m68k machines
that rely on the nested hardirqs today, but I think they only get away
with it because
a) there is no SMP support on m68k, so it likely doesn't run into the
more subtle cases with lock ordering that you could get when you have
hardirq handlers on multiple CPUs in parallel
And that's relevant because SMP support is now mandatory? Is this the
logical consequence of your intention to "remove the architecture
differences"?
b) there is a very limited number of device drivers that are actually
used on m68k, in particular only M54xx has PCI support, but that in
turn has a different interrupt scheme.
Everyone is afraid of some mysterious bug somewhere, yet no one can point
to it.
Again, I submit that the bug doesn't exist. That's because there is no
material difference in semantics between the irqs_disabled()
implementation that says "all interrupts are disabled except for NMI (and
some others that some ARM platform cares about)" and the implementation
that says "interrupts are disabled except higher priority ones than you
may be enabled".
If you can point to code that cares about such semantics, I predict you've
found either a coding anti-pattern or perhaps some obscure hardware design
flaw. Either way, there is no justification for your plan.
Changing the behavior on m68k clearly has its own regression risk, but
it could be done as a configuration option that defaults to the
traditional behavior on machines that have not been verified to be
well-behaved without nested hardirqs, and hidden on machines that do not
need it (any more).
This plan will quantifiably increase interrupt latency. It's not some
vague risk that you can hand-wave away. It's unavoidable.
As far as I can tell, the only reason you would actually need nested
hardirqs is when a low-priority interrupt has to perform expensive I/O
processing, but we've had countless other methods to do the same over
the years (at least bottom half, softirq, taskqueue, tasklet, keventd,
workqueue, kthread, threaded interrupt handlers and probably others).
Nope. Interrupt priority masking is there to place an upper bound
interrupt latency. That's why this feature is shipping in contemporary
hardware (e.g. ARM GIC). If you care about real time workloads on arm64,
that may interest you.
If you don't care about arm hardware or real time workloads, that's fine
too, but here's the rub. Song Bao Hua's plan involves reworking the
locking in existing drivers (which may be portable enough to support
interrerupt priority) without justification.
