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

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

[Yishai Hadas]

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)
-- 
2.18.1

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

[Yishai Hadas]

Gentle ping for review, CCing more people who may be involved.

Thanks,
Yishai

On 11/11/2021 11:50 AM, Yishai Hadas 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)

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

[Alex Williamson]

On Mon, 22 Nov 2021 09:40:56 +0200
Yishai Hadas <yishaih@nvidia.com> wrote:

> Gentle ping for review, CCing more people who may be involved.

I'll wait for comments from others, but since we're already in the 6.2
freeze and vfio migration is still experimental (and I'm on PTO this
week), I expect this to be queued when the next development window
opens.  Thanks,

Alex


> On 11/11/2021 11:50 AM, Yishai Hadas 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)  
> 
> 

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?