Re: [PATCH] multifd: Copy pages before compressing them with zlib

[Dr. David Alan Gilbert]

* Ilya Leoshkevich (iii@linux.ibm.com) wrote:
> On Tue, 2022-07-05 at 16:27 +0100, Dr. David Alan Gilbert wrote:
> > * Ilya Leoshkevich (iii@linux.ibm.com) wrote:
> > > zlib_send_prepare() compresses pages of a running VM. zlib does not
> > > make any thread-safety guarantees with respect to changing
> > > deflate()
> > > input concurrently with deflate() [1].
> > > 
> > > One can observe problems due to this with the IBM zEnterprise Data
> > > Compression accelerator capable zlib [2]. When the hardware
> > > acceleration is enabled, migration/multifd/tcp/plain/zlib test
> > > fails
> > > intermittently [3] due to sliding window corruption. The
> > > accelerator's
> > > architecture explicitly discourages concurrent accesses [4]:
> > > 
> > >     Page 26-57, "Other Conditions":
> > > 
> > >     As observed by this CPU, other CPUs, and channel
> > >     programs, references to the parameter block, first,
> > >     second, and third operands may be multiple-access
> > >     references, accesses to these storage locations are
> > >     not necessarily block-concurrent, and the sequence
> > >     of these accesses or references is undefined.
> > > 
> > > Mark Adler pointed out that vanilla zlib performs double fetches
> > > under
> > > certain circumstances as well [5], therefore we need to copy data
> > > before passing it to deflate().
> > 
> > Thanks for fixing that!
> > 
> > > [1] https://zlib.net/manual.html
> > > [2] https://github.com/madler/zlib/pull/410
> > > [3]
> > > https://lists.nongnu.org/archive/html/qemu-devel/2022-03/msg03988.html
> > > [4] http://publibfp.dhe.ibm.com/epubs/pdf/a227832c.pdf
> > > [5] https://gitlab.com/qemu-project/qemu/-/issues/1099
> > > 
> > > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> > > ---
> > > 
> > > v1:
> > > https://lists.gnu.org/archive/html/qemu-devel/2022-03/msg06841.html
> > > v1 -> v2: Rebase, mention Mark Adler's reply in the commit message.
> > > 
> > >  migration/multifd-zlib.c | 35 ++++++++++++++++++++++-------------
> > >  1 file changed, 22 insertions(+), 13 deletions(-)
> > > 
> > > diff --git a/migration/multifd-zlib.c b/migration/multifd-zlib.c
> > > index 3a7ae44485..b6b22b7d1f 100644
> > > --- a/migration/multifd-zlib.c
> > > +++ b/migration/multifd-zlib.c
> > > @@ -27,6 +27,8 @@ struct zlib_data {
> > >      uint8_t *zbuff;
> > >      /* size of compressed buffer */
> > >      uint32_t zbuff_len;
> > > +    /* uncompressed buffer */
> > > +    uint8_t buf[];
> > >  };
> > >  
> > >  /* Multifd zlib compression */
> > > @@ -43,9 +45,18 @@ struct zlib_data {
> > >   */
> > >  static int zlib_send_setup(MultiFDSendParams *p, Error **errp)
> > >  {
> > > -    struct zlib_data *z = g_new0(struct zlib_data, 1);
> > > -    z_stream *zs = &z->zs;
> > > +    /* This is the maximum size of the compressed buffer */
> > > +    uint32_t zbuff_len = compressBound(MULTIFD_PACKET_SIZE);
> > > +    size_t buf_len = qemu_target_page_size();
> > > +    struct zlib_data *z;
> > > +    z_stream *zs;
> > >  
> > > +    z = g_try_malloc0(sizeof(struct zlib_data) + buf_len +
> > > zbuff_len);
> > 
> > So I think this works; but wouldn't life be easier if you just used
> > separate malloc's for the buffers?  You've got a lot of hairy pointer
> > maths below that would go away if they were separate.
> > 
> > Dave
> 
> I was trying to avoid an (IMHO equally hairy) error handling sequence
> here. But I don't mind changing this if an alternative would be more
> maintainable.

It's probably worth trying; I bet it works out a lot simpler.
Remember that g_free(NULL) is safe; so if you want to do a cleanup
of a bunch of pointers you can do:
  g_free(a);
  g_free(b);
  g_free(c);

even if some combination of those hadn't been allocated yet.

Dave

> Best regards,
> Ilya
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK