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

[Douglas Freimuth <freimuth@linux.ibm.com>]

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