Re: [External] Re: Re: [PATCH 2/6] migration/multifd: Add zero pages and zero bytes counter to migration status interface.

[Peter Xu <peterx@redhat.com>]

On Wed, Feb 07, 2024 at 03:44:18PM -0800, Hao Xiang wrote:
> On Wed, Feb 7, 2024 at 12:41 AM Jiri Denemark <jdenemar@redhat.com> wrote:
> >
> > On Wed, Feb 07, 2024 at 12:37:15 +0800, Peter Xu wrote:
> > > On Wed, Feb 07, 2024 at 12:13:10PM +0800, Peter Xu wrote:
> > > > On Tue, Feb 06, 2024 at 11:19:04PM +0000, Hao Xiang wrote:
> > > > > This change extends the MigrationStatus interface to track zero pages
> > > > > and zero bytes counter.
> > > > >
> > > > > Signed-off-by: Hao Xiang <hao.xiang@bytedance.com>
> > > >
> > > > Reviewed-by: Peter Xu <peterx@redhat.com>
> > >
> > > I'll need to scratch this, sorry..
> > >
> > > The issue is I forgot we have "duplicate" which is exactly "zero
> > > page"s.. See:
> > >
> > >     info->ram->duplicate = stat64_get(&mig_stats.zero_pages);
> > >
> > > If you think the name too confusing and want a replacement, maybe it's fine
> > > and maybe we can do that.  Then we can keep this zero page counter
> > > introduced, reporting the same value as duplicates, then with a follow up
> > > patch to deprecate "duplicate" parameter.  See an exmaple on how to
> > > deprecate in 7b24d326348e1672.
> > >
> > > One thing I'm not sure is whether Libvirt will be fine on losing
> > > "duplicates" after 2+ QEMU major releases.  Copy Jiri for this.  My
> > > understanding is that Libvirt should be keeping an eye on deprecation list
> > > and react, but I'd like to double check..
> >
> > This should not be a big deal as we can internally map either one
> > (depending on what QEMU supports) to the same libvirt's field. AFAIK
> > there is a consensus on Cc-ing libvirt-devel on patches that deprecate

I see.

> > QEMU interfaces so that we can update our code in time before the
> > deprecated interface is dropped.

Right.

What I mostly worried is "old libvirt" + "new qemu", where the old libvirt
only knows "duplicates", while the new (after 2 releases) will only report
"zeros".

> >
> > BTW, libvirt maps "duplicate" to:
> >
> > /**
> >  * VIR_DOMAIN_JOB_MEMORY_CONSTANT:
> >  *
> >  * virDomainGetJobStats field: number of pages filled with a constant
> >  * byte (all bytes in a single page are identical) transferred since the
> >  * beginning of the migration job, as VIR_TYPED_PARAM_ULLONG.
> >  *
> >  * The most common example of such pages are zero pages, i.e., pages filled
> >  * with zero bytes.
> >  *
> >  * Since: 1.0.3
> >  */
> > # define VIR_DOMAIN_JOB_MEMORY_CONSTANT          "memory_constant"
> >
> > Jirka
> >
> 
> Interesting. I didn't notice the existence of "duplicate" for zero
> pages. I do think the name is quite confusing. I will create the
> "zero/zero_bytes" counter and a separate commit to deprecate
> "duplicate". Will add libvirt devs per instruction above.

Yeah, please go ahead, and I hope my worry is not a real concern above; we
can figure that out later.  Even without deprecating "duplicate", maybe
it'll at least still be worthwhile we start having "zeros" reported
alongside.  Then after 10/20/30/N years we always have a chance to
deprecate the other one, just a matter of compatible window.

Thanks,

-- 
Peter Xu