On 27 February 2024 17:25:52 GMT, Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:
>On Tue, 27 Feb 2024 17:00:42 +0000
>David Woodhouse <dwmw2@xxxxxxxxxxxxx> 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).
>
>Yes, but if the trylock fails, you then call:
>
>+ if (!read_trylock(&gpc->lock)) {
>+ /*
>+ * When PREEMPT_RT turns locks into mutexes, rwlocks are
>+ * turned into mutexes and most interrupts are threaded.
>+ * But timer events may be delivered in hardirq mode due
>+ * to using HRTIMER_MODE_ABS_HARD. So bail to the slow
>+ * path if the trylock fails in interrupt context.
>+ */
>+ if (in_interrupt())
>+ goto out;
>+
>+ read_lock(&gpc->lock);
>
>That takes the read_lock() with interrupts still disabled. On RT, that will
>likely schedule as it had just failed a trylock.
Oh... pants. Right, so it really does need to use read_trylock_irqsave() in the non-IRQ case, *and* it needs to either read_unlock_irqrestore() or separately unlock and local_irq_restore(), according to which way it did the locking in the first place. Ick.
