Hi Avihai,
On 31.10.2024 15:21, Avihai Horon wrote:
Hi Maciej,
On 29/10/2024 16:58, Maciej S. Szmigiero wrote:
External email: Use caution opening links or attachments
From: "Maciej S. Szmigiero" <maciej.szmigi...@oracle.com>
This way both the start and end points of migrating a particular VFIO
device are known.
Add also a vfio_save_iterate_empty_hit trace event so it is known when
there's no more data to send for that device.
Signed-off-by: Maciej S. Szmigiero <maciej.szmigi...@oracle.com>
---
hw/vfio/migration.c | 13 +++++++++++++
hw/vfio/trace-events | 3 +++
include/hw/vfio/vfio-common.h | 3 +++
3 files changed, 19 insertions(+)
diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
index 992dc3b10257..1b1ddf527d69 100644
--- a/hw/vfio/migration.c
+++ b/hw/vfio/migration.c
@@ -472,6 +472,9 @@ static int vfio_save_setup(QEMUFile *f, void *opaque, Error
**errp)
return -ENOMEM;
}
+ migration->save_iterate_started = false;
+ migration->save_iterate_empty_hit = false;
+
if (vfio_precopy_supported(vbasedev)) {
switch (migration->device_state) {
case VFIO_DEVICE_STATE_RUNNING:
@@ -602,9 +605,17 @@ static int vfio_save_iterate(QEMUFile *f, void *opaque)
VFIOMigration *migration = vbasedev->migration;
ssize_t data_size;
+ if (!migration->save_iterate_started) {
+ trace_vfio_save_iterate_started(vbasedev->name);
+ migration->save_iterate_started = true;
+ }
+
data_size = vfio_save_block(f, migration);
if (data_size < 0) {
return data_size;
+ } else if (data_size == 0 && !migration->save_iterate_empty_hit) {
+ trace_vfio_save_iterate_empty_hit(vbasedev->name);
+ migration->save_iterate_empty_hit = true;
}
Can we instead use trace_vfio_save_iterate to understand if the device reached
0?
AFAIK there's not way to filter trace events by their parameters,
like only logging vfio_save_iterate trace event if both parameters
are zero.
It means that vfio_save_iterate has to be enabled unconditionally to
serve as a replacement for vfio_save_iterate_empty_hit, which could
result in it being logged/emitted many extra times (with non-zero
parameters).
Because of that I think having a dedicated trace event for such
occasion makes sense (it is also easily grep-able).
In any case, I think the above could fit better in vfio_save_block(), where
ENOMSG indicates that the device has no more data to send during pre-copy phase:
...
if (data_size < 0) {
/*
* Pre-copy emptied all the device state for now. For more information,
* please refer to the Linux kernel VFIO uAPI.
*/
if (errno == ENOMSG) {
trace_vfio_save_iterate_empty_hit(vbasedev->name) <--------------- move it here
return 0;
}
return -errno;
}
...
If you move the trace there, maybe renaming it to
trace_vfio_precopy_empty_hit() will be more accurate?
This move and rename seems sensible to me.
And trying to avoid adding the extra VFIOMigration->save_iterate_empty_hit
flag, can we simply trace it every time?
Will have to do some tests to be sure but if there's possibility that
we get ENOMSG many times then obviously we don't want to flood logs with
this trace event in this case - we want to only log the
"data present" -> "data not present" edge/change.
vfio_update_estimated_pending_data(migration, data_size);
@@ -630,6 +641,8 @@ static int vfio_save_complete_precopy(QEMUFile *f, void
*opaque)
int ret;
Error *local_err = NULL;
+ trace_vfio_save_complete_precopy_started(vbasedev->name);
I assume this trace is used to measure how long it takes for
vfio_save_complete_precopy() to run? If so, can we use
trace_vmstate_downtime_save to achieve the same goal?
With an appropriate filtering I guess it could be used to
extract the same data, although explicit VFIO trace point
makes it easier to just look at these traces directly
(less noise from other devices).
But at the same time trace_vfio_save_complete_precopy
already exists and by this metric it would also be
unnecessary.
Thanks.
Thanks,
Maciej
