Re: [PATCH v2] vfio/migration: Add save_{iterate,complete_precopy}_started trace events

["Cédric Le Goater" <clg@redhat.com>]

Hello Maciej,

On 11/4/24 15:58, Maciej S. Szmigiero wrote:
> From: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com>
> 
> This way both the start and end points of migrating a particular VFIO
> device are known.
> 
> Add also a vfio_precopy_empty_hit trace event so it is known when
> there's no more data to send for that device.

Would you mind splitting the patch in 2, one patch for each event flag
added ?

> Signed-off-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
> ---
> 
> This is just the lone remaining functionality-affecting patch from this
> series of 4 trivial patches for QEMU 9.2:
> https://lore.kernel.org/qemu-devel/cover.1730203967.git.maciej.szmigiero@oracle.com/
> Two other such patches were already queued and the fourth one is only
> an annotation in a code comment block.
> 
> Changes from the v1 that was posted as a part of the above series:
> * Move the vfio_save_iterate_empty_hit trace event to vfio_save_block(),
> trigger it on ENOMSG errno and rename it to vfio_precopy_empty_hit.
> 
> * Re-arm the above trace event if we see another data read so not only
> the first "data present" -> "data not present" edge is logged.
> 
>   hw/vfio/migration.c           | 17 +++++++++++++++++
>   hw/vfio/trace-events          |  3 +++
>   include/hw/vfio/vfio-common.h |  3 +++
>   3 files changed, 23 insertions(+)
> 
> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
> index 992dc3b10257..e7b81f99e595 100644
> --- a/hw/vfio/migration.c
> +++ b/hw/vfio/migration.c
> @@ -370,6 +370,10 @@ static ssize_t vfio_save_block(QEMUFile *f, VFIOMigration *migration)
>            * please refer to the Linux kernel VFIO uAPI.
>            */
>           if (errno == ENOMSG) {
> +            if (!migration->precopy_empty_hit) {
> +                trace_vfio_precopy_empty_hit(migration->vbasedev->name);


There is a kind of implicit rule that is to keep the name of the
routine in the trace event. This is true for this file at least.
In this regard, could you please rename the event to :

	vfio_save_block_precopy_empty_hit
or
	vfio_save_block_precopy_empty

as you wish.

> +                migration->precopy_empty_hit = true;
> +            }
>               return 0;
>           }
>   
> @@ -379,6 +383,9 @@ static ssize_t vfio_save_block(QEMUFile *f, VFIOMigration *migration)
>           return 0;
>       }
>   
> +    /* Non-empty read -> re-arm the trace event */
> +    migration->precopy_empty_hit = false;
> +
>       qemu_put_be64(f, VFIO_MIG_FLAG_DEV_DATA_STATE);
>       qemu_put_be64(f, data_size);
>       qemu_put_buffer(f, migration->data_buffer, data_size);
> @@ -472,6 +479,9 @@ static int vfio_save_setup(QEMUFile *f, void *opaque, Error **errp)
>           return -ENOMEM;
>       }
>   
> +    migration->save_iterate_started = false;
> +    migration->precopy_empty_hit = false;
> +
>       if (vfio_precopy_supported(vbasedev)) {
>           switch (migration->device_state) {
>           case VFIO_DEVICE_STATE_RUNNING:
> @@ -602,6 +612,11 @@ 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;
> @@ -630,6 +645,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);
> +
>       /* We reach here with device state STOP or STOP_COPY only */
>       ret = vfio_migration_set_state(vbasedev, VFIO_DEVICE_STATE_STOP_COPY,
>                                      VFIO_DEVICE_STATE_STOP, &local_err);
> diff --git a/hw/vfio/trace-events b/hw/vfio/trace-events
> index 29789e8d276d..d5277cb7697a 100644
> --- a/hw/vfio/trace-events
> +++ b/hw/vfio/trace-events
> @@ -156,11 +156,14 @@ vfio_migration_realize(const char *name) " (%s)"
>   vfio_migration_set_device_state(const char *name, const char *state) " (%s) state %s"
>   vfio_migration_set_state(const char *name, const char *new_state, const char *recover_state) " (%s) new state %s, recover state %s"
>   vfio_migration_state_notifier(const char *name, int state) " (%s) state %d"
> +vfio_precopy_empty_hit(const char *name) " (%s)"
>   vfio_save_block(const char *name, int data_size) " (%s) data_size %d"
>   vfio_save_cleanup(const char *name) " (%s)"
>   vfio_save_complete_precopy(const char *name, int ret) " (%s) ret %d"
> +vfio_save_complete_precopy_started(const char *name) " (%s)"
>   vfio_save_device_config_state(const char *name) " (%s)"
>   vfio_save_iterate(const char *name, uint64_t precopy_init_size, uint64_t precopy_dirty_size) " (%s) precopy initial size %"PRIu64" precopy dirty size %"PRIu64
> +vfio_save_iterate_started(const char *name) " (%s)"
>   vfio_save_setup(const char *name, uint64_t data_buffer_size) " (%s) data buffer size %"PRIu64
>   vfio_state_pending_estimate(const char *name, uint64_t precopy, uint64_t postcopy, uint64_t precopy_init_size, uint64_t precopy_dirty_size) " (%s) precopy %"PRIu64" postcopy %"PRIu64" precopy initial size %"PRIu64" precopy dirty size %"PRIu64
>   vfio_state_pending_exact(const char *name, uint64_t precopy, uint64_t postcopy, uint64_t stopcopy_size, uint64_t precopy_init_size, uint64_t precopy_dirty_size) " (%s) precopy %"PRIu64" postcopy %"PRIu64" stopcopy size %"PRIu64" precopy initial size %"PRIu64" precopy dirty size %"PRIu64
> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> index fed499b199f0..0410111e9868 100644
> --- a/include/hw/vfio/vfio-common.h
> +++ b/include/hw/vfio/vfio-common.h
> @@ -73,6 +73,9 @@ typedef struct VFIOMigration {
>       uint64_t precopy_init_size;
>       uint64_t precopy_dirty_size;
>       bool initial_data_sent;
> +
> +    bool save_iterate_started;
> +    bool precopy_empty_hit;

Additionally, I would add an 'event_' prefix to give some indication
of what these new attributes are.

Also, how about replacing 'started' with 'start' ? That's fine if you
prefer start.

Tomorrow is soft-freeze, let's get these changes in for QEMU 9.2.

Thanks,

C.