From: "Liu, Yuan1" <yuan1.liu@intel.com>
To: Fabiano Rosas <farosas@suse.de>, "peterx@redhat.com" <peterx@redhat.com>
Cc: "qemu-devel@nongnu.org" <qemu-devel@nongnu.org>,
"Zou, Nanhai" <nanhai.zou@intel.com>
Subject: RE: [PATCH v6 5/7] migration/multifd: implement initialization of qpl compression
Date: Sat, 11 May 2024 12:55:13 +0000 [thread overview]
Message-ID: <PH7PR11MB59412D4757F9B176EB9B2C6FA3E02@PH7PR11MB5941.namprd11.prod.outlook.com> (raw)
In-Reply-To: <87o79dflb3.fsf@suse.de>
> -----Original Message-----
> From: Fabiano Rosas <farosas@suse.de>
> Sent: Saturday, May 11, 2024 4:45 AM
> To: Liu, Yuan1 <yuan1.liu@intel.com>; peterx@redhat.com
> Cc: qemu-devel@nongnu.org; Liu, Yuan1 <yuan1.liu@intel.com>; Zou, Nanhai
> <nanhai.zou@intel.com>
> Subject: Re: [PATCH v6 5/7] migration/multifd: implement initialization of
> qpl compression
>
> Yuan Liu <yuan1.liu@intel.com> writes:
>
> > the qpl initialization includes memory allocation for compressed
> > data and the qpl job initialization.
> >
> > the qpl job initialization will check if the In-Memory Analytics
> > Accelerator(IAA) device is available and use the IAA device first.
> > If the platform does not have IAA device or the IAA device is not
> > available, the qpl compression will fallback to the software path.
> >
> > Signed-off-by: Yuan Liu <yuan1.liu@intel.com>
> > Reviewed-by: Nanhai Zou <nanhai.zou@intel.com>
>
> Looks good, just some nits below.
>
> > ---
> > migration/multifd-qpl.c | 272 +++++++++++++++++++++++++++++++++++++++-
> > 1 file changed, 271 insertions(+), 1 deletion(-)
> >
> > diff --git a/migration/multifd-qpl.c b/migration/multifd-qpl.c
> > index 056a68a060..89fa51091a 100644
> > --- a/migration/multifd-qpl.c
> > +++ b/migration/multifd-qpl.c
> > @@ -9,12 +9,282 @@
> > * 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 "qemu/module.h"
> > +#include "qapi/error.h"
> > +#include "migration.h"
> > +#include "multifd.h"
> > +#include "qpl/qpl.h"
> > +
> > +typedef struct {
> > + qpl_job **job_array;
> > + /* the number of allocated jobs */
> > + uint32_t total_job_num;
> > + /* compressed data buffer */
> > + uint8_t *zbuf;
> > + /* the length of compressed data */
>
> array of lenghts
Thanks for the comment, I will improve the comment next version.
> > + uint32_t *zbuf_hdr;
>
> Why the _hdr suffix if the lengths are the only data stored here?
This zbuf_hdr is confusing, I will use lens instead of zbuf_hdr next version
> > + /* the status of IAA device */
> > + bool iaa_avail;
> > +} QplData;
> > +
> > +/**
> > + * check_iaa_avail: check if IAA device is available
> > + *
> > + * If the system does not have an IAA device, the IAA device is
> > + * not enabled or the IAA work queue is not configured as a shared
> > + * mode, the QPL hardware path initialization will fail.
> > + *
> > + * Returns true if IAA device is available, otherwise false.
> > + */
> > +static bool check_iaa_avail(void)
> > +{
> > + qpl_job *job = NULL;
> > + uint32_t job_size = 0;
> > + qpl_path_t path = qpl_path_hardware;
> > +
> > + if (qpl_get_job_size(path, &job_size) != QPL_STS_OK) {
> > + return false;
> > + }
> > + job = g_malloc0(job_size);
> > + if (qpl_init_job(path, job) != QPL_STS_OK) {
> > + g_free(job);
> > + return false;
> > + }
> > + g_free(job);
> > + return true;
> > +}
> > +
> > +/**
> > + * multifd_qpl_free_jobs: cleanup jobs
> > + *
> > + * Free all job resources.
> > + *
> > + * @qpl: pointer to the QplData structure
> > + */
> > +static void multifd_qpl_free_jobs(QplData *qpl)
> > +{
> > + assert(qpl != NULL);
> > + for (int i = 0; i < qpl->total_job_num; i++) {
> > + qpl_fini_job(qpl->job_array[i]);
> > + g_free(qpl->job_array[i]);
> > + qpl->job_array[i] = NULL;
> > + }
> > + g_free(qpl->job_array);
> > + qpl->job_array = NULL;
> > +}
> > +
> > +/**
> > + * multifd_qpl_init_jobs: initialize jobs
> > + *
> > + * Initialize all jobs
> > + *
> > + * @qpl: pointer to the QplData structure
> > + * @chan_id: multifd channel number
> > + * @errp: pointer to an error
> > + */
> > +static int multifd_qpl_init_jobs(QplData *qpl, uint8_t chan_id, Error
> **errp)
> > +{
> > + qpl_path_t path;
> > + qpl_status status;
> > + uint32_t job_size = 0;
> > + qpl_job *job = NULL;
> > +
> > + path = qpl->iaa_avail ? qpl_path_hardware : qpl_path_software;
> > + status = qpl_get_job_size(path, &job_size);
> > + if (status != QPL_STS_OK) {
> > + error_setg(errp, "multifd: %u: qpl_get_job_size failed with
> error %d",
> > + chan_id, status);
> > + return -1;
> > + }
> > + qpl->job_array = g_new0(qpl_job *, qpl->total_job_num);
> > + for (int i = 0; i < qpl->total_job_num; i++) {
> > + job = g_malloc0(job_size);
> > + status = qpl_init_job(path, job);
> > + if (status != QPL_STS_OK) {
> > + error_setg(errp, "multifd: %u: qpl_init_job failed with
> error %d",
> > + chan_id, status);
> > + multifd_qpl_free_jobs(qpl);
> > + return -1;
> > + }
> > + qpl->job_array[i] = job;
> > + }
> > + return 0;
> > +}
> > +
> > +/**
> > + * multifd_qpl_init: initialize QplData structure
> > + *
> > + * Allocate and initialize a QplData structure
> > + *
> > + * Returns QplData pointer for success or NULL for error
> > + *
> > + * @job_num: pointer to the QplData structure
>
> This needs updating^
Yes, thanks for the comment, I will fix it next version
> > + * @job_size: the buffer size of the job
> > + * @chan_id: multifd channel number
> > + * @errp: pointer to an error
> > + */
> > +static QplData *multifd_qpl_init(uint32_t job_num, uint32_t job_size,
> > + uint8_t chan_id, Error **errp)
> > +{
> > + QplData *qpl;
> > +
> > + qpl = g_new0(QplData, 1);
> > + qpl->total_job_num = job_num;
> > + qpl->iaa_avail = check_iaa_avail();
> > + if (multifd_qpl_init_jobs(qpl, chan_id, errp) != 0) {
> > + g_free(qpl);
> > + return NULL;
> > + }
> > + qpl->zbuf = g_malloc0(job_size * job_num);
> > + qpl->zbuf_hdr = g_new0(uint32_t, job_num);
> > + return qpl;
> > +}
> > +
> > +/**
> > + * multifd_qpl_deinit: cleanup QplData structure
> > + *
> > + * Free jobs, comprssed buffers and QplData structure
>
> compressed
Thanks for the comment, I will fix the typo next version
> > + *
> > + * @qpl: pointer to the QplData structure
> > + */
> > +static void multifd_qpl_deinit(QplData *qpl)
> > +{
> > + if (qpl != NULL) {
> > + multifd_qpl_free_jobs(qpl);
> > + g_free(qpl->zbuf_hdr);
> > + g_free(qpl->zbuf);
> > + g_free(qpl);
> > + }
> > +}
> > +
> > +/**
> > + * multifd_qpl_send_setup: setup send side
> > + *
> > + * Setup each channel with QPL compression.
>
> s/each/the/
Sure, I will refine the comment next version
> > + *
> > + * Returns 0 for success or -1 for error
> > + *
> > + * @p: Params for the channel that we are using
> > + * @errp: pointer to an error
> > + */
> > +static int multifd_qpl_send_setup(MultiFDSendParams *p, Error **errp)
> > +{
> > + QplData *qpl;
> > +
> > + qpl = multifd_qpl_init(p->page_count, p->page_size, p->id, errp);
> > + if (!qpl) {
> > + return -1;
> > + }
> > + p->compress_data = qpl;
> > +
> > + /*
> > + * Each page will be compressed independently and sent using an
> IOV. The
> > + * additional two IOVs are used to store packet header and
> compressed data
> > + * length
> > + */
> > + p->iov = g_new0(struct iovec, p->page_count + 2);
> > + return 0;
> > +}
> > +
> > +/**
> > + * multifd_qpl_send_cleanup: cleanup send side
> > + *
> > + * Close the channel and return memory.
> > + *
> > + * @p: Params for the channel that we are using
> > + * @errp: pointer to an error
> > + */
> > +static void multifd_qpl_send_cleanup(MultiFDSendParams *p, Error
> **errp)
> > +{
> > + multifd_qpl_deinit(p->compress_data);
> > + p->compress_data = NULL;
> > + g_free(p->iov);
> > + p->iov = NULL;
> > +}
> > +
> > +/**
> > + * multifd_qpl_send_prepare: prepare data to be able to send
> > + *
> > + * Create a compressed buffer with all the pages that we are going to
> > + * send.
> > + *
> > + * Returns 0 for success or -1 for error
> > + *
> > + * @p: Params for the channel that we are using
> > + * @errp: pointer to an error
> > + */
> > +static int multifd_qpl_send_prepare(MultiFDSendParams *p, Error **errp)
> > +{
> > + /* Implement in next patch */
> > + return -1;
> > +}
> > +
> > +/**
> > + * multifd_qpl_recv_setup: setup receive side
> > + *
> > + * Create the compressed channel and buffer.
> > + *
> > + * Returns 0 for success or -1 for error
> > + *
> > + * @p: Params for the channel that we are using
> > + * @errp: pointer to an error
> > + */
> > +static int multifd_qpl_recv_setup(MultiFDRecvParams *p, Error **errp)
> > +{
> > + QplData *qpl;
> > +
> > + qpl = multifd_qpl_init(p->page_count, p->page_size, p->id, errp);
> > + if (!qpl) {
> > + return -1;
> > + }
> > + p->compress_data = qpl;
> > + return 0;
> > +}
> > +
> > +/**
> > + * multifd_qpl_recv_cleanup: setup receive side
> > + *
> > + * Close the channel and return memory.
> > + *
> > + * @p: Params for the channel that we are using
> > + */
> > +static void multifd_qpl_recv_cleanup(MultiFDRecvParams *p)
> > +{
> > + multifd_qpl_deinit(p->compress_data);
> > + p->compress_data = NULL;
> > +}
> > +
> > +/**
> > + * multifd_qpl_recv: read the data from the channel into actual pages
> > + *
> > + * Read the compressed buffer, and uncompress it into the actual
> > + * pages.
> > + *
> > + * Returns 0 for success or -1 for error
> > + *
> > + * @p: Params for the channel that we are using
> > + * @errp: pointer to an error
> > + */
> > +static int multifd_qpl_recv(MultiFDRecvParams *p, Error **errp)
> > +{
> > + /* Implement in next patch */
> > + return -1;
> > +}
> > +
> > +static MultiFDMethods multifd_qpl_ops = {
> > + .send_setup = multifd_qpl_send_setup,
> > + .send_cleanup = multifd_qpl_send_cleanup,
> > + .send_prepare = multifd_qpl_send_prepare,
> > + .recv_setup = multifd_qpl_recv_setup,
> > + .recv_cleanup = multifd_qpl_recv_cleanup,
> > + .recv = multifd_qpl_recv,
> > +};
> >
> > static void multifd_qpl_register(void)
> > {
> > - /* noop */
> > + multifd_register_ops(MULTIFD_COMPRESSION_QPL, &multifd_qpl_ops);
> > }
> >
> > migration_init(multifd_qpl_register);
next prev parent reply other threads:[~2024-05-11 12:56 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-05 16:57 [PATCH v6 0/7] Live Migration With IAA Yuan Liu
2024-05-05 16:57 ` [PATCH v6 1/7] docs/migration: add qpl compression feature Yuan Liu
2024-05-05 16:57 ` [PATCH v6 2/7] migration/multifd: put IOV initialization into compression method Yuan Liu
2024-05-10 20:21 ` Fabiano Rosas
2024-05-27 20:50 ` Peter Xu
2024-05-28 13:36 ` Liu, Yuan1
2024-05-28 15:32 ` Peter Xu
2024-05-05 16:57 ` [PATCH v6 3/7] configure: add --enable-qpl build option Yuan Liu
2024-05-05 16:57 ` [PATCH v6 4/7] migration/multifd: add qpl compression method Yuan Liu
2024-05-10 14:12 ` Fabiano Rosas
2024-05-10 14:23 ` Liu, Yuan1
2024-05-05 16:57 ` [PATCH v6 5/7] migration/multifd: implement initialization of qpl compression Yuan Liu
2024-05-10 20:45 ` Fabiano Rosas
2024-05-11 12:55 ` Liu, Yuan1 [this message]
2024-05-10 20:52 ` Fabiano Rosas
2024-05-11 12:39 ` Liu, Yuan1
2024-05-05 16:57 ` [PATCH v6 6/7] migration/multifd: implement qpl compression and decompression Yuan Liu
2024-05-13 15:13 ` Fabiano Rosas
2024-05-14 6:30 ` Liu, Yuan1
2024-05-14 14:08 ` Fabiano Rosas
2024-05-15 6:36 ` Liu, Yuan1
2024-05-05 16:57 ` [PATCH v6 7/7] tests/migration-test: add qpl compression test Yuan Liu
2024-05-27 20:56 ` Peter Xu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=PH7PR11MB59412D4757F9B176EB9B2C6FA3E02@PH7PR11MB5941.namprd11.prod.outlook.com \
--to=yuan1.liu@intel.com \
--cc=farosas@suse.de \
--cc=nanhai.zou@intel.com \
--cc=peterx@redhat.com \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).