Re: [PATCH] vfio/migration: Improve to read/write full migration region per chunk

[Cornelia Huck]

On Thu, Nov 11 2021, Yishai Hadas <yishaih@nvidia.com> wrote:

> Upon reading/writing the migration data there is no real reason to limit
> the read/write system call from the file to be 8 bytes.
>
> In addition, there is no reason to depend on the file offset alignment.
> The offset is just some logical value which depends also on the region
> index and has nothing to do with the amount of data that can be
> accessed.
>
> Move to read/write the full region size per chunk, this reduces
> dramatically the number of the systems calls that are needed and improve
> performance.
>
> Signed-off-by: Yishai Hadas <yishaih@nvidia.com>
> ---
>  hw/vfio/migration.c | 36 ++----------------------------------
>  1 file changed, 2 insertions(+), 34 deletions(-)
>
> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
> index ff6b45de6b5..b5f310bb831 100644
> --- a/hw/vfio/migration.c
> +++ b/hw/vfio/migration.c
> @@ -62,40 +62,8 @@ static inline int vfio_mig_access(VFIODevice *vbasedev, 
> void *val, int count,
>      return 0;
>  }
>  
> -static int vfio_mig_rw(VFIODevice *vbasedev, __u8 *buf, size_t count,
> -                       off_t off, bool iswrite)
> -{
> -    int ret, done = 0;
> -    __u8 *tbuf = buf;
> -
> -    while (count) {
> -        int bytes = 0;
> -
> -        if (count >= 8 && !(off % 8)) {
> -            bytes = 8;
> -        } else if (count >= 4 && !(off % 4)) {
> -            bytes = 4;
> -        } else if (count >= 2 && !(off % 2)) {
> -            bytes = 2;
> -        } else {
> -            bytes = 1;
> -        }
> -
> -        ret = vfio_mig_access(vbasedev, tbuf, bytes, off, iswrite);
> -        if (ret) {
> -            return ret;
> -        }
> -
> -        count -= bytes;
> -        done += bytes;
> -        off += bytes;
> -        tbuf += bytes;
> -    }
> -    return done;
> -}
> -
> -#define vfio_mig_read(f, v, c, o)       vfio_mig_rw(f, (__u8 *)v, c, o, 
> false)
> -#define vfio_mig_write(f, v, c, o)      vfio_mig_rw(f, (__u8 *)v, c, o, true)
> +#define vfio_mig_read(f, v, c, o)       vfio_mig_access(f, (__u8 *)v, c, o, 
> false)
> +#define vfio_mig_write(f, v, c, o)      vfio_mig_access(f, (__u8 *)v, c, o, 
> true)
>  
>  #define VFIO_MIG_STRUCT_OFFSET(f)       \
>                                   offsetof(struct vfio_device_migration_info, 
> f)

I've been looking at this patch and it doesn't look wrong to me in any
obvious way. The question is: why had it been done like that in the
first place?

I dug through the old mailing list discussions, and it seems it had been
introduced in v26, but I don't see any explanation for that. Kirti, do
you remember why you added this?