Patchwork [4/4] migration: simplify writev vs. non-writev logic

login
register
mail settings
Submitter Paolo Bonzini
Date April 8, 2013, 11:29 a.m.
Message ID <1365420597-5506-5-git-send-email-pbonzini@redhat.com>
Download mbox | patch
Permalink /patch/234745/
State New
Headers show

Comments

Paolo Bonzini - April 8, 2013, 11:29 a.m.
Check f->iovcnt in add_to_iovec, f->buf_index in qemu_put_buffer/byte.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 savevm.c | 22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)
Juan Quintela - April 9, 2013, 11:43 a.m.
Paolo Bonzini <pbonzini@redhat.com> wrote:
> Check f->iovcnt in add_to_iovec, f->buf_index in qemu_put_buffer/byte.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  savevm.c | 22 +++++++++-------------
>  1 file changed, 9 insertions(+), 13 deletions(-)
>
> diff --git a/savevm.c b/savevm.c
> index a2f6bc0..63f4c82 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -626,7 +626,7 @@ static void add_to_iovec(QEMUFile *f, const uint8_t *buf, int size)
>          f->iov[f->iovcnt++].iov_len = size;
>      }
>  
> -    if (f->buf_index >= IO_BUF_SIZE || f->iovcnt >= MAX_IOV_SIZE) {
> +    if (f->iovcnt >= MAX_IOV_SIZE) {
>          qemu_fflush(f);
>      }
>  }
> @@ -662,12 +662,10 @@ void qemu_put_buffer(QEMUFile *f, const uint8_t *buf, int size)
>          f->bytes_xfer += size;
>          if (f->ops->writev_buffer) {
>              add_to_iovec(f, f->buf + f->buf_index, l);
> -            f->buf_index += l;
> -        } else {
> -            f->buf_index += l;
> -            if (f->buf_index == IO_BUF_SIZE) {
> -                qemu_fflush(f);
> -            }
> +        }
> +        f->buf_index += l;
> +        if (f->buf_index == IO_BUF_SIZE) {
> +            qemu_fflush(f);
>          }
>          if (qemu_file_get_error(f)) {
>              break;
> @@ -687,12 +685,10 @@ void qemu_put_byte(QEMUFile *f, int v)
>      f->bytes_xfer++;
>      if (f->ops->writev_buffer) {
>          add_to_iovec(f, f->buf + f->buf_index, 1);
> -        f->buf_index++;
> -    } else {
> -        f->buf_index++;
> -        if (f->buf_index == IO_BUF_SIZE) {
> -            qemu_fflush(f);
> -        }
> +    }
> +    f->buf_index++;
> +    if (f->buf_index == IO_BUF_SIZE) {
> +        qemu_fflush(f);
>      }
>  }

If you follow my advice of moving the call to add_to_iovec() you get
this one simplified and only one place to do this.

Thanks,  Juan.
Paolo Bonzini - April 9, 2013, 11:53 a.m.
Il 09/04/2013 13:43, Juan Quintela ha scritto:
>> > @@ -687,12 +685,10 @@ void qemu_put_byte(QEMUFile *f, int v)
>> >      f->bytes_xfer++;
>> >      if (f->ops->writev_buffer) {
>> >          add_to_iovec(f, f->buf + f->buf_index, 1);
>> > -        f->buf_index++;
>> > -    } else {
>> > -        f->buf_index++;
>> > -        if (f->buf_index == IO_BUF_SIZE) {
>> > -            qemu_fflush(f);
>> > -        }
>> > +    }
>> > +    f->buf_index++;
>> > +    if (f->buf_index == IO_BUF_SIZE) {
>> > +        qemu_fflush(f);
>> >      }
>> >  }
> If you follow my advice of moving the call to add_to_iovec() you get
> this one simplified and only one place to do this.

Moving what call?  The apparent complication is because the old logic
was a bit more involute than necessary.  If you look at the code after
the patches, not the patches themselves, you'll see for yourself.

The logic now is:

   add byte
   if using iovs
       add byte to iov list
   if buffer full
       flush

add_to_iovec has no business checking the buffer.  Why should
qemu_put_buffer_async() check the buffer?

The duplication between qemu_put_byte and qemu_put_buffer is a different
topic.  I think it's acceptable in the name of performance, but perhaps
you can just call qemu_put_buffer(f, &c, 1).

Paolo
Juan Quintela - April 9, 2013, 12:16 p.m.
Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 09/04/2013 13:43, Juan Quintela ha scritto:
>>> > @@ -687,12 +685,10 @@ void qemu_put_byte(QEMUFile *f, int v)
>>> >      f->bytes_xfer++;
>>> >      if (f->ops->writev_buffer) {
>>> >          add_to_iovec(f, f->buf + f->buf_index, 1);
>>> > -        f->buf_index++;
>>> > -    } else {
>>> > -        f->buf_index++;
>>> > -        if (f->buf_index == IO_BUF_SIZE) {
>>> > -            qemu_fflush(f);
>>> > -        }
>>> > +    }
>>> > +    f->buf_index++;
>>> > +    if (f->buf_index == IO_BUF_SIZE) {
>>> > +        qemu_fflush(f);
>>> >      }
>>> >  }
>> If you follow my advice of moving the call to add_to_iovec() you get
>> this one simplified and only one place to do this.
>
> Moving what call?  The apparent complication is because the old logic
> was a bit more involute than necessary.  If you look at the code after
> the patches, not the patches themselves, you'll see for yourself.
>
> The logic now is:
>
>    add byte
>    if using iovs
>        add byte to iov list
>    if buffer full
>        flush
>
> add_to_iovec has no business checking the buffer.  Why should
> qemu_put_buffer_async() check the buffer?

We can rename the function.  I agree with that.

>
> The duplication between qemu_put_byte and qemu_put_buffer is a different
> topic.  I think it's acceptable in the name of performance, but perhaps
> you can just call qemu_put_buffer(f, &c, 1).

Right now,  it is important because all the integer types are sent as
callas to qemu_put_byte(),  but Orit had a patch to rewrote those using
qemu_put_buffer() dirrectly,  and then we don't use so many
qemu_put_byte()'s anymore.

Later,  Juan.
Orit Wasserman - April 9, 2013, 12:22 p.m.
On 04/09/2013 02:53 PM, Paolo Bonzini wrote:
> Il 09/04/2013 13:43, Juan Quintela ha scritto:
>>>> @@ -687,12 +685,10 @@ void qemu_put_byte(QEMUFile *f, int v)
>>>>      f->bytes_xfer++;
>>>>      if (f->ops->writev_buffer) {
>>>>          add_to_iovec(f, f->buf + f->buf_index, 1);
>>>> -        f->buf_index++;
>>>> -    } else {
>>>> -        f->buf_index++;
>>>> -        if (f->buf_index == IO_BUF_SIZE) {
>>>> -            qemu_fflush(f);
>>>> -        }
>>>> +    }
>>>> +    f->buf_index++;
>>>> +    if (f->buf_index == IO_BUF_SIZE) {
>>>> +        qemu_fflush(f);
>>>>      }
>>>>  }
>> If you follow my advice of moving the call to add_to_iovec() you get
>> this one simplified and only one place to do this.
> 
> Moving what call?  The apparent complication is because the old logic
> was a bit more involute than necessary.  If you look at the code after
> the patches, not the patches themselves, you'll see for yourself.
> 
> The logic now is:
> 
>    add byte
>    if using iovs
>        add byte to iov list
>    if buffer full
>        flush
> 
> add_to_iovec has no business checking the buffer.  Why should
> qemu_put_buffer_async() check the buffer?
> 
> The duplication between qemu_put_byte and qemu_put_buffer is a different
> topic.  I think it's acceptable in the name of performance, but perhaps
> you can just call qemu_put_buffer(f, &c, 1).
I thought about it too, we can keep the optimization by checking the size

Orit
> 
> Paolo
>

Patch

diff --git a/savevm.c b/savevm.c
index a2f6bc0..63f4c82 100644
--- a/savevm.c
+++ b/savevm.c
@@ -626,7 +626,7 @@  static void add_to_iovec(QEMUFile *f, const uint8_t *buf, int size)
         f->iov[f->iovcnt++].iov_len = size;
     }
 
-    if (f->buf_index >= IO_BUF_SIZE || f->iovcnt >= MAX_IOV_SIZE) {
+    if (f->iovcnt >= MAX_IOV_SIZE) {
         qemu_fflush(f);
     }
 }
@@ -662,12 +662,10 @@  void qemu_put_buffer(QEMUFile *f, const uint8_t *buf, int size)
         f->bytes_xfer += size;
         if (f->ops->writev_buffer) {
             add_to_iovec(f, f->buf + f->buf_index, l);
-            f->buf_index += l;
-        } else {
-            f->buf_index += l;
-            if (f->buf_index == IO_BUF_SIZE) {
-                qemu_fflush(f);
-            }
+        }
+        f->buf_index += l;
+        if (f->buf_index == IO_BUF_SIZE) {
+            qemu_fflush(f);
         }
         if (qemu_file_get_error(f)) {
             break;
@@ -687,12 +685,10 @@  void qemu_put_byte(QEMUFile *f, int v)
     f->bytes_xfer++;
     if (f->ops->writev_buffer) {
         add_to_iovec(f, f->buf + f->buf_index, 1);
-        f->buf_index++;
-    } else {
-        f->buf_index++;
-        if (f->buf_index == IO_BUF_SIZE) {
-            qemu_fflush(f);
-        }
+    }
+    f->buf_index++;
+    if (f->buf_index == IO_BUF_SIZE) {
+        qemu_fflush(f);
     }
 }