On 27 February 2024 17:18:58 GMT, Thomas Gleixner <tglx@xxxxxxxxxxxxx> wrote:
>On Tue, Feb 27 2024 at 17:00, David Woodhouse wrote:
>> On 27 February 2024 14:43:26 GMT, Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:
>>>On Tue, 27 Feb 2024 11:49:21 +0000
>>>David Woodhouse <dwmw2@xxxxxxxxxxxxx> wrote:
>>>
>>>> diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
>>>> index c16b6d394d55..d8b5326ecebc 100644
>>>> --- a/arch/x86/kvm/xen.c
>>>> +++ b/arch/x86/kvm/xen.c
>>>> @@ -1736,9 +1736,23 @@ static int set_shinfo_evtchn_pending(struct kvm_vcpu *vcpu, u32 port)
>>>> unsigned long flags;
>>>> int rc = -EWOULDBLOCK;
>>>>
>>>> - read_lock_irqsave(&gpc->lock, flags);
>>>> + local_irq_save(flags);
>>>> + if (!read_trylock(&gpc->lock)) {
>>>
>>>Note, directly disabling interrupts in PREEMPT_RT is just as bad as doing
>>>so in non RT and the taking a mutex.
>>
>> Well, it *isn't* a mutex in non-RT. It's basically a spinlock. And
>> even though RT turns it into a mutex this is only a trylock with a
>> fall back to a slow path in process context, so it ends up being a
>> very short period of irqdisable/lock – just to validate the target
>> address of the cache, and write there. (Or fall back to that slow
>> path, if the cache needs revalidating).
>>
>> So I think this is probably OK, but...
>>
>>>Worse yet, in PREEMPT_RT read_unlock_irqrestore() isn't going to re-enable
>>>interrupts.
>>
>> ... that's kind of odd. I think there's code elsewhere which assumes
>> it will, and pairs it with an explicit local_irq_save() like the
>> above, in a different atomic context (updating runstate time info when
>> being descheduled).
>
>While it is legit, it simply cannot work for RT. We fixed the few places
>which had it open coded (mostly for no good reason).
>
>> So *that* needs changing for RT?
>
>No. It's not required anywhere and we really don't change that just to
>accomodate the xen shim.
It's open-coded in the runstate update in the Xen shim, as I said. That's what needs changing, which is separate from the problem at hand in this patch.
>I'll look at the problem at hand tomorrow.
Ta.
