Re: [PATCH 5/5] migration iaa-compress: Implement IAA compression

[Juan Quintela <quintela@redhat.com>]

Yuan Liu <yuan1.liu@intel.com> wrote:
> Implement the functions of IAA for data compression and decompression.
> The implementation uses non-blocking job submission and polling to check
> the job completion status to reduce IAA's overhead in the live migration
> process.
>
> Signed-off-by: Yuan Liu <yuan1.liu@intel.com>
> Reviewed-by: Nanhai Zou <nanhai.zou@intel.com>


> +static void process_completed_job(IaaJob *job, send_iaa_data send_page)
> +{
> +    if (job->is_compression) {
> +        send_page(job->param.comp.block, job->param.comp.offset,
> +                  job->out_buf, job->out_len, job->param.comp.result);
> +    } else {
> +        assert(job->out_len == qemu_target_page_size());
> +        memcpy(job->param.decomp.host, job->out_buf, job->out_len);
> +    }
> +    put_job(job);
> +}

Shouldn't it be easier to add a helper to job struct and not having that
if here?  I.e. become:

static void process_completed_job(IaaJob *job, send_iaa_data send_page)
{
    job->completed(job, send_page);
    put_job(job);
}

And do proper initializations.  You can even put the send_page callback
in the job struct.

> +static qpl_status check_job_status(IaaJob *job, bool block)
> +{
> +    qpl_status status;
> +    qpl_job *qpl = job->qpl;
> +
> +    status = block ? qpl_wait_job(qpl) : qpl_check_job(qpl);
> +    if (status == QPL_STS_OK) {
> +        job->out_len = qpl->total_out;
> +        if (job->is_compression) {
> +            job->param.comp.result = RES_COMPRESS;
> +            /* if no compression benefit, send a normal page for migration */
> +            if (job->out_len == qemu_target_page_size()) {
> +                iaa_comp_param *param = &(job->param.comp);
> +                memcpy(job->out_buf, (param->block->host + param->offset),
> +                       job->out_len);
> +                job->param.comp.result = RES_NONE;
> +            }
> +        }
> +    } else if (status == QPL_STS_MORE_OUTPUT_NEEDED) {
> +        if (job->is_compression) {
> +            /*
> +             * if the compressed data is larger than the original data, send a
> +             * normal page for migration, in this case, IAA has copied the
> +             * original data to job->out_buf automatically.
> +             */
> +            job->out_len = qemu_target_page_size();
> +            job->param.comp.result = RES_NONE;
> +            status = QPL_STS_OK;
> +        }
> +    }

Again, this function for decompression becomes a single line:

    status = block ? qpl_wait_job(qpl) : qpl_check_job(qpl);
    if (status == QPL_STS_OK) {
        job->out_len = qpl->total_out;
    }

Wait complicate it?

> +static void check_polling_jobs(send_iaa_data send_page)
> +{
> +    IaaJob *job, *job_next;
> +    qpl_status status;
> +
> +    QSIMPLEQ_FOREACH_SAFE(job, &polling_queue, entry, job_next) {
> +        status = check_job_status(job, false);
> +        if (status == QPL_STS_OK) { /* job has done */
> +            process_completed_job(job, send_page);
> +            QSIMPLEQ_REMOVE_HEAD(&polling_queue, entry);
> +        } else if (status == QPL_STS_BEING_PROCESSED) { /* job is running */
> +            break;
> +        } else {
> +            abort();

Not even printing an error message?

The two callers of check_polling_jobs() can return an error, so no
reason to abort() here.

Later, Juan.