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

[hao . xiang]

> 
> On Sun, Mar 3, 2024 at 11:16 PM Peter Xu <peterx@redhat.com> wrote:
> 
> > 
> > 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.

Let's see if we can catch that.

> > 
> >  [...]
> > 
> >  +static bool multifd_zero_page(void)
> > 
> >  multifd_zero_page_enabled()?

Changed.

> > 
> >  +{
> > 
> >  + 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.


Renamed.

> > 
> >  +{
> > 
> >  + 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.

Dropped.

> > 
> >  + if (multifd_zero_page()) {
> > 
> >  Shouldn't this be "!multifd_zero_page_enabled()"?

Thanks for catching this! My bad. Fixed.

> > 
> >  + pages->normal_num = pages->num;
> > 
> >  + return;
> > 
> >  + }
> > 
> >  The rest looks all sane.
> > 
> >  Thanks,
> > 
> >  --
> > 
> >  Peter Xu
> >
>