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

[Matthew Rosato <mjrosato@linux.ibm.com>]

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

>>> +
>>> +    /* 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.