[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] migration: Remove interface query-migrationthreads
From: |
Daniel P . Berrangé |
Subject: |
Re: [PATCH] migration: Remove interface query-migrationthreads |
Date: |
Mon, 14 Oct 2024 15:44:13 +0100 |
User-agent: |
Mutt/2.2.12 (2023-09-09) |
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 :|