Re: [PATCH v2 7/8] KVM: x86/xen: avoid blocking in hardirq context in kvm_xen_set_evtchn_fast()

From: David Woodhouse <dwmw2@infradead.org>
To: Thomas Gleixner <tglx@linutronix.de>,
	Steven Rostedt <rostedt@goodmis.org>
Cc: kvm@vger.kernel.org, Sean Christopherson <seanjc@google.com>,
	Paul Durrant <paul@xen.org>, Paolo Bonzini <pbonzini@redhat.com>,
	Michal Luczaj <mhal@rbox.co>, Paul Durrant <pdurrant@amazon.com>,
	David Woodhouse <dwmw@amazon.co.uk>,
	Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
	Peter Zijlstra <peterz@infradead.org>,
	Dave Hansen <dave.hansen@linux.intel.com>,
	"H. Peter Anvin" <hpa@zytor.com>,
	x86@kernel.org
Subject: Re: [PATCH v2 7/8] KVM: x86/xen: avoid blocking in hardirq context in kvm_xen_set_evtchn_fast()
Date: Tue, 27 Feb 2024 17:22:59 +0000	[thread overview]
Message-ID: <3A220705-5BEB-4247-96D0-9054E1164CCE@infradead.org> (raw)
In-Reply-To: <871q8xakql.ffs@tglx>

On 27 February 2024 17:18:58 GMT, Thomas Gleixner <tglx@linutronix.de> wrote:
>On Tue, Feb 27 2024 at 17:00, David Woodhouse wrote:
>> On 27 February 2024 14:43:26 GMT, Steven Rostedt <rostedt@goodmis.org> wrote:
>>>On Tue, 27 Feb 2024 11:49:21 +0000
>>>David Woodhouse <dwmw2@infradead.org> 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.