Re: [PATCH 17/20] include: Define constants for parallel save/restore
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>
>
> 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>
> Signed-off-by: Jim Fehlig <jfehlig(a)suse.com>
> ---
> 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.
Thanks,
Claudio
> +
> /* See below for virDomainSaveImageXMLFlags */
> char * virDomainSaveImageGetXMLDesc (virConnectPtr conn,
> const char *file,
> diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
> index 5aedae1b49..32508a4ede 100644
> --- a/src/libvirt-domain.c
> +++ b/src/libvirt-domain.c
> @@ -1010,6 +1010,9 @@ virDomainSaveFlags(virDomainPtr domain, const char *to,
> * If VIR_DOMAIN_SAVE_PARAM_FILE is not provided then a managed save is
> * performed (see virDomainManagedSave).
> *
> + * See VIR_DOMAIN_SAVE_PARAM_* for detailed description of accepted save
> + * parameters.
> + *
> * Returns 0 in case of success and -1 in case of failure.
> *
> * Since: 8.4.0
> @@ -1222,6 +1225,9 @@ virDomainRestoreFlags(virConnectPtr conn, const char *from,
const char *dxml,
> * now, VIR_DOMAIN_SAVE_PARAM_FILE is required but this requirement may
> * be lifted in the future.
> *
> + * See VIR_DOMAIN_SAVE_PARAM_* for detailed description of accepted
> + * restore parameters.
> + *
> * Returns 0 in case of success and -1 in case of failure.
> *
> * Since: 8.4.0
> --
> 2.35.3
>
With regards,
Daniel