Re: [PATCH] KVM: arm64: vgic: Fix soft lockup during VM teardown

[Marc Zyngier]

[dropping the now dead old kvmarm list]

On Wed, 18 Jan 2023 19:24:01 +0000,
Shanker Donthineni <sdonthineni@xxxxxxxxxx> wrote:
> 
> 
> 
> On 1/18/23 05:54, Marc Zyngier wrote:
> > External email: Use caution opening links or attachments
> > 
> > 
> > Shanker,
> > 
> > Please Cc all the KVM/arm64 reviewers when sending KVM/arm64 patches.
> > 
> > On Wed, 18 Jan 2023 02:23:48 +0000,
> > Shanker Donthineni <sdonthineni@xxxxxxxxxx> wrote:
> >> 
> >> Getting intermittent CPU soft lockups during the virtual machines
> >> teardown on a system with GICv4 features enabled. The function
> >> __synchronize_hardirq() has been waiting for IRQD_IRQ_INPROGRESS
> >> to be cleared forever as per the current implementation.
> >> 
> >> CPU stuck here for a long time leads to soft lockup:
> >>    while (irqd_irq_inprogress(&desc->irq_data))
> >>        cpu_relax();
> > 
> > Is it a soft-lockup from which the system recovers? or a livelock
> > which leaves the system dead?
> > 
> The system is not recovering, did a power cycle to recover.

This isn't a soft-lockup then. This is at best a livelock.

> > Are these two traces an indication of concurrent events? Or are they
> > far apart?
> > 
> Concurrent.

So you can see the VM being torn down while the vgic save sequence is
still in progress?

If you can actually see that, then this is a much bigger bug than the
simple race you are describing, and we're missing a reference on the
kvm structure. This would be a *MAJOR* bug.

Please post the full traces, not snippets. The absolutely full kernel
log, the configuration, what you run, how you run it, *EVERYTHING*. I
need to be able to reproduce this.

> 
> >> 
> >> irqreturn_t handle_irq_event(struct irq_desc *desc)
> >> {
> >>      irqd_set(&desc->irq_data, IRQD_IRQ_INPROGRESS);
> >>      raw_spin_unlock(&desc->lock);
> >> 
> >>      ret = handle_irq_event_percpu(desc);
> >> 
> >>      raw_spin_lock(&desc->lock);
> >>      irqd_clear(&desc->irq_data, IRQD_IRQ_INPROGRESS);
> >> }
> > 
> > How is that relevant to this trace? Do you see this function running
> > concurrently with the teardown? If it matters here, it must be a VPE
> > doorbell, right? But you claim that this is on a GICv4 platform, while
> > this would only affect GICv4.1... Or are you using GICv4.1?
> > 
> handle_irq_event() is running concurrently with irq_domain_activate_irq()
> which happens before free_irq() called. Corruption at [78.983544] and
> teardown started at [87.360891].

But that doesn't match the description you made of concurrent
events. Does it take more than 9 seconds for the vgic state to be
saved to memory?

> 
> [   78.983544] irqd_set_activated: lost IRQD_IRQ_INPROGRESS old=0x10401400, new=0x10441600
> 
> [   87.360891]  __synchronize_hardirq+0x48/0x140
> 
> Yes, I'm using GICv4.1, used these 2 functions to trace the issue.

Then *please* be precise in your descriptions. You send people in the
wrong direction.

[...]

> I ran stress test launch/teardown multiple VMs for 3hrs. The issue
> is not reproducible. The same test fails in 10-30min without code
> changes.

That doesn't add up with the earlier description of concurrent events,
with the VM teardown and the vgic saving running in parallel. My patch
doesn't prevent this.

So either your testing is insufficient, or your description of
concurrent events is inaccurate.

	M.

-- 
Without deviation from the norm, progress is not possible.