On Wed, Feb 28, 2024 at 11:46 AM Fabiano Rosas <faro...@suse.de> wrote:
>
> Hao Xiang <hao.xi...@bytedance.com> writes:
>
> > 1. Add zero_pages field in MultiFDPacket_t.
> > 2. Implements the zero page detection and handling on the multifd
> > threads for non-compression, zlib and zstd compression backends.
> > 3. Added a new value 'multifd' in ZeroPageDetection enumeration.
> > 4. Handle migration QEMU9.0 -> QEMU8.2 compatibility.
> > 5. Adds zero page counters and updates multifd send/receive tracing
> > format to track the newly added counters.
> >
> > Signed-off-by: Hao Xiang <hao.xi...@bytedance.com>
> > ---
> > hw/core/machine.c | 4 +-
> > hw/core/qdev-properties-system.c | 2 +-
> > migration/meson.build | 1 +
> > migration/multifd-zero-page.c | 78 ++++++++++++++++++++++++++++++
> > migration/multifd-zlib.c | 21 ++++++--
> > migration/multifd-zstd.c | 20 ++++++--
> > migration/multifd.c | 83 +++++++++++++++++++++++++++-----
> > migration/multifd.h | 24 ++++++++-
> > migration/ram.c | 1 -
> > migration/trace-events | 8 +--
> > qapi/migration.json | 5 +-
> > 11 files changed, 214 insertions(+), 33 deletions(-)
> > create mode 100644 migration/multifd-zero-page.c
> >
> > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > index fb5afdcae4..746da219a4 100644
> > --- a/hw/core/machine.c
> > +++ b/hw/core/machine.c
> > @@ -32,7 +32,9 @@
> > #include "hw/virtio/virtio-net.h"
> > #include "audio/audio.h"
> >
> > -GlobalProperty hw_compat_8_2[] = {};
> > +GlobalProperty hw_compat_8_2[] = {
> > + { "migration", "zero-page-detection", "legacy"},
> > +};
> > const size_t hw_compat_8_2_len = G_N_ELEMENTS(hw_compat_8_2);
> >
> > GlobalProperty hw_compat_8_1[] = {
> > diff --git a/hw/core/qdev-properties-system.c
> > b/hw/core/qdev-properties-system.c
> > index 228e685f52..6e6f68ae1b 100644
> > --- a/hw/core/qdev-properties-system.c
> > +++ b/hw/core/qdev-properties-system.c
> > @@ -682,7 +682,7 @@ const PropertyInfo qdev_prop_mig_mode = {
> > const PropertyInfo qdev_prop_zero_page_detection = {
> > .name = "ZeroPageDetection",
> > .description = "zero_page_detection values, "
> > - "none,legacy",
> > + "none,legacy,multifd",
> > .enum_table = &ZeroPageDetection_lookup,
> > .get = qdev_propinfo_get_enum,
> > .set = qdev_propinfo_set_enum,
> > diff --git a/migration/meson.build b/migration/meson.build
> > index 92b1cc4297..1eeb915ff6 100644
> > --- a/migration/meson.build
> > +++ b/migration/meson.build
> > @@ -22,6 +22,7 @@ system_ss.add(files(
> > 'migration.c',
> > 'multifd.c',
> > 'multifd-zlib.c',
> > + 'multifd-zero-page.c',
> > 'ram-compress.c',
> > 'options.c',
> > 'postcopy-ram.c',
> > diff --git a/migration/multifd-zero-page.c b/migration/multifd-zero-page.c
> > new file mode 100644
> > index 0000000000..1650c41b26
> > --- /dev/null
> > +++ b/migration/multifd-zero-page.c
> > @@ -0,0 +1,78 @@
> > +/*
> > + * Multifd zero page detection implementation.
> > + *
> > + * Copyright (c) 2024 Bytedance Inc
> > + *
> > + * Authors:
> > + * Hao Xiang <hao.xi...@bytedance.com>
> > + *
> > + * 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 "qemu/cutils.h"
> > +#include "exec/ramblock.h"
> > +#include "migration.h"
> > +#include "multifd.h"
> > +#include "options.h"
> > +#include "ram.h"
> > +
> > +static void swap_page_offset(ram_addr_t *pages_offset, int a, int b)
> > +{
> > + ram_addr_t temp;
> > +
> > + if (a == b) {
> > + return;
> > + }
> > +
> > + temp = pages_offset[a];
> > + pages_offset[a] = pages_offset[b];
> > + pages_offset[b] = temp;
> > +}
> > +
> > +/**
> > + * multifd_zero_page_check_send: Perform zero page detection on all pages.
> > + *
> > + * Sort the page offset array by moving all normal pages to
> > + * the left and all zero pages to the right of the array.
>
> Let's move this to the loop as a comment. Here it's better to just
> inform about the side effects:
>
> Sorts normal pages before zero pages in p->pages->offset and updates
> p->pages->normal_num.
>
> > + *
> > + * @param p A pointer to the send params.
> > + */
> > +void multifd_zero_page_check_send(MultiFDSendParams *p)
>
> Use multifd_send_zero_page_check for consistency with the rest of the code.
>
> > +{
> > + /*
> > + * QEMU older than 9.0 don't understand zero page
> > + * on multifd channel. This switch is required to
> > + * maintain backward compatibility.
> > + */
> > + bool use_multifd_zero_page =
> > + (migrate_zero_page_detection() == ZERO_PAGE_DETECTION_MULTIFD);
>
> Could introduce a helper for this.
>
> static bool multifd_zero_page(void)
> {
> return migrate_zero_page_detection() == ZERO_PAGE_DETECTION_MULTIFD;
> }
>
> > + MultiFDPages_t *pages = p->pages;
> > + RAMBlock *rb = pages->block;
> > + int index_normal = 0;
> > + int index_zero = pages->num - 1;
>
> IMHO, i and j are more idiomatic, makes the code easier on the eyes.
>
> > +
> > + while (index_normal <= index_zero) {
> > + uint64_t offset = pages->offset[index_normal];
> > + if (use_multifd_zero_page &&
>
> You don't need to check this at every iteration. Could check at the top
> and exit right away.
>
> > + buffer_is_zero(rb->host + offset, p->page_size)) {
> > + swap_page_offset(pages->offset, index_normal, index_zero);
> > + index_zero--;
> > + ram_release_page(rb->idstr, offset);
> > + } else {
> > + index_normal++;
> > + pages->normal_num++;
>
> Not a fan of changing pages->normal_num like this. Let's make the loop
> just sort and update normal_num at the end.
>
> Putting all together:
>
> void multifd_send_zero_page_check(MultiFDSendParams *p)
> {
> MultiFDPages_t *pages = p->pages;
> RAMBlock *rb = pages->block;
> int i = 0;
> int j = pages->num - 1;
>
> if (!multifd_zero_page()) {
> pages->normal_num = pages->num;
> return;
> }
>
> /*
> * Sort the offsets array by moving all normal pages to the start
> * and all zero pages to the end of the array.
> */
> while (i <= j) {
> uint64_t offset = pages->offset[i];
>
> if (!buffer_is_zero(rb->host + offset, p->page_size)) {
> i++;
> continue;
> }
>
> swap_page_offset(pages->offset, i, j);
> ram_release_page(rb->idstr, offset);
> j--;
> }
>
> pages->normal_num = i;
> }
>
Sure. Will fix these in the next version.
> > + }
> > + }
> > +}
> > +
> > +void multifd_zero_page_check_recv(MultiFDRecvParams *p)
> > +{
> > + for (int i = 0; i < p->zero_num; i++) {
> > + void *page = p->host + p->zero[i];
> > + if (!buffer_is_zero(page, p->page_size)) {
> > + memset(page, 0, p->page_size);
> > + }
> > + }
> > +}
