Yichen Wang <yichen.w...@bytedance.com> writes:
> On Fri, Jul 12, 2024 at 7:17 AM Fabiano Rosas <faro...@suse.de> wrote:
>>
>> Yichen Wang <yichen.w...@bytedance.com> writes:
>>
>> > From: Bryan Zhang <bryan.zh...@bytedance.com>
>> >
>> > Adds support for 'qatzip' as an option for the multifd compression
>> > method parameter, and implements using QAT for 'qatzip' compression and
>> > decompression.
>> >
>> > Signed-off-by: Bryan Zhang <bryan.zh...@bytedance.com>
>> > Signed-off-by: Hao Xiang <hao.xi...@linux.dev>
>> > Signed-off-by: Yichen Wang <yichen.w...@bytedance.com>
>> > ---
>> > hw/core/qdev-properties-system.c | 6 +-
>> > migration/meson.build | 1 +
>> > migration/multifd-qatzip.c | 403 +++++++++++++++++++++++++++++++
>> > migration/multifd.h | 5 +-
>> > qapi/migration.json | 3 +
>> > tests/qtest/meson.build | 4 +
>> > 6 files changed, 419 insertions(+), 3 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 f13350b4fb..eb50d6ec5b 100644
>> > --- a/hw/core/qdev-properties-system.c
>> > +++ b/hw/core/qdev-properties-system.c
>> > @@ -659,7 +659,11 @@ const PropertyInfo qdev_prop_fdc_drive_type = {
>> > const PropertyInfo qdev_prop_multifd_compression = {
>> > .name = "MultiFDCompression",
>> > .description = "multifd_compression values, "
>> > - "none/zlib/zstd/qpl/uadk",
>> > + "none/zlib/zstd/qpl/uadk"
>> > +#ifdef CONFIG_QATZIP
>> > + "/qatzip"
>> > +#endif
>>
>> It seems the other accelerators don't need the ifdef. What's different
>> here?
>
> Just changed and align to other methods. Will fix in next version.
>
>>
>> > + ,
>> > .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 5ce2acb41e..c9454c26ae 100644
>> > --- a/migration/meson.build
>> > +++ b/migration/meson.build
>> > @@ -41,6 +41,7 @@ system_ss.add(when: rdma, if_true: files('rdma.c'))
>> > system_ss.add(when: zstd, if_true: files('multifd-zstd.c'))
>> > system_ss.add(when: qpl, if_true: files('multifd-qpl.c'))
>> > system_ss.add(when: uadk, if_true: files('multifd-uadk.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..d01d51de8f
>> > --- /dev/null
>> > +++ b/migration/multifd-qatzip.c
>> > @@ -0,0 +1,403 @@
>> > +/*
>> > + * Multifd QATzip compression implementation
>> > + *
>> > + * Copyright (c) Bytedance
>> > + *
>> > + * Authors:
>> > + * Bryan Zhang <bryan.zh...@bytedance.com>
>> > + * Hao Xiang <hao.xi...@bytedance.com>
>> > + * Yichen Wang <yichen.w...@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 "qapi/error.h"
>> > +#include "qemu/error-report.h"
>> > +#include "qapi/qapi-types-migration.h"
>> > +#include "options.h"
>> > +#include "multifd.h"
>> > +#include <qatzip.h>
>> > +
>> > +typedef struct {
>> > + /*
>> > + * Unique session for use with QATzip API
>> > + */
>> > + QzSession_T sess;
>> > +
>> > + /*
>> > + * For compression: Buffer for pages to compress
>> > + * For decompression: Buffer for data to decompress
>> > + */
>> > + uint8_t *in_buf;
>> > + uint32_t in_len;
>> > +
>> > + /*
>> > + * For compression: Output buffer of compressed data
>> > + * For decompression: Output buffer of decompressed data
>> > + */
>> > + uint8_t *out_buf;
>> > + uint32_t out_len;
>> > +} QatzipData;
>> > +
>> > +/**
>> > + * qatzip_send_setup: Set up QATzip session and private buffers.
>> > + *
>> > + * @param p Multifd channel params
>> > + * @param errp Pointer to error, which will be set in case of error
>> > + * @return 0 on success, -1 on error (and *errp will be set)
>> > + */
>> > +static int qatzip_send_setup(MultiFDSendParams *p, Error **errp)
>> > +{
>> > + QatzipData *q;
>> > + QzSessionParamsDeflate_T params;
>> > + const char *err_msg;
>> > + int ret;
>> > +
>> > + q = g_new0(QatzipData, 1);
>> > + p->compress_data = q;
>> > + /* We need one extra place for the packet header */
>> > + p->iov = g_new0(struct iovec, 2);
>> > +
>> > + /* Prefer without sw_fallback because of bad performance with
>> > sw_fallback.
>> > + * Warn if sw_fallback needs to be used. */
>>
>> Please run scripts/checkpatch.pl on your series. This style of comments
>> should have been flagged as non-conformant with our guidelines.
>
> Sorry for that. Will fix in next version.
>
>>
>> > + ret = qzInit(&q->sess, false);
>> > + if (ret != QZ_OK && ret != QZ_DUPLICATE) {
>> > + /* Warn, and try with sw_fallback. */
>> > + warn_report("Initilizing QAT with sw_fallback...");
>>
>> This will warn for each multifd channel, maybe use warn_report_once
>> instead. Also s/Initilizing/Initializing/ and let's spell out "software
>> fallback".
>>
>
> Will fix in next version.
>
>> > + ret = qzInit(&q->sess, true);
>> > + if (ret != QZ_OK && ret != QZ_DUPLICATE) {
>> > + /* Warn, and try with sw_fallback. */
>> > + err_msg = "qzInit failed";
>> > + goto err_free_q;
>> > + }
>> > + }
>> > +
>> > + ret = qzGetDefaultsDeflate(¶ms);
>> > + if (ret != QZ_OK) {
>> > + err_msg = "qzGetDefaultsDeflate failed";
>> > + goto err_close;
>> > + }
>> > +
>> > + /* Make sure to use configured QATzip compression level. */
>> > + params.common_params.comp_lvl = migrate_multifd_qatzip_level();
>> > +
>> > + ret = qzSetupSessionDeflate(&q->sess, ¶ms);
>> > + if (ret != QZ_OK && ret != QZ_DUPLICATE) {
>> > + err_msg = "qzSetupSessionDeflate failed";
>> > + goto err_close;
>> > + }
>> > +
>> > + if (MULTIFD_PACKET_SIZE > UINT32_MAX) {
>> > + err_msg = "packet size too large for QAT";
>> > + goto err_close;
>> > + }
>> > +
>> > + q->in_len = MULTIFD_PACKET_SIZE;
>> > + q->in_buf = qzMalloc(q->in_len, 0, PINNED_MEM);
>> > + if (!q->in_buf) {
>> > + err_msg = "qzMalloc failed";
>> > + goto err_close;
>> > + }
>> > +
>> > + q->out_len = qzMaxCompressedLength(MULTIFD_PACKET_SIZE, &q->sess);
>> > + q->out_buf = qzMalloc(q->out_len, 0, PINNED_MEM);
>> > + if (!q->out_buf) {
>> > + err_msg = "qzMalloc failed";
>> > + goto err_free_inbuf;
>> > + }
>> > +
>> > + return 0;
>> > +
>> > +err_free_inbuf:
>> > + qzFree(q->in_buf);
>> > +err_close:
>> > + qzClose(&q->sess);
>> > +err_free_q:
>> > + g_free(q);
>> > + g_free(p->iov);
>> > + p->iov = NULL;
>> > + p->compress_data = NULL;
>> > + error_setg(errp, "multifd %u: %s", p->id, err_msg);
>> > + return -1;
>> > +}
>> > +
>> > +/**
>> > + * qatzip_send_cleanup: Tear down QATzip session and release private
>> > buffers.
>> > + *
>> > + * @param p Multifd channel params
>> > + * @param errp Pointer to error, which will be set in case of error
>> > + * @return None
>> > + */
>> > +static void qatzip_send_cleanup(MultiFDSendParams *p, Error **errp)
>> > +{
>> > + QatzipData *q = p->compress_data;
>> > + const char *err_msg;
>> > + int ret;
>> > +
>> > + ret = qzTeardownSession(&q->sess);
>> > + if (ret != QZ_OK) {
>> > + err_msg = "qzTeardownSession failed";
>> > + goto err;
>> > + }
>> > +
>> > + ret = qzClose(&q->sess);
>> > + if (ret != QZ_OK) {
>> > + err_msg = "qzClose failed";
>> > + goto err;
>> > + }
>>
>> Can qzClose() be called twice on the same session pointer? It's possible
>> that we have already failed at multifd_send_setup() and still reach
>> here.
>>
>> And what about qzTeardownSession()? Can it cope with an already closed
>> session?
>>
>> And what about the sessions that never got created because we might have
>> exited early at the ops->send_setup() loop?
>>
>
> qzTeardownSession() and qzClose() are safe to call on NULL pointers.
> But thanks to your comments which corrected my understanding. These
> patch was wrote under the impression that when setup() failed,
> cleanup() won't be fired. After learning in gdb, apparently I was
> wrong. The cleanup() will be called from another thread, which will be
> called regardless if setup() returns zero or non-zero. I will rewrite
> the setup()/cleanup() logics in my next patchset.
>
>> > +
>> > + qzFree(q->in_buf);
>> > + q->in_buf = NULL;
>> > + qzFree(q->out_buf);
>> > + q->out_buf = NULL;
>>
>> These will double free here if send_setup has already freed.
>>
>> > + g_free(p->iov);
>> > + p->iov = NULL;
>> > + g_free(p->compress_data);
>> > + p->compress_data = NULL;
>> > + return;
>> > +
>> > +err:
>> > + error_setg(errp, "multifd %u: %s", p->id, err_msg);
>> > +}
>> > +
>> > +/**
>> > + * qatzip_send_prepare: Compress pages and update IO channel info.
>> > + *
>> > + * @param p Multifd channel params
>> > + * @param errp Pointer to error, which will be set in case of error
>> > + * @return 0 on success, -1 on error (and *errp will be set)
>> > + */
>> > +static int qatzip_send_prepare(MultiFDSendParams *p, Error **errp)
>> > +{
>> > + MultiFDPages_t *pages = p->pages;
>> > + QatzipData *q = p->compress_data;
>> > + int ret;
>> > + unsigned int in_len, out_len;
>> > +
>> > + if (!multifd_send_prepare_common(p)) {
>> > + goto out;
>> > + }
>> > +
>> > + /* Unlike other multifd compression implementations, we use a
>> > + * non-streaming API and place all the data into one buffer, rather
>> > than
>> > + * sending each page to the compression API at a time. */
>> > + for (int i = 0; i < pages->normal_num; i++) {
>> > + memcpy(q->in_buf + (i * p->page_size),
>> > + p->pages->block->host + pages->offset[i],
>>
>> pages->block->host
>>
>
> I am not sure if I understand your comment here?
You're holding the pointer to p->pages in the local pages variable. I'm
suggesting you use it instead of p->pages here. In another series[1], we're
looking into changing p->pages into something else and having to change
it all over the code gets bothersome.
1- https://lore.kernel.org/r/20240620212111.29319-2-faro...@suse.de
