Re: [PATCH] multifd: Add colo support

[Lukas Straub <lukasstraub2@web.de>]

[-- Attachment #1: Type: text/plain, Size: 5603 bytes --]

On Thu, 11 May 2023 11:52:55 +0200
Juan Quintela <quintela@redhat.com> wrote:

> Lukas Straub <lukasstraub2@web.de> wrote:
> > On Tue,  9 May 2023 20:15:28 +0200
> > Juan Quintela <quintela@redhat.com> wrote:
> >  
> >> From: Lukas Straub <lukasstraub2@web.de>
> >> 
> >> Like in the normal ram_load() path, put the received pages into the
> >> colo cache and mark the pages in the bitmap so that they will be
> >> flushed to the guest later.
> >> 
> >> Signed-off-by: Lukas Straub <lukasstraub2@web.de>
> >> 
> >> ---
> >> 
> >> Hi Lukas
> >> 
> >> What about this instead of your other three patches?  I think it is
> >> clearer, and I don't think that we are going to have anything else
> >> that is going to hook there anytime soon.
> >> 
> >> Notice that I put CONFIG_COLO waiting for Vladimir changes to get in
> >> before I merge this.
> >> 
> >> Notice also that I "lost" the line:
> >> 
> >>   p->host = p->block->host;
> >> 
> >> In the error case.  But in that case we are aborting the migration, so
> >> we don't care.
> >> 
> >> Can you check if it works for you?
> >> Here it compiles, so it must be perfect.
> >> 
> >> Thanks, Juan.  
> >
> > This way is okay for now. Though I will send a patch.  
> 
> 
> If it works for you, you can add a review-by and I will integrate in
> next PULL.

Nack, There are quite few things wrong at a glance. I will cook up my
own patch, similar to this one.

> 
> Later, Juan.
> 
> >
> > Regards,
> > Lukas Straub
> >  
> >> ---
> >>  migration/meson.build    |  2 +-
> >>  migration/multifd-colo.c | 49 ++++++++++++++++++++++++++++++++++++++++
> >>  migration/multifd-colo.h | 24 ++++++++++++++++++++
> >>  3 files changed, 74 insertions(+), 1 deletion(-)
> >>  create mode 100644 migration/multifd-colo.c
> >>  create mode 100644 migration/multifd-colo.h
> >> 
> >> diff --git a/migration/meson.build b/migration/meson.build
> >> index 75de868bb7..c9db40d4d4 100644
> >> --- a/migration/meson.build
> >> +++ b/migration/meson.build
> >> @@ -23,7 +23,7 @@ softmmu_ss.add(files(
> >>    'migration.c',
> >>    'multifd.c',
> >>    'multifd-zlib.c',
> >> -  'multifd-zlib.c',

???

> >> +  'multifd-colo.c',
> >>    'ram-compress.c',
> >>    'options.c',
> >>    'postcopy-ram.c',
> >> diff --git a/migration/multifd-colo.c b/migration/multifd-colo.c

With just this patch, this file is not compiled in at all. And
multifd_colo_prepare_recv_pages and multifd_colo_process_recv_pages
aren't called from anywhere.

I guess they should be called before and after
multifd_recv_state->ops->recv_pages(p, &local_err);
?

> >> new file mode 100644
> >> index 0000000000..10fa1467fa
> >> --- /dev/null
> >> +++ b/migration/multifd-colo.c
> >> @@ -0,0 +1,49 @@
> >> +/*
> >> + * multifd colo implementation
> >> + *
> >> + * Copyright (c) Lukas Straub <lukasstraub2@web.de>
> >> + *
> >> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> >> + * See the COPYING file in the top-level directory.
> >> + */
> >> +
> >> +#include "qemu/osdep.h"
> >> +#include "exec/target_page.h"
> >> +#include "exec/ramblock.h"
> >> +#include "qemu/error-report.h"
> >> +#include "qapi/error.h"
> >> +#include "ram.h"
> >> +#include "multifd.h"
> >> +#include "options.h"
> >> +#include "io/channel-socket.h"
> >> +#include "migration/colo.h"
> >> +#include "multifd-colo.h"
> >> +
> >> +void multifd_colo_prepare_recv_pages(MultiFDRecvParams *p)
> >> +{
> >> +    if (migrate_colo()) {
> >> +        /*
> >> +         * While we're still in precopy mode, we copy received pages to both guest
> >> +         * and cache. No need to set dirty bits, since guest and cache memory are
> >> +         * in sync.
> >> +         */
> >> +        if (migration_incoming_in_colo_state()) {
> >> +            colo_record_bitmap(p->block, p->normal, p->normal_num);
> >> +        }
> >> +        p->host = p->block->colo_cache;
> >> +    }
> >> +}
> >> +
> >> +void multifd_colo_process_recv_pages(MultiFDRecvParams *p)
> >> +{
> >> +    if (migrate_colo()) {
> >> +        if (!migration_incoming_in_colo_state()) {
> >> +            for (int i = 0; i < p->normal_num; i++) {
> >> +                void *guest = p->block->host + p->normal[i];
> >> +                void *cache = p->host + p->normal[i];
> >> +                memcpy(guest, cache, p->page_size);
> >> +            }
> >> +        }
> >> +        p->host = p->block->host;
> >> +    }
> >> +}
> >> diff --git a/migration/multifd-colo.h b/migration/multifd-colo.h
> >> new file mode 100644
> >> index 0000000000..1636c617fc
> >> --- /dev/null
> >> +++ b/migration/multifd-colo.h
> >> @@ -0,0 +1,24 @@
> >> +/*
> >> + * multifd colo header
> >> + *
> >> + * Copyright (c) Lukas Straub <lukasstraub2@web.de>
> >> + *
> >> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> >> + * See the COPYING file in the top-level directory.
> >> + */
> >> +
> >> +#ifndef QEMU_MIGRATION_MULTIFD_COLO_H
> >> +#define QEMU_MIGRATION_MULTIFD_COLO_H
> >> +
> >> +#ifndef CONFIG_COLO

Should be CONFIG_REPLICATION

> >> +
> >> +void multifd_colo_prepare_recv_pages(MultiFDRecvParams *p);
> >> +void multifd_colo_process_recv_pages(MultiFDRecvParams *p);
> >> +
> >> +#else
> >> +
> >> +static inline void multifd_colo_prepare_recv_pages(MultiFDRecvParams *p) {}
> >> +static inline void multifd_colo_process_recv_pages(MultiFDRecvParams *p) {}
> >> +
> >> +#endif /* CONFIG_COLO */
> >> +#endif  
> 



-- 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]