On Tue, May 28, 2024 at 01:36:38PM +0000, Liu, Yuan1 wrote:
> > -----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);
Hmm indeed, thanks.
>
> > 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()
In that case maybe we should at some time move iov out of MultiFDRecvParams
as that's not a shared object, making MultiFDSendParams.compress_data
points to iov for nocomp case. But we can leave that for later.
Then I've no question on this patch, feel free to take (with/without the
conditional assertions):
Reviewed-by: Peter Xu <pet...@redhat.com>
Thanks,
>
> > >
> > > /**
> > > @@ -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
>
--
Peter Xu
