Re: [PATCH v11 08/11] vfio/migration: Implement VFIO migration protocol v2

[Juan Quintela]

Avihai Horon <avihaih@nvidia.com> wrote:

Reviewed-by: Juan Quintela <quintela@redhat.com>.


1st comment is a bug, but as you just remove it.


Not that it matters a lot in the end (you are removing v1 on the
following patch).

> @@ -438,7 +445,13 @@ static bool 
> vfio_devices_all_running_and_mig_active(VFIOContainer *container)
>                  return false;
>              }
>  
> -            if (migration->device_state_v1 & VFIO_DEVICE_STATE_V1_RUNNING) {
> +            if (!migration->v2 &&
> +                migration->device_state_v1 & VFIO_DEVICE_STATE_V1_RUNNING) {
> +                continue;
> +            }

Are you missing here:


               } else {
                   return false;
               }

Or I am missunderstanding the code.


> +            if (migration->v2 &&
> +                migration->device_state == VFIO_DEVICE_STATE_RUNNING) {
>                  continue;
>              } else {
>                  return false;


[...]



> +/*
> + * This is an arbitrary size based on migration of mlx5 devices, where 
> typically
> + * total device migration size is on the order of 100s of MB. Testing with
> + * larger values, e.g. 128MB and 1GB, did not show a performance improvement.
> + */
> +#define VFIO_MIG_DEFAULT_DATA_BUFFER_SIZE (1 * MiB)

Just in case that it makes a difference.  You are using here a 1MB
buffer.
But qemu_file_get_to_fd() uses a 32KB buffer.



>      }
>  }
>  
> +static int vfio_query_stop_copy_size(VFIODevice *vbasedev,
> +                                     uint64_t *stop_copy_size)
> +{
> +    uint64_t buf[DIV_ROUND_UP(sizeof(struct vfio_device_feature) +
> +                              sizeof(struct 
> vfio_device_feature_mig_data_size),
> +                              sizeof(uint64_t))] = {};
> +    struct vfio_device_feature *feature = (struct vfio_device_feature *)buf;

suggestion:

       size_t size = DIV_ROUND_UP(sizeof(struct vfio_device_feature) +
                                 sizeof(struct 
vfio_device_feature_mig_data_size),
                                 sizeof(uint64_t));
       g_autofree struct vfio_device_feature *feature = g_malloc0(size * 
sizeof(uint64_t));

I have to reread several times to see what was going on there.


And call it a day?



Later, Juan.