[PATCH 09/10] migration/colo: Move qemu_fflush() closer to its user for fb

Peter Xu posted 10 patches 2 weeks, 5 days ago
Maintainers: Hailiang Zhang <zhanghailiang@xfusion.com>, Peter Xu <peterx@redhat.com>, Fabiano Rosas <farosas@suse.de>, Markus Armbruster <armbru@redhat.com>
[PATCH 09/10] migration/colo: Move qemu_fflush() closer to its user for fb
Posted by Peter Xu 2 weeks, 5 days ago
COLO caches all device states in a buffer channel `fb'.  Flush it right
before pushing the buffer to make sure everything needed will be flushed,
add a comment explaning it.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/colo.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/migration/colo.c b/migration/colo.c
index f92803dd29..e2307f1907 100644
--- a/migration/colo.c
+++ b/migration/colo.c
@@ -472,8 +472,6 @@ static int colo_do_checkpoint_transaction(MigrationState *s,
     qemu_savevm_state_complete_precopy_iterable(s->to_dst_file, false);
     qemu_put_byte(s->to_dst_file, QEMU_VM_EOF);
 
-    qemu_fflush(fb);
-
     /*
      * We need the size of the VMstate data in Secondary side,
      * With which we can decide how much data should be read.
@@ -484,6 +482,8 @@ static int colo_do_checkpoint_transaction(MigrationState *s,
         goto out;
     }
 
+    /* Flush to make sure everything lands bioc->data */
+    qemu_fflush(fb);
     qemu_put_buffer(s->to_dst_file, bioc->data, bioc->usage);
     ret = qemu_fflush(s->to_dst_file);
     if (ret < 0) {
-- 
2.50.1
Re: [PATCH 09/10] migration/colo: Move qemu_fflush() closer to its user for fb
Posted by Fabiano Rosas 2 weeks, 4 days ago
Peter Xu <peterx@redhat.com> writes:

> COLO caches all device states in a buffer channel `fb'.  Flush it right
> before pushing the buffer to make sure everything needed will be flushed,
> add a comment explaning it.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  migration/colo.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/migration/colo.c b/migration/colo.c
> index f92803dd29..e2307f1907 100644
> --- a/migration/colo.c
> +++ b/migration/colo.c
> @@ -472,8 +472,6 @@ static int colo_do_checkpoint_transaction(MigrationState *s,
>      qemu_savevm_state_complete_precopy_iterable(s->to_dst_file, false);
>      qemu_put_byte(s->to_dst_file, QEMU_VM_EOF);
>  
> -    qemu_fflush(fb);
> -
>      /*
>       * We need the size of the VMstate data in Secondary side,
>       * With which we can decide how much data should be read.
> @@ -484,6 +482,8 @@ static int colo_do_checkpoint_transaction(MigrationState *s,
>          goto out;
>      }
>  
> +    /* Flush to make sure everything lands bioc->data */
> +    qemu_fflush(fb);
>      qemu_put_buffer(s->to_dst_file, bioc->data, bioc->usage);
>      ret = qemu_fflush(s->to_dst_file);

Reviewed-by: Fabiano Rosas <farosas@suse.de>

Wait, aren't we wasting a memcpy here? Isn't this the same, but without
the copy of bioc->data to s->to_dst_file->buf:

qemu_fflush(fb);                                        // ensure bioc->data is populated
qemu_fflush(s->to_dst_file);                            // send buffered data from migration stream
qemu_put_buffer_async(s->to_dst_file, bioc->data, ...); // point s->to_dst_file->iov to bioc->data
qemu_fflush(s->to_dst_file);                            // send s->to_dst_file->iov contents

>      if (ret < 0) {
Re: [PATCH 09/10] migration/colo: Move qemu_fflush() closer to its user for fb
Posted by Peter Xu 2 weeks ago
On Fri, Jan 23, 2026 at 11:10:54AM -0300, Fabiano Rosas wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > COLO caches all device states in a buffer channel `fb'.  Flush it right
> > before pushing the buffer to make sure everything needed will be flushed,
> > add a comment explaning it.
> >
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  migration/colo.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/migration/colo.c b/migration/colo.c
> > index f92803dd29..e2307f1907 100644
> > --- a/migration/colo.c
> > +++ b/migration/colo.c
> > @@ -472,8 +472,6 @@ static int colo_do_checkpoint_transaction(MigrationState *s,
> >      qemu_savevm_state_complete_precopy_iterable(s->to_dst_file, false);
> >      qemu_put_byte(s->to_dst_file, QEMU_VM_EOF);
> >  
> > -    qemu_fflush(fb);
> > -
> >      /*
> >       * We need the size of the VMstate data in Secondary side,
> >       * With which we can decide how much data should be read.
> > @@ -484,6 +482,8 @@ static int colo_do_checkpoint_transaction(MigrationState *s,
> >          goto out;
> >      }
> >  
> > +    /* Flush to make sure everything lands bioc->data */
> > +    qemu_fflush(fb);
> >      qemu_put_buffer(s->to_dst_file, bioc->data, bioc->usage);
> >      ret = qemu_fflush(s->to_dst_file);
> 
> Reviewed-by: Fabiano Rosas <farosas@suse.de>
> 
> Wait, aren't we wasting a memcpy here? Isn't this the same, but without
> the copy of bioc->data to s->to_dst_file->buf:
> 
> qemu_fflush(fb);                                        // ensure bioc->data is populated
> qemu_fflush(s->to_dst_file);                            // send buffered data from migration stream
> qemu_put_buffer_async(s->to_dst_file, bioc->data, ...); // point s->to_dst_file->iov to bioc->data
> qemu_fflush(s->to_dst_file);                            // send s->to_dst_file->iov contents

Good point!  Same to postcopy..

I quickly measured a small VM in postcopy path, async put can shrink the
time needed (plus the flush) from 91us to 38us.  Not a huge difference, but
I don't see why we don't do it either.  I'll test more and add those when I
repost.

-- 
Peter Xu