On 2/17/25 23:13, Maciej S. Szmigiero wrote:
On 17.02.2025 10:38, Cédric Le Goater wrote:
On 2/14/25 21:55, Maciej S. Szmigiero wrote:
On 12.02.2025 11:55, Cédric Le Goater wrote:
On 1/30/25 11:08, Maciej S. Szmigiero wrote:
From: "Maciej S. Szmigiero" <maciej.szmigi...@oracle.com>
Add support for VFIOMultifd data structure that will contain most of the
receive-side data together with its init/cleanup methods.
Signed-off-by: Maciej S. Szmigiero <maciej.szmigi...@oracle.com>
---
hw/vfio/migration.c | 52 +++++++++++++++++++++++++++++++++--
include/hw/vfio/vfio-common.h | 5 ++++
2 files changed, 55 insertions(+), 2 deletions(-)
diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
index 3211041939c6..bcdf204d5cf4 100644
--- a/hw/vfio/migration.c
+++ b/hw/vfio/migration.c
@@ -300,6 +300,9 @@ typedef struct VFIOStateBuffer {
size_t len;
} VFIOStateBuffer;
+typedef struct VFIOMultifd {
+} VFIOMultifd;
+
static void vfio_state_buffer_clear(gpointer data)
{
VFIOStateBuffer *lb = data;
@@ -398,6 +401,18 @@ static int vfio_load_device_config_state(QEMUFile *f, void
*opaque)
return qemu_file_get_error(f);
}
+static VFIOMultifd *vfio_multifd_new(void)
+{
+ VFIOMultifd *multifd = g_new(VFIOMultifd, 1);
+
+ return multifd;
+}
+
+static void vfio_multifd_free(VFIOMultifd *multifd)
+{
+ g_free(multifd);
+}
+
static void vfio_migration_cleanup(VFIODevice *vbasedev)
{
VFIOMigration *migration = vbasedev->migration;
@@ -785,14 +800,47 @@ static void vfio_save_state(QEMUFile *f, void *opaque)
static int vfio_load_setup(QEMUFile *f, void *opaque, Error **errp)
{
VFIODevice *vbasedev = opaque;
+ VFIOMigration *migration = vbasedev->migration;
+ int ret;
+
+ /*
+ * Make a copy of this setting at the start in case it is changed
+ * mid-migration.
+ */
+ if (vbasedev->migration_multifd_transfer == ON_OFF_AUTO_AUTO) {
+ migration->multifd_transfer = vfio_multifd_transfer_supported();
Attribute "migration->multifd_transfer" is not necessary. It can be
replaced by a small inline helper testing pointer migration->multifd
and this routine can use a local variable instead.
It's necessary for the send side since it does not need/allocate VFIOMultifd
at migration->multifd, so this (receive) side can use it for commonality too.
Hmm, we can allocate migration->multifd on the send side too, even
if the attributes are unused and it is up to vfio_multifd_free() to
make the difference between the send/recv side.
Allocating an unnecessary VFIOMultifd structure that has 12 members,
some of them complex like QemuThread, QemuCond or QemuMutex, just
to avoid having one extra bool variable (migration_multifd_transfer or
whatever it ends being named) seem like a poor trade-off for me.
Something that is bothering me is the lack of introspection tools
and statistics. What could be possibly added under VFIOMultifd and
VfioStats ?
There's already VFIO bytes transferred counter and also a
multifd bytes transferred counter.
There are quite a few trace events (both existing and newly added
by this patch).
While even more statistics and traces may help with tuning/debugging
in some cases that's something easily added in the future.
I don't think the '_transfer' suffix adds much to the understanding.
The migration->multifd was already taken by VFIOMultifd struct, but
it could use other name (migration->multifd_switch? migration->multifd_on?).
yeah. Let's try to get rid of it first.
+ } else {
+ migration->multifd_transfer =
+ vbasedev->migration_multifd_transfer == ON_OFF_AUTO_ON;
+ }
+
+ if (migration->multifd_transfer && !vfio_multifd_transfer_supported()) {
+ error_setg(errp,
+ "%s: Multifd device transfer requested but unsupported in the
current config",
+ vbasedev->name);
+ return -EINVAL;
+ }
The above checks are also introduced in vfio_save_setup(). Please
implement a common routine vfio_multifd_is_enabled() or some other
name.
Done (as common vfio_multifd_transfer_setup()).
vfio_multifd_is_enabled() please, returning a bool.
Functions named *_is_something() normally just check some conditions
and return a computed value without having any side effects.
Here, vfio_multifd_transfer_setup() also sets migration->multifd_transfer
appropriately (or could migration->multifd) - that's common code for
save and load.
I guess you meant to move something else rather than this block
of code into vfio_multifd_is_enabled() - see my answer below.
vfio_migration_cleanup(vbasedev);
trace_vfio_load_cleanup(vbasedev->name);
diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
index 153d03745dc7..c0c9c0b1b263 100644
--- a/include/hw/vfio/vfio-common.h
+++ b/include/hw/vfio/vfio-common.h
@@ -61,6 +61,8 @@ typedef struct VFIORegion {
uint8_t nr; /* cache the region number for debug */
} VFIORegion;
+typedef struct VFIOMultifd VFIOMultifd;
+
typedef struct VFIOMigration {
struct VFIODevice *vbasedev;
VMChangeStateEntry *vm_state;
@@ -72,6 +74,8 @@ typedef struct VFIOMigration {
uint64_t mig_flags;
uint64_t precopy_init_size;
uint64_t precopy_dirty_size;
+ bool multifd_transfer;
+ VFIOMultifd *multifd;
bool initial_data_sent;
bool event_save_iterate_started;
@@ -133,6 +137,7 @@ typedef struct VFIODevice {
bool no_mmap;
bool ram_block_discard_allowed;
OnOffAuto enable_migration;
+ OnOffAuto migration_multifd_transfer;
This property should be added at the end of the series, with documentation,
and used in the vfio_multifd_some_name() routine I mentioned above.
The property behind this variable *is* in fact introduced at the end of the
series -
in a commit called "vfio/migration: Add x-migration-multifd-transfer VFIO
property"
after which there are only commits adding the related compat entry and a VFIO
developer doc update.
The variable itself needs to be introduced earlier since various newly
introduced code blocks depend on its value to only get activated when multifd
transfer is enabled.
Not if you introduce a vfio_multifd_is_enabled() routine hiding
the details. In that case, the property and attribute can be added
at the end of the series and you don't need to add the attribute
earlier.
The part above that you wanted to be moved into vfio_multifd_is_enabled()
is one-time check for load or save setup time.
That's *not* the switch to be tested by other parts of the code
during the migration process to determine whether multifd transfer
is in use.
If you want vfio_multifd_is_enabled() to be that switch that's tested by
other parts of the VFIO migration code then it will finally consist of
just a single line of code:
"return migration->multifd_transfer" (or "return migration->multifd").
Then indeed the variable could be introduced with the property than
controls it, but a dummy vfio_multifd_is_enabled() will need to be
introduced earlier as "return false" to not break the build.
Sorry but I have switched to another series now, the one for live update,
and I will recheck your v5 proposal.
Thanks,
C.
