Re: [PATCH] migration: Remove interface query-migrationthreads

[Daniel P . Berrangé]

On Mon, Oct 14, 2024 at 11:22:21AM -0300, Fabiano Rosas wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > On Fri, Oct 11, 2024 at 11:34:17AM -0400, Peter Xu wrote:
> >> This reverts two commits:
> >> 
> >> 671326201dac8fe91222ba0045709f04a8ec3af4
> >> 1b1f4ab69c41279a45ccd0d3178e83471e6e4ec1
> >> 
> >> Meanwhile it adds an entry to removed-features.rst for the
> >> query-migrationthreads QMP command.
> >> 
> >> This patch originates from another patchset [1] that wanted to cleanup the
> >> interface and add corresponding HMP command, as lots of things are missing
> >> in the query report; so far it only reports the main thread and multifd
> >> sender threads; all the rest migration threads are not reported, including
> >> multifd recv threads.
> >> 
> >> As pointed out by Dan in the follow up discussions [1], the API is designed
> >> in an awkward way where CPU pinning may not cover the whole lifecycle of
> >> even the thread being reported.  When asked, we also didn't get chance to
> >> hear from the developer who introduced this feature to explain how this API
> >> can be properly used.
> >> 
> >> OTOH, this feature from debugging POV isn't very helpful either, as all
> >> these information can be easily obtained by GDB.  Esepcially, if with
> >> "-name $VM,debug-threads=on" we do already have names for each migration
> >> threads (which covers more than multifd sender threads).
> >> 
> >> So it looks like the API isn't helpful in any form as of now, besides it
> >> only adds maintenance burden to migration code, even if not much.
> >> 
> >> Considering that so far there's totally no justification on how to use this
> >> interface correctly, let's remove this interface instead of cleaning it up.
> >> 
> >> In this special case, we even go beyond normal deprecation procedure,
> >> because a deprecation process would only make sense when there are existing
> >> users. In this specific case, we expect zero serious users with this API.
> >
> > We have no way of knowing whether there are existing users of this, or
> > any other feature in QEMU. This is why we have a formal deprecation
> > period, rather than immediately deleting existing features.
> >
> > Yes, there are plenty of reasons why this feature is sub-optimal, but
> > it is not broken to the extent that it is *impossible* for people to
> > be using it.
> >
> > IOW, I don't see that there's anything special here to justify bypassing
> > our deprecation process here.
> 
> I have no dog in this race, but as a data point, I see that this was
> submitted to libvirt as a new migrationpin command:
> 
> https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/FVNAUEVIMLG6F2VCRKHZDUEOLBJCXQHO/#BVEGJVZMMLQMXE263GO5BSIWUDIYIFZU

And unforunately it seems we dropped the ball on reviewing the v2 of
their series and they never ping'd for a response, so this was not
merged. Possibly they're just running it as a local patch to libvirt...


With 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 :|