Re: [PATCHv2] efi/unaccepted: Fix soft lockups caused by parallel memory acceptance

[Michael Roth]

On Thu, Nov 02, 2023 at 04:56:11PM +0300, Kirill A. Shutemov wrote:
> On Tue, Oct 31, 2023 at 07:45:23PM -0500, Michael Roth wrote:
> > > If you mean the guest has as many cpus as the host provides to it, but you
> > > stress with many more than that number of processes, then I wonder how
> > 
> > Yes, this is what I meant. If there are more memory-hog worker threads in
> > the guest than there are vCPUs, I'm better able to reproduce soft-lockups.
> > That sort of makes sense since those threads will spend more time waiting on
> > an available vCPU to handle memory acceptance.
> > 
> > But it actually isn't a requirement, I've also been able to reproduce this
> > with equal numbers of worker threads and vCPUs if I run 4 VMs, each
> > running the stress/acceptance workload at the same time.
> > 
> > And if I force 4K pages in gmem backend (technically a supported
> > configuration) then I can reproduce it much more easily since the 2MB
> > acceptance path takes much longer and it makes it easier to expose any
> > potential remaining concurrency issues.
> 
> This all sounds like we are solidly in "system is overloaded" territory.
> 
> Soft-lockups are still not good in this case. But I am not sure what we
> can do about it.

After spending more time on it I'm starting to reach a similar conclusion,
but I'm not yet convinced it's so much the system being overloaded as it
is the handling for KVM_SET_MEMORY_ATTRIBUTES being particularly punishing
for this sort of workload and starving vCPUs for execution time due to
it causing MMU invalidations that cause #NPFs to need restarting and
frequent NMIs due KVM_REQ_TLB_FLUSH requests. For non-CoCo guests I think
this activity would be much more infrequent.

For instance here's the journey of a particular 4MB range that ends up
triggering a soft-lockup in the guest according to host-side ftraces (in
this case I've disabled the additional 2MB region that gets taken for
the zero-padding issue, and implemented the bug fix mentioned earlier,
so there vCPUs don't ever end up waiting on each other):

== Acceptance for 4MB GPA range 0x18cbc00000:18cc000000 ==

  <...>-1946910 [226] ...1. 324797.313982: kvm_page_fault: vcpu 219 rip 0x0 address 0x00000018cbc00000 error_code 0x500000004
  <...>-1946910 [098] ...1. 324797.631256: kvm_page_fault: vcpu 219 rip 0x0 address 0x00000018cbdff000 error_code 0x500000004
  <...>-1946910 [107] ...1. 324835.184044: kvm_page_fault: vcpu 219 rip 0x0 address 0x00000018cbe00000 error_code 0x500000004
  <...>-1946910 [235] ...1. 324835.208404: kvm_page_fault: vcpu 219 rip 0x0 address 0x00000018cbfff000 error_code 0x500000004

It's a pretty wild ride that spans 38s across 4 CPUs. I seem to get these
for 2 or 3 unlucky GPA ranges for each run and the other ranges stay
well below the soft-lockup threshold.

Maybe there are ways to improve on that situation, like accepting using
larger chunk sizes (which is sort of the opposite of what I was suggesting
earlier, but maybe when done to a degree that significantly batches
invalidations and KVM_REQ_TLB_FLUSH requests it becomes less of an issue to
have vCPUs waiting on each other).

> 
> One silly idea is to prevent all vCPUs to do accept simultaneously and
> reserve one (or several) to do housekeeping. The idea is that this vCPU
> can be preempted to do job on other tasks.

Maybe if larger chunk sizes / more batching does end up helping, a
worker thread/pool of this sort makes even more sense. But maybe there
are simpler ways to experiment with that.

> 
> It would only make a difference for PREEMPT_FULL case and if the
> housekeeping CPU will hit the accept path from preemptable context. And it
> is obviously not applicable if there's only single vCPU.
> 
> > > softlockups would happen due to the extra processes. Since irqs are disabled
> > > through the whole operation, the extra processes can't become scheduled, and
> > > not being scheduled due to overloading doesn't trigger softlockups, hmm...
> > 
> > The soft lock-ups happen as soon as IRQs are re-enabled, either:
> > 
> >   a) right after a thread sees that its range intersects something
> >      that's in the process of being accepted
> > 
> >   b) right after a thread finishes accepting its whole range and is
> >      about to return from accept_memory()
> > 
> > I see a) occur more in the 4K test scenario, b) is more difficult to
> > reproduce and seems to need a larger system to reproduce more reliably.
> 
> I am not sure why you differentiate these scenarios. Kernel just hits
> place where it can be preempted and observes that it is overdue to
> scheduling.

It just seemed like a) was more similar to the original issue of threads
becoming serialized on a few CPUs, but with the changes noted above to
completely decouple vCPUs from each other I was still able to trigger soft
lock-ups, but instead of a storm of lock-ups from vCPU threads suffering
secondary effects, these were purely lock-ups of type b), which
point to there ultimately being something on the host-side which was
causing all the threads to trip over themselves.

> 
> > The fact that b) seems to depend on larger systems sort of makes sense.
> > When we need to covert a page to private as part of accepting it, there
> > is a guest->host request that eventually goes off to host userspace which
> > will call the KVM ioctl KVM_SET_MEMORY_ATTRIBUTES to mark the memory as
> > private so that it will get faulted in from the guest_memfd backend. When
> > this happens, any guest page faults that are currently in flight will get
> > invalidated and require a retry, and there's also a guest TLB flush
> > that results in an NMI to all the cores the guest was scheduled on so that
> > it can exit and acknowledge new updates. So the higher the rate of
> > KVM_SET_MEMORY_ATTRIBUTES the system is able to process, the higher the
> > frequency of this sort of activity on the host side that can impact each
> > vCPUs ability to make progress on accepting a particular range.
> > 
> > Also I was running 4 guests, each with as many vCPUs as the host, so
> > contention for physical resources would probably be a factor as well.
> 
> Yeah, at some point you will just saturate memory bandwidth.
> 
> > I'm not sure what can be done about b), but they seem to be host-side
> > optimizations that aren't too relevant to this patch, and they seem to
> > occur less frequently than a), which seems to be more guest side.
> > 
> > Still not sure what is causing type a) lock-ups exactly, but through
> > various traces and debug statements I think I've at least gotten some idea
> > that there are certain conditions where the vCPUs become more and more
> > dependent on each other completing certain ranges, and they spend longer
> > and longer amounts of time looping through the accepting_list.
> > 
> > There are 3 things I've noticed that might lead to vCPUs getting hung up
> > on each other:
> > 
> >  1) try_to_accept_memory_one() calls accept_page(page, MAX_ORDER), which
> >     is a 4MB range
> 
> This should not make one vCPU to setup on work on another. Page allocator
> owns full 4MB. It is not shared with anyone.

Indeed, with 2) and 3) addressed there no longer seem to be any
dependencies between threads.

> 
> >  2) There's an extra 2MB region taken after each unit to account for
> >     load_unaligned_zeropad() 
> 
> Okay, yes, this is true.
> 
> >  3) There is what appears to be a bug here:
> > 
> >         list_for_each_entry(entry, &accepting_list, list) {
> >                 if (entry->end < range.start)
> >                         continue;
> >                 if (entry->start >= range.end)
> >                         continue;
> > 
> >     where if entry->end == range.start, the thread will wait on the owner
> >     of that range even though it doesn't actually intersect.
> 
> Good catch. Care to send a patch?

Sure, I will get that posted by tomorrow after a bit more testing.

> 
> > I don't quite know how all this lines up to a dependency chain that would
> > potentially explain the lock-ups, but to mitigate that scenario, I tried only
> > adding the specific 2MB range that is being accepted to accepting_list, rather
> > than the whole range, and then just iterate through 2MB at a time in
> > accept_memory() instead of passing the larger range on to arch_accept_memory().
> 
> This might improve situation with soft lockups a bit, but would hurt
> accept bandwidth.

Yah, I think it was helpful for getting rid of some noise and getting a
better idea of the main source of the bottleneck, but the underlying issue
still remains even with these changes in place. 

I'll continue to experiment with it, but it makes me feel better at least
that there isn't something strange going on with the current guest-side
implementation.

Thanks,

Mike

> 
> > That seems to have resolved the soft lock-ups for the forced-4K scenario, but
> > I haven't had much time to test larger configurations yet.
> 
> -- 
>   Kiryl Shutsemau / Kirill A. Shutemov