Re: [PATCH V8 2/2] virtio-gpio: Add support for interrupts

[Marc Zyngier <maz@kernel.org>]

On Mon, 02 Aug 2021 11:49:42 +0100,
Viresh Kumar <viresh.kumar@linaro.org> wrote:
> 
> Hi Marc,
> 
> On 02-08-21, 10:45, Marc Zyngier wrote:
> > On Sun, 01 Aug 2021 14:43:37 +0100,
> > Arnd Bergmann <arnd@kernel.org> wrote:
> > > What are the requirements for a ->irq_mask() operation? Can this
> > > be called in atomic context? If it can, is it allowed to be posted
> > > (e.g. by queuing a new command without waiting for the reply)?
> > 
> > irq_mask() can definitely be called in atomic context. But in this
> > case the driver must implement the bus_lock madness and commit changes
> > to the backend on unlock. See how most of the I2C GPIO implementations
> > work.
> 
> Right, I already have that in place but I have a few doubts on that.
> 
> I can see irq_bus_lock/unlock() getting called around
> irq_mask/unmask/set_type() during request_irq(), and so I can issue
> all the virtio calls from irq_bus_unlock() there.
> 
> But I don't see the same sequence being followed by the irq core
> when an interrupt actually happens, i.e. when I call
> generic_handle_irq() from virtqueue callback for eventq vq. In this
> case, the irq core can directly call mask/unmask() around irq_eoi(),
> without calling the irq_bus_lock/unlock() guards.

You can't take that lock from the core irq code, for obvious reasons:
you're in IRQ context, and you cannot sleep. You definitely need to
use threaded interrupts for this.

> 
> How should the driver take care of these mask/unmask calls, or should it take
> care of it at all, as they should negate each other eventually ?
> 
> > Obviously, this has an effect on performance and complexity. It would
> > be a lot better if there was a way to perform the mask/unmask
> > operations synchronously, without the need of a buffer. For example,
> > virtio-console has the so called 'emergency write' feature, which
> > allows for write operations without any buffer.
> 
> We discussed this sometime back over IRC, and Jean Philippe said this in context
> of virtio-iommu:
> 
>  "From some profiling the overhead of context switching is much greater than
>  that of adding buffers, so adding a new virtio interface to bypass the
>  virtqueue might not show any improvement."
>

If you need something to be synchronous, there is not exactly a
choice. Masking an interrupt is something that is much better as a
synchronous interface, because it leads to tons of further
simplifications. And in my book, simplicity is a much easier path to
correctness.

> We were looking to explore the config space for normal GPIO
> operations as well then.
> 
> The host would be required to perform some action here on a config update and
> the guest would be waiting over an atomic operation for it to complete. So
> eventually it should also be slow.
> 
> > The later would allow the implementation of a "fire and forget"
> > mask/unmask that would still be synchronous.
> 
> > > Alternatively, would it be possible to simply emulate the 'irq_mask()'
> > > operation in software, by not delivering the next interrupt to the
> > > handler until the driver calls irq_unmask()? Once it's been delivered,
> > > it would just not get requeued in this case.
> > 
> > That'd be a poor-man's solution, as you would still get host
> > notifications, which could have an impact in the case of a screaming
> > interrupt.
> 
> Right, irq_bus_unlock() works fine for this though.

To some extent (insert a comment about thrust and flying pigs here).
It imposes a lot of restrictions, adds latency, and makes it harder to
reason about when what happens when. It also means that you are
probably designing for Linux instead of building something more
generic.

Having contributed to a PV interrupt architecture in the recent past,
I quickly realised that keeping things as simple as possible was a
good way to avoid baked-in assumptions. Yes, a synchronous interface
results in a few extra traps. But you can also now reason about it and
*prove* things. YMMV.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.