Re: [PATCH 17/20] include: Define constants for parallel save/restore

On 10/14/24 11:42, Daniel P. Berrangé wrote: > On Mon, Oct 14, 2024 at 06:00:53PM +0200, Claudio Fontana wrote: >> On 10/10/24 15:29, Daniel P. Berrangé wrote: >>> On Thu, Aug 08, 2024 at 05:38:10PM -0600, Jim Fehlig via Devel wrote: >>>> From: Claudio Fontana <cfontana(a)suse.de&gt; >>>> >>>> Add a new VIR_DOMAIN_SAVE_PARALLEL flag to the save and restore >>>> APIs, which can be used to specify the use of multiple, parallel >>>> channels for saving a domain. The number of parallel channels >>>> can be set using the VIR_DOMAIN_SAVE_PARAM_PARALLEL_CONNECTIONS >>>> typed parameter. >>>> >>>> Signed-off-by: Claudio Fontana <cfontana(a)suse.de&gt; >>>> Signed-off-by: Jim Fehlig <jfehlig(a)suse.com&gt; >>>> --- >>>> include/libvirt/libvirt-domain.h | 13 +++++++++++++ >>>> src/libvirt-domain.c | 6 ++++++ >>>> 2 files changed, 19 insertions(+) >>>> >>>> diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h >>>> index 4266237abe..f4bf9c3cdb 100644 >>>> --- a/include/libvirt/libvirt-domain.h >>>> +++ b/include/libvirt/libvirt-domain.h >>>> @@ -1596,6 +1596,7 @@ typedef enum { >>>> VIR_DOMAIN_SAVE_RUNNING = 1 << 1, /* Favor running over paused (Since: 0.9.5) */ >>>> VIR_DOMAIN_SAVE_PAUSED = 1 << 2, /* Favor paused over running (Since: 0.9.5) */ >>>> VIR_DOMAIN_SAVE_RESET_NVRAM = 1 << 3, /* Re-initialize NVRAM from template (Since: 8.1.0) */ >>>> + VIR_DOMAIN_SAVE_PARALLEL = 1 << 4, /* Save and restore using parallel channels (Since: 10.6.0) */ >>>> } virDomainSaveRestoreFlags; >>>> >>>> int virDomainSave (virDomainPtr domain, >>>> @@ -1641,6 +1642,18 @@ int virDomainRestoreParams (virConnectPtr conn, >>>> */ >>>> # define VIR_DOMAIN_SAVE_PARAM_DXML "dxml" >>>> >>>> +/** >>>> + * VIR_DOMAIN_SAVE_PARAM_PARALLEL_CONNECTIONS: >>>> + * >>>> + * this optional parameter mirrors the migration parameter >>>> + * VIR_MIGRATE_PARAM_PARALLEL_CONNECTIONS. >>>> + * >>>> + * It specifies the number of extra connections to use when transfering state. >>>> + * >>>> + * Since: 10.6.0 >>>> + */ >>>> +# define VIR_DOMAIN_SAVE_PARAM_PARALLEL_CONNECTIONS "parallel.connections" >>> >>> "CONNECTIONS" is perhaps the wrong name, as that's kinda an impl >> >> I would suggest CONNECTIONS it is the correct name. >> >>> detail. How about VIR_MIGRATE_PARAM_PARALLEL_CPUS to reflect >>> the number of CPUs we're going to burn time on. >> >> This is another aspect of it, but I would suggest that _this_ is much more an implementation detail (inside qemu) on how the parallel connection is actually managed. >> Maybe in the future there will be a pool of cpus serving this, or another optional way to serve the connections. >> >> Considering the fact that libvirt _already has_ for regular migrations >> >> {.typedParam = VIR_MIGRATE_PARAM_PARALLEL_CONNECTIONS, >> .param = QEMU_MIGRATION_PARAM_MULTIFD_CHANNELS, >> .party = QEMU_MIGRATION_SOURCE | QEMU_MIGRATION_DESTINATION}, >> >> which controls exactly the same QEMU parameter (ie the multifd-channels), >> using anything else than --parallel-connections is in my opinion very surprising for the users (which would know what to expect from the same command line option), >> and honestly simply absurd. > > This is comparing two different scenarios which work in varying ways. > > In the case of live migration there are network connections that

> we're controlling. So calling the parameter "parallel connections" > is contextually relevant, though we could equally have called > it "parallel vcpus" - depends whether you're thinking about multifd > as being there to max out TCP connections, or to max our local CPU > usage. > > In the case of migrating to a file though, there is no concept > of network connections at all. QEMU is opening a file and directly > writing to that file from multiple threads. Calling the parameter > 'parallel connections' makes no sense in this context, as there > are no connections in use. "parallel CPUs" reflects that we're > consuming multiple CPUs to write out data, though we could also > call it "parallel threads" for similar reasons.

I can change this to PARALLEL_CPUS if really desired, but there's something about it that doesn't feel right. It's hard to put to words, probably because the reasons are mostly subjective. Claudio already raised the objective ones. I do prefer PARALLEL_THREADS over PARALLEL_CPUS. How about reusing the name already selected by QEMU and going with PARALLEL_CHANNELS?

Regards, Jim