Re: [External] Re: [PATCH v2 3/7] migration/multifd: Zero page transmission on the multifd thread.
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
Re: [External] Re: [PATCH v2 3/7] migration/multifd: Zero page transmission on the multifd thread.
On Thu, Feb 22, 2024 at 9:15 PM Hao Xiang wrote:
>
> On Thu, Feb 22, 2024 at 6:21 PM Peter Xu wrote:
> >
> > On Wed, Feb 21, 2024 at 06:04:10PM -0300, Fabiano Rosas wrote:
> > > Hao Xiang writes:
> > >
> > > > 1. Implements the zero page detection and handling on the multifd
> > > > threads for non-compression, zlib and zstd compression backends.
> > > > 2. Added a new value 'multifd' in ZeroPageDetection enumeration.
> > > > 3. Add proper asserts to ensure pages->normal are used for normal pages
> > > > in all scenarios.
> > > >
> > > > Signed-off-by: Hao Xiang
> > > > ---
> > > > migration/meson.build | 1 +
> > > > migration/multifd-zero-page.c | 59 +++
> > > > migration/multifd-zlib.c | 26 ---
> > > > migration/multifd-zstd.c | 25 ---
> > > > migration/multifd.c | 50 +++--
> > > > migration/multifd.h | 7 +
> > > > qapi/migration.json | 4 ++-
> > > > 7 files changed, 151 insertions(+), 21 deletions(-)
> > > > create mode 100644 migration/multifd-zero-page.c
> > > >
> > > > diff --git a/migration/meson.build b/migration/meson.build
> > > > index 92b1cc4297..1eeb915ff6 100644
> > > > --- a/migration/meson.build
> > > > +++ b/migration/meson.build
> > > > @@ -22,6 +22,7 @@ system_ss.add(files(
> > > >'migration.c',
> > > >'multifd.c',
> > > >'multifd-zlib.c',
> > > > + 'multifd-zero-page.c',
> > > >'ram-compress.c',
> > > >'options.c',
> > > >'postcopy-ram.c',
> > > > diff --git a/migration/multifd-zero-page.c
> > > > b/migration/multifd-zero-page.c
> > > > new file mode 100644
> > > > index 00..f0cd8e2c53
> > > > --- /dev/null
> > > > +++ b/migration/multifd-zero-page.c
> > > > @@ -0,0 +1,59 @@
> > > > +/*
> > > > + * Multifd zero page detection implementation.
> > > > + *
> > > > + * Copyright (c) 2024 Bytedance Inc
> > > > + *
> > > > + * Authors:
> > > > + * Hao Xiang
> > > > + *
> > > > + * This work is licensed under the terms of the GNU GPL, version 2 or
> > > > later.
> > > > + * See the COPYING file in the top-level directory.
> > > > + */
> > > > +
> > > > +#include "qemu/osdep.h"
> > > > +#include "qemu/cutils.h"
> > > > +#include "exec/ramblock.h"
> > > > +#include "migration.h"
> > > > +#include "multifd.h"
> > > > +#include "options.h"
> > > > +#include "ram.h"
> > > > +
> > > > +void multifd_zero_page_check_send(MultiFDSendParams *p)
> > > > +{
> > > > +/*
> > > > + * QEMU older than 9.0 don't understand zero page
> > > > + * on multifd channel. This switch is required to
> > > > + * maintain backward compatibility.
> > > > + */
> > > > +bool use_multifd_zero_page =
> > > > +(migrate_zero_page_detection() == ZERO_PAGE_DETECTION_MULTIFD);
> > > > +MultiFDPages_t *pages = p->pages;
> > > > +RAMBlock *rb = pages->block;
> > > > +
> > > > +assert(pages->num != 0);
> > > > +assert(pages->normal_num == 0);
> > > > +assert(pages->zero_num == 0);
> > >
> > > We can drop these before the final version.
> > >
> > > > +
> > > > +for (int i = 0; i < pages->num; i++) {
> > > > +uint64_t offset = pages->offset[i];
> > > > +if (use_multifd_zero_page &&
> > > > +buffer_is_zero(rb->host + offset, p->page_size)) {
> > > > +pages->zero[pages->zero_num] = offset;
> > > > +pages->zero_num++;
> > > > +ram_release_page(rb->idstr, offset);
> > > > +} else {
> > > > +pages->normal[pages->normal_num] = offset;
> > > > +pages->normal_num++;
> > > > +}
> > > > +}
> > >
> > > 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
>
Re: [External] Re: [PATCH v2 3/7] migration/multifd: Zero page transmission on the multifd thread.
On Thu, Feb 22, 2024 at 8:38 PM Hao Xiang wrote:
>
> On Fri, Feb 16, 2024 at 9:08 PM Richard Henderson
> wrote:
> >
> > On 2/16/24 12:39, Hao Xiang wrote:
> > > +void multifd_zero_page_check_recv(MultiFDRecvParams *p)
> > > +{
> > > +for (int i = 0; i < p->zero_num; i++) {
> > > +void *page = p->host + p->zero[i];
> > > +if (!buffer_is_zero(page, p->page_size)) {
> > > +memset(page, 0, p->page_size);
> > > +}
> > > +}
> > > +}
> >
> > You should not check the buffer is zero here, you should just zero it.
>
> I will fix it in the next version.
I tested with zero out all pages but the performance is bad compared
to previously. In my test case, most pages are zero pages. I think
what happened is that the destination host already has the pages being
zero so performing a memcmp is much faster than memset on all zero
pages.
>
> >
> >
> > r~
Re: [External] Re: [PATCH v2 3/7] migration/multifd: Zero page transmission on the multifd thread.
Hao Xiang writes:
> On Wed, Feb 21, 2024 at 1:04 PM Fabiano Rosas wrote:
>>
>> Hao Xiang writes:
>>
>> > 1. Implements the zero page detection and handling on the multifd
>> > threads for non-compression, zlib and zstd compression backends.
>> > 2. Added a new value 'multifd' in ZeroPageDetection enumeration.
>> > 3. Add proper asserts to ensure pages->normal are used for normal pages
>> > in all scenarios.
>> >
>> > Signed-off-by: Hao Xiang
>> > ---
>> > migration/meson.build | 1 +
>> > migration/multifd-zero-page.c | 59 +++
>> > migration/multifd-zlib.c | 26 ---
>> > migration/multifd-zstd.c | 25 ---
>> > migration/multifd.c | 50 +++--
>> > migration/multifd.h | 7 +
>> > qapi/migration.json | 4 ++-
>> > 7 files changed, 151 insertions(+), 21 deletions(-)
>> > create mode 100644 migration/multifd-zero-page.c
>> >
>> > diff --git a/migration/meson.build b/migration/meson.build
>> > index 92b1cc4297..1eeb915ff6 100644
>> > --- a/migration/meson.build
>> > +++ b/migration/meson.build
>> > @@ -22,6 +22,7 @@ system_ss.add(files(
>> >'migration.c',
>> >'multifd.c',
>> >'multifd-zlib.c',
>> > + 'multifd-zero-page.c',
>> >'ram-compress.c',
>> >'options.c',
>> >'postcopy-ram.c',
>> > diff --git a/migration/multifd-zero-page.c b/migration/multifd-zero-page.c
>> > new file mode 100644
>> > index 00..f0cd8e2c53
>> > --- /dev/null
>> > +++ b/migration/multifd-zero-page.c
>> > @@ -0,0 +1,59 @@
>> > +/*
>> > + * Multifd zero page detection implementation.
>> > + *
>> > + * Copyright (c) 2024 Bytedance Inc
>> > + *
>> > + * Authors:
>> > + * Hao Xiang
>> > + *
>> > + * This work is licensed under the terms of the GNU GPL, version 2 or
>> > later.
>> > + * See the COPYING file in the top-level directory.
>> > + */
>> > +
>> > +#include "qemu/osdep.h"
>> > +#include "qemu/cutils.h"
>> > +#include "exec/ramblock.h"
>> > +#include "migration.h"
>> > +#include "multifd.h"
>> > +#include "options.h"
>> > +#include "ram.h"
>> > +
>> > +void multifd_zero_page_check_send(MultiFDSendParams *p)
>> > +{
>> > +/*
>> > + * QEMU older than 9.0 don't understand zero page
>> > + * on multifd channel. This switch is required to
>> > + * maintain backward compatibility.
>> > + */
>> > +bool use_multifd_zero_page =
>> > +(migrate_zero_page_detection() == ZERO_PAGE_DETECTION_MULTIFD);
>> > +MultiFDPages_t *pages = p->pages;
>> > +RAMBlock *rb = pages->block;
>> > +
>> > +assert(pages->num != 0);
>> > +assert(pages->normal_num == 0);
>> > +assert(pages->zero_num == 0);
>>
>> We can drop these before the final version.
>
> Elena has the same concern. I will drop these.
>
>>
>> > +
>> > +for (int i = 0; i < pages->num; i++) {
>> > +uint64_t offset = pages->offset[i];
>> > +if (use_multifd_zero_page &&
>> > +buffer_is_zero(rb->host + offset, p->page_size)) {
>> > +pages->zero[pages->zero_num] = offset;
>> > +pages->zero_num++;
>> > +ram_release_page(rb->idstr, offset);
>> > +} else {
>> > +pages->normal[pages->normal_num] = offset;
>> > +pages->normal_num++;
>> > +}
>> > +}
>>
>> 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?
>>
>> > +}
>> > +
>> > +void multifd_zero_page_check_recv(MultiFDRecvParams *p)
>> > +{
>> > +for (int i = 0; i < p->zero_num; i++) {
>> > +void *page = p->host + p->zero[i];
>> > +if (!buffer_is_zero(page, p->page_size)) {
>> > +memset(page, 0, p->page_size);
>> > +}
>> > +}
>> > +}
>> > diff --git a/migration/multifd-zlib.c b/migration/multifd-zlib.c
>> > index 012e3bdea1..cdfe0fa70e 100644
>> > --- a/migration/multifd-zlib.c
>> > +++ b/migration/multifd-zlib.c
>> > @@ -123,13 +123,20 @@ static int zlib_send_prepare(MultiFDSendParams *p,
>> > Error **errp)
>> > int ret;
>> > uint32_t i;
>> >
>> > +multifd_zero_page_check_send(p);
>> > +
>> > +if (!pages->normal_num) {
>> > +p->next_packet_size = 0;
>> > +goto out;
>> > +}
>> > +
>> > multifd_send_prepare_header(p);
>> >
>> > -for (i = 0; i < pages->num; i++) {
>> > +for (i = 0; i < pages->normal_num; i++) {
>> > uint32_t available = z->zbuff_len - out_size;
>> > int flush = Z_NO_FLUSH;
>> >
>> > -if (i == pages->num - 1) {
>> > +if (i == pages->normal_num - 1) {
>> > flush = Z_SYNC_FLUSH;
>> > }
>> >
>> > @@ -138,7 +145,7 @@ static int zlib_send_prepare(MultiFDSendParams *p,
>> > Error **errp)
>> >
Re: [External] Re: [PATCH v2 3/7] migration/multifd: Zero page transmission on the multifd thread.
On Wed, Feb 21, 2024 at 1:04 PM Fabiano Rosas wrote:
>
> Hao Xiang writes:
>
> > 1. Implements the zero page detection and handling on the multifd
> > threads for non-compression, zlib and zstd compression backends.
> > 2. Added a new value 'multifd' in ZeroPageDetection enumeration.
> > 3. Add proper asserts to ensure pages->normal are used for normal pages
> > in all scenarios.
> >
> > Signed-off-by: Hao Xiang
> > ---
> > migration/meson.build | 1 +
> > migration/multifd-zero-page.c | 59 +++
> > migration/multifd-zlib.c | 26 ---
> > migration/multifd-zstd.c | 25 ---
> > migration/multifd.c | 50 +++--
> > migration/multifd.h | 7 +
> > qapi/migration.json | 4 ++-
> > 7 files changed, 151 insertions(+), 21 deletions(-)
> > create mode 100644 migration/multifd-zero-page.c
> >
> > diff --git a/migration/meson.build b/migration/meson.build
> > index 92b1cc4297..1eeb915ff6 100644
> > --- a/migration/meson.build
> > +++ b/migration/meson.build
> > @@ -22,6 +22,7 @@ system_ss.add(files(
> >'migration.c',
> >'multifd.c',
> >'multifd-zlib.c',
> > + 'multifd-zero-page.c',
> >'ram-compress.c',
> >'options.c',
> >'postcopy-ram.c',
> > diff --git a/migration/multifd-zero-page.c b/migration/multifd-zero-page.c
> > new file mode 100644
> > index 00..f0cd8e2c53
> > --- /dev/null
> > +++ b/migration/multifd-zero-page.c
> > @@ -0,0 +1,59 @@
> > +/*
> > + * Multifd zero page detection implementation.
> > + *
> > + * Copyright (c) 2024 Bytedance Inc
> > + *
> > + * Authors:
> > + * Hao Xiang
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2 or
> > later.
> > + * See the COPYING file in the top-level directory.
> > + */
> > +
> > +#include "qemu/osdep.h"
> > +#include "qemu/cutils.h"
> > +#include "exec/ramblock.h"
> > +#include "migration.h"
> > +#include "multifd.h"
> > +#include "options.h"
> > +#include "ram.h"
> > +
> > +void multifd_zero_page_check_send(MultiFDSendParams *p)
> > +{
> > +/*
> > + * QEMU older than 9.0 don't understand zero page
> > + * on multifd channel. This switch is required to
> > + * maintain backward compatibility.
> > + */
> > +bool use_multifd_zero_page =
> > +(migrate_zero_page_detection() == ZERO_PAGE_DETECTION_MULTIFD);
> > +MultiFDPages_t *pages = p->pages;
> > +RAMBlock *rb = pages->block;
> > +
> > +assert(pages->num != 0);
> > +assert(pages->normal_num == 0);
> > +assert(pages->zero_num == 0);
>
> We can drop these before the final version.
Elena has the same concern. I will drop these.
>
> > +
> > +for (int i = 0; i < pages->num; i++) {
> > +uint64_t offset = pages->offset[i];
> > +if (use_multifd_zero_page &&
> > +buffer_is_zero(rb->host + offset, p->page_size)) {
> > +pages->zero[pages->zero_num] = offset;
> > +pages->zero_num++;
> > +ram_release_page(rb->idstr, offset);
> > +} else {
> > +pages->normal[pages->normal_num] = offset;
> > +pages->normal_num++;
> > +}
> > +}
>
> 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?
>
> > +}
> > +
> > +void multifd_zero_page_check_recv(MultiFDRecvParams *p)
> > +{
> > +for (int i = 0; i < p->zero_num; i++) {
> > +void *page = p->host + p->zero[i];
> > +if (!buffer_is_zero(page, p->page_size)) {
> > +memset(page, 0, p->page_size);
> > +}
> > +}
> > +}
> > diff --git a/migration/multifd-zlib.c b/migration/multifd-zlib.c
> > index 012e3bdea1..cdfe0fa70e 100644
> > --- a/migration/multifd-zlib.c
> > +++ b/migration/multifd-zlib.c
> > @@ -123,13 +123,20 @@ static int zlib_send_prepare(MultiFDSendParams *p,
> > Error **errp)
> > int ret;
> > uint32_t i;
> >
> > +multifd_zero_page_check_send(p);
> > +
> > +if (!pages->normal_num) {
> > +p->next_packet_size = 0;
> > +goto out;
> > +}
> > +
> > multifd_send_prepare_header(p);
> >
> > -for (i = 0; i < pages->num; i++) {
> > +for (i = 0; i < pages->normal_num; i++) {
> > uint32_t available = z->zbuff_len - out_size;
> > int flush = Z_NO_FLUSH;
> >
> > -if (i == pages->num - 1) {
> > +if (i == pages->normal_num - 1) {
> > flush = Z_SYNC_FLUSH;
> > }
> >
> > @@ -138,7 +145,7 @@ static int zlib_send_prepare(MultiFDSendParams *p,
> > Error **errp)
> > * with compression. zlib does not guarantee that this is safe,
> > * therefore copy the page before calling deflate().
> > */
> > -me
Re: [External] Re: [PATCH v2 3/7] migration/multifd: Zero page transmission on the multifd thread.
On Thu, Feb 22, 2024 at 6:21 PM Peter Xu wrote:
>
> On Wed, Feb 21, 2024 at 06:04:10PM -0300, Fabiano Rosas wrote:
> > Hao Xiang writes:
> >
> > > 1. Implements the zero page detection and handling on the multifd
> > > threads for non-compression, zlib and zstd compression backends.
> > > 2. Added a new value 'multifd' in ZeroPageDetection enumeration.
> > > 3. Add proper asserts to ensure pages->normal are used for normal pages
> > > in all scenarios.
> > >
> > > Signed-off-by: Hao Xiang
> > > ---
> > > migration/meson.build | 1 +
> > > migration/multifd-zero-page.c | 59 +++
> > > migration/multifd-zlib.c | 26 ---
> > > migration/multifd-zstd.c | 25 ---
> > > migration/multifd.c | 50 +++--
> > > migration/multifd.h | 7 +
> > > qapi/migration.json | 4 ++-
> > > 7 files changed, 151 insertions(+), 21 deletions(-)
> > > create mode 100644 migration/multifd-zero-page.c
> > >
> > > diff --git a/migration/meson.build b/migration/meson.build
> > > index 92b1cc4297..1eeb915ff6 100644
> > > --- a/migration/meson.build
> > > +++ b/migration/meson.build
> > > @@ -22,6 +22,7 @@ system_ss.add(files(
> > >'migration.c',
> > >'multifd.c',
> > >'multifd-zlib.c',
> > > + 'multifd-zero-page.c',
> > >'ram-compress.c',
> > >'options.c',
> > >'postcopy-ram.c',
> > > diff --git a/migration/multifd-zero-page.c b/migration/multifd-zero-page.c
> > > new file mode 100644
> > > index 00..f0cd8e2c53
> > > --- /dev/null
> > > +++ b/migration/multifd-zero-page.c
> > > @@ -0,0 +1,59 @@
> > > +/*
> > > + * Multifd zero page detection implementation.
> > > + *
> > > + * Copyright (c) 2024 Bytedance Inc
> > > + *
> > > + * Authors:
> > > + * Hao Xiang
> > > + *
> > > + * This work is licensed under the terms of the GNU GPL, version 2 or
> > > later.
> > > + * See the COPYING file in the top-level directory.
> > > + */
> > > +
> > > +#include "qemu/osdep.h"
> > > +#include "qemu/cutils.h"
> > > +#include "exec/ramblock.h"
> > > +#include "migration.h"
> > > +#include "multifd.h"
> > > +#include "options.h"
> > > +#include "ram.h"
> > > +
> > > +void multifd_zero_page_check_send(MultiFDSendParams *p)
> > > +{
> > > +/*
> > > + * QEMU older than 9.0 don't understand zero page
> > > + * on multifd channel. This switch is required to
> > > + * maintain backward compatibility.
> > > + */
> > > +bool use_multifd_zero_page =
> > > +(migrate_zero_page_detection() == ZERO_PAGE_DETECTION_MULTIFD);
> > > +MultiFDPages_t *pages = p->pages;
> > > +RAMBlock *rb = pages->block;
> > > +
> > > +assert(pages->num != 0);
> > > +assert(pages->normal_num == 0);
> > > +assert(pages->zero_num == 0);
> >
> > We can drop these before the final version.
> >
> > > +
> > > +for (int i = 0; i < pages->num; i++) {
> > > +uint64_t offset = pages->offset[i];
> > > +if (use_multifd_zero_page &&
> > > +buffer_is_zero(rb->host + offset, p->page_size)) {
> > > +pages->zero[pages->zero_num] = offset;
> > > +pages->zero_num++;
> > > +ram_release_page(rb->idstr, offset);
> > > +} else {
> > > +pages->normal[pages->normal_num] = offset;
> > > +pages->normal_num++;
> > > +}
> > > +}
> >
> > 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 t
Re: [External] Re: [PATCH v2 3/7] migration/multifd: Zero page transmission on the multifd thread.
On Wed, Feb 21, 2024 at 8:00 AM Elena Ufimtseva wrote:
>
>
>
> On Fri, Feb 16, 2024 at 2:42 PM Hao Xiang wrote:
>>
>> 1. Implements the zero page detection and handling on the multifd
>> threads for non-compression, zlib and zstd compression backends.
>> 2. Added a new value 'multifd' in ZeroPageDetection enumeration.
>> 3. Add proper asserts to ensure pages->normal are used for normal pages
>> in all scenarios.
>>
>> Signed-off-by: Hao Xiang
>> ---
>> migration/meson.build | 1 +
>> migration/multifd-zero-page.c | 59 +++
>> migration/multifd-zlib.c | 26 ---
>> migration/multifd-zstd.c | 25 ---
>> migration/multifd.c | 50 +++--
>> migration/multifd.h | 7 +
>> qapi/migration.json | 4 ++-
>> 7 files changed, 151 insertions(+), 21 deletions(-)
>> create mode 100644 migration/multifd-zero-page.c
>>
>> diff --git a/migration/meson.build b/migration/meson.build
>> index 92b1cc4297..1eeb915ff6 100644
>> --- a/migration/meson.build
>> +++ b/migration/meson.build
>> @@ -22,6 +22,7 @@ system_ss.add(files(
>>'migration.c',
>>'multifd.c',
>>'multifd-zlib.c',
>> + 'multifd-zero-page.c',
>>'ram-compress.c',
>>'options.c',
>>'postcopy-ram.c',
>> diff --git a/migration/multifd-zero-page.c b/migration/multifd-zero-page.c
>> new file mode 100644
>> index 00..f0cd8e2c53
>> --- /dev/null
>> +++ b/migration/multifd-zero-page.c
>> @@ -0,0 +1,59 @@
>> +/*
>> + * Multifd zero page detection implementation.
>> + *
>> + * Copyright (c) 2024 Bytedance Inc
>> + *
>> + * Authors:
>> + * Hao Xiang
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
>> + * See the COPYING file in the top-level directory.
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "qemu/cutils.h"
>> +#include "exec/ramblock.h"
>> +#include "migration.h"
>> +#include "multifd.h"
>> +#include "options.h"
>> +#include "ram.h"
>> +
>> +void multifd_zero_page_check_send(MultiFDSendParams *p)
>> +{
>> +/*
>> + * QEMU older than 9.0 don't understand zero page
>> + * on multifd channel. This switch is required to
>> + * maintain backward compatibility.
>> + */
>> +bool use_multifd_zero_page =
>> +(migrate_zero_page_detection() == ZERO_PAGE_DETECTION_MULTIFD);
>> +MultiFDPages_t *pages = p->pages;
>> +RAMBlock *rb = pages->block;
>> +
>> +assert(pages->num != 0);
>
>
> Not needed, the check is done right before calling send_prepare.
>
>>
>> +assert(pages->normal_num == 0);
>> +assert(pages->zero_num == 0);
>
>
> Why these asserts are needed?
The idea is that when multifd_zero_page_check_send is called, I want
to make sure zero page checking was not processed on this packet
before. It is perhaps redundant. It won't compile in free build.
>>
>> +
>>
>> +for (int i = 0; i < pages->num; i++) {
>> +uint64_t offset = pages->offset[i];
>> +if (use_multifd_zero_page &&
>> +buffer_is_zero(rb->host + offset, p->page_size)) {
>> +pages->zero[pages->zero_num] = offset;
>> +pages->zero_num++;
>> +ram_release_page(rb->idstr, offset);
>> +} else {
>> +pages->normal[pages->normal_num] = offset;
>> +pages->normal_num++;
>> +}
>> +}
>> +}
>> +
>> +void multifd_zero_page_check_recv(MultiFDRecvParams *p)
>> +{
>> +for (int i = 0; i < p->zero_num; i++) {
>> +void *page = p->host + p->zero[i];
>> +if (!buffer_is_zero(page, p->page_size)) {
>> +memset(page, 0, p->page_size);
>> +}
>> +}
>> +}
>> diff --git a/migration/multifd-zlib.c b/migration/multifd-zlib.c
>> index 012e3bdea1..cdfe0fa70e 100644
>> --- a/migration/multifd-zlib.c
>> +++ b/migration/multifd-zlib.c
>> @@ -123,13 +123,20 @@ static int zlib_send_prepare(MultiFDSendParams *p,
>> Error **errp)
>> int ret;
>> uint32_t i;
>>
>> +multifd_zero_page_check_send(p);
>> +
>> +if (!pages->normal_num) {
>> +p->next_packet_size = 0;
>> +goto out;
>> +}
>> +
>> multifd_send_prepare_header(p);
>>
>> -for (i = 0; i < pages->num; i++) {
>> +for (i = 0; i < pages->normal_num; i++) {
>> uint32_t available = z->zbuff_len - out_size;
>> int flush = Z_NO_FLUSH;
>>
>> -if (i == pages->num - 1) {
>> +if (i == pages->normal_num - 1) {
>> flush = Z_SYNC_FLUSH;
>> }
>>
>> @@ -138,7 +145,7 @@ static int zlib_send_prepare(MultiFDSendParams *p, Error
>> **errp)
>> * with compression. zlib does not guarantee that this is safe,
>> * therefore copy the page before calling deflate().
>> */
>> -memcpy(z->buf, p->pages->block->host + pages->offset[i],
>> p->page_size);
>> +memcpy(z->buf, p->pages->block->host + pages->normal[i],
>> p->page_size);
>> zs->avail_in = p->page_size;
>>
Re: [External] Re: [PATCH v2 3/7] migration/multifd: Zero page transmission on the multifd thread.
On Fri, Feb 16, 2024 at 9:08 PM Richard Henderson
wrote:
>
> On 2/16/24 12:39, Hao Xiang wrote:
> > +void multifd_zero_page_check_recv(MultiFDRecvParams *p)
> > +{
> > +for (int i = 0; i < p->zero_num; i++) {
> > +void *page = p->host + p->zero[i];
> > +if (!buffer_is_zero(page, p->page_size)) {
> > +memset(page, 0, p->page_size);
> > +}
> > +}
> > +}
>
> You should not check the buffer is zero here, you should just zero it.
I will fix it in the next version.
>
>
> r~
Re: [PATCH v2 3/7] migration/multifd: Zero page transmission on the multifd thread.
On Wed, Feb 21, 2024 at 06:04:10PM -0300, Fabiano Rosas wrote:
> Hao Xiang writes:
>
> > 1. Implements the zero page detection and handling on the multifd
> > threads for non-compression, zlib and zstd compression backends.
> > 2. Added a new value 'multifd' in ZeroPageDetection enumeration.
> > 3. Add proper asserts to ensure pages->normal are used for normal pages
> > in all scenarios.
> >
> > Signed-off-by: Hao Xiang
> > ---
> > migration/meson.build | 1 +
> > migration/multifd-zero-page.c | 59 +++
> > migration/multifd-zlib.c | 26 ---
> > migration/multifd-zstd.c | 25 ---
> > migration/multifd.c | 50 +++--
> > migration/multifd.h | 7 +
> > qapi/migration.json | 4 ++-
> > 7 files changed, 151 insertions(+), 21 deletions(-)
> > create mode 100644 migration/multifd-zero-page.c
> >
> > diff --git a/migration/meson.build b/migration/meson.build
> > index 92b1cc4297..1eeb915ff6 100644
> > --- a/migration/meson.build
> > +++ b/migration/meson.build
> > @@ -22,6 +22,7 @@ system_ss.add(files(
> >'migration.c',
> >'multifd.c',
> >'multifd-zlib.c',
> > + 'multifd-zero-page.c',
> >'ram-compress.c',
> >'options.c',
> >'postcopy-ram.c',
> > diff --git a/migration/multifd-zero-page.c b/migration/multifd-zero-page.c
> > new file mode 100644
> > index 00..f0cd8e2c53
> > --- /dev/null
> > +++ b/migration/multifd-zero-page.c
> > @@ -0,0 +1,59 @@
> > +/*
> > + * Multifd zero page detection implementation.
> > + *
> > + * Copyright (c) 2024 Bytedance Inc
> > + *
> > + * Authors:
> > + * Hao Xiang
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2 or
> > later.
> > + * See the COPYING file in the top-level directory.
> > + */
> > +
> > +#include "qemu/osdep.h"
> > +#include "qemu/cutils.h"
> > +#include "exec/ramblock.h"
> > +#include "migration.h"
> > +#include "multifd.h"
> > +#include "options.h"
> > +#include "ram.h"
> > +
> > +void multifd_zero_page_check_send(MultiFDSendParams *p)
> > +{
> > +/*
> > + * QEMU older than 9.0 don't understand zero page
> > + * on multifd channel. This switch is required to
> > + * maintain backward compatibility.
> > + */
> > +bool use_multifd_zero_page =
> > +(migrate_zero_page_detection() == ZERO_PAGE_DETECTION_MULTIFD);
> > +MultiFDPages_t *pages = p->pages;
> > +RAMBlock *rb = pages->block;
> > +
> > +assert(pages->num != 0);
> > +assert(pages->normal_num == 0);
> > +assert(pages->zero_num == 0);
>
> We can drop these before the final version.
>
> > +
> > +for (int i = 0; i < pages->num; i++) {
> > +uint64_t offset = pages->offset[i];
> > +if (use_multifd_zero_page &&
> > +buffer_is_zero(rb->host + offset, p->page_size)) {
> > +pages->zero[pages->zero_num] = offset;
> > +pages->zero_num++;
> > +ram_release_page(rb->idstr, offset);
> > +} else {
> > +pages->normal[pages->normal_num] = offset;
> > +pages->normal_num++;
> > +}
> > +}
>
> 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.
>
> > +}
> > +
> > +void multifd_zero_page_check_recv(MultiFDRecvParams *p)
> > +{
> > +for (int i = 0; i < p->zero_num; i++) {
> > +void *page = p->host + p->zero[i];
> > +if (!buffer_is_zero(page, p->page_size)) {
> > +memset(page, 0, p->page_size);
> > +}
> > +}
> > +}
> > diff --git a/migration/multifd-zlib.c b/migration/multifd-zlib.c
> > index 012e3bdea1..cdfe
Re: [PATCH v2 3/7] migration/multifd: Zero page transmission on the multifd thread.
Hao Xiang writes:
> 1. Implements the zero page detection and handling on the multifd
> threads for non-compression, zlib and zstd compression backends.
> 2. Added a new value 'multifd' in ZeroPageDetection enumeration.
> 3. Add proper asserts to ensure pages->normal are used for normal pages
> in all scenarios.
>
> Signed-off-by: Hao Xiang
> ---
> migration/meson.build | 1 +
> migration/multifd-zero-page.c | 59 +++
> migration/multifd-zlib.c | 26 ---
> migration/multifd-zstd.c | 25 ---
> migration/multifd.c | 50 +++--
> migration/multifd.h | 7 +
> qapi/migration.json | 4 ++-
> 7 files changed, 151 insertions(+), 21 deletions(-)
> create mode 100644 migration/multifd-zero-page.c
>
> diff --git a/migration/meson.build b/migration/meson.build
> index 92b1cc4297..1eeb915ff6 100644
> --- a/migration/meson.build
> +++ b/migration/meson.build
> @@ -22,6 +22,7 @@ system_ss.add(files(
>'migration.c',
>'multifd.c',
>'multifd-zlib.c',
> + 'multifd-zero-page.c',
>'ram-compress.c',
>'options.c',
>'postcopy-ram.c',
> diff --git a/migration/multifd-zero-page.c b/migration/multifd-zero-page.c
> new file mode 100644
> index 00..f0cd8e2c53
> --- /dev/null
> +++ b/migration/multifd-zero-page.c
> @@ -0,0 +1,59 @@
> +/*
> + * Multifd zero page detection implementation.
> + *
> + * Copyright (c) 2024 Bytedance Inc
> + *
> + * Authors:
> + * Hao Xiang
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/cutils.h"
> +#include "exec/ramblock.h"
> +#include "migration.h"
> +#include "multifd.h"
> +#include "options.h"
> +#include "ram.h"
> +
> +void multifd_zero_page_check_send(MultiFDSendParams *p)
> +{
> +/*
> + * QEMU older than 9.0 don't understand zero page
> + * on multifd channel. This switch is required to
> + * maintain backward compatibility.
> + */
> +bool use_multifd_zero_page =
> +(migrate_zero_page_detection() == ZERO_PAGE_DETECTION_MULTIFD);
> +MultiFDPages_t *pages = p->pages;
> +RAMBlock *rb = pages->block;
> +
> +assert(pages->num != 0);
> +assert(pages->normal_num == 0);
> +assert(pages->zero_num == 0);
We can drop these before the final version.
> +
> +for (int i = 0; i < pages->num; i++) {
> +uint64_t offset = pages->offset[i];
> +if (use_multifd_zero_page &&
> +buffer_is_zero(rb->host + offset, p->page_size)) {
> +pages->zero[pages->zero_num] = offset;
> +pages->zero_num++;
> +ram_release_page(rb->idstr, offset);
> +} else {
> +pages->normal[pages->normal_num] = offset;
> +pages->normal_num++;
> +}
> +}
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?
> +}
> +
> +void multifd_zero_page_check_recv(MultiFDRecvParams *p)
> +{
> +for (int i = 0; i < p->zero_num; i++) {
> +void *page = p->host + p->zero[i];
> +if (!buffer_is_zero(page, p->page_size)) {
> +memset(page, 0, p->page_size);
> +}
> +}
> +}
> diff --git a/migration/multifd-zlib.c b/migration/multifd-zlib.c
> index 012e3bdea1..cdfe0fa70e 100644
> --- a/migration/multifd-zlib.c
> +++ b/migration/multifd-zlib.c
> @@ -123,13 +123,20 @@ static int zlib_send_prepare(MultiFDSendParams *p,
> Error **errp)
> int ret;
> uint32_t i;
>
> +multifd_zero_page_check_send(p);
> +
> +if (!pages->normal_num) {
> +p->next_packet_size = 0;
> +goto out;
> +}
> +
> multifd_send_prepare_header(p);
>
> -for (i = 0; i < pages->num; i++) {
> +for (i = 0; i < pages->normal_num; i++) {
> uint32_t available = z->zbuff_len - out_size;
> int flush = Z_NO_FLUSH;
>
> -if (i == pages->num - 1) {
> +if (i == pages->normal_num - 1) {
> flush = Z_SYNC_FLUSH;
> }
>
> @@ -138,7 +145,7 @@ static int zlib_send_prepare(MultiFDSendParams *p, Error
> **errp)
> * with compression. zlib does not guarantee that this is safe,
> * therefore copy the page before calling deflate().
> */
> -memcpy(z->buf, p->pages->block->host + pages->offset[i],
> p->page_size);
> +memcpy(z->buf, p->pages->block->host + pages->normal[i],
> p->page_size);
> zs->avail_in = p->page_size;
> zs->next_in = z->buf;
>
> @@ -172,10 +179,10 @@ static int zlib_send_prepare(MultiFDSendParams *p,
> Error **errp)
> p->iov[p->iovs_num].iov_len = out_size;
> p
Re: [PATCH v2 3/7] migration/multifd: Zero page transmission on the multifd thread.
On Fri, Feb 16, 2024 at 2:42 PM Hao Xiang wrote:
> 1. Implements the zero page detection and handling on the multifd
> threads for non-compression, zlib and zstd compression backends.
> 2. Added a new value 'multifd' in ZeroPageDetection enumeration.
> 3. Add proper asserts to ensure pages->normal are used for normal pages
> in all scenarios.
>
> Signed-off-by: Hao Xiang
> ---
> migration/meson.build | 1 +
> migration/multifd-zero-page.c | 59 +++
> migration/multifd-zlib.c | 26 ---
> migration/multifd-zstd.c | 25 ---
> migration/multifd.c | 50 +++--
> migration/multifd.h | 7 +
> qapi/migration.json | 4 ++-
> 7 files changed, 151 insertions(+), 21 deletions(-)
> create mode 100644 migration/multifd-zero-page.c
>
> diff --git a/migration/meson.build b/migration/meson.build
> index 92b1cc4297..1eeb915ff6 100644
> --- a/migration/meson.build
> +++ b/migration/meson.build
> @@ -22,6 +22,7 @@ system_ss.add(files(
>'migration.c',
>'multifd.c',
>'multifd-zlib.c',
> + 'multifd-zero-page.c',
>'ram-compress.c',
>'options.c',
>'postcopy-ram.c',
> diff --git a/migration/multifd-zero-page.c b/migration/multifd-zero-page.c
> new file mode 100644
> index 00..f0cd8e2c53
> --- /dev/null
> +++ b/migration/multifd-zero-page.c
> @@ -0,0 +1,59 @@
> +/*
> + * Multifd zero page detection implementation.
> + *
> + * Copyright (c) 2024 Bytedance Inc
> + *
> + * Authors:
> + * Hao Xiang
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or
> later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/cutils.h"
> +#include "exec/ramblock.h"
> +#include "migration.h"
> +#include "multifd.h"
> +#include "options.h"
> +#include "ram.h"
> +
> +void multifd_zero_page_check_send(MultiFDSendParams *p)
> +{
> +/*
> + * QEMU older than 9.0 don't understand zero page
> + * on multifd channel. This switch is required to
> + * maintain backward compatibility.
> + */
> +bool use_multifd_zero_page =
> +(migrate_zero_page_detection() == ZERO_PAGE_DETECTION_MULTIFD);
> +MultiFDPages_t *pages = p->pages;
> +RAMBlock *rb = pages->block;
> +
> +assert(pages->num != 0);
>
Not needed, the check is done right before calling send_prepare.
> +assert(pages->normal_num == 0);
> +assert(pages->zero_num == 0);
>
Why these asserts are needed?
> +
>
+for (int i = 0; i < pages->num; i++) {
> +uint64_t offset = pages->offset[i];
> +if (use_multifd_zero_page &&
> +buffer_is_zero(rb->host + offset, p->page_size)) {
> +pages->zero[pages->zero_num] = offset;
> +pages->zero_num++;
> +ram_release_page(rb->idstr, offset);
> +} else {
> +pages->normal[pages->normal_num] = offset;
> +pages->normal_num++;
> +}
> +}
> +}
> +
> +void multifd_zero_page_check_recv(MultiFDRecvParams *p)
> +{
> +for (int i = 0; i < p->zero_num; i++) {
> +void *page = p->host + p->zero[i];
> +if (!buffer_is_zero(page, p->page_size)) {
> +memset(page, 0, p->page_size);
> +}
> +}
> +}
> diff --git a/migration/multifd-zlib.c b/migration/multifd-zlib.c
> index 012e3bdea1..cdfe0fa70e 100644
> --- a/migration/multifd-zlib.c
> +++ b/migration/multifd-zlib.c
> @@ -123,13 +123,20 @@ static int zlib_send_prepare(MultiFDSendParams *p,
> Error **errp)
> int ret;
> uint32_t i;
>
> +multifd_zero_page_check_send(p);
> +
> +if (!pages->normal_num) {
> +p->next_packet_size = 0;
> +goto out;
> +}
> +
> multifd_send_prepare_header(p);
>
> -for (i = 0; i < pages->num; i++) {
> +for (i = 0; i < pages->normal_num; i++) {
> uint32_t available = z->zbuff_len - out_size;
> int flush = Z_NO_FLUSH;
>
> -if (i == pages->num - 1) {
> +if (i == pages->normal_num - 1) {
> flush = Z_SYNC_FLUSH;
> }
>
> @@ -138,7 +145,7 @@ static int zlib_send_prepare(MultiFDSendParams *p,
> Error **errp)
> * with compression. zlib does not guarantee that this is safe,
> * therefore copy the page before calling deflate().
> */
> -memcpy(z->buf, p->pages->block->host + pages->offset[i],
> p->page_size);
> +memcpy(z->buf, p->pages->block->host + pages->normal[i],
> p->page_size);
> zs->avail_in = p->page_size;
> zs->next_in = z->buf;
>
> @@ -172,10 +179,10 @@ static int zlib_send_prepare(MultiFDSendParams *p,
> Error **errp)
> p->iov[p->iovs_num].iov_len = out_size;
> p->iovs_num++;
> p->next_packet_size = out_size;
> -p->flags |= MULTIFD_FLAG_ZLIB;
>
> +out:
> +p->flags |= MULTIFD_FLAG_ZLIB;
> multifd_send_fill_packet(p);
> -
>
Spurious?
return 0;
> }
>
> @@ -261,6 +268,14 @@ s
Re: [PATCH v2 3/7] migration/multifd: Zero page transmission on the multifd thread.
Hao Xiang writes:
> 1. Implements the zero page detection and handling on the multifd
> threads for non-compression, zlib and zstd compression backends.
> 2. Added a new value 'multifd' in ZeroPageDetection enumeration.
> 3. Add proper asserts to ensure pages->normal are used for normal pages
> in all scenarios.
>
> Signed-off-by: Hao Xiang
[...]
> diff --git a/qapi/migration.json b/qapi/migration.json
> index 99843a8e95..e2450b92d4 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -660,9 +660,11 @@
> #
> # @none: Do not perform zero page checking.
> #
> +# @multifd: Perform zero page checking on the multifd sender thread. (since
> 9.0)
As pointed out in my review of PATCH 3, the entire type is since 9.0.
> +#
> ##
> { 'enum': 'ZeroPageDetection',
> - 'data': [ 'legacy', 'none' ] }
> + 'data': [ 'legacy', 'none', 'multifd' ] }
>
> ##
> # @BitmapMigrationBitmapAliasTransform:
I don't like having 'none' (don't detect) between the two ways to
detect. Put it either first or last.
Re: [PATCH v2 3/7] migration/multifd: Zero page transmission on the multifd thread.
On 2/16/24 12:39, Hao Xiang wrote:
+void multifd_zero_page_check_recv(MultiFDRecvParams *p)
+{
+for (int i = 0; i < p->zero_num; i++) {
+void *page = p->host + p->zero[i];
+if (!buffer_is_zero(page, p->page_size)) {
+memset(page, 0, p->page_size);
+}
+}
+}
You should not check the buffer is zero here, you should just zero it.
r~
