Re: [External] Re: [PATCH 3/5] migration: Introduce unimplemented 'qatzip' compression method

[Hao Xiang]

On Fri, Jan 5, 2024 at 3:52 PM Hao Xiang <hao.xiang@bytedance.com> wrote:
>
> On Fri, Jan 5, 2024 at 12:07 PM Fabiano Rosas <farosas@suse.de> wrote:
> >
> > Bryan Zhang <bryan.zhang@bytedance.com> writes:
> >
> > +cc Yuan Liu, Daniel Berrangé
> >
> > > Adds support for 'qatzip' as an option for the multifd compression
> > > method parameter, but copy-pastes the no-op logic to leave the actual
> > > methods effectively unimplemented. This is in preparation of a
> > > subsequent commit that will implement actually using QAT for compression
> > > and decompression.
> > >
> > > Signed-off-by: Bryan Zhang <bryan.zhang@bytedance.com>
> > > Signed-off-by: Hao Xiang <hao.xiang@bytedance.com>
> > > ---
> > >  hw/core/qdev-properties-system.c |  6 ++-
> > >  migration/meson.build            |  1 +
> > >  migration/multifd-qatzip.c       | 81 ++++++++++++++++++++++++++++++++
> > >  migration/multifd.h              |  1 +
> > >  qapi/migration.json              |  5 +-
> > >  5 files changed, 92 insertions(+), 2 deletions(-)
> > >  create mode 100644 migration/multifd-qatzip.c
> > >
> > > diff --git a/hw/core/qdev-properties-system.c 
> > > b/hw/core/qdev-properties-system.c
> > > index 1a396521d5..d8e48dcb0e 100644
> > > --- a/hw/core/qdev-properties-system.c
> > > +++ b/hw/core/qdev-properties-system.c
> > > @@ -658,7 +658,11 @@ const PropertyInfo qdev_prop_fdc_drive_type = {
> > >  const PropertyInfo qdev_prop_multifd_compression = {
> > >      .name = "MultiFDCompression",
> > >      .description = "multifd_compression values, "
> > > -                   "none/zlib/zstd",
> > > +                   "none/zlib/zstd"
> > > +#ifdef CONFIG_QATZIP
> > > +                   "/qatzip"
> > > +#endif
> > > +                   ,
> > >      .enum_table = &MultiFDCompression_lookup,
> > >      .get = qdev_propinfo_get_enum,
> > >      .set = qdev_propinfo_set_enum,
> > > diff --git a/migration/meson.build b/migration/meson.build
> > > index 92b1cc4297..e20f318379 100644
> > > --- a/migration/meson.build
> > > +++ b/migration/meson.build
> > > @@ -40,6 +40,7 @@ if get_option('live_block_migration').allowed()
> > >    system_ss.add(files('block.c'))
> > >  endif
> > >  system_ss.add(when: zstd, if_true: files('multifd-zstd.c'))
> > > +system_ss.add(when: qatzip, if_true: files('multifd-qatzip.c'))
> > >
> > >  specific_ss.add(when: 'CONFIG_SYSTEM_ONLY',
> > >                  if_true: files('ram.c',
> > > diff --git a/migration/multifd-qatzip.c b/migration/multifd-qatzip.c
> > > new file mode 100644
> > > index 0000000000..1733bbddb7
> > > --- /dev/null
> > > +++ b/migration/multifd-qatzip.c
> > > @@ -0,0 +1,81 @@
> > > +/*
> > > + * Multifd QATzip compression implementation
> > > + *
> > > + * Copyright (c) Bytedance
> > > + *
> > > + * Authors:
> > > + *  Bryan Zhang <bryan.zhang@bytedance.com>
> > > + *  Hao Xiang   <hao.xiang@bytedance.com>
> > > + *
> > > + * 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 "exec/ramblock.h"
> > > +#include "exec/target_page.h"
> > > +#include "qapi/error.h"
> > > +#include "migration.h"
> > > +#include "options.h"
> > > +#include "multifd.h"
> > > +
> > > +static int qatzip_send_setup(MultiFDSendParams *p, Error **errp)
> > > +{
> > > +    return 0;
> > > +}
> > > +
> > > +static void qatzip_send_cleanup(MultiFDSendParams *p, Error **errp) {};
> > > +
> > > +static int qatzip_send_prepare(MultiFDSendParams *p, Error **errp)
> > > +{
> > > +    MultiFDPages_t *pages = p->pages;
> > > +
> > > +    for (int i = 0; i < p->normal_num; i++) {
> > > +        p->iov[p->iovs_num].iov_base = pages->block->host + p->normal[i];
> > > +        p->iov[p->iovs_num].iov_len = p->page_size;
> > > +        p->iovs_num++;
> > > +    }
> > > +
> > > +    p->next_packet_size = p->normal_num * p->page_size;
> > > +    p->flags |= MULTIFD_FLAG_NOCOMP;
> > > +    return 0;
> > > +}
> > > +
> > > +static int qatzip_recv_setup(MultiFDRecvParams *p, Error **errp)
> > > +{
> > > +    return 0;
> > > +}
> > > +
> > > +static void qatzip_recv_cleanup(MultiFDRecvParams *p) {};
> > > +
> > > +static int qatzip_recv_pages(MultiFDRecvParams *p, Error **errp)
> > > +{
> > > +    uint32_t flags = p->flags & MULTIFD_FLAG_COMPRESSION_MASK;
> > > +
> > > +    if (flags != MULTIFD_FLAG_NOCOMP) {
> > > +        error_setg(errp, "multifd %u: flags received %x flags expected 
> > > %x",
> > > +                   p->id, flags, MULTIFD_FLAG_NOCOMP);
> > > +        return -1;
> > > +    }
> > > +    for (int i = 0; i < p->normal_num; i++) {
> > > +        p->iov[i].iov_base = p->host + p->normal[i];
> > > +        p->iov[i].iov_len = p->page_size;
> > > +    }
> > > +    return qio_channel_readv_all(p->c, p->iov, p->normal_num, errp);
> > > +}
> > > +
> > > +static MultiFDMethods multifd_qatzip_ops = {
> > > +    .send_setup = qatzip_send_setup,
> > > +    .send_cleanup = qatzip_send_cleanup,
> > > +    .send_prepare = qatzip_send_prepare,
> > > +    .recv_setup = qatzip_recv_setup,
> > > +    .recv_cleanup = qatzip_recv_cleanup,
> > > +    .recv_pages = qatzip_recv_pages
> > > +};
> > > +
> > > +static void multifd_qatzip_register(void)
> > > +{
> > > +    multifd_register_ops(MULTIFD_COMPRESSION_QATZIP, 
> > > &multifd_qatzip_ops);
> > > +}
> > > +
> > > +migration_init(multifd_qatzip_register);
> > > diff --git a/migration/multifd.h b/migration/multifd.h
> > > index a835643b48..5600f7fc82 100644
> > > --- a/migration/multifd.h
> > > +++ b/migration/multifd.h
> > > @@ -33,6 +33,7 @@ int multifd_queue_page(QEMUFile *f, RAMBlock *block, 
> > > ram_addr_t offset);
> > >  #define MULTIFD_FLAG_NOCOMP (0 << 1)
> > >  #define MULTIFD_FLAG_ZLIB (1 << 1)
> > >  #define MULTIFD_FLAG_ZSTD (2 << 1)
> > > +#define MULTIFD_FLAG_QATZIP (3 << 1)
> > >
> > >  /* This value needs to be a multiple of qemu_target_page_size() */
> > >  #define MULTIFD_PACKET_SIZE (512 * 1024)
> > > diff --git a/qapi/migration.json b/qapi/migration.json
> > > index 6d5a4b0489..e3cc195aed 100644
> > > --- a/qapi/migration.json
> > > +++ b/qapi/migration.json
> > > @@ -625,11 +625,14 @@
> > >  #
> > >  # @zstd: use zstd compression method.
> > >  #
> > > +# @qatzip: use qatzip compression method.
> > > +#
> > >  # Since: 5.0
> > >  ##
> > >  { 'enum': 'MultiFDCompression',
> > >    'data': [ 'none', 'zlib',
> > > -            { 'name': 'zstd', 'if': 'CONFIG_ZSTD' } ] }
> > > +            { 'name': 'zstd', 'if': 'CONFIG_ZSTD' },
> > > +            { 'name': 'qatzip', 'if': 'CONFIG_QATZIP'} ] }
> >
> > In another thread adding support to another Intel accelerator (IAA) we
> > decided that it was better to select the offloading as an accelerator
> > method to multifd zlib rather than as an entirely new compression
> > format. Take a look at that discussion:
> > https://lore.kernel.org/r/ZTFCnqbbqlmsUkRC@redhat.com
>
> We had some early discussion with Intel folks (probably a different
> team than the one with the above patchset). The understanding at the
> time is that QAT is good at both bulk data compression and
> decompression. IAA is good at decompression with smaller data size but
> compression performance is not the best. In multifd, we are
> compressing up to 128 4k pages at a time and potentially this can
> increase by configuring the packet size, at the time we thought QAT
> could be a better fit in the multifd live migration scenario. We would
> like to hear more from Intel.
>
> From our benchmark testing, with two QAT devices, we can get deflate
> compression throughout to around 7GB/s with ~160% CPU. That's beating
> the current software implementation (zlib and zstd) by a lot. We are
> still tuning the configuration in QEMU live migration now.
>
> >
> > As I understand it, QAT + QATzip would be compatible with both zlib and
> > IAA + QPL, so we'd add another accelerator method like this:
> >
> > 20240103112851.908082-3-yuan1.liu@intel.com">https://lore.kernel.org/r/20240103112851.908082-3-yuan1.liu@intel.com
> >
>
> I quickly read over the IAA patchset and I saw this:
>
> "However, due to some reasons, QPL is currently
> not compatible with the existing Zlib method that Zlib compressed data
> can be decompressed by QPl and vice versa."
>
> The above probably means the current zlib software implementation and
> IAA are not compatible.
>
> For QAT, although, both Intel's QATzip and zlib are internally using
> deflate, QATzip only supports deflate with a 4 byte header, deflate
> wrapped by Gzip header and footer, or deflate wrapped by Intel® QAT
> Gzip* extension header and footer. None of the headers can be
> recognized by zlib software implementation is my understanding. So if
> we want to make them compatible with zlib, the QATzip library needs to
> support that.

I took a closer look at Intel's documentation here
https://github.com/intel/QATzip
QATzip does have a compression format QZ_DEFLATE_RAW, which uses the
deflate format but no header at all. This looks like the same as what
QEMU's current zlib implementation does - using the raw deflate
format. I can have a quick test to confirm that. Meanwhile, the
documentation mentioned that if using the raw deflate format,
decompression cannot be offloaded by QAT hardware. So there is a
trade-off here if we want to avoid creating a new compression format
in QEMU.

>
> > All that, of course, assuming we even want to support both
> > accelerators. They're addressing the same problem after all. I wonder
> > how we'd choose a precedence, since both seem to be present in the same
> > processor family.
> >
> >
>
> That's an interesting question :-) I think overall performance
> (throughput and CPU overhead) should both be considered. IAA and QAT
> accelerators don't present on all systems. We Bytedance choose to have
> both on our platform when purchasing from Intel.