> -----Original Message-----
> From: Peter Xu <pet...@redhat.com>
> Sent: Tuesday, May 28, 2024 4:51 AM
> To: Liu, Yuan1 <yuan1....@intel.com>
> Cc: faro...@suse.de; qemu-devel@nongnu.org; Zou, Nanhai
> <nanhai....@intel.com>
> Subject: Re: [PATCH v6 2/7] migration/multifd: put IOV initialization into
> compression method
>
> On Mon, May 06, 2024 at 12:57:46AM +0800, Yuan Liu wrote:
> > Different compression methods may require different numbers of IOVs.
> > Based on streaming compression of zlib and zstd, all pages will be
> > compressed to a data block, so two IOVs are needed for packet header
> > and compressed data block.
> >
> > Signed-off-by: Yuan Liu <yuan1....@intel.com>
> > Reviewed-by: Nanhai Zou <nanhai....@intel.com>
> > ---
> > migration/multifd-zlib.c | 7 +++++++
> > migration/multifd-zstd.c | 8 +++++++-
> > migration/multifd.c | 22 ++++++++++++----------
> > 3 files changed, 26 insertions(+), 11 deletions(-)
> >
> > diff --git a/migration/multifd-zlib.c b/migration/multifd-zlib.c
> > index 737a9645d2..2ced69487e 100644
> > --- a/migration/multifd-zlib.c
> > +++ b/migration/multifd-zlib.c
> > @@ -70,6 +70,10 @@ static int zlib_send_setup(MultiFDSendParams *p,
> Error **errp)
> > goto err_free_zbuff;
> > }
> > p->compress_data = z;
> > +
> > + /* Needs 2 IOVs, one for packet header and one for compressed data
> */
> > + p->iov = g_new0(struct iovec, 2);
> > +
> > return 0;
> >
> > err_free_zbuff:
> > @@ -101,6 +105,9 @@ static void zlib_send_cleanup(MultiFDSendParams *p,
> Error **errp)
> > z->buf = NULL;
> > g_free(p->compress_data);
> > p->compress_data = NULL;
> > +
> > + g_free(p->iov);
> > + p->iov = NULL;
> > }
> >
> > /**
> > diff --git a/migration/multifd-zstd.c b/migration/multifd-zstd.c
> > index 256858df0a..ca17b7e310 100644
> > --- a/migration/multifd-zstd.c
> > +++ b/migration/multifd-zstd.c
> > @@ -52,7 +52,6 @@ static int zstd_send_setup(MultiFDSendParams *p, Error
> **errp)
> > struct zstd_data *z = g_new0(struct zstd_data, 1);
> > int res;
> >
> > - p->compress_data = z;
> > z->zcs = ZSTD_createCStream();
> > if (!z->zcs) {
> > g_free(z);
> > @@ -77,6 +76,10 @@ static int zstd_send_setup(MultiFDSendParams *p,
> Error **errp)
> > error_setg(errp, "multifd %u: out of memory for zbuff", p->id);
> > return -1;
> > }
> > + p->compress_data = z;
> > +
> > + /* Needs 2 IOVs, one for packet header and one for compressed data
> */
> > + p->iov = g_new0(struct iovec, 2);
> > return 0;
> > }
> >
> > @@ -98,6 +101,9 @@ static void zstd_send_cleanup(MultiFDSendParams *p,
> Error **errp)
> > z->zbuff = NULL;
> > g_free(p->compress_data);
> > p->compress_data = NULL;
> > +
> > + g_free(p->iov);
> > + p->iov = NULL;
> > }
> >
> > /**
> > diff --git a/migration/multifd.c b/migration/multifd.c
> > index f317bff077..d82885fdbb 100644
> > --- a/migration/multifd.c
> > +++ b/migration/multifd.c
> > @@ -137,6 +137,13 @@ static int nocomp_send_setup(MultiFDSendParams *p,
> Error **errp)
> > p->write_flags |= QIO_CHANNEL_WRITE_FLAG_ZERO_COPY;
> > }
> >
> > + if (multifd_use_packets()) {
> > + /* We need one extra place for the packet header */
> > + p->iov = g_new0(struct iovec, p->page_count + 1);
> > + } else {
> > + p->iov = g_new0(struct iovec, p->page_count);
> > + }
> > +
> > return 0;
> > }
> >
> > @@ -150,6 +157,8 @@ static int nocomp_send_setup(MultiFDSendParams *p,
> Error **errp)
> > */
> > static void nocomp_send_cleanup(MultiFDSendParams *p, Error **errp)
> > {
> > + g_free(p->iov);
> > + p->iov = NULL;
> > return;
> > }
> >
> > @@ -228,6 +237,7 @@ static int nocomp_send_prepare(MultiFDSendParams *p,
> Error **errp)
> > */
> > static int nocomp_recv_setup(MultiFDRecvParams *p, Error **errp)
> > {
> > + p->iov = g_new0(struct iovec, p->page_count);
> > return 0;
> > }
> >
> > @@ -240,6 +250,8 @@ static int nocomp_recv_setup(MultiFDRecvParams *p,
> Error **errp)
> > */
> > static void nocomp_recv_cleanup(MultiFDRecvParams *p)
> > {
> > + g_free(p->iov);
> > + p->iov = NULL;
> > }
>
> Are recv_setup()/recv_cleanup() for zstd/zlib missing?
Zstd/zlib does not request the IOVs on the receiving side.
The zstd/zlib reads the compressed data into the buffer directly
qio_channel_read_all(p->c, (void *)z->zbuff, in_size, errp);
> If the iov will be managed by the compressors, I'm wondering whether we
> should start assert(p->iov) after send|recv_setup(), and assert(!p->iov)
> after send|recv_cleanup(). That'll guard all these.
Yes, I agree with adding a check of IOV in multifd.c since different
compressors
may have different requirements for IOV.
We can add assert(p->iov) after send_setup, not recv_setup. The sending side
always
uses IOV to send pages by qio_channel_writev_full_all in multifd.c, but the
receiving
side may not require the IOV for reading pages.
It is better to add assert(!p->iov) after send|recv_cleanup()
> >
> > /**
> > @@ -783,8 +795,6 @@ static bool
> multifd_send_cleanup_channel(MultiFDSendParams *p, Error **errp)
> > p->packet_len = 0;
> > g_free(p->packet);
> > p->packet = NULL;
> > - g_free(p->iov);
> > - p->iov = NULL;
> > multifd_send_state->ops->send_cleanup(p, errp);
> >
> > return *errp == NULL;
> > @@ -1179,11 +1189,6 @@ bool multifd_send_setup(void)
> > p->packet = g_malloc0(p->packet_len);
> > p->packet->magic = cpu_to_be32(MULTIFD_MAGIC);
> > p->packet->version = cpu_to_be32(MULTIFD_VERSION);
> > -
> > - /* We need one extra place for the packet header */
> > - p->iov = g_new0(struct iovec, page_count + 1);
> > - } else {
> > - p->iov = g_new0(struct iovec, page_count);
> > }
> > p->name = g_strdup_printf("multifdsend_%d", i);
> > p->page_size = qemu_target_page_size();
> > @@ -1353,8 +1358,6 @@ static void
> multifd_recv_cleanup_channel(MultiFDRecvParams *p)
> > p->packet_len = 0;
> > g_free(p->packet);
> > p->packet = NULL;
> > - g_free(p->iov);
> > - p->iov = NULL;
> > g_free(p->normal);
> > p->normal = NULL;
> > g_free(p->zero);
> > @@ -1602,7 +1605,6 @@ int multifd_recv_setup(Error **errp)
> > p->packet = g_malloc0(p->packet_len);
> > }
> > p->name = g_strdup_printf("multifdrecv_%d", i);
> > - p->iov = g_new0(struct iovec, page_count);
> > p->normal = g_new0(ram_addr_t, page_count);
> > p->zero = g_new0(ram_addr_t, page_count);
> > p->page_count = page_count;
> > --
> > 2.39.3
> >
>
> --
> Peter Xu
