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

From: Peter Xu <peterx@redhat.com>
To: Hao Xiang <hao.xiang@bytedance.com>
Cc: pbonzini@redhat.com, berrange@redhat.com, eduardo@habkost.net,
	farosas@suse.de, eblake@redhat.com, armbru@redhat.com,
	thuth@redhat.com, lvivier@redhat.com, jdenemar@redhat.com,
	marcel.apfelbaum@gmail.com, philmd@linaro.org,
	wangyanan55@huawei.com, qemu-devel@nongnu.org
Subject: Re: [PATCH v4 2/7] migration/multifd: Implement zero page transmission on the multifd thread.
Date: Mon, 4 Mar 2024 15:16:30 +0800	[thread overview]
Message-ID: <ZeV1TrOvERHN_LOo@x1n> (raw)
In-Reply-To: <20240301022829.3390548-3-hao.xiang@bytedance.com>

On Fri, Mar 01, 2024 at 02:28:24AM +0000, Hao Xiang wrote:
> -GlobalProperty hw_compat_8_2[] = {};
> +GlobalProperty hw_compat_8_2[] = {
> +    { "migration", "zero-page-detection", "legacy"},
> +};

I hope we can make it for 9.0, then this (and many rest places) can be kept
as-is.  Let's see..  soft-freeze is March 12th.

One thing to mention is I just sent a pull which has mapped-ram feature
merged.  You may need a rebase onto that, and hopefully mapped-ram can also
use your feature too within the same patch when you repost.

https://lore.kernel.org/all/20240229153017.2221-1-farosas@suse.de/

That rebase may or may not need much caution, I apologize for that:
mapped-ram as a feature was discussed 1+ years, so it was a plan to merge
it (actually still partly of it) into QEMU 9.0.

[...]

> +static bool multifd_zero_page(void)

multifd_zero_page_enabled()?

> +{
> +    return migrate_zero_page_detection() == ZERO_PAGE_DETECTION_MULTIFD;
> +}
> +
> +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_send_zero_page_check: Perform zero page detection on all pages.
> + *
> + * Sorts normal pages before zero pages in p->pages->offset and updates
> + * p->pages->normal_num.
> + *
> + * @param p A pointer to the send params.

Nit: the majority of doc style in QEMU (it seems to me) is:

  @p: pointer to @MultiFDSendParams.

> + */
> +void multifd_send_zero_page_check(MultiFDSendParams *p)

multifd_send_zero_page_detect()?

This patch used "check" on both sides, but neither of them is a pure check
to me.  For the other side, maybe multifd_recv_zero_page_process()?  As
that one applies the zero pages.

> +{
> +    MultiFDPages_t *pages = p->pages;
> +    RAMBlock *rb = pages->block;
> +    int i = 0;
> +    int j = pages->num - 1;
> +
> +    /*
> +     * QEMU older than 9.0 don't understand zero page
> +     * on multifd channel. This switch is required to
> +     * maintain backward compatibility.
> +     */

IMHO we can drop this comment; it is not accurate as the user can disable
it explicitly through the parameter, then it may not always about compatibility.

> +    if (multifd_zero_page()) {

Shouldn't this be "!multifd_zero_page_enabled()"?

> +        pages->normal_num = pages->num;
> +        return;
> +    }

The rest looks all sane.

Thanks,

-- 
Peter Xu