[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 5/5] migration iaa-compress: Implement IAA compression
From: |
Juan Quintela |
Subject: |
Re: [PATCH 5/5] migration iaa-compress: Implement IAA compression |
Date: |
Thu, 19 Oct 2023 13:36:40 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/28.3 (gnu/linux) |
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.
- Re: [PATCH 0/5] Live Migration Acceleration with IAA Compression, (continued)
- Re: [PATCH 0/5] Live Migration Acceleration with IAA Compression, Juan Quintela, 2023/10/19
- [PATCH 1/5] configure: add qpl meson option, Yuan Liu, 2023/10/19
- [PATCH 2/5] qapi/migration: Introduce compress-with-iaa migration parameter, Yuan Liu, 2023/10/19
- [PATCH 4/5] migration iaa-compress: Add IAA initialization and deinitialization, Yuan Liu, 2023/10/19
- [PATCH 3/5] ram compress: Refactor ram compression functions, Yuan Liu, 2023/10/19
- [PATCH 5/5] migration iaa-compress: Implement IAA compression, Yuan Liu, 2023/10/19
- Re: [PATCH 5/5] migration iaa-compress: Implement IAA compression,
Juan Quintela <=