On Tue, 2022-07-05 at 16:27 +0100, Dr. David Alan Gilbert wrote: > * Ilya Leoshkevich (i...@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 <i...@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. Best regards, Ilya
