Re: [PATCH v1 3/3] Introducing kvm_arch_set_irq_inatomic fast inject

[Douglas Freimuth]

On 3/16/26 11:41 AM, Matthew Rosato wrote:
> On 3/13/26 10:01 AM, Douglas Freimuth wrote:
>
> > > > @@ -450,6 +450,10 @@ struct kvm_vm_stat {
> > > >        u64 inject_io;
> > > >        u64 io_390_adapter_map;
> > > >        u64 io_390_adapter_unmap;
> > > > +    u64 io_390_inatomic;
> > > > +    u64 io_flic_inject_airq;
> > > > +    u64 io_set_adapter_int;
> > > > +    u64 io_390_inatomic_adapter_masked;
> > > >        u64 inject_float_mchk;
> > > >        u64 inject_pfault_done;
> > > >        u64 inject_service_signal;
> > > > @@ -481,7 +485,7 @@ struct s390_io_adapter {
> > > >        bool masked;
> > > >        bool swap;
> > > >        bool suppressible;
> > > > -    struct rw_semaphore maps_lock;
> > > > +    spinlock_t maps_lock;
> > >
> > > Patch 1 (re-)introduced the maps_lock, now you are converting it to a spinlock 2 patches later.
> > >
> > > I realize that you were bringing back code that was previously removed by
> > >
> > > f65470661f36 KVM: s390/interrupt: do not pin adapter interrupt pages
> > >
> > > but for this series wouldn't it make sense to just start by introducing maps_lock as a spinlock from patch 1 vs re-introducing the semaphore for the span of 2 commits?
> >
> > Matt, thank you for your input. The policy of individual patches not only compiling but having individual utility and standing on their own applies here. In patches 1 and 2 the maps_lock is more flexible as a semaphore providing utility for the patch. While in patch 3 the maps_lock has to be a spin_lock so inatomic can use it with interrupts disabled.
>
> I would agree completely if these were 2 separate series, with some period of time in between when the semaphore is implemented and a subsequent switch to a spinlock.
>
> But here you are introducing a semaphore and immediately replacing it with a spinlock _all within a single series_, such that the semaphore implementation will never be used in practice.
>
> In the end, that just creates a bunch of extra insertions and subsequent deletions of those insertions all within this series.
>
> Yes, the semaphore would be the preferred implementation if allowed to sleep but since the goal of the series is it implement kvm_arch_set_irq_inatomic then you can just indicate in patch 1 why you are already switching to a spinlock when you re-introduce these ioctl functions (e.g. in preparation for kvm_arch_set_irq_inatomic support which requires a thread of execution that will not sleep).

I will make the change and note the reasoning in Patch 1.

> > > > +
> > > > +    /* We're only interested in the 0->1 transition. */
> > > > +    if (!level)
> > > > +        return -EWOULDBLOCK;
> > > > +    if (e->type != KVM_IRQ_ROUTING_S390_ADAPTER)
> > > > +        return -EWOULDBLOCK;
> > > > +
> > > > +    adapter = get_io_adapter(kvm, e->adapter.adapter_id);
> > > > +    if (!adapter)
> > > > +        return -EWOULDBLOCK;
> > > > +
> > > > +    s390int.parm64 = isc_to_int_word(adapter->isc);
> > > > +    ret = adapter_indicators_set_fast(kvm, adapter, &e->adapter);
> > > > +    if (ret < 0)
> > > > +        return -EWOULDBLOCK;
> > >
> > > We know for sure that a guest that is running in SE will always hit this because no mappings will be available.
> > > Did you test if it would be more efficient to check the kvm for SE at the start of kvm_arch_set_irq_inatomic() and immediately return -EWOULDBLOCK there?
> >
> > It might be slightly more efficient in SE environments to immediately return -EWOULDBLOCK at the start of kvm_arch_set_irq_inatomic. But the change would be fairly broad since it would require changing the mutex for kvm->lock to a spin_lock to allow checking if pv is present with interrupts disabled. I would recommend this for a follow-on if behavior in the field requires it.
>
> Right, I forgot about the mutex.
>
> OK, then I agree let's leave this for follow-on after this series lands.
>
> I suspect that you could get away with peeking the value without the mutex held as a heuristic.  If we get it wrong it would be during a transition into/out of SE and either...
>
> 1) we -EWOULDBLOCK and fallback to irqfd_inject when we could have continued down the inatomic path -- irqfd_inject should always work.
>
> or
>
> 2) we would proceed into the inatomic path and then get kicked out as we do with this current implementation: when we attempt to find the pre-pinned mapping (which is protected by a spinlock).
>
> The questions to answer would be whether it's permissible to peek the value and whether it buys us enough to be worth it.