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-faro...@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
