Re: [External] Re: [PATCH v5 4/5] migration: Introduce 'qatzip' compression method

[Fabiano Rosas <farosas@suse.de>]

Yichen Wang <yichen.wang@bytedance.com> writes:

> On Fri, Jul 12, 2024 at 7:17 AM Fabiano Rosas <farosas@suse.de> wrote:
>>
>> Yichen Wang <yichen.wang@bytedance.com> writes:
>>
>> > From: Bryan Zhang <bryan.zhang@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.zhang@bytedance.com>
>> > Signed-off-by: Hao Xiang <hao.xiang@linux.dev>
>> > Signed-off-by: Yichen Wang <yichen.wang@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.zhang@bytedance.com>
>> > + *  Hao Xiang <hao.xiang@bytedance.com>
>> > + *  Yichen Wang <yichen.wang@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(&params);
>> > +    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, &params);
>> > +    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-farosas@suse.de