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

[Yichen Wang]

On Thu, Aug 22, 2024 at 4:06 AM Prasad Pandit <ppan...@redhat.com> wrote:
>
> Hello,
>
> On Tue, 20 Aug 2024 at 22:40, Yichen Wang <yichen.w...@bytedance.com> wrote:
> > +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);
> > +
> > +    /*
> > +     * Initialize QAT device with software fallback by default. This allows
> > +     * QATzip to use CPU path when QAT hardware reaches maximum throughput.
> > +     */
> > +    ret = qzInit(&q->sess, true);
> > +    if (ret != QZ_OK && ret != QZ_DUPLICATE) {
> > +        err_msg = "qzInit failed";
> > +        goto err;
> > +    }
> > +
> > +    ret = qzGetDefaultsDeflate(&params);
> > +    if (ret != QZ_OK) {
> > +        err_msg = "qzGetDefaultsDeflate failed";
> > +        goto err;
> > +    }
> > +
> > +    /* 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;
> > +    }
> > +
> > +    if (MULTIFD_PACKET_SIZE > UINT32_MAX) {
> > +        err_msg = "packet size too large for QAT";
> > +        goto err;
> > +    }
> > +
> > +    q->in_len = MULTIFD_PACKET_SIZE;
> > +    /*
> > +     * PINNED_MEM is an enum from qatzip headers, which means to use
> > +     * kzalloc_node() to allocate memory for QAT DMA purposes. When QAT 
> > device
> > +     * is not available or software fallback is used, the malloc flag 
> > needs to
> > +     * be set as COMMON_MEM.
> > +     */
> > +    q->in_buf = qzMalloc(q->in_len, 0, PINNED_MEM);
> > +    if (!q->in_buf) {
> > +        q->in_buf = qzMalloc(q->in_len, 0, COMMON_MEM);
> > +        if (!q->in_buf) {
> > +            err_msg = "qzMalloc failed";
> > +            goto err;
> > +        }
> > +    }
> > +
> > +    q->out_len = qzMaxCompressedLength(MULTIFD_PACKET_SIZE, &q->sess);
> > +    q->out_buf = qzMalloc(q->out_len, 0, PINNED_MEM);
> > +    if (!q->out_buf) {
> > +        q->out_buf = qzMalloc(q->out_len, 0, COMMON_MEM);
> > +        if (!q->out_buf) {
> > +            err_msg = "qzMalloc failed";
> > +            goto err;
> > +        }
> > +    }
> > +
> > +    return 0;
> > +
> > +err:
> > +    error_setg(errp, "multifd %u: [sender] %s", p->id, err_msg);
> > +    return -1;
> > +}
>
> * Need to release (g_free OR qatzip_send_cleanup) allocated memory in
> the error (err:) path.
>

The patch was originally written exactly like what you suggest,
cleanup in the error path of the same function. However, later I
realized in gdb that I was wrong. The qatzip_send_cleanup() function
will be called later in another thread in both normal and error paths.
So I revised the patch to this behavior, otherwise I will run into
double free in the error path.

>
> > +static int qatzip_recv_setup(MultiFDRecvParams *p, Error **errp)
> > +{
> > +    QatzipData *q;
> > +    QzSessionParamsDeflate_T params;
> > +    const char *err_msg;
> > +    int ret;
> > +
> > +    q = g_new0(QatzipData, 1);
> > +    p->compress_data = q;
> > +
> > +    /*
> > +     * Initialize QAT device with software fallback by default. This allows
> > +     * QATzip to use CPU path when QAT hardware reaches maximum throughput.
> > +     */
> > +    ret = qzInit(&q->sess, true);
> > +    if (ret != QZ_OK && ret != QZ_DUPLICATE) {
> > +        err_msg = "qzInit failed";
> > +        goto err;
> > +    }
> > +
> > +    ret = qzGetDefaultsDeflate(&params);
> > +    if (ret != QZ_OK) {
> > +        err_msg = "qzGetDefaultsDeflate failed";
> > +        goto err;
> > +    }
> > +
> > +    ret = qzSetupSessionDeflate(&q->sess, &params);
> > +    if (ret != QZ_OK && ret != QZ_DUPLICATE) {
> > +        err_msg = "qzSetupSessionDeflate failed";
> > +        goto err;
> > +    }
> > +
> > +    /*
> > +     * Reserve extra spaces for the incoming packets. Current 
> > implementation
> > +     * doesn't send uncompressed pages in case the compression gets too 
> > big.
> > +     */
> > +    q->in_len = MULTIFD_PACKET_SIZE * 2;
> > +    /*
> > +     * PINNED_MEM is an enum from qatzip headers, which means to use
> > +     * kzalloc_node() to allocate memory for QAT DMA purposes. When QAT 
> > device
> > +     * is not available or software fallback is used, the malloc flag 
> > needs to
> > +     * be set as COMMON_MEM.
> > +     */
> > +    q->in_buf = qzMalloc(q->in_len, 0, PINNED_MEM);
> > +    if (!q->in_buf) {
> > +        q->in_buf = qzMalloc(q->in_len, 0, COMMON_MEM);
> > +        if (!q->in_buf) {
> > +            err_msg = "qzMalloc failed";
> > +            goto err;
> > +        }
> > +    }
> > +
> > +    q->out_len = MULTIFD_PACKET_SIZE;
> > +    q->out_buf = qzMalloc(q->out_len, 0, PINNED_MEM);
> > +    if (!q->out_buf) {
> > +        q->out_buf = qzMalloc(q->out_len, 0, COMMON_MEM);
> > +        if (!q->out_buf) {
> > +            err_msg = "qzMalloc failed";
> > +            goto err;
> > +        }
> > +    }
> > +
> > +    return 0;
> > +
> > +err:
> > +    error_setg(errp, "multifd %u: [receiver] %s", p->id, err_msg);
> > +    return -1;
> > +}
>
> * Need to release (g_free OR qatzip_recv_cleanup) allocated memory in
> the error (err:) path.
>
> Thank you.
> ---
>   - Prasad
>