[PATCH 5/5] migration: Add integration test for 'qatzip' compression method

Bryan Zhang posted 5 patches 11 months ago
Maintainers: Paolo Bonzini <pbonzini@redhat.com>, "Daniel P. Berrangé" <berrange@redhat.com>, Eduardo Habkost <eduardo@habkost.net>, "Marc-André Lureau" <marcandre.lureau@redhat.com>, Thomas Huth <thuth@redhat.com>, "Philippe Mathieu-Daudé" <philmd@linaro.org>, Juan Quintela <quintela@redhat.com>, Peter Xu <peterx@redhat.com>, Fabiano Rosas <farosas@suse.de>, Leonardo Bras <leobras@redhat.com>, Eric Blake <eblake@redhat.com>, Markus Armbruster <armbru@redhat.com>, Laurent Vivier <lvivier@redhat.com>
There is a newer version of this series
[PATCH 5/5] migration: Add integration test for 'qatzip' compression method
Posted by Bryan Zhang 11 months ago
Adds an integration test for 'qatzip'.

Signed-off-by: Bryan Zhang <bryan.zhang@bytedance.com>
Signed-off-by: Hao Xiang <hao.xiang@bytedance.com>
---
 tests/qtest/meson.build      |  4 ++++
 tests/qtest/migration-test.c | 37 ++++++++++++++++++++++++++++++++++++
 2 files changed, 41 insertions(+)

diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
index 47dabf91d0..5931bd6418 100644
--- a/tests/qtest/meson.build
+++ b/tests/qtest/meson.build
@@ -302,6 +302,10 @@ if gnutls.found()
   endif
 endif
 
+if qatzip.found()
+  migration_files += [qatzip]
+endif
+
 qtests = {
   'bios-tables-test': [io, 'boot-sector.c', 'acpi-utils.c', 'tpm-emu.c'],
   'cdrom-test': files('boot-sector.c'),
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index d520c587f7..f51bc4056f 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -35,6 +35,10 @@
 # endif /* CONFIG_TASN1 */
 #endif /* CONFIG_GNUTLS */
 
+#ifdef CONFIG_QATZIP
+#include <qatzip.h>
+#endif /* CONFIG_QATZIP */
+
 /* For dirty ring test; so far only x86_64 is supported */
 #if defined(__linux__) && defined(HOST_X86_64)
 #include "linux/kvm.h"
@@ -2572,6 +2576,15 @@ test_migrate_precopy_tcp_multifd_zstd_start(QTestState *from,
 }
 #endif /* CONFIG_ZSTD */
 
+#ifdef CONFIG_QATZIP
+static void *
+test_migrate_precopy_tcp_multifd_qatzip_start(QTestState *from,
+                                              QTestState *to)
+{
+    return test_migrate_precopy_tcp_multifd_start_common(from, to, "qatzip");
+}
+#endif
+
 static void test_multifd_tcp_none(void)
 {
     MigrateCommon args = {
@@ -2607,6 +2620,17 @@ static void test_multifd_tcp_zstd(void)
 }
 #endif
 
+#ifdef CONFIG_QATZIP
+static void test_multifd_tcp_qatzip(void)
+{
+    MigrateCommon args = {
+        .listen_uri = "defer",
+        .start_hook = test_migrate_precopy_tcp_multifd_qatzip_start,
+    };
+    test_precopy_common(&args);
+}
+#endif
+
 #ifdef CONFIG_GNUTLS
 static void *
 test_migrate_multifd_tcp_tls_psk_start_match(QTestState *from,
@@ -3480,6 +3504,19 @@ int main(int argc, char **argv)
     qtest_add_func("/migration/multifd/tcp/plain/zstd",
                    test_multifd_tcp_zstd);
 #endif
+#ifdef CONFIG_QATZIP
+    /*
+     * Use QATzip's qzInit() function as a runtime hardware check.
+     * Ideally there might be a cleaner way to probe for the presence of QAT.
+     */
+    QzSession_T sess;
+    memset(&sess, 0, sizeof(QzSession_T));
+    if (qzInit(&sess, 0) == QZ_OK) {
+        qzClose(&sess);
+        qtest_add_func("/migration/multifd/tcp/plain/qatzip",
+                    test_multifd_tcp_qatzip);
+    }
+#endif
 #ifdef CONFIG_GNUTLS
     qtest_add_func("/migration/multifd/tcp/tls/psk/match",
                    test_multifd_tcp_tls_psk_match);
-- 
2.30.2
Re: [PATCH 5/5] migration: Add integration test for 'qatzip' compression method
Posted by Peter Xu 10 months ago
On Sun, Dec 31, 2023 at 08:58:04PM +0000, Bryan Zhang wrote:
> Adds an integration test for 'qatzip'.

Please use "tests" as prefix of this patch.  It can be "tests/migration:",
"tests/migration-test:", etc.

> 
> Signed-off-by: Bryan Zhang <bryan.zhang@bytedance.com>
> Signed-off-by: Hao Xiang <hao.xiang@bytedance.com>

[...]

>  test_migrate_multifd_tcp_tls_psk_start_match(QTestState *from,
> @@ -3480,6 +3504,19 @@ int main(int argc, char **argv)
>      qtest_add_func("/migration/multifd/tcp/plain/zstd",
>                     test_multifd_tcp_zstd);
>  #endif
> +#ifdef CONFIG_QATZIP
> +    /*
> +     * Use QATzip's qzInit() function as a runtime hardware check.
> +     * Ideally there might be a cleaner way to probe for the presence of QAT.
> +     */
> +    QzSession_T sess;
> +    memset(&sess, 0, sizeof(QzSession_T));
> +    if (qzInit(&sess, 0) == QZ_OK) {

Does "0" means it'll fail if no hardware is available, no matter whether
due to processor too old, or limited resources?

Would it make sense to test it even if only software fallbacks are
available?  IIUC the migration path will still be covered in software
fallbacks, so it may still makes sense to me.  It can be very likely that
most CIs will not cover the qatzip paths otherwise, because of either no
control over hardwares, or limited privileges of the CI user (does qat HWs
need special privilege normally?  I have no idea how QAT resource
management is done if there're only limited HW resources).

Besides, I believe all the data points you provided in the cover letter are
collected with 2 QAT HWs enabled?  I'm curious how's the performance of the
software fallback of qatzip comparing to existing algorithm: iiuc zstd is
mostly always preferred if sololy comparing to zlib?  where does qatzip
soft-mode stand in the picture?

Thanks,

-- 
Peter Xu
Re: [External] Re: [PATCH 5/5] migration: Add integration test for 'qatzip' compression method
Posted by Bryan Zhang . 9 months ago
On Mon, Jan 29, 2024 at 12:53 AM Peter Xu <peterx@redhat.com> wrote:

> On Sun, Dec 31, 2023 at 08:58:04PM +0000, Bryan Zhang wrote:
> > Adds an integration test for 'qatzip'.
>
> Please use "tests" as prefix of this patch.  It can be "tests/migration:",
> "tests/migration-test:", etc.
>
> Will do.

> >
> > Signed-off-by: Bryan Zhang <bryan.zhang@bytedance.com>
> > Signed-off-by: Hao Xiang <hao.xiang@bytedance.com>
>
> [...]
>
> >  test_migrate_multifd_tcp_tls_psk_start_match(QTestState *from,
> > @@ -3480,6 +3504,19 @@ int main(int argc, char **argv)
> >      qtest_add_func("/migration/multifd/tcp/plain/zstd",
> >                     test_multifd_tcp_zstd);
> >  #endif
> > +#ifdef CONFIG_QATZIP
> > +    /*
> > +     * Use QATzip's qzInit() function as a runtime hardware check.
> > +     * Ideally there might be a cleaner way to probe for the presence
> of QAT.
> > +     */
> > +    QzSession_T sess;
> > +    memset(&sess, 0, sizeof(QzSession_T));
> > +    if (qzInit(&sess, 0) == QZ_OK) {
>
> Does "0" means it'll fail if no hardware is available, no matter whether
> due to processor too old, or limited resources?
>
> Would it make sense to test it even if only software fallbacks are
> available?  IIUC the migration path will still be covered in software
> fallbacks, so it may still makes sense to me.  It can be very likely that
> most CIs will not cover the qatzip paths otherwise, because of either no
> control over hardwares, or limited privileges of the CI user (does qat HWs
> need special privilege normally?  I have no idea how QAT resource
> management is done if there're only limited HW resources).
>
> Besides, I believe all the data points you provided in the cover letter are
> collected with 2 QAT HWs enabled?  I'm curious how's the performance of the
> software fallback of qatzip comparing to existing algorithm: iiuc zstd is
> mostly always preferred if sololy comparing to zlib?  where does qatzip
> soft-mode stand in the picture?


> Yes, as I understand it, 0 means the code will error if the hardware is
unavailable for whatever reason.

We can enable software fallback in the live migration path, which will also
enable using software to run the QATzip tests (and will conveniently allow
us to remove the awkward `qzInit` check in the test code). It also might be
a good idea since it would also avoid failure in case of, e.g., a transient
hardware problem?

Performance: The software fallback seems prohibitively slow. QATzip
fallback uses the built-in zlib implementation, but I ran a migration test
that normally takes zlib about 150 seconds and the QATzip fallback took
about 30 minutes before my SSH session disconnected :P

Also, a note: When enabling software fallback, QATzip and the QAT driver
will both repeatedly print a complaint to the QEMU monitor when trying to
compress without hardware. Is it bad form to introduce
seemingly-unsuppressable prints from dependencies, or is this acceptable?

>
> Thanks,
>
> --
> Peter Xu
>
> Thanks for your feedback.

--
Bryan Zhang
Re: [External] Re: [PATCH 5/5] migration: Add integration test for 'qatzip' compression method
Posted by Peter Xu 9 months ago
On Wed, Feb 28, 2024 at 06:55:37PM -0800, Bryan Zhang . wrote:
> We can enable software fallback in the live migration path, which will also
> enable using software to run the QATzip tests (and will conveniently allow
> us to remove the awkward `qzInit` check in the test code). It also might be
> a good idea since it would also avoid failure in case of, e.g., a transient
> hardware problem?
> 
> Performance: The software fallback seems prohibitively slow. QATzip
> fallback uses the built-in zlib implementation, but I ran a migration test
> that normally takes zlib about 150 seconds and the QATzip fallback took
> about 30 minutes before my SSH session disconnected :P

When preparing the qatzip documentataion file, please mention this.  This
is definitely not expected by myself, we should suggest mostly never use
qatzip compatible (software) mode unless the admin is sure to have the
hardware support.

It should then only be used in qtest to cover the qatzip paths only.

> 
> Also, a note: When enabling software fallback, QATzip and the QAT driver
> will both repeatedly print a complaint to the QEMU monitor when trying to
> compress without hardware. Is it bad form to introduce
> seemingly-unsuppressable prints from dependencies, or is this acceptable?

It is definitely bad to keep printing the same message, no matter from
where.

Why it repeatedly do so?  Do you know why the library cannot make it
print_once()?

-- 
Peter Xu