Re: [PATCH v2 00/25] migration: Postcopy Preemption

[Daniel P . Berrangé]

On Tue, Mar 01, 2022 at 04:39:00PM +0800, Peter Xu wrote:
> This is v2 of postcopy preempt series.  It can also be found here:
> 
>   https://github.com/xzpeter/qemu/tree/postcopy-preempt
> 
> RFC: 
> https://lore.kernel.org/qemu-devel/20220119080929.39485-1-peterx@redhat.com
> V1:  
> https://lore.kernel.org/qemu-devel/20220216062809.57179-1-peterx@redhat.com
> 
> v1->v2 changelog:
> - Picked up more r-bs from Dave
> - Rename both fault threads to drop "qemu/" prefix [Dave]
> - Further rework on postcopy recovery, to be able to detect qemufile errors
>   from either main channel or postcopy one [Dave]
> - shutdown() qemufile before close on src postcopy channel when postcopy is
>   paused [Dave]
> - In postcopy_preempt_new_channel(), explicitly set the new channel in
>   blocking state, even if it's the default [Dave]
> - Make RAMState.postcopy_channel unsigned int [Dave]
> - Added patches:
>   - "migration: Create the postcopy preempt channel asynchronously"
>   - "migration: Parameter x-postcopy-preempt-break-huge"
>   - "migration: Add helpers to detect TLS capability"
>   - "migration: Fail postcopy preempt with TLS"
>   - "tests: Pass in MigrateStart** into test_migrate_start()"
> 
> Abstract
> ========
> 
> This series added a new migration capability called "postcopy-preempt".  It 
> can
> be enabled when postcopy is enabled, and it'll simply (but greatly) speed up
> postcopy page requests handling process.

Is there no way we can just automatically enable this new feature, rather
than requiring apps to specify yet another new flag ?

> TODO List
> =========
> 
> TLS support
> -----------
> 
> I only noticed its missing very recently.  Since soft freeze is coming, and
> obviously I'm still growing this series, so I tend to have the existing
> material discussed. Let's see if it can still catch the train for QEMU 7.0
> release (soft freeze on 2022-03-08)..

I don't like the idea of shipping something that is only half finished.
It means that when apps probe for the feature, they'll see preempt
capability present, but have no idea whether they're using a QEMU that
is broken when combined with TLS or not. We shouldn't merge something
just to meet the soft freeze deadline if we know key features are broken.

> Multi-channel for preemption threads
> ------------------------------------
> 
> Currently the postcopy preempt feature use only one extra channel and one
> extra thread on dest (no new thread on src QEMU).  It should be mostly good
> enough for major use cases, but when the postcopy queue is long enough
> (e.g. hundreds of vCPUs faulted on different pages) logically we could
> still observe more delays in average.  Whether growing threads/channels can
> solve it is debatable, but sounds worthwhile a try.  That's yet another
> thing we can think about after this patchset lands.

If we don't think about it upfront, then we'll possibly end up with
yet another tunable flag that apps have to worry about. It also
could make migration code even more complex if we have to support
two different scenarios. If we think multiple threads are goign to
be a benefit lets check that and if so, design it into the exposed
application facing interface from the start rather than retrofitting
afterwards.

> Logically the design provides space for that - the receiving postcopy
> preempt thread can understand all ram-layer migration protocol, and for
> multi channel and multi threads we could simply grow that into multile
> threads handling the same protocol (with multiple PostcopyTmpPage).  The
> source needs more thoughts on synchronizations, though, but it shouldn't
> affect the whole protocol layer, so should be easy to keep compatible.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|