Re: [PATCH v3 00/24] Multifd 🔀 device state transfer support with VFIO consumer

[Peter Xu <peterx@redhat.com>]

On Fri, Dec 06, 2024 at 07:03:36PM +0100, Maciej S. Szmigiero wrote:
> On 4.12.2024 20:10, Peter Xu wrote:
> > On Sun, Nov 17, 2024 at 08:19:55PM +0100, Maciej S. Szmigiero wrote:
> > > Important note:
> > > 4 VF benchmarks were done with commit 5504a8126115
> > > ("KVM: Dynamic sized kvm memslots array") and its revert-dependencies
> > > reverted since this seems to improve performance in this VM config if the
> > > multifd transfer is enabled: the downtime performance with this commit
> > > present is 1141 ms enabled / 1730 ms disabled.
> > > 
> > > Smaller VF counts actually do seem to benefit from this commit, so it's
> > > likely that in the future adding some kind of a memslot pre-allocation
> > > bit stream message might make sense to avoid this downtime regression for
> > > 4 VF configs (and likely higher VF count too).
> > 
> > I'm confused why revert 5504a8126115 could be faster, and it affects as
> > much as 600ms.  Also how that effect differs can relevant to num of VFs.
> > 
> > Could you share more on this regression?  Because if that's problematic we
> > need to fix it, or upstream QEMU (after this series merged) will still not
> > work.
> > 
> 
> The number of memslots that the VM uses seems to differ depending on its
> VF count, each VF using 2 memslots:
> 2 VFs, used slots: 13
> 4 VFs, used slots: 17
> 5 VFs, used slots: 19

It's still pretty less.

> 
> So I suspect this performance difference is due to these higher counts
> of memslots possibly benefiting from being preallocated on the previous
> QEMU code (before commit 5504a8126115).
> 
> I can see that with this commit:
> > #define  KVM_MEMSLOTS_NR_ALLOC_DEFAULT                      16
> 
> So it would explain why the difference is visible on 4 VFs only (and
> possibly higher VF counts, just I don't have an ability to test migrating
> it) since with 4 VF configs we exceed KVM_MEMSLOTS_NR_ALLOC_DEFAULT.

I suppose it means kvm_slots_grow() is called once, but I don't understand
why it caused 500ms downtime!

Not to mention, that patchset should at least reduce downtime OTOH due to
the small num of slots, because some of the dirty sync / clear path would
need to walk the whole slot array (our lookup is pretty slow for now, but
probably no good reason to rework it yet if it's mostly 10-20).

In general, I would still expect that dynamic memslot work to speedup
(instead of slowing down) VFIO migrations.

There's something off here, or something I overlooked.  I suggest we figure
it out..  Even if we need to revert the kvm series on master, but I so far
doubt it.

Otherwise we should at least report the number with things on the master
branch, and we evaluate merging this series with that real number, because
fundamentally that's the numbers people will get when start using this
feature on master later.

-- 
Peter Xu