Re: [External] Re: [PATCH v2 3/7] migration/multifd: Zero page transmission on the multifd thread.

[Peter Xu <peterx@redhat.com>]

On Sat, Feb 24, 2024 at 02:56:15PM -0800, Hao Xiang wrote:
> > > > I don't think it's super clean to have three arrays offset, zero and
> > > > normal, all sized for the full packet size. It might be possible to just
> > > > carry a bitmap of non-zero pages along with pages->offset and operate on
> > > > that instead.
> > > >
> > > > What do you think?
> > > >
> > > > Peter, any ideas? Should we just leave this for another time?
> > >
> > > Yeah I think a bitmap should save quite a few fields indeed, it'll however
> > > make the latter iteration slightly harder by walking both (offset[],
> > > bitmap), process the page only if bitmap is set for the offset.
> > >
> > > IIUC we perhaps don't even need a bitmap?  AFAIU what we only need in
> > > Multifdpages_t is one extra field to mark "how many normal pages", aka,
> > > normal_num here (zero_num can be calculated from num-normal_num).  Then
> > > the zero page detection logic should do two things:
> > >
> > >   - Sort offset[] array so that it starts with normal pages, followed up by
> > >     zero pages
> > >
> > >   - Setup normal_num to be the number of normal pages
> > >
> > > Then we reduce 2 new arrays (normal[], zero[]) + 2 new fields (normal_num,
> > > zero_num) -> 1 new field (normal_num).  It'll also be trivial to fill the
> > > packet header later because offset[] is exactly that.
> > >
> > > Side note - I still think it's confusing to read this patch and previous
> > > patch separately.  Obviously previous patch introduced these new fields
> > > without justifying their values yet.  IMHO it'll be easier to review if you
> > > merge the two patches.
> >
> > Fabiano, thanks for catching this. I totally missed the backward
> > compatibility thing.
> > Peter, I will code the sorting and merge this patch with the previous one.
> >
> It turns out that we still need to add a "zero_pages" field in
> MultiFDPacket_t because the existing field "pages_alloc" is not the
> total number of pages in "offset". So source can set "zero_pages" from
> pages->num - pages->num_normal but "zero_pages" needs to be set in the
> packet.

Yes, one more field should be needed in MultiFDPacket_t.  Noet that what I
said above was about Multifdpages_t, not MultiFDPacket_t (which is the wire
protocol instead).  To support zero page offloading we should need one more
field for each.

IMHO MultiFDPacket_t.pages_alloc is redundant and actually not useful..
It's just that it existed in the wire protocol already so maybe we'd still
better keep it there..

-- 
Peter Xu