Re: [PATCH v3 2/7] migration/multifd: Implement zero page transmission on the multifd thread.

[Fabiano Rosas <farosas@suse.de>]

Hao Xiang <hao.xiang@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.xiang@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.xiang@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;
}

> +        }
> +    }
> +}
> +
> +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);
> +        }
> +    }
> +}