* [PATCH v5 0/5] Implement QATzip compression method
@ 2024-07-11 2:52 Yichen Wang
2024-07-11 2:52 ` [PATCH v5 1/5] docs/migration: add qatzip compression feature Yichen Wang
` (5 more replies)
0 siblings, 6 replies; 14+ messages in thread
From: Yichen Wang @ 2024-07-11 2:52 UTC (permalink / raw)
To: Peter Xu, Fabiano Rosas, Paolo Bonzini, Daniel P. Berrangé,
Eduardo Habkost, Marc-André Lureau, Thomas Huth,
Philippe Mathieu-Daudé, Eric Blake, Markus Armbruster,
Laurent Vivier, qemu-devel
Cc: Hao Xiang, Liu, Yuan1, Zou, Nanhai, Ho-Ren (Jack) Chuang,
Yichen Wang
v5:
- Rebase changes on top of 59084feb256c617063e0dbe7e64821ae8852d7cf
- Add documentations about migration with qatzip accerlation
- Remove multifd-qatzip-sw-fallback option
v4:
- Rebase changes on top of 1a2d52c7fcaeaaf4f2fe8d4d5183dccaeab67768
- Move the IOV initialization to qatzip implementation
- Only use qatzip to compress normal pages
v3:
- Rebase changes on top of master
- Merge two patches per Fabiano Rosas's comment
- Add versions into comments and documentations
v2:
- Rebase changes on top of recent multifd code changes.
- Use QATzip API 'qzMalloc' and 'qzFree' to allocate QAT buffers.
- Remove parameter tuning and use QATzip's defaults for better
performance.
- Add parameter to enable QAT software fallback.
v1:
https://lists.nongnu.org/archive/html/qemu-devel/2023-12/msg03761.html
* Performance
We present updated performance results. For circumstantial reasons, v1
presented performance on a low-bandwidth (1Gbps) network.
Here, we present updated results with a similar setup as before but with
two main differences:
1. Our machines have a ~50Gbps connection, tested using 'iperf3'.
2. We had a bug in our memory allocation causing us to only use ~1/2 of
the VM's RAM. Now we properly allocate and fill nearly all of the VM's
RAM.
Thus, the test setup is as follows:
We perform multifd live migration over TCP using a VM with 64GB memory.
We prepare the machine's memory by powering it on, allocating a large
amount of memory (60GB) as a single buffer, and filling the buffer with
the repeated contents of the Silesia corpus[0]. This is in lieu of a more
realistic memory snapshot, which proved troublesome to acquire.
We analyze CPU usage by averaging the output of 'top' every second
during migration. This is admittedly imprecise, but we feel that it
accurately portrays the different degrees of CPU usage of varying
compression methods.
We present the latency, throughput, and CPU usage results for all of the
compression methods, with varying numbers of multifd threads (4, 8, and
16).
[0] The Silesia corpus can be accessed here:
https://sun.aei.polsl.pl//~sdeor/index.php?page=silesia
** Results
4 multifd threads:
|---------------|---------------|----------------|---------|---------|
|method |time(sec) |throughput(mbps)|send cpu%|recv cpu%|
|---------------|---------------|----------------|---------|---------|
|qatzip | 23.13 | 8749.94 |117.50 |186.49 |
|---------------|---------------|----------------|---------|---------|
|zlib |254.35 | 771.87 |388.20 |144.40 |
|---------------|---------------|----------------|---------|---------|
|zstd | 54.52 | 3442.59 |414.59 |149.77 |
|---------------|---------------|----------------|---------|---------|
|none | 12.45 |43739.60 |159.71 |204.96 |
|---------------|---------------|----------------|---------|---------|
8 multifd threads:
|---------------|---------------|----------------|---------|---------|
|method |time(sec) |throughput(mbps)|send cpu%|recv cpu%|
|---------------|---------------|----------------|---------|---------|
|qatzip | 16.91 |12306.52 |186.37 |391.84 |
|---------------|---------------|----------------|---------|---------|
|zlib |130.11 | 1508.89 |753.86 |289.35 |
|---------------|---------------|----------------|---------|---------|
|zstd | 27.57 | 6823.23 |786.83 |303.80 |
|---------------|---------------|----------------|---------|---------|
|none | 11.82 |46072.63 |163.74 |238.56 |
|---------------|---------------|----------------|---------|---------|
16 multifd threads:
|---------------|---------------|----------------|---------|---------|
|method |time(sec) |throughput(mbps)|send cpu%|recv cpu%|
|---------------|---------------|----------------|---------|---------|
|qatzip |18.64 |11044.52 | 573.61 |437.65 |
|---------------|---------------|----------------|---------|---------|
|zlib |66.43 | 2955.79 |1469.68 |567.47 |
|---------------|---------------|----------------|---------|---------|
|zstd |14.17 |13290.66 |1504.08 |615.33 |
|---------------|---------------|----------------|---------|---------|
|none |16.82 |32363.26 | 180.74 |217.17 |
|---------------|---------------|----------------|---------|---------|
** Observations
- In general, not using compression outperforms using compression in a
non-network-bound environment.
- 'qatzip' outperforms other compression workers with 4 and 8 workers,
achieving a ~91% latency reduction over 'zlib' with 4 workers, and a
~58% latency reduction over 'zstd' with 4 workers.
- 'qatzip' maintains comparable performance with 'zstd' at 16 workers,
showing a ~32% increase in latency. This performance difference
becomes more noticeable with more workers, as CPU compression is highly
parallelizable.
- 'qatzip' compression uses considerably less CPU than other compression
methods. At 8 workers, 'qatzip' demonstrates a ~75% reduction in
compression CPU usage compared to 'zstd' and 'zlib'.
- 'qatzip' decompression CPU usage is less impressive, and is even
slightly worse than 'zstd' and 'zlib' CPU usage at 4 and 16 workers.
Bryan Zhang (4):
meson: Introduce 'qatzip' feature to the build system
migration: Add migration parameters for QATzip
migration: Introduce 'qatzip' compression method
tests/migration: Add integration test for 'qatzip' compression method
Yuan Liu (1):
docs/migration: add qatzip compression feature
docs/devel/migration/features.rst | 1 +
docs/devel/migration/qatzip-compression.rst | 251 ++++++++++++
hw/core/qdev-properties-system.c | 6 +-
meson.build | 10 +
meson_options.txt | 2 +
migration/meson.build | 1 +
migration/migration-hmp-cmds.c | 4 +
migration/multifd-qatzip.c | 403 ++++++++++++++++++++
migration/multifd.h | 5 +-
migration/options.c | 34 ++
migration/options.h | 1 +
qapi/migration.json | 21 +
scripts/meson-buildoptions.sh | 3 +
tests/qtest/meson.build | 4 +
tests/qtest/migration-test.c | 35 ++
15 files changed, 778 insertions(+), 3 deletions(-)
create mode 100644 docs/devel/migration/qatzip-compression.rst
create mode 100644 migration/multifd-qatzip.c
--
Yichen Wang
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v5 1/5] docs/migration: add qatzip compression feature
2024-07-11 2:52 [PATCH v5 0/5] Implement QATzip compression method Yichen Wang
@ 2024-07-11 2:52 ` Yichen Wang
2024-07-11 2:52 ` [PATCH v5 2/5] meson: Introduce 'qatzip' feature to the build system Yichen Wang
` (4 subsequent siblings)
5 siblings, 0 replies; 14+ messages in thread
From: Yichen Wang @ 2024-07-11 2:52 UTC (permalink / raw)
To: Peter Xu, Fabiano Rosas, Paolo Bonzini, Daniel P. Berrangé,
Eduardo Habkost, Marc-André Lureau, Thomas Huth,
Philippe Mathieu-Daudé, Eric Blake, Markus Armbruster,
Laurent Vivier, qemu-devel
Cc: Hao Xiang, Liu, Yuan1, Zou, Nanhai, Ho-Ren (Jack) Chuang,
Yichen Wang
From: Yuan Liu <yuan1.liu@intel.com>
add Intel QATzip compression method introduction
Signed-off-by: Yuan Liu <yuan1.liu@intel.com>
Reviewed-by: Nanhai Zou <nanhai.zou@intel.com>
Reviewed-by: Peter Xu <peterx@redhat.com>
Reviewed-by: Yichen Wang <yichen.wang@bytedance.com>
---
docs/devel/migration/features.rst | 1 +
docs/devel/migration/qatzip-compression.rst | 251 ++++++++++++++++++++
2 files changed, 252 insertions(+)
create mode 100644 docs/devel/migration/qatzip-compression.rst
diff --git a/docs/devel/migration/features.rst b/docs/devel/migration/features.rst
index 58f8fd9e16..8f431d52f9 100644
--- a/docs/devel/migration/features.rst
+++ b/docs/devel/migration/features.rst
@@ -14,3 +14,4 @@ Migration has plenty of features to support different use cases.
CPR
qpl-compression
uadk-compression
+ qatzip-compression
diff --git a/docs/devel/migration/qatzip-compression.rst b/docs/devel/migration/qatzip-compression.rst
new file mode 100644
index 0000000000..72fa3e2826
--- /dev/null
+++ b/docs/devel/migration/qatzip-compression.rst
@@ -0,0 +1,251 @@
+==================
+QATzip Compression
+==================
+In scenarios with limited network bandwidth, the ``QATzip`` solution can help
+users save a lot of host CPU resources by accelerating compression and
+decompression through the Intel QuickAssist Technology(``QAT``) hardware.
+
+``QATzip`` is a user space library which builds on top of the Intel QuickAssist
+Technology user space library, to provide extended accelerated compression and
+decompression services.
+
+For more ``QATzip`` introduction, please refer to `QATzip Introduction
+<https://github.com/intel/QATzip?tab=readme-ov-file#introductionl>`_
+
+QATzip Compression Framework
+============================
+
+::
+
+ +----------------+
+ | MultiFd Thread |
+ +-------+--------+
+ |
+ | compress/decompress
+ +-------+--------+
+ | QATzip library |
+ +-------+--------+
+ |
+ +-------+--------+
+ | QAT library |
+ +-------+--------+
+ | user space
+ --------+---------------------
+ | kernel space
+ +------+-------+
+ | QAT Driver |
+ +------+-------+
+ |
+ +------+-------+
+ | QAT Devices |
+ +--------------+
+
+
+QATzip Installation
+-------------------
+
+The ``QATzip`` installation package has been integrated into some Linux
+distributions and can be installed directly. For example, the Ubuntu Server
+24.04 LTS system can be installed using below command
+
+.. code-block:: shell
+
+ #apt search qatzip
+ libqatzip-dev/noble 1.2.0-0ubuntu3 amd64
+ Intel QuickAssist user space library development files
+
+ libqatzip3/noble 1.2.0-0ubuntu3 amd64
+ Intel QuickAssist user space library
+
+ qatzip/noble,now 1.2.0-0ubuntu3 amd64 [installed]
+ Compression user-space tool for Intel QuickAssist Technology
+
+ #sudo apt install libqatzip-dev libqatzip3 qatzip
+
+If your system does not support the ``QATzip`` installation package, you can
+use the source code to build and install, please refer to `QATzip source code installation
+<https://github.com/intel/QATzip?tab=readme-ov-file#build-intel-quickassist-technology-driver>`_
+
+QAT Hardware Deployment
+-----------------------
+
+``QAT`` supports physical functions(PFs) and virtual functions(VFs) for
+deployment, and users can configure ``QAT`` resources for migration according
+to actual needs. For more details about ``QAT`` deployment, please refer to
+`Intel QuickAssist Technology Documentation
+<https://intel.github.io/quickassist/index.html>`_
+
+For more ``QAT`` hardware introduction, please refer to `intel-quick-assist-technology-overview
+<https://www.intel.com/content/www/us/en/architecture-and-technology/intel-quick-assist-technology-overview.html>`_
+
+How To Use QATzip Compression
+=============================
+
+1 - Install ``QATzip`` library
+
+2 - Build ``QEMU`` with ``--enable-qatzip`` parameter
+
+ E.g. configure --target-list=x86_64-softmmu --enable-kvm ``--enable-qatzip``
+
+3 - Set ``migrate_set_parameter multifd-compression qatzip``
+
+4 - Set ``migrate_set_parameter multifd-qatzip-level comp_level``, the default
+comp_level value is 1, and it supports levels from 1 to 9
+
+
+Performance Testing with QATzip
+===============================
+
+Testing environment is being set as below:
+
+VM configuration:16 vCPU, 64G memory;
+
+VM Workload: all vCPUs are idle and 54G memory is filled with Silesia data;
+
+QAT Devices: 4;
+
+Sender migration parameters:
+
+.. code-block:: shell
+
+ migrate_set_capability multifd on
+ migrate_set_parameter multifd-channels 2/4/8
+ migrate_set_parameter max-bandwidth 1G/10G
+ migrate_set_parameter multifd-compression qatzip/zstd
+
+Receiver migration parameters:
+
+.. code-block:: shell
+
+ migrate_set_capability multifd on
+ migrate_set_parameter multifd-channels 2
+ migrate_set_parameter multifd-compression qatzip/zstd
+
+max-bandwidth: 1 GBps (Gbytes/sec)
+
+.. code-block:: text
+
+ |-----------|--------|---------|----------|------|------|
+ |2 Channels |Total |down |throughput| send | recv |
+ | |time(ms)|time(ms) |(mbps) | cpu %| cpu% |
+ |-----------|--------|---------|----------|------|------|
+ |qatzip | 21607| 77| 8051| 88| 125|
+ |-----------|--------|---------|----------|------|------|
+ |zstd | 78351| 96| 2199| 204| 80|
+ |-----------|--------|---------|----------|------|------|
+
+ |-----------|--------|---------|----------|------|------|
+ |4 Channels |Total |down |throughput| send | recv |
+ | |time(ms)|time(ms) |(mbps) | cpu %| cpu% |
+ |-----------|--------|---------|----------|------|------|
+ |qatzip | 20336| 25| 8557| 110| 190|
+ |-----------|--------|---------|----------|------|------|
+ |zstd | 39324| 31| 4389| 406| 160|
+ |-----------|--------|---------|----------|------|------|
+
+ |-----------|--------|---------|----------|------|------|
+ |8 Channels |Total |down |throughput| send | recv |
+ | |time(ms)|time(ms) |(mbps) | cpu %| cpu% |
+ |-----------|--------|---------|----------|------|------|
+ |qatzip | 20208| 22| 8613| 125| 300|
+ |-----------|--------|---------|----------|------|------|
+ |zstd | 20515| 22| 8438| 800| 340|
+ |-----------|--------|---------|----------|------|------|
+
+max-bandwidth: 10 GBps (Gbytes/sec)
+
+.. code-block:: text
+
+ |-----------|--------|---------|----------|------|------|
+ |2 Channels |Total |down |throughput| send | recv |
+ | |time(ms)|time(ms) |(mbps) | cpu %| cpu% |
+ |-----------|--------|---------|----------|------|------|
+ |qatzip | 22450| 77| 7748| 80| 125|
+ |-----------|--------|---------|----------|------|------|
+ |zstd | 78339| 76| 2199| 204| 80|
+ |-----------|--------|---------|----------|------|------|
+
+ |-----------|--------|---------|----------|------|------|
+ |4 Channels |Total |down |throughput| send | recv |
+ | |time(ms)|time(ms) |(mbps) | cpu %| cpu% |
+ |-----------|--------|---------|----------|------|------|
+ |qatzip | 13017| 24| 13401| 180| 285|
+ |-----------|--------|---------|----------|------|------|
+ |zstd | 39466| 21| 4373| 406| 160|
+ |-----------|--------|---------|----------|------|------|
+
+ |-----------|--------|---------|----------|------|------|
+ |8 Channels |Total |down |throughput| send | recv |
+ | |time(ms)|time(ms) |(mbps) | cpu %| cpu% |
+ |-----------|--------|---------|----------|------|------|
+ |qatzip | 10255| 22| 17037| 280| 590|
+ |-----------|--------|---------|----------|------|------|
+ |zstd | 20126| 77| 8595| 810| 340|
+ |-----------|--------|---------|----------|------|------|
+
+max-bandwidth: 1.25 GBps (Gbytes/sec)
+
+.. code-block:: text
+
+ |-----------|--------|---------|----------|----------|------|------|
+ |8 Channels |Total |down |throughput|pages per | send | recv |
+ | |time(ms)|time(ms) |(mbps) |second | cpu %| cpu% |
+ |-----------|--------|---------|----------|----------|------|------|
+ |qatzip | 16630| 28| 10467| 2940235| 160| 360|
+ |-----------|--------|---------|----------|----------|------|------|
+ |zstd | 20165| 24| 8579| 2391465| 810| 340|
+ |-----------|--------|---------|----------|----------|------|------|
+ |none | 46063| 40| 10848| 330240| 45| 85|
+ |-----------|--------|---------|----------|----------|------|------|
+
+If the user has enabled compression in live migration, using QAT can save the
+host CPU resources.
+
+When compression is enabled, the bottleneck of migration is usually the
+compression throughput on the sender side, since CPU decompression throughput
+is higher than compression, some reference data
+https://github.com/inikep/lzbench, so more CPU resources need to be allocated
+to the sender side.
+
+Summary:
+
+1. In the 1GBps case, QAT only uses 88% CPU utilization to reach 1GBps, but
+ ZSTD needs 800%.
+
+2. In the 10Gbps case, QAT uses 180% CPU utilization to reach 10GBps. but ZSTD
+ still cannot reach 10Gbps even if it uses 810%.
+
+3. The QAT decompression CPU utilization is higher than compression and ZSTD,
+ because:
+
+ a. When using QAT compression, the data needs to be copied to the QAT memory
+ (for DMA operations), and the same for decompression. However,
+ do_user_addr_fault will be triggered during decompression because the QAT
+ decompressed data is copied to the VM address space for the first time, in
+ addition, both compression and decompression are processed by QAT and do not
+ consume CPU resources, so the CPU utilization of the receiver is slightly
+ higher than the sender.
+
+ b. Since zstd decompression decompresses data directly into the VM address
+ space, there is one less memory copy than QAT, so the CPU utilization on the
+ receiver is better than QAT. For the 1GBps case, the receiver CPU
+ utilization is 125%, and the memory copy occupies ~80% of CPU utilization.
+
+How To Choose Between QATzip and QPL
+====================================
+Starting from Intel 4th Gen Intel Xeon Scalable processors, codenamed Sapphire
+Rapids processor(``SPR``), it supports multiple build-in accelerators including
+``QAT`` and ``IAA``, the former can accelerate ``QATzip``, and the latter is
+used to accelerate ``QPL``.
+
+Here are some suggestions:
+
+1 - If your live migration scenario is limited network bandwidth and ``QAT``
+hardware resources exceed ``IAA``, then use the ``QATzip`` method, which
+can save a lot of host CPU resources for compression.
+
+2 - If your system cannot support shared virtual memory(SVM) technology, please
+use ``QATzip`` method because ``QPL`` performance is not good without SVM
+support.
+
+3 - For other scenarios, please use the ``QPL`` method first.
--
Yichen Wang
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v5 2/5] meson: Introduce 'qatzip' feature to the build system
2024-07-11 2:52 [PATCH v5 0/5] Implement QATzip compression method Yichen Wang
2024-07-11 2:52 ` [PATCH v5 1/5] docs/migration: add qatzip compression feature Yichen Wang
@ 2024-07-11 2:52 ` Yichen Wang
2024-07-11 2:52 ` [PATCH v5 3/5] migration: Add migration parameters for QATzip Yichen Wang
` (3 subsequent siblings)
5 siblings, 0 replies; 14+ messages in thread
From: Yichen Wang @ 2024-07-11 2:52 UTC (permalink / raw)
To: Peter Xu, Fabiano Rosas, Paolo Bonzini, Daniel P. Berrangé,
Eduardo Habkost, Marc-André Lureau, Thomas Huth,
Philippe Mathieu-Daudé, Eric Blake, Markus Armbruster,
Laurent Vivier, qemu-devel
Cc: Hao Xiang, Liu, Yuan1, Zou, Nanhai, Ho-Ren (Jack) Chuang,
Yichen Wang, Bryan Zhang
From: Bryan Zhang <bryan.zhang@bytedance.com>
Add a 'qatzip' feature, which is automatically disabled, and which
depends on the QATzip library if enabled.
Signed-off-by: Bryan Zhang <bryan.zhang@bytedance.com>
Signed-off-by: Hao Xiang <hao.xiang@linux.dev>
Signed-off-by: Yichen Wang <yichen.wang@bytedance.com>
---
meson.build | 10 ++++++++++
meson_options.txt | 2 ++
scripts/meson-buildoptions.sh | 3 +++
3 files changed, 15 insertions(+)
diff --git a/meson.build b/meson.build
index 6a93da48e1..ea977c6cbf 100644
--- a/meson.build
+++ b/meson.build
@@ -1244,6 +1244,14 @@ if not get_option('uadk').auto() or have_system
uadk = declare_dependency(dependencies: [libwd, libwd_comp])
endif
endif
+
+qatzip = not_found
+if get_option('qatzip').enabled()
+ qatzip = dependency('qatzip', version: '>=1.1.2',
+ required: get_option('qatzip'),
+ method: 'pkg-config')
+endif
+
virgl = not_found
have_vhost_user_gpu = have_tools and host_os == 'linux' and pixman.found()
@@ -2378,6 +2386,7 @@ config_host_data.set('CONFIG_STATX_MNT_ID', has_statx_mnt_id)
config_host_data.set('CONFIG_ZSTD', zstd.found())
config_host_data.set('CONFIG_QPL', qpl.found())
config_host_data.set('CONFIG_UADK', uadk.found())
+config_host_data.set('CONFIG_QATZIP', qatzip.found())
config_host_data.set('CONFIG_FUSE', fuse.found())
config_host_data.set('CONFIG_FUSE_LSEEK', fuse_lseek.found())
config_host_data.set('CONFIG_SPICE_PROTOCOL', spice_protocol.found())
@@ -4484,6 +4493,7 @@ summary_info += {'lzfse support': liblzfse}
summary_info += {'zstd support': zstd}
summary_info += {'Query Processing Library support': qpl}
summary_info += {'UADK Library support': uadk}
+summary_info += {'qatzip support': qatzip}
summary_info += {'NUMA host support': numa}
summary_info += {'capstone': capstone}
summary_info += {'libpmem support': libpmem}
diff --git a/meson_options.txt b/meson_options.txt
index 0269fa0f16..35a69f6697 100644
--- a/meson_options.txt
+++ b/meson_options.txt
@@ -261,6 +261,8 @@ option('qpl', type : 'feature', value : 'auto',
description: 'Query Processing Library support')
option('uadk', type : 'feature', value : 'auto',
description: 'UADK Library support')
+option('qatzip', type: 'feature', value: 'disabled',
+ description: 'QATzip compression support')
option('fuse', type: 'feature', value: 'auto',
description: 'FUSE block device export')
option('fuse_lseek', type : 'feature', value : 'auto',
diff --git a/scripts/meson-buildoptions.sh b/scripts/meson-buildoptions.sh
index cfadb5ea86..1ce467e9cc 100644
--- a/scripts/meson-buildoptions.sh
+++ b/scripts/meson-buildoptions.sh
@@ -163,6 +163,7 @@ meson_options_help() {
printf "%s\n" ' pixman pixman support'
printf "%s\n" ' plugins TCG plugins via shared library loading'
printf "%s\n" ' png PNG support with libpng'
+ printf "%s\n" ' qatzip QATzip compression support'
printf "%s\n" ' qcow1 qcow1 image format support'
printf "%s\n" ' qed qed image format support'
printf "%s\n" ' qga-vss build QGA VSS support (broken with MinGW)'
@@ -427,6 +428,8 @@ _meson_option_parse() {
--enable-png) printf "%s" -Dpng=enabled ;;
--disable-png) printf "%s" -Dpng=disabled ;;
--prefix=*) quote_sh "-Dprefix=$2" ;;
+ --enable-qatzip) printf "%s" -Dqatzip=enabled ;;
+ --disable-qatzip) printf "%s" -Dqatzip=disabled ;;
--enable-qcow1) printf "%s" -Dqcow1=enabled ;;
--disable-qcow1) printf "%s" -Dqcow1=disabled ;;
--enable-qed) printf "%s" -Dqed=enabled ;;
--
Yichen Wang
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v5 3/5] migration: Add migration parameters for QATzip
2024-07-11 2:52 [PATCH v5 0/5] Implement QATzip compression method Yichen Wang
2024-07-11 2:52 ` [PATCH v5 1/5] docs/migration: add qatzip compression feature Yichen Wang
2024-07-11 2:52 ` [PATCH v5 2/5] meson: Introduce 'qatzip' feature to the build system Yichen Wang
@ 2024-07-11 2:52 ` Yichen Wang
2024-07-11 2:52 ` [PATCH v5 4/5] migration: Introduce 'qatzip' compression method Yichen Wang
` (2 subsequent siblings)
5 siblings, 0 replies; 14+ messages in thread
From: Yichen Wang @ 2024-07-11 2:52 UTC (permalink / raw)
To: Peter Xu, Fabiano Rosas, Paolo Bonzini, Daniel P. Berrangé,
Eduardo Habkost, Marc-André Lureau, Thomas Huth,
Philippe Mathieu-Daudé, Eric Blake, Markus Armbruster,
Laurent Vivier, qemu-devel
Cc: Hao Xiang, Liu, Yuan1, Zou, Nanhai, Ho-Ren (Jack) Chuang,
Yichen Wang, Bryan Zhang
From: Bryan Zhang <bryan.zhang@bytedance.com>
Adds support for migration parameters to control QATzip compression
level and to enable/disable software fallback when QAT hardware is
unavailable. This is a preparatory commit for a subsequent commit that
will actually use QATzip compression.
Signed-off-by: Bryan Zhang <bryan.zhang@bytedance.com>
Signed-off-by: Hao Xiang <hao.xiang@linux.dev>
Signed-off-by: Yichen Wang <yichen.wang@bytedance.com>
---
migration/migration-hmp-cmds.c | 4 ++++
migration/options.c | 34 ++++++++++++++++++++++++++++++++++
migration/options.h | 1 +
qapi/migration.json | 18 ++++++++++++++++++
4 files changed, 57 insertions(+)
diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
index 7d608d26e1..28165cfc9e 100644
--- a/migration/migration-hmp-cmds.c
+++ b/migration/migration-hmp-cmds.c
@@ -576,6 +576,10 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
p->has_multifd_zlib_level = true;
visit_type_uint8(v, param, &p->multifd_zlib_level, &err);
break;
+ case MIGRATION_PARAMETER_MULTIFD_QATZIP_LEVEL:
+ p->has_multifd_qatzip_level = true;
+ visit_type_uint8(v, param, &p->multifd_qatzip_level, &err);
+ break;
case MIGRATION_PARAMETER_MULTIFD_ZSTD_LEVEL:
p->has_multifd_zstd_level = true;
visit_type_uint8(v, param, &p->multifd_zstd_level, &err);
diff --git a/migration/options.c b/migration/options.c
index 645f55003d..147cd2b8fd 100644
--- a/migration/options.c
+++ b/migration/options.c
@@ -55,6 +55,13 @@
#define DEFAULT_MIGRATE_MULTIFD_COMPRESSION MULTIFD_COMPRESSION_NONE
/* 0: means nocompress, 1: best speed, ... 9: best compress ratio */
#define DEFAULT_MIGRATE_MULTIFD_ZLIB_LEVEL 1
+/*
+ * 1: best speed, ... 9: best compress ratio
+ * There is some nuance here. Refer to QATzip documentation to understand
+ * the mapping of QATzip levels to standard deflate levels.
+ */
+#define DEFAULT_MIGRATE_MULTIFD_QATZIP_LEVEL 1
+
/* 0: means nocompress, 1: best speed, ... 20: best compress ratio */
#define DEFAULT_MIGRATE_MULTIFD_ZSTD_LEVEL 1
@@ -123,6 +130,9 @@ Property migration_properties[] = {
DEFINE_PROP_UINT8("multifd-zlib-level", MigrationState,
parameters.multifd_zlib_level,
DEFAULT_MIGRATE_MULTIFD_ZLIB_LEVEL),
+ DEFINE_PROP_UINT8("multifd-qatzip-level", MigrationState,
+ parameters.multifd_qatzip_level,
+ DEFAULT_MIGRATE_MULTIFD_QATZIP_LEVEL),
DEFINE_PROP_UINT8("multifd-zstd-level", MigrationState,
parameters.multifd_zstd_level,
DEFAULT_MIGRATE_MULTIFD_ZSTD_LEVEL),
@@ -787,6 +797,13 @@ int migrate_multifd_zlib_level(void)
return s->parameters.multifd_zlib_level;
}
+int migrate_multifd_qatzip_level(void)
+{
+ MigrationState *s = migrate_get_current();
+
+ return s->parameters.multifd_qatzip_level;
+}
+
int migrate_multifd_zstd_level(void)
{
MigrationState *s = migrate_get_current();
@@ -892,6 +909,8 @@ MigrationParameters *qmp_query_migrate_parameters(Error **errp)
params->multifd_compression = s->parameters.multifd_compression;
params->has_multifd_zlib_level = true;
params->multifd_zlib_level = s->parameters.multifd_zlib_level;
+ params->has_multifd_qatzip_level = true;
+ params->multifd_qatzip_level = s->parameters.multifd_qatzip_level;
params->has_multifd_zstd_level = true;
params->multifd_zstd_level = s->parameters.multifd_zstd_level;
params->has_xbzrle_cache_size = true;
@@ -946,6 +965,7 @@ void migrate_params_init(MigrationParameters *params)
params->has_multifd_channels = true;
params->has_multifd_compression = true;
params->has_multifd_zlib_level = true;
+ params->has_multifd_qatzip_level = true;
params->has_multifd_zstd_level = true;
params->has_xbzrle_cache_size = true;
params->has_max_postcopy_bandwidth = true;
@@ -1038,6 +1058,14 @@ bool migrate_params_check(MigrationParameters *params, Error **errp)
return false;
}
+ if (params->has_multifd_qatzip_level &&
+ ((params->multifd_qatzip_level > 9) ||
+ (params->multifd_qatzip_level < 1))) {
+ error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "multifd_qatzip_level",
+ "a value between 1 and 9");
+ return false;
+ }
+
if (params->has_multifd_zstd_level &&
(params->multifd_zstd_level > 20)) {
error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "multifd_zstd_level",
@@ -1195,6 +1223,9 @@ static void migrate_params_test_apply(MigrateSetParameters *params,
if (params->has_multifd_compression) {
dest->multifd_compression = params->multifd_compression;
}
+ if (params->has_multifd_qatzip_level) {
+ dest->multifd_qatzip_level = params->multifd_qatzip_level;
+ }
if (params->has_multifd_zlib_level) {
dest->multifd_zlib_level = params->multifd_zlib_level;
}
@@ -1315,6 +1346,9 @@ static void migrate_params_apply(MigrateSetParameters *params, Error **errp)
if (params->has_multifd_compression) {
s->parameters.multifd_compression = params->multifd_compression;
}
+ if (params->has_multifd_qatzip_level) {
+ s->parameters.multifd_qatzip_level = params->multifd_qatzip_level;
+ }
if (params->has_multifd_zlib_level) {
s->parameters.multifd_zlib_level = params->multifd_zlib_level;
}
diff --git a/migration/options.h b/migration/options.h
index a2397026db..a0bd6edc06 100644
--- a/migration/options.h
+++ b/migration/options.h
@@ -78,6 +78,7 @@ uint64_t migrate_max_postcopy_bandwidth(void);
int migrate_multifd_channels(void);
MultiFDCompression migrate_multifd_compression(void);
int migrate_multifd_zlib_level(void);
+int migrate_multifd_qatzip_level(void);
int migrate_multifd_zstd_level(void);
uint8_t migrate_throttle_trigger_threshold(void);
const char *migrate_tls_authz(void);
diff --git a/qapi/migration.json b/qapi/migration.json
index 1234bef888..cd08f2f710 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -789,6 +789,11 @@
# speed, and 9 means best compression ratio which will consume
# more CPU. Defaults to 1. (Since 5.0)
#
+# @multifd-qatzip-level: Set the compression level to be used in live
+# migration. The level is an integer between 1 and 9, where 1 means
+# the best compression speed, and 9 means the best compression
+# ratio which will consume more CPU. Defaults to 1. (Since 9.1)
+#
# @multifd-zstd-level: Set the compression level to be used in live
# migration, the compression level is an integer between 0 and 20,
# where 0 means no compression, 1 means the best compression
@@ -849,6 +854,7 @@
'xbzrle-cache-size', 'max-postcopy-bandwidth',
'max-cpu-throttle', 'multifd-compression',
'multifd-zlib-level', 'multifd-zstd-level',
+ 'multifd-qatzip-level',
'block-bitmap-mapping',
{ 'name': 'x-vcpu-dirty-limit-period', 'features': ['unstable'] },
'vcpu-dirty-limit',
@@ -964,6 +970,11 @@
# speed, and 9 means best compression ratio which will consume
# more CPU. Defaults to 1. (Since 5.0)
#
+# @multifd-qatzip-level: Set the compression level to be used in live
+# migration. The level is an integer between 1 and 9, where 1 means
+# the best compression speed, and 9 means the best compression
+# ratio which will consume more CPU. Defaults to 1. (Since 9.1)
+#
# @multifd-zstd-level: Set the compression level to be used in live
# migration, the compression level is an integer between 0 and 20,
# where 0 means no compression, 1 means the best compression
@@ -1037,6 +1048,7 @@
'*max-cpu-throttle': 'uint8',
'*multifd-compression': 'MultiFDCompression',
'*multifd-zlib-level': 'uint8',
+ '*multifd-qatzip-level': 'uint8',
'*multifd-zstd-level': 'uint8',
'*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ],
'*x-vcpu-dirty-limit-period': { 'type': 'uint64',
@@ -1168,6 +1180,11 @@
# speed, and 9 means best compression ratio which will consume
# more CPU. Defaults to 1. (Since 5.0)
#
+# @multifd-qatzip-level: Set the compression level to be used in live
+# migration. The level is an integer between 1 and 9, where 1 means
+# the best compression speed, and 9 means the best compression
+# ratio which will consume more CPU. Defaults to 1. (Since 9.1)
+#
# @multifd-zstd-level: Set the compression level to be used in live
# migration, the compression level is an integer between 0 and 20,
# where 0 means no compression, 1 means the best compression
@@ -1238,6 +1255,7 @@
'*max-cpu-throttle': 'uint8',
'*multifd-compression': 'MultiFDCompression',
'*multifd-zlib-level': 'uint8',
+ '*multifd-qatzip-level': 'uint8',
'*multifd-zstd-level': 'uint8',
'*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ],
'*x-vcpu-dirty-limit-period': { 'type': 'uint64',
--
Yichen Wang
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v5 4/5] migration: Introduce 'qatzip' compression method
2024-07-11 2:52 [PATCH v5 0/5] Implement QATzip compression method Yichen Wang
` (2 preceding siblings ...)
2024-07-11 2:52 ` [PATCH v5 3/5] migration: Add migration parameters for QATzip Yichen Wang
@ 2024-07-11 2:52 ` Yichen Wang
2024-07-12 14:17 ` Fabiano Rosas
2024-07-11 2:52 ` [PATCH v5 5/5] tests/migration: Add integration test for " Yichen Wang
2024-07-11 15:44 ` [PATCH v5 0/5] Implement QATzip " Peter Xu
5 siblings, 1 reply; 14+ messages in thread
From: Yichen Wang @ 2024-07-11 2:52 UTC (permalink / raw)
To: Peter Xu, Fabiano Rosas, Paolo Bonzini, Daniel P. Berrangé,
Eduardo Habkost, Marc-André Lureau, Thomas Huth,
Philippe Mathieu-Daudé, Eric Blake, Markus Armbruster,
Laurent Vivier, qemu-devel
Cc: Hao Xiang, Liu, Yuan1, Zou, Nanhai, Ho-Ren (Jack) Chuang,
Yichen Wang, Bryan Zhang
From: Bryan Zhang <bryan.zhang@bytedance.com>
Adds support for 'qatzip' as an option for the multifd compression
method parameter, and implements using QAT for 'qatzip' compression and
decompression.
Signed-off-by: Bryan Zhang <bryan.zhang@bytedance.com>
Signed-off-by: Hao Xiang <hao.xiang@linux.dev>
Signed-off-by: Yichen Wang <yichen.wang@bytedance.com>
---
hw/core/qdev-properties-system.c | 6 +-
migration/meson.build | 1 +
migration/multifd-qatzip.c | 403 +++++++++++++++++++++++++++++++
migration/multifd.h | 5 +-
qapi/migration.json | 3 +
tests/qtest/meson.build | 4 +
6 files changed, 419 insertions(+), 3 deletions(-)
create mode 100644 migration/multifd-qatzip.c
diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
index f13350b4fb..eb50d6ec5b 100644
--- a/hw/core/qdev-properties-system.c
+++ b/hw/core/qdev-properties-system.c
@@ -659,7 +659,11 @@ const PropertyInfo qdev_prop_fdc_drive_type = {
const PropertyInfo qdev_prop_multifd_compression = {
.name = "MultiFDCompression",
.description = "multifd_compression values, "
- "none/zlib/zstd/qpl/uadk",
+ "none/zlib/zstd/qpl/uadk"
+#ifdef CONFIG_QATZIP
+ "/qatzip"
+#endif
+ ,
.enum_table = &MultiFDCompression_lookup,
.get = qdev_propinfo_get_enum,
.set = qdev_propinfo_set_enum,
diff --git a/migration/meson.build b/migration/meson.build
index 5ce2acb41e..c9454c26ae 100644
--- a/migration/meson.build
+++ b/migration/meson.build
@@ -41,6 +41,7 @@ system_ss.add(when: rdma, if_true: files('rdma.c'))
system_ss.add(when: zstd, if_true: files('multifd-zstd.c'))
system_ss.add(when: qpl, if_true: files('multifd-qpl.c'))
system_ss.add(when: uadk, if_true: files('multifd-uadk.c'))
+system_ss.add(when: qatzip, if_true: files('multifd-qatzip.c'))
specific_ss.add(when: 'CONFIG_SYSTEM_ONLY',
if_true: files('ram.c',
diff --git a/migration/multifd-qatzip.c b/migration/multifd-qatzip.c
new file mode 100644
index 0000000000..d01d51de8f
--- /dev/null
+++ b/migration/multifd-qatzip.c
@@ -0,0 +1,403 @@
+/*
+ * Multifd QATzip compression implementation
+ *
+ * Copyright (c) Bytedance
+ *
+ * Authors:
+ * Bryan Zhang <bryan.zhang@bytedance.com>
+ * Hao Xiang <hao.xiang@bytedance.com>
+ * Yichen Wang <yichen.wang@bytedance.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "exec/ramblock.h"
+#include "qapi/error.h"
+#include "qemu/error-report.h"
+#include "qapi/qapi-types-migration.h"
+#include "options.h"
+#include "multifd.h"
+#include <qatzip.h>
+
+typedef struct {
+ /*
+ * Unique session for use with QATzip API
+ */
+ QzSession_T sess;
+
+ /*
+ * For compression: Buffer for pages to compress
+ * For decompression: Buffer for data to decompress
+ */
+ uint8_t *in_buf;
+ uint32_t in_len;
+
+ /*
+ * For compression: Output buffer of compressed data
+ * For decompression: Output buffer of decompressed data
+ */
+ uint8_t *out_buf;
+ uint32_t out_len;
+} QatzipData;
+
+/**
+ * qatzip_send_setup: Set up QATzip session and private buffers.
+ *
+ * @param p Multifd channel params
+ * @param errp Pointer to error, which will be set in case of error
+ * @return 0 on success, -1 on error (and *errp will be set)
+ */
+static int qatzip_send_setup(MultiFDSendParams *p, Error **errp)
+{
+ QatzipData *q;
+ QzSessionParamsDeflate_T params;
+ const char *err_msg;
+ int ret;
+
+ q = g_new0(QatzipData, 1);
+ p->compress_data = q;
+ /* We need one extra place for the packet header */
+ p->iov = g_new0(struct iovec, 2);
+
+ /* Prefer without sw_fallback because of bad performance with sw_fallback.
+ * Warn if sw_fallback needs to be used. */
+ ret = qzInit(&q->sess, false);
+ if (ret != QZ_OK && ret != QZ_DUPLICATE) {
+ /* Warn, and try with sw_fallback. */
+ warn_report("Initilizing QAT with sw_fallback...");
+ ret = qzInit(&q->sess, true);
+ if (ret != QZ_OK && ret != QZ_DUPLICATE) {
+ /* Warn, and try with sw_fallback. */
+ err_msg = "qzInit failed";
+ goto err_free_q;
+ }
+ }
+
+ ret = qzGetDefaultsDeflate(¶ms);
+ if (ret != QZ_OK) {
+ err_msg = "qzGetDefaultsDeflate failed";
+ goto err_close;
+ }
+
+ /* Make sure to use configured QATzip compression level. */
+ params.common_params.comp_lvl = migrate_multifd_qatzip_level();
+
+ ret = qzSetupSessionDeflate(&q->sess, ¶ms);
+ if (ret != QZ_OK && ret != QZ_DUPLICATE) {
+ err_msg = "qzSetupSessionDeflate failed";
+ goto err_close;
+ }
+
+ if (MULTIFD_PACKET_SIZE > UINT32_MAX) {
+ err_msg = "packet size too large for QAT";
+ goto err_close;
+ }
+
+ q->in_len = MULTIFD_PACKET_SIZE;
+ q->in_buf = qzMalloc(q->in_len, 0, PINNED_MEM);
+ if (!q->in_buf) {
+ err_msg = "qzMalloc failed";
+ goto err_close;
+ }
+
+ q->out_len = qzMaxCompressedLength(MULTIFD_PACKET_SIZE, &q->sess);
+ q->out_buf = qzMalloc(q->out_len, 0, PINNED_MEM);
+ if (!q->out_buf) {
+ err_msg = "qzMalloc failed";
+ goto err_free_inbuf;
+ }
+
+ return 0;
+
+err_free_inbuf:
+ qzFree(q->in_buf);
+err_close:
+ qzClose(&q->sess);
+err_free_q:
+ g_free(q);
+ g_free(p->iov);
+ p->iov = NULL;
+ p->compress_data = NULL;
+ error_setg(errp, "multifd %u: %s", p->id, err_msg);
+ return -1;
+}
+
+/**
+ * qatzip_send_cleanup: Tear down QATzip session and release private buffers.
+ *
+ * @param p Multifd channel params
+ * @param errp Pointer to error, which will be set in case of error
+ * @return None
+ */
+static void qatzip_send_cleanup(MultiFDSendParams *p, Error **errp)
+{
+ QatzipData *q = p->compress_data;
+ const char *err_msg;
+ int ret;
+
+ ret = qzTeardownSession(&q->sess);
+ if (ret != QZ_OK) {
+ err_msg = "qzTeardownSession failed";
+ goto err;
+ }
+
+ ret = qzClose(&q->sess);
+ if (ret != QZ_OK) {
+ err_msg = "qzClose failed";
+ goto err;
+ }
+
+ qzFree(q->in_buf);
+ q->in_buf = NULL;
+ qzFree(q->out_buf);
+ q->out_buf = NULL;
+ g_free(p->iov);
+ p->iov = NULL;
+ g_free(p->compress_data);
+ p->compress_data = NULL;
+ return;
+
+err:
+ error_setg(errp, "multifd %u: %s", p->id, err_msg);
+}
+
+/**
+ * qatzip_send_prepare: Compress pages and update IO channel info.
+ *
+ * @param p Multifd channel params
+ * @param errp Pointer to error, which will be set in case of error
+ * @return 0 on success, -1 on error (and *errp will be set)
+ */
+static int qatzip_send_prepare(MultiFDSendParams *p, Error **errp)
+{
+ MultiFDPages_t *pages = p->pages;
+ QatzipData *q = p->compress_data;
+ int ret;
+ unsigned int in_len, out_len;
+
+ if (!multifd_send_prepare_common(p)) {
+ goto out;
+ }
+
+ /* Unlike other multifd compression implementations, we use a
+ * non-streaming API and place all the data into one buffer, rather than
+ * sending each page to the compression API at a time. */
+ for (int i = 0; i < pages->normal_num; i++) {
+ memcpy(q->in_buf + (i * p->page_size),
+ p->pages->block->host + pages->offset[i],
+ p->page_size);
+ }
+
+ in_len = pages->normal_num * p->page_size;
+ if (in_len > q->in_len) {
+ error_setg(errp, "multifd %u: unexpectedly large input", p->id);
+ return -1;
+ }
+ out_len = q->out_len;
+
+ /*
+ * Unlike other multifd compression implementations, we use a non-streaming
+ * API and place all the data into one buffer, rather than sending each page
+ * to the compression API at a time. Based on initial benchmarks, the
+ * non-streaming API outperforms the streaming API. Plus, the logic in QEMU
+ * is friendly to using the non-streaming API anyway. If either of these
+ * statements becomes no longer true, we can revisit adding a streaming
+ * implementation.
+ */
+ ret = qzCompress(&q->sess, q->in_buf, &in_len, q->out_buf, &out_len, 1);
+ if (ret != QZ_OK) {
+ error_setg(errp, "multifd %u: QATzip returned %d instead of QZ_OK",
+ p->id, ret);
+ return -1;
+ }
+ if (in_len != pages->normal_num * p->page_size) {
+ error_setg(errp, "multifd %u: QATzip failed to compress all input",
+ p->id);
+ return -1;
+ }
+
+ p->iov[p->iovs_num].iov_base = q->out_buf;
+ p->iov[p->iovs_num].iov_len = out_len;
+ p->iovs_num++;
+ p->next_packet_size = out_len;
+
+out:
+ p->flags |= MULTIFD_FLAG_QATZIP;
+ multifd_send_fill_packet(p);
+ return 0;
+}
+
+/**
+ * qatzip_recv_setup: Set up QATzip session and allocate private buffers.
+ *
+ * @param p Multifd channel params
+ * @param errp Pointer to error, which will be set in case of error
+ * @return 0 on success, -1 on error (and *errp will be set)
+ */
+static int qatzip_recv_setup(MultiFDRecvParams *p, Error **errp)
+{
+ QatzipData *q;
+ QzSessionParamsDeflate_T params;
+ const char *err_msg;
+ int ret;
+
+ q = g_new0(QatzipData, 1);
+ p->compress_data = q;
+
+ /* Prefer without sw_fallback because of bad performance with sw_fallback.
+ * Warn if sw_fallback needs to be used. */
+ ret = qzInit(&q->sess, false);
+ if (ret != QZ_OK && ret != QZ_DUPLICATE) {
+ /* Warn, and try with sw_fallback. */
+ warn_report("Initilizing QAT with sw_fallback...");
+ ret = qzInit(&q->sess, true);
+ if (ret != QZ_OK && ret != QZ_DUPLICATE) {
+ /* Warn, and try with sw_fallback. */
+ err_msg = "qzInit failed";
+ goto err_free_q;
+ }
+ }
+
+ ret = qzGetDefaultsDeflate(¶ms);
+ if (ret != QZ_OK) {
+ err_msg = "qzGetDefaultsDeflate failed";
+ goto err_close;
+ }
+
+ ret = qzSetupSessionDeflate(&q->sess, ¶ms);
+ if (ret != QZ_OK && ret != QZ_DUPLICATE) {
+ err_msg = "qzSetupSessionDeflate failed";
+ goto err_close;
+ }
+
+ /*
+ * Mimic multifd-zlib, which reserves extra space for the
+ * incoming packet.
+ */
+ q->in_len = MULTIFD_PACKET_SIZE * 2;
+ /* PINNED_MEM is an enum from qatzip headers, which means to use
+ * kzalloc_node() to allocate memory for QAT DMA purposes. */
+ q->in_buf = qzMalloc(q->in_len, 0, PINNED_MEM);
+ if (!q->in_buf) {
+ err_msg = "qzMalloc failed";
+ goto err_close;
+ }
+
+ q->out_len = MULTIFD_PACKET_SIZE;
+ q->out_buf = qzMalloc(q->out_len, 0, PINNED_MEM);
+ if (!q->out_buf) {
+ err_msg = "qzMalloc failed";
+ goto err_free_inbuf;
+ }
+
+ return 0;
+
+err_free_inbuf:
+ qzFree(q->in_buf);
+err_close:
+ qzClose(&q->sess);
+err_free_q:
+ g_free(q);
+ p->compress_data = NULL;
+ error_setg(errp, "multifd %u: %s", p->id, err_msg);
+ return -1;
+}
+
+/**
+ * qatzip_recv_cleanup: Tear down QATzip session and release private buffers.
+ *
+ * @param p Multifd channel params
+ * @return None
+ */
+static void qatzip_recv_cleanup(MultiFDRecvParams *p)
+{
+ QatzipData *q = p->compress_data;
+
+ /* Ignoring return values here due to function signature. */
+ qzTeardownSession(&q->sess);
+ qzClose(&q->sess);
+ qzFree(q->in_buf);
+ qzFree(q->out_buf);
+ g_free(p->compress_data);
+}
+
+
+/**
+ * qatzip_recv: Decompress pages and copy them to the appropriate
+ * locations.
+ *
+ * @param p Multifd channel params
+ * @param errp Pointer to error, which will be set in case of error
+ * @return 0 on success, -1 on error (and *errp will be set)
+ */
+static int qatzip_recv(MultiFDRecvParams *p, Error **errp)
+{
+ QatzipData *q = p->compress_data;
+ int ret;
+ unsigned int in_len, out_len;
+ uint32_t in_size = p->next_packet_size;
+ uint32_t expected_size = p->normal_num * p->page_size;
+ uint32_t flags = p->flags & MULTIFD_FLAG_COMPRESSION_MASK;
+
+ if (in_size > q->in_len) {
+ error_setg(errp, "multifd %u: received unexpectedly large packet",
+ p->id);
+ return -1;
+ }
+
+ if (flags != MULTIFD_FLAG_QATZIP) {
+ error_setg(errp, "multifd %u: flags received %x flags expected %x",
+ p->id, flags, MULTIFD_FLAG_QATZIP);
+ return -1;
+ }
+
+ multifd_recv_zero_page_process(p);
+ if (!p->normal_num) {
+ assert(in_size == 0);
+ return 0;
+ }
+
+ ret = qio_channel_read_all(p->c, (void *)q->in_buf, in_size, errp);
+ if (ret != 0) {
+ return ret;
+ }
+
+ in_len = in_size;
+ out_len = q->out_len;
+ ret = qzDecompress(&q->sess, q->in_buf, &in_len, q->out_buf, &out_len);
+ if (ret != QZ_OK) {
+ error_setg(errp, "multifd %u: qzDecompress failed", p->id);
+ return -1;
+ }
+ if (out_len != expected_size) {
+ error_setg(errp, "multifd %u: packet size received %u size expected %u",
+ p->id, out_len, expected_size);
+ return -1;
+ }
+
+ /* Copy each page to its appropriate location. */
+ for (int i = 0; i < p->normal_num; i++) {
+ memcpy(p->host + p->normal[i],
+ q->out_buf + p->page_size * i,
+ p->page_size);
+ }
+ return 0;
+}
+
+static MultiFDMethods multifd_qatzip_ops = {
+ .send_setup = qatzip_send_setup,
+ .send_cleanup = qatzip_send_cleanup,
+ .send_prepare = qatzip_send_prepare,
+ .recv_setup = qatzip_recv_setup,
+ .recv_cleanup = qatzip_recv_cleanup,
+ .recv = qatzip_recv
+};
+
+static void multifd_qatzip_register(void)
+{
+ multifd_register_ops(MULTIFD_COMPRESSION_QATZIP, &multifd_qatzip_ops);
+}
+
+migration_init(multifd_qatzip_register);
diff --git a/migration/multifd.h b/migration/multifd.h
index 0ecd6f47d7..adceb65050 100644
--- a/migration/multifd.h
+++ b/migration/multifd.h
@@ -34,14 +34,15 @@ MultiFDRecvData *multifd_get_recv_data(void);
/* Multifd Compression flags */
#define MULTIFD_FLAG_SYNC (1 << 0)
-/* We reserve 4 bits for compression methods */
-#define MULTIFD_FLAG_COMPRESSION_MASK (0xf << 1)
+/* We reserve 5 bits for compression methods */
+#define MULTIFD_FLAG_COMPRESSION_MASK (0x1f << 1)
/* we need to be compatible. Before compression value was 0 */
#define MULTIFD_FLAG_NOCOMP (0 << 1)
#define MULTIFD_FLAG_ZLIB (1 << 1)
#define MULTIFD_FLAG_ZSTD (2 << 1)
#define MULTIFD_FLAG_QPL (4 << 1)
#define MULTIFD_FLAG_UADK (8 << 1)
+#define MULTIFD_FLAG_QATZIP (16 << 1)
/* This value needs to be a multiple of qemu_target_page_size() */
#define MULTIFD_PACKET_SIZE (512 * 1024)
diff --git a/qapi/migration.json b/qapi/migration.json
index cd08f2f710..42b5363449 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -558,6 +558,8 @@
#
# @zstd: use zstd compression method.
#
+# @qatzip: use qatzip compression method. (Since 9.1)
+#
# @qpl: use qpl compression method. Query Processing Library(qpl) is
# based on the deflate compression algorithm and use the Intel
# In-Memory Analytics Accelerator(IAA) accelerated compression
@@ -570,6 +572,7 @@
{ 'enum': 'MultiFDCompression',
'data': [ 'none', 'zlib',
{ 'name': 'zstd', 'if': 'CONFIG_ZSTD' },
+ { 'name': 'qatzip', 'if': 'CONFIG_QATZIP'},
{ 'name': 'qpl', 'if': 'CONFIG_QPL' },
{ 'name': 'uadk', 'if': 'CONFIG_UADK' } ] }
diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
index 6508bfb1a2..3068d73e08 100644
--- a/tests/qtest/meson.build
+++ b/tests/qtest/meson.build
@@ -327,6 +327,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'),
--
Yichen Wang
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v5 5/5] tests/migration: Add integration test for 'qatzip' compression method
2024-07-11 2:52 [PATCH v5 0/5] Implement QATzip compression method Yichen Wang
` (3 preceding siblings ...)
2024-07-11 2:52 ` [PATCH v5 4/5] migration: Introduce 'qatzip' compression method Yichen Wang
@ 2024-07-11 2:52 ` Yichen Wang
2024-07-11 14:23 ` Peter Xu
2024-07-11 15:44 ` [PATCH v5 0/5] Implement QATzip " Peter Xu
5 siblings, 1 reply; 14+ messages in thread
From: Yichen Wang @ 2024-07-11 2:52 UTC (permalink / raw)
To: Peter Xu, Fabiano Rosas, Paolo Bonzini, Daniel P. Berrangé,
Eduardo Habkost, Marc-André Lureau, Thomas Huth,
Philippe Mathieu-Daudé, Eric Blake, Markus Armbruster,
Laurent Vivier, qemu-devel
Cc: Hao Xiang, Liu, Yuan1, Zou, Nanhai, Ho-Ren (Jack) Chuang,
Yichen Wang, Bryan Zhang
From: Bryan Zhang <bryan.zhang@bytedance.com>
Adds an integration test for 'qatzip'.
Signed-off-by: Bryan Zhang <bryan.zhang@bytedance.com>
Signed-off-by: Hao Xiang <hao.xiang@linux.dev>
Signed-off-by: Yichen Wang <yichen.wang@bytedance.com>
Reviewed-by: Fabiano Rosas <farosas@suse.de>
Signed-off-by: Yichen Wang <yichen.wang@bytedance.com>
---
tests/qtest/migration-test.c | 35 +++++++++++++++++++++++++++++++++++
1 file changed, 35 insertions(+)
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 70b606b888..b796dd21cb 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -32,6 +32,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"
@@ -2992,6 +2996,22 @@ 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)
+{
+ migrate_set_parameter_int(from, "multifd-qatzip-level", 2);
+ migrate_set_parameter_int(to, "multifd-qatzip-level", 2);
+
+ /* SW fallback is disabled by default, so enable it for testing. */
+ migrate_set_parameter_bool(from, "multifd-qatzip-sw-fallback", true);
+ migrate_set_parameter_bool(to, "multifd-qatzip-sw-fallback", true);
+
+ return test_migrate_precopy_tcp_multifd_start_common(from, to, "qatzip");
+}
+#endif
+
#ifdef CONFIG_QPL
static void *
test_migrate_precopy_tcp_multifd_qpl_start(QTestState *from,
@@ -3089,6 +3109,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_QPL
static void test_multifd_tcp_qpl(void)
{
@@ -3992,6 +4023,10 @@ int main(int argc, char **argv)
migration_test_add("/migration/multifd/tcp/plain/zstd",
test_multifd_tcp_zstd);
#endif
+#ifdef CONFIG_QATZIP
+ migration_test_add("/migration/multifd/tcp/plain/qatzip",
+ test_multifd_tcp_qatzip);
+#endif
#ifdef CONFIG_QPL
migration_test_add("/migration/multifd/tcp/plain/qpl",
test_multifd_tcp_qpl);
--
Yichen Wang
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v5 5/5] tests/migration: Add integration test for 'qatzip' compression method
2024-07-11 2:52 ` [PATCH v5 5/5] tests/migration: Add integration test for " Yichen Wang
@ 2024-07-11 14:23 ` Peter Xu
2024-07-11 16:45 ` [External] " Yichen Wang
0 siblings, 1 reply; 14+ messages in thread
From: Peter Xu @ 2024-07-11 14:23 UTC (permalink / raw)
To: Yichen Wang
Cc: Fabiano Rosas, Paolo Bonzini, Daniel P. Berrangé,
Eduardo Habkost, Marc-André Lureau, Thomas Huth,
Philippe Mathieu-Daudé, Eric Blake, Markus Armbruster,
Laurent Vivier, qemu-devel, Hao Xiang, Liu, Yuan1, Zou, Nanhai,
Ho-Ren (Jack) Chuang, Bryan Zhang
On Wed, Jul 10, 2024 at 07:52:29PM -0700, Yichen Wang wrote:
> From: Bryan Zhang <bryan.zhang@bytedance.com>
>
> Adds an integration test for 'qatzip'.
>
> Signed-off-by: Bryan Zhang <bryan.zhang@bytedance.com>
> Signed-off-by: Hao Xiang <hao.xiang@linux.dev>
> Signed-off-by: Yichen Wang <yichen.wang@bytedance.com>
> Reviewed-by: Fabiano Rosas <farosas@suse.de>
> Signed-off-by: Yichen Wang <yichen.wang@bytedance.com>
> ---
> tests/qtest/migration-test.c | 35 +++++++++++++++++++++++++++++++++++
> 1 file changed, 35 insertions(+)
>
> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
> index 70b606b888..b796dd21cb 100644
> --- a/tests/qtest/migration-test.c
> +++ b/tests/qtest/migration-test.c
> @@ -32,6 +32,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"
> @@ -2992,6 +2996,22 @@ 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)
> +{
> + migrate_set_parameter_int(from, "multifd-qatzip-level", 2);
> + migrate_set_parameter_int(to, "multifd-qatzip-level", 2);
> +
> + /* SW fallback is disabled by default, so enable it for testing. */
> + migrate_set_parameter_bool(from, "multifd-qatzip-sw-fallback", true);
> + migrate_set_parameter_bool(to, "multifd-qatzip-sw-fallback", true);
Shouldn't this already crash when without the parameter?
> +
> + return test_migrate_precopy_tcp_multifd_start_common(from, to, "qatzip");
> +}
> +#endif
> +
> #ifdef CONFIG_QPL
> static void *
> test_migrate_precopy_tcp_multifd_qpl_start(QTestState *from,
> @@ -3089,6 +3109,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_QPL
> static void test_multifd_tcp_qpl(void)
> {
> @@ -3992,6 +4023,10 @@ int main(int argc, char **argv)
> migration_test_add("/migration/multifd/tcp/plain/zstd",
> test_multifd_tcp_zstd);
> #endif
> +#ifdef CONFIG_QATZIP
> + migration_test_add("/migration/multifd/tcp/plain/qatzip",
> + test_multifd_tcp_qatzip);
> +#endif
> #ifdef CONFIG_QPL
> migration_test_add("/migration/multifd/tcp/plain/qpl",
> test_multifd_tcp_qpl);
> --
> Yichen Wang
>
--
Peter Xu
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v5 0/5] Implement QATzip compression method
2024-07-11 2:52 [PATCH v5 0/5] Implement QATzip compression method Yichen Wang
` (4 preceding siblings ...)
2024-07-11 2:52 ` [PATCH v5 5/5] tests/migration: Add integration test for " Yichen Wang
@ 2024-07-11 15:44 ` Peter Xu
2024-07-11 16:48 ` [External] " Yichen Wang
5 siblings, 1 reply; 14+ messages in thread
From: Peter Xu @ 2024-07-11 15:44 UTC (permalink / raw)
To: Yichen Wang
Cc: Fabiano Rosas, Paolo Bonzini, Daniel P. Berrangé,
Eduardo Habkost, Marc-André Lureau, Thomas Huth,
Philippe Mathieu-Daudé, Eric Blake, Markus Armbruster,
Laurent Vivier, qemu-devel, Hao Xiang, Liu, Yuan1, Zou, Nanhai,
Ho-Ren (Jack) Chuang
On Wed, Jul 10, 2024 at 07:52:24PM -0700, Yichen Wang wrote:
> v5:
> - Rebase changes on top of 59084feb256c617063e0dbe7e64821ae8852d7cf
> - Add documentations about migration with qatzip accerlation
> - Remove multifd-qatzip-sw-fallback option
I think Yuan provided quite a few meaningful comments, did you address all
of them?
You didn't reply in the previous version, and you didn't add anything in
the changelog. I suggest you at least do one of them in the future so that
reviewers can understand what happen.
Thanks,
--
Peter Xu
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [External] Re: [PATCH v5 5/5] tests/migration: Add integration test for 'qatzip' compression method
2024-07-11 14:23 ` Peter Xu
@ 2024-07-11 16:45 ` Yichen Wang
0 siblings, 0 replies; 14+ messages in thread
From: Yichen Wang @ 2024-07-11 16:45 UTC (permalink / raw)
To: Peter Xu
Cc: Fabiano Rosas, Paolo Bonzini, Daniel P. Berrangé,
Eduardo Habkost, Marc-André Lureau, Thomas Huth,
Philippe Mathieu-Daudé, Eric Blake, Markus Armbruster,
Laurent Vivier, qemu-devel, Hao Xiang, Liu, Yuan1, Zou, Nanhai,
Ho-Ren (Jack) Chuang, Bryan Zhang
On Thu, Jul 11, 2024 at 7:23 AM Peter Xu <peterx@redhat.com> wrote:
>
> On Wed, Jul 10, 2024 at 07:52:29PM -0700, Yichen Wang wrote:
> > From: Bryan Zhang <bryan.zhang@bytedance.com>
> >
> > Adds an integration test for 'qatzip'.
> >
> > Signed-off-by: Bryan Zhang <bryan.zhang@bytedance.com>
> > Signed-off-by: Hao Xiang <hao.xiang@linux.dev>
> > Signed-off-by: Yichen Wang <yichen.wang@bytedance.com>
> > Reviewed-by: Fabiano Rosas <farosas@suse.de>
> > Signed-off-by: Yichen Wang <yichen.wang@bytedance.com>
> > ---
> > tests/qtest/migration-test.c | 35 +++++++++++++++++++++++++++++++++++
> > 1 file changed, 35 insertions(+)
> >
> > diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
> > index 70b606b888..b796dd21cb 100644
> > --- a/tests/qtest/migration-test.c
> > +++ b/tests/qtest/migration-test.c
> > @@ -32,6 +32,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"
> > @@ -2992,6 +2996,22 @@ 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)
> > +{
> > + migrate_set_parameter_int(from, "multifd-qatzip-level", 2);
> > + migrate_set_parameter_int(to, "multifd-qatzip-level", 2);
> > +
> > + /* SW fallback is disabled by default, so enable it for testing. */
> > + migrate_set_parameter_bool(from, "multifd-qatzip-sw-fallback", true);
> > + migrate_set_parameter_bool(to, "multifd-qatzip-sw-fallback", true);
>
> Shouldn't this already crash when without the parameter?
Ah, my bad. I tested the features manually with two machines, and
didn't run this. I will fix it in my next version.
>
> > +
> > + return test_migrate_precopy_tcp_multifd_start_common(from, to, "qatzip");
> > +}
> > +#endif
> > +
> > #ifdef CONFIG_QPL
> > static void *
> > test_migrate_precopy_tcp_multifd_qpl_start(QTestState *from,
> > @@ -3089,6 +3109,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_QPL
> > static void test_multifd_tcp_qpl(void)
> > {
> > @@ -3992,6 +4023,10 @@ int main(int argc, char **argv)
> > migration_test_add("/migration/multifd/tcp/plain/zstd",
> > test_multifd_tcp_zstd);
> > #endif
> > +#ifdef CONFIG_QATZIP
> > + migration_test_add("/migration/multifd/tcp/plain/qatzip",
> > + test_multifd_tcp_qatzip);
> > +#endif
> > #ifdef CONFIG_QPL
> > migration_test_add("/migration/multifd/tcp/plain/qpl",
> > test_multifd_tcp_qpl);
> > --
> > Yichen Wang
> >
>
> --
> Peter Xu
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [External] Re: [PATCH v5 0/5] Implement QATzip compression method
2024-07-11 15:44 ` [PATCH v5 0/5] Implement QATzip " Peter Xu
@ 2024-07-11 16:48 ` Yichen Wang
2024-07-11 19:36 ` Peter Xu
0 siblings, 1 reply; 14+ messages in thread
From: Yichen Wang @ 2024-07-11 16:48 UTC (permalink / raw)
To: Peter Xu
Cc: Fabiano Rosas, Paolo Bonzini, Daniel P. Berrangé,
Eduardo Habkost, Marc-André Lureau, Thomas Huth,
Philippe Mathieu-Daudé, Eric Blake, Markus Armbruster,
Laurent Vivier, qemu-devel, Hao Xiang, Liu, Yuan1, Zou, Nanhai,
Ho-Ren (Jack) Chuang
On Thu, Jul 11, 2024 at 8:45 AM Peter Xu <peterx@redhat.com> wrote:
>
> On Wed, Jul 10, 2024 at 07:52:24PM -0700, Yichen Wang wrote:
> > v5:
> > - Rebase changes on top of 59084feb256c617063e0dbe7e64821ae8852d7cf
> > - Add documentations about migration with qatzip accerlation
> > - Remove multifd-qatzip-sw-fallback option
>
> I think Yuan provided quite a few meaningful comments, did you address all
> of them?
Yes. I do.
>
> You didn't reply in the previous version, and you didn't add anything in
> the changelog. I suggest you at least do one of them in the future so that
> reviewers can understand what happen.
They are all very good comments, and instead of replying I just fix
them all and include it in my next patch. In my changelog I do include
all the changes and comments we discussed in v4. Sorry I am new to the
community, so I will reply "fixed" in the previous email before
pushing the next version. Thanks a lot, and sorry for that.
>
> Thanks,
>
> --
> Peter Xu
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [External] Re: [PATCH v5 0/5] Implement QATzip compression method
2024-07-11 16:48 ` [External] " Yichen Wang
@ 2024-07-11 19:36 ` Peter Xu
0 siblings, 0 replies; 14+ messages in thread
From: Peter Xu @ 2024-07-11 19:36 UTC (permalink / raw)
To: Yichen Wang
Cc: Fabiano Rosas, Paolo Bonzini, Daniel P. Berrangé,
Eduardo Habkost, Marc-André Lureau, Thomas Huth,
Philippe Mathieu-Daudé, Eric Blake, Markus Armbruster,
Laurent Vivier, qemu-devel, Hao Xiang, Liu, Yuan1, Zou, Nanhai,
Ho-Ren (Jack) Chuang
On Thu, Jul 11, 2024 at 09:48:02AM -0700, Yichen Wang wrote:
> On Thu, Jul 11, 2024 at 8:45 AM Peter Xu <peterx@redhat.com> wrote:
> >
> > On Wed, Jul 10, 2024 at 07:52:24PM -0700, Yichen Wang wrote:
> > > v5:
> > > - Rebase changes on top of 59084feb256c617063e0dbe7e64821ae8852d7cf
> > > - Add documentations about migration with qatzip accerlation
> > > - Remove multifd-qatzip-sw-fallback option
> >
> > I think Yuan provided quite a few meaningful comments, did you address all
> > of them?
> Yes. I do.
> >
> > You didn't reply in the previous version, and you didn't add anything in
> > the changelog. I suggest you at least do one of them in the future so that
> > reviewers can understand what happen.
> They are all very good comments, and instead of replying I just fix
> them all and include it in my next patch. In my changelog I do include
> all the changes and comments we discussed in v4. Sorry I am new to the
> community, so I will reply "fixed" in the previous email before
> pushing the next version. Thanks a lot, and sorry for that.
That's all fine! You can definitely mention them too here in the changelog
if you think that's easier.
One last nitpick is in the major patch you duplicated part of the comment
when I was requesting a movement (the part explaining why you used a buffer
rather than submit compression for each page without memcpy), I suggest you
can simply move that whole comment above, rather than copying.
I don't have any further questions on this series.
Thanks,
--
Peter Xu
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v5 4/5] migration: Introduce 'qatzip' compression method
2024-07-11 2:52 ` [PATCH v5 4/5] migration: Introduce 'qatzip' compression method Yichen Wang
@ 2024-07-12 14:17 ` Fabiano Rosas
2024-07-14 5:53 ` [External] " Yichen Wang
0 siblings, 1 reply; 14+ messages in thread
From: Fabiano Rosas @ 2024-07-12 14:17 UTC (permalink / raw)
To: Yichen Wang, Peter Xu, Paolo Bonzini, Daniel P. Berrangé,
Eduardo Habkost, Marc-André Lureau, Thomas Huth,
Philippe Mathieu-Daudé, Eric Blake, Markus Armbruster,
Laurent Vivier, qemu-devel
Cc: Hao Xiang, Liu, Yuan1, Zou, Nanhai, Ho-Ren (Jack) Chuang,
Yichen Wang, Bryan Zhang
Yichen Wang <yichen.wang@bytedance.com> writes:
> From: Bryan Zhang <bryan.zhang@bytedance.com>
>
> Adds support for 'qatzip' as an option for the multifd compression
> method parameter, and implements using QAT for 'qatzip' compression and
> decompression.
>
> Signed-off-by: Bryan Zhang <bryan.zhang@bytedance.com>
> Signed-off-by: Hao Xiang <hao.xiang@linux.dev>
> Signed-off-by: Yichen Wang <yichen.wang@bytedance.com>
> ---
> hw/core/qdev-properties-system.c | 6 +-
> migration/meson.build | 1 +
> migration/multifd-qatzip.c | 403 +++++++++++++++++++++++++++++++
> migration/multifd.h | 5 +-
> qapi/migration.json | 3 +
> tests/qtest/meson.build | 4 +
> 6 files changed, 419 insertions(+), 3 deletions(-)
> create mode 100644 migration/multifd-qatzip.c
>
> diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
> index f13350b4fb..eb50d6ec5b 100644
> --- a/hw/core/qdev-properties-system.c
> +++ b/hw/core/qdev-properties-system.c
> @@ -659,7 +659,11 @@ const PropertyInfo qdev_prop_fdc_drive_type = {
> const PropertyInfo qdev_prop_multifd_compression = {
> .name = "MultiFDCompression",
> .description = "multifd_compression values, "
> - "none/zlib/zstd/qpl/uadk",
> + "none/zlib/zstd/qpl/uadk"
> +#ifdef CONFIG_QATZIP
> + "/qatzip"
> +#endif
It seems the other accelerators don't need the ifdef. What's different
here?
> + ,
> .enum_table = &MultiFDCompression_lookup,
> .get = qdev_propinfo_get_enum,
> .set = qdev_propinfo_set_enum,
> diff --git a/migration/meson.build b/migration/meson.build
> index 5ce2acb41e..c9454c26ae 100644
> --- a/migration/meson.build
> +++ b/migration/meson.build
> @@ -41,6 +41,7 @@ system_ss.add(when: rdma, if_true: files('rdma.c'))
> system_ss.add(when: zstd, if_true: files('multifd-zstd.c'))
> system_ss.add(when: qpl, if_true: files('multifd-qpl.c'))
> system_ss.add(when: uadk, if_true: files('multifd-uadk.c'))
> +system_ss.add(when: qatzip, if_true: files('multifd-qatzip.c'))
>
> specific_ss.add(when: 'CONFIG_SYSTEM_ONLY',
> if_true: files('ram.c',
> diff --git a/migration/multifd-qatzip.c b/migration/multifd-qatzip.c
> new file mode 100644
> index 0000000000..d01d51de8f
> --- /dev/null
> +++ b/migration/multifd-qatzip.c
> @@ -0,0 +1,403 @@
> +/*
> + * Multifd QATzip compression implementation
> + *
> + * Copyright (c) Bytedance
> + *
> + * Authors:
> + * Bryan Zhang <bryan.zhang@bytedance.com>
> + * Hao Xiang <hao.xiang@bytedance.com>
> + * Yichen Wang <yichen.wang@bytedance.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "exec/ramblock.h"
> +#include "qapi/error.h"
> +#include "qemu/error-report.h"
> +#include "qapi/qapi-types-migration.h"
> +#include "options.h"
> +#include "multifd.h"
> +#include <qatzip.h>
> +
> +typedef struct {
> + /*
> + * Unique session for use with QATzip API
> + */
> + QzSession_T sess;
> +
> + /*
> + * For compression: Buffer for pages to compress
> + * For decompression: Buffer for data to decompress
> + */
> + uint8_t *in_buf;
> + uint32_t in_len;
> +
> + /*
> + * For compression: Output buffer of compressed data
> + * For decompression: Output buffer of decompressed data
> + */
> + uint8_t *out_buf;
> + uint32_t out_len;
> +} QatzipData;
> +
> +/**
> + * qatzip_send_setup: Set up QATzip session and private buffers.
> + *
> + * @param p Multifd channel params
> + * @param errp Pointer to error, which will be set in case of error
> + * @return 0 on success, -1 on error (and *errp will be set)
> + */
> +static int qatzip_send_setup(MultiFDSendParams *p, Error **errp)
> +{
> + QatzipData *q;
> + QzSessionParamsDeflate_T params;
> + const char *err_msg;
> + int ret;
> +
> + q = g_new0(QatzipData, 1);
> + p->compress_data = q;
> + /* We need one extra place for the packet header */
> + p->iov = g_new0(struct iovec, 2);
> +
> + /* Prefer without sw_fallback because of bad performance with sw_fallback.
> + * Warn if sw_fallback needs to be used. */
Please run scripts/checkpatch.pl on your series. This style of comments
should have been flagged as non-conformant with our guidelines.
> + ret = qzInit(&q->sess, false);
> + if (ret != QZ_OK && ret != QZ_DUPLICATE) {
> + /* Warn, and try with sw_fallback. */
> + warn_report("Initilizing QAT with sw_fallback...");
This will warn for each multifd channel, maybe use warn_report_once
instead. Also s/Initilizing/Initializing/ and let's spell out "software
fallback".
> + ret = qzInit(&q->sess, true);
> + if (ret != QZ_OK && ret != QZ_DUPLICATE) {
> + /* Warn, and try with sw_fallback. */
> + err_msg = "qzInit failed";
> + goto err_free_q;
> + }
> + }
> +
> + ret = qzGetDefaultsDeflate(¶ms);
> + if (ret != QZ_OK) {
> + err_msg = "qzGetDefaultsDeflate failed";
> + goto err_close;
> + }
> +
> + /* Make sure to use configured QATzip compression level. */
> + params.common_params.comp_lvl = migrate_multifd_qatzip_level();
> +
> + ret = qzSetupSessionDeflate(&q->sess, ¶ms);
> + if (ret != QZ_OK && ret != QZ_DUPLICATE) {
> + err_msg = "qzSetupSessionDeflate failed";
> + goto err_close;
> + }
> +
> + if (MULTIFD_PACKET_SIZE > UINT32_MAX) {
> + err_msg = "packet size too large for QAT";
> + goto err_close;
> + }
> +
> + q->in_len = MULTIFD_PACKET_SIZE;
> + q->in_buf = qzMalloc(q->in_len, 0, PINNED_MEM);
> + if (!q->in_buf) {
> + err_msg = "qzMalloc failed";
> + goto err_close;
> + }
> +
> + q->out_len = qzMaxCompressedLength(MULTIFD_PACKET_SIZE, &q->sess);
> + q->out_buf = qzMalloc(q->out_len, 0, PINNED_MEM);
> + if (!q->out_buf) {
> + err_msg = "qzMalloc failed";
> + goto err_free_inbuf;
> + }
> +
> + return 0;
> +
> +err_free_inbuf:
> + qzFree(q->in_buf);
> +err_close:
> + qzClose(&q->sess);
> +err_free_q:
> + g_free(q);
> + g_free(p->iov);
> + p->iov = NULL;
> + p->compress_data = NULL;
> + error_setg(errp, "multifd %u: %s", p->id, err_msg);
> + return -1;
> +}
> +
> +/**
> + * qatzip_send_cleanup: Tear down QATzip session and release private buffers.
> + *
> + * @param p Multifd channel params
> + * @param errp Pointer to error, which will be set in case of error
> + * @return None
> + */
> +static void qatzip_send_cleanup(MultiFDSendParams *p, Error **errp)
> +{
> + QatzipData *q = p->compress_data;
> + const char *err_msg;
> + int ret;
> +
> + ret = qzTeardownSession(&q->sess);
> + if (ret != QZ_OK) {
> + err_msg = "qzTeardownSession failed";
> + goto err;
> + }
> +
> + ret = qzClose(&q->sess);
> + if (ret != QZ_OK) {
> + err_msg = "qzClose failed";
> + goto err;
> + }
Can qzClose() be called twice on the same session pointer? It's possible
that we have already failed at multifd_send_setup() and still reach
here.
And what about qzTeardownSession()? Can it cope with an already closed
session?
And what about the sessions that never got created because we might have
exited early at the ops->send_setup() loop?
> +
> + qzFree(q->in_buf);
> + q->in_buf = NULL;
> + qzFree(q->out_buf);
> + q->out_buf = NULL;
These will double free here if send_setup has already freed.
> + g_free(p->iov);
> + p->iov = NULL;
> + g_free(p->compress_data);
> + p->compress_data = NULL;
> + return;
> +
> +err:
> + error_setg(errp, "multifd %u: %s", p->id, err_msg);
> +}
> +
> +/**
> + * qatzip_send_prepare: Compress pages and update IO channel info.
> + *
> + * @param p Multifd channel params
> + * @param errp Pointer to error, which will be set in case of error
> + * @return 0 on success, -1 on error (and *errp will be set)
> + */
> +static int qatzip_send_prepare(MultiFDSendParams *p, Error **errp)
> +{
> + MultiFDPages_t *pages = p->pages;
> + QatzipData *q = p->compress_data;
> + int ret;
> + unsigned int in_len, out_len;
> +
> + if (!multifd_send_prepare_common(p)) {
> + goto out;
> + }
> +
> + /* Unlike other multifd compression implementations, we use a
> + * non-streaming API and place all the data into one buffer, rather than
> + * sending each page to the compression API at a time. */
> + for (int i = 0; i < pages->normal_num; i++) {
> + memcpy(q->in_buf + (i * p->page_size),
> + p->pages->block->host + pages->offset[i],
pages->block->host
> + p->page_size);
> + }
> +
> + in_len = pages->normal_num * p->page_size;
> + if (in_len > q->in_len) {
> + error_setg(errp, "multifd %u: unexpectedly large input", p->id);
> + return -1;
> + }
> + out_len = q->out_len;
> +
> + /*
> + * Unlike other multifd compression implementations, we use a non-streaming
> + * API and place all the data into one buffer, rather than sending each page
> + * to the compression API at a time. Based on initial benchmarks, the
> + * non-streaming API outperforms the streaming API. Plus, the logic in QEMU
> + * is friendly to using the non-streaming API anyway. If either of these
> + * statements becomes no longer true, we can revisit adding a streaming
> + * implementation.
> + */
> + ret = qzCompress(&q->sess, q->in_buf, &in_len, q->out_buf, &out_len, 1);
> + if (ret != QZ_OK) {
> + error_setg(errp, "multifd %u: QATzip returned %d instead of QZ_OK",
> + p->id, ret);
> + return -1;
> + }
> + if (in_len != pages->normal_num * p->page_size) {
> + error_setg(errp, "multifd %u: QATzip failed to compress all input",
> + p->id);
> + return -1;
> + }
> +
> + p->iov[p->iovs_num].iov_base = q->out_buf;
> + p->iov[p->iovs_num].iov_len = out_len;
> + p->iovs_num++;
> + p->next_packet_size = out_len;
> +
> +out:
> + p->flags |= MULTIFD_FLAG_QATZIP;
> + multifd_send_fill_packet(p);
> + return 0;
> +}
> +
> +/**
> + * qatzip_recv_setup: Set up QATzip session and allocate private buffers.
> + *
> + * @param p Multifd channel params
> + * @param errp Pointer to error, which will be set in case of error
> + * @return 0 on success, -1 on error (and *errp will be set)
> + */
> +static int qatzip_recv_setup(MultiFDRecvParams *p, Error **errp)
> +{
> + QatzipData *q;
> + QzSessionParamsDeflate_T params;
> + const char *err_msg;
> + int ret;
> +
> + q = g_new0(QatzipData, 1);
> + p->compress_data = q;
> +
> + /* Prefer without sw_fallback because of bad performance with sw_fallback.
> + * Warn if sw_fallback needs to be used. */
> + ret = qzInit(&q->sess, false);
> + if (ret != QZ_OK && ret != QZ_DUPLICATE) {
> + /* Warn, and try with sw_fallback. */
> + warn_report("Initilizing QAT with sw_fallback...");
Same here. Also please add a hint that this is recv and the other one is
send. It helps with debug.
> + ret = qzInit(&q->sess, true);
> + if (ret != QZ_OK && ret != QZ_DUPLICATE) {
> + /* Warn, and try with sw_fallback. */
> + err_msg = "qzInit failed";
> + goto err_free_q;
> + }
> + }
> +
> + ret = qzGetDefaultsDeflate(¶ms);
> + if (ret != QZ_OK) {
> + err_msg = "qzGetDefaultsDeflate failed";
> + goto err_close;
> + }
> +
> + ret = qzSetupSessionDeflate(&q->sess, ¶ms);
> + if (ret != QZ_OK && ret != QZ_DUPLICATE) {
> + err_msg = "qzSetupSessionDeflate failed";
> + goto err_close;
> + }
> +
> + /*
> + * Mimic multifd-zlib, which reserves extra space for the
> + * incoming packet.
I'd put the actual rationale here. It will also help in the future to
spot that this implementation doesn't send uncompressed pages in case
the compression got too big.
> + */
> + q->in_len = MULTIFD_PACKET_SIZE * 2;
> + /* PINNED_MEM is an enum from qatzip headers, which means to use
> + * kzalloc_node() to allocate memory for QAT DMA purposes. */
> + q->in_buf = qzMalloc(q->in_len, 0, PINNED_MEM);
> + if (!q->in_buf) {
> + err_msg = "qzMalloc failed";
> + goto err_close;
> + }
> +
> + q->out_len = MULTIFD_PACKET_SIZE;
> + q->out_buf = qzMalloc(q->out_len, 0, PINNED_MEM);
> + if (!q->out_buf) {
> + err_msg = "qzMalloc failed";
> + goto err_free_inbuf;
> + }
> +
> + return 0;
> +
> +err_free_inbuf:
> + qzFree(q->in_buf);
> +err_close:
> + qzClose(&q->sess);
> +err_free_q:
> + g_free(q);
> + p->compress_data = NULL;
> + error_setg(errp, "multifd %u: %s", p->id, err_msg);
Or maybe put the recv/send information on this string.
> + return -1;
> +}
> +
> +/**
> + * qatzip_recv_cleanup: Tear down QATzip session and release private buffers.
> + *
> + * @param p Multifd channel params
> + * @return None
> + */
> +static void qatzip_recv_cleanup(MultiFDRecvParams *p)
> +{
> + QatzipData *q = p->compress_data;
> +
> + /* Ignoring return values here due to function signature. */
> + qzTeardownSession(&q->sess);
> + qzClose(&q->sess);
> + qzFree(q->in_buf);
> + qzFree(q->out_buf);
> + g_free(p->compress_data);
> +}
> +
> +
> +/**
> + * qatzip_recv: Decompress pages and copy them to the appropriate
> + * locations.
> + *
> + * @param p Multifd channel params
> + * @param errp Pointer to error, which will be set in case of error
> + * @return 0 on success, -1 on error (and *errp will be set)
> + */
> +static int qatzip_recv(MultiFDRecvParams *p, Error **errp)
> +{
> + QatzipData *q = p->compress_data;
> + int ret;
> + unsigned int in_len, out_len;
> + uint32_t in_size = p->next_packet_size;
> + uint32_t expected_size = p->normal_num * p->page_size;
> + uint32_t flags = p->flags & MULTIFD_FLAG_COMPRESSION_MASK;
> +
> + if (in_size > q->in_len) {
> + error_setg(errp, "multifd %u: received unexpectedly large packet",
> + p->id);
> + return -1;
> + }
> +
> + if (flags != MULTIFD_FLAG_QATZIP) {
> + error_setg(errp, "multifd %u: flags received %x flags expected %x",
> + p->id, flags, MULTIFD_FLAG_QATZIP);
> + return -1;
> + }
> +
> + multifd_recv_zero_page_process(p);
> + if (!p->normal_num) {
> + assert(in_size == 0);
> + return 0;
> + }
> +
> + ret = qio_channel_read_all(p->c, (void *)q->in_buf, in_size, errp);
> + if (ret != 0) {
> + return ret;
> + }
> +
> + in_len = in_size;
> + out_len = q->out_len;
> + ret = qzDecompress(&q->sess, q->in_buf, &in_len, q->out_buf, &out_len);
> + if (ret != QZ_OK) {
> + error_setg(errp, "multifd %u: qzDecompress failed", p->id);
> + return -1;
> + }
> + if (out_len != expected_size) {
> + error_setg(errp, "multifd %u: packet size received %u size expected %u",
> + p->id, out_len, expected_size);
> + return -1;
> + }
> +
> + /* Copy each page to its appropriate location. */
> + for (int i = 0; i < p->normal_num; i++) {
> + memcpy(p->host + p->normal[i],
> + q->out_buf + p->page_size * i,
> + p->page_size);
> + }
> + return 0;
> +}
> +
> +static MultiFDMethods multifd_qatzip_ops = {
> + .send_setup = qatzip_send_setup,
> + .send_cleanup = qatzip_send_cleanup,
> + .send_prepare = qatzip_send_prepare,
> + .recv_setup = qatzip_recv_setup,
> + .recv_cleanup = qatzip_recv_cleanup,
> + .recv = qatzip_recv
> +};
> +
> +static void multifd_qatzip_register(void)
> +{
> + multifd_register_ops(MULTIFD_COMPRESSION_QATZIP, &multifd_qatzip_ops);
> +}
> +
> +migration_init(multifd_qatzip_register);
> diff --git a/migration/multifd.h b/migration/multifd.h
> index 0ecd6f47d7..adceb65050 100644
> --- a/migration/multifd.h
> +++ b/migration/multifd.h
> @@ -34,14 +34,15 @@ MultiFDRecvData *multifd_get_recv_data(void);
> /* Multifd Compression flags */
> #define MULTIFD_FLAG_SYNC (1 << 0)
>
> -/* We reserve 4 bits for compression methods */
> -#define MULTIFD_FLAG_COMPRESSION_MASK (0xf << 1)
> +/* We reserve 5 bits for compression methods */
> +#define MULTIFD_FLAG_COMPRESSION_MASK (0x1f << 1)
> /* we need to be compatible. Before compression value was 0 */
> #define MULTIFD_FLAG_NOCOMP (0 << 1)
> #define MULTIFD_FLAG_ZLIB (1 << 1)
> #define MULTIFD_FLAG_ZSTD (2 << 1)
> #define MULTIFD_FLAG_QPL (4 << 1)
> #define MULTIFD_FLAG_UADK (8 << 1)
> +#define MULTIFD_FLAG_QATZIP (16 << 1)
>
> /* This value needs to be a multiple of qemu_target_page_size() */
> #define MULTIFD_PACKET_SIZE (512 * 1024)
> diff --git a/qapi/migration.json b/qapi/migration.json
> index cd08f2f710..42b5363449 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -558,6 +558,8 @@
> #
> # @zstd: use zstd compression method.
> #
> +# @qatzip: use qatzip compression method. (Since 9.1)
> +#
> # @qpl: use qpl compression method. Query Processing Library(qpl) is
> # based on the deflate compression algorithm and use the Intel
> # In-Memory Analytics Accelerator(IAA) accelerated compression
> @@ -570,6 +572,7 @@
> { 'enum': 'MultiFDCompression',
> 'data': [ 'none', 'zlib',
> { 'name': 'zstd', 'if': 'CONFIG_ZSTD' },
> + { 'name': 'qatzip', 'if': 'CONFIG_QATZIP'},
> { 'name': 'qpl', 'if': 'CONFIG_QPL' },
> { 'name': 'uadk', 'if': 'CONFIG_UADK' } ] }
>
> diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
> index 6508bfb1a2..3068d73e08 100644
> --- a/tests/qtest/meson.build
> +++ b/tests/qtest/meson.build
> @@ -327,6 +327,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'),
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [External] Re: [PATCH v5 4/5] migration: Introduce 'qatzip' compression method
2024-07-12 14:17 ` Fabiano Rosas
@ 2024-07-14 5:53 ` Yichen Wang
2024-07-15 16:32 ` Fabiano Rosas
0 siblings, 1 reply; 14+ messages in thread
From: Yichen Wang @ 2024-07-14 5:53 UTC (permalink / raw)
To: Fabiano Rosas
Cc: Peter Xu, Paolo Bonzini, Daniel P. Berrangé, Eduardo Habkost,
Marc-André Lureau, Thomas Huth, Philippe Mathieu-Daudé,
Eric Blake, Markus Armbruster, Laurent Vivier, qemu-devel,
Hao Xiang, Liu, Yuan1, Zou, Nanhai, Ho-Ren (Jack) Chuang,
Bryan Zhang
On Fri, Jul 12, 2024 at 7:17 AM Fabiano Rosas <farosas@suse.de> wrote:
>
> Yichen Wang <yichen.wang@bytedance.com> writes:
>
> > From: Bryan Zhang <bryan.zhang@bytedance.com>
> >
> > Adds support for 'qatzip' as an option for the multifd compression
> > method parameter, and implements using QAT for 'qatzip' compression and
> > decompression.
> >
> > Signed-off-by: Bryan Zhang <bryan.zhang@bytedance.com>
> > Signed-off-by: Hao Xiang <hao.xiang@linux.dev>
> > Signed-off-by: Yichen Wang <yichen.wang@bytedance.com>
> > ---
> > hw/core/qdev-properties-system.c | 6 +-
> > migration/meson.build | 1 +
> > migration/multifd-qatzip.c | 403 +++++++++++++++++++++++++++++++
> > migration/multifd.h | 5 +-
> > qapi/migration.json | 3 +
> > tests/qtest/meson.build | 4 +
> > 6 files changed, 419 insertions(+), 3 deletions(-)
> > create mode 100644 migration/multifd-qatzip.c
> >
> > diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
> > index f13350b4fb..eb50d6ec5b 100644
> > --- a/hw/core/qdev-properties-system.c
> > +++ b/hw/core/qdev-properties-system.c
> > @@ -659,7 +659,11 @@ const PropertyInfo qdev_prop_fdc_drive_type = {
> > const PropertyInfo qdev_prop_multifd_compression = {
> > .name = "MultiFDCompression",
> > .description = "multifd_compression values, "
> > - "none/zlib/zstd/qpl/uadk",
> > + "none/zlib/zstd/qpl/uadk"
> > +#ifdef CONFIG_QATZIP
> > + "/qatzip"
> > +#endif
>
> It seems the other accelerators don't need the ifdef. What's different
> here?
Just changed and align to other methods. Will fix in next version.
>
> > + ,
> > .enum_table = &MultiFDCompression_lookup,
> > .get = qdev_propinfo_get_enum,
> > .set = qdev_propinfo_set_enum,
> > diff --git a/migration/meson.build b/migration/meson.build
> > index 5ce2acb41e..c9454c26ae 100644
> > --- a/migration/meson.build
> > +++ b/migration/meson.build
> > @@ -41,6 +41,7 @@ system_ss.add(when: rdma, if_true: files('rdma.c'))
> > system_ss.add(when: zstd, if_true: files('multifd-zstd.c'))
> > system_ss.add(when: qpl, if_true: files('multifd-qpl.c'))
> > system_ss.add(when: uadk, if_true: files('multifd-uadk.c'))
> > +system_ss.add(when: qatzip, if_true: files('multifd-qatzip.c'))
> >
> > specific_ss.add(when: 'CONFIG_SYSTEM_ONLY',
> > if_true: files('ram.c',
> > diff --git a/migration/multifd-qatzip.c b/migration/multifd-qatzip.c
> > new file mode 100644
> > index 0000000000..d01d51de8f
> > --- /dev/null
> > +++ b/migration/multifd-qatzip.c
> > @@ -0,0 +1,403 @@
> > +/*
> > + * Multifd QATzip compression implementation
> > + *
> > + * Copyright (c) Bytedance
> > + *
> > + * Authors:
> > + * Bryan Zhang <bryan.zhang@bytedance.com>
> > + * Hao Xiang <hao.xiang@bytedance.com>
> > + * Yichen Wang <yichen.wang@bytedance.com>
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> > + * See the COPYING file in the top-level directory.
> > + */
> > +
> > +#include "qemu/osdep.h"
> > +#include "exec/ramblock.h"
> > +#include "qapi/error.h"
> > +#include "qemu/error-report.h"
> > +#include "qapi/qapi-types-migration.h"
> > +#include "options.h"
> > +#include "multifd.h"
> > +#include <qatzip.h>
> > +
> > +typedef struct {
> > + /*
> > + * Unique session for use with QATzip API
> > + */
> > + QzSession_T sess;
> > +
> > + /*
> > + * For compression: Buffer for pages to compress
> > + * For decompression: Buffer for data to decompress
> > + */
> > + uint8_t *in_buf;
> > + uint32_t in_len;
> > +
> > + /*
> > + * For compression: Output buffer of compressed data
> > + * For decompression: Output buffer of decompressed data
> > + */
> > + uint8_t *out_buf;
> > + uint32_t out_len;
> > +} QatzipData;
> > +
> > +/**
> > + * qatzip_send_setup: Set up QATzip session and private buffers.
> > + *
> > + * @param p Multifd channel params
> > + * @param errp Pointer to error, which will be set in case of error
> > + * @return 0 on success, -1 on error (and *errp will be set)
> > + */
> > +static int qatzip_send_setup(MultiFDSendParams *p, Error **errp)
> > +{
> > + QatzipData *q;
> > + QzSessionParamsDeflate_T params;
> > + const char *err_msg;
> > + int ret;
> > +
> > + q = g_new0(QatzipData, 1);
> > + p->compress_data = q;
> > + /* We need one extra place for the packet header */
> > + p->iov = g_new0(struct iovec, 2);
> > +
> > + /* Prefer without sw_fallback because of bad performance with sw_fallback.
> > + * Warn if sw_fallback needs to be used. */
>
> Please run scripts/checkpatch.pl on your series. This style of comments
> should have been flagged as non-conformant with our guidelines.
Sorry for that. Will fix in next version.
>
> > + ret = qzInit(&q->sess, false);
> > + if (ret != QZ_OK && ret != QZ_DUPLICATE) {
> > + /* Warn, and try with sw_fallback. */
> > + warn_report("Initilizing QAT with sw_fallback...");
>
> This will warn for each multifd channel, maybe use warn_report_once
> instead. Also s/Initilizing/Initializing/ and let's spell out "software
> fallback".
>
Will fix in next version.
> > + ret = qzInit(&q->sess, true);
> > + if (ret != QZ_OK && ret != QZ_DUPLICATE) {
> > + /* Warn, and try with sw_fallback. */
> > + err_msg = "qzInit failed";
> > + goto err_free_q;
> > + }
> > + }
> > +
> > + ret = qzGetDefaultsDeflate(¶ms);
> > + if (ret != QZ_OK) {
> > + err_msg = "qzGetDefaultsDeflate failed";
> > + goto err_close;
> > + }
> > +
> > + /* Make sure to use configured QATzip compression level. */
> > + params.common_params.comp_lvl = migrate_multifd_qatzip_level();
> > +
> > + ret = qzSetupSessionDeflate(&q->sess, ¶ms);
> > + if (ret != QZ_OK && ret != QZ_DUPLICATE) {
> > + err_msg = "qzSetupSessionDeflate failed";
> > + goto err_close;
> > + }
> > +
> > + if (MULTIFD_PACKET_SIZE > UINT32_MAX) {
> > + err_msg = "packet size too large for QAT";
> > + goto err_close;
> > + }
> > +
> > + q->in_len = MULTIFD_PACKET_SIZE;
> > + q->in_buf = qzMalloc(q->in_len, 0, PINNED_MEM);
> > + if (!q->in_buf) {
> > + err_msg = "qzMalloc failed";
> > + goto err_close;
> > + }
> > +
> > + q->out_len = qzMaxCompressedLength(MULTIFD_PACKET_SIZE, &q->sess);
> > + q->out_buf = qzMalloc(q->out_len, 0, PINNED_MEM);
> > + if (!q->out_buf) {
> > + err_msg = "qzMalloc failed";
> > + goto err_free_inbuf;
> > + }
> > +
> > + return 0;
> > +
> > +err_free_inbuf:
> > + qzFree(q->in_buf);
> > +err_close:
> > + qzClose(&q->sess);
> > +err_free_q:
> > + g_free(q);
> > + g_free(p->iov);
> > + p->iov = NULL;
> > + p->compress_data = NULL;
> > + error_setg(errp, "multifd %u: %s", p->id, err_msg);
> > + return -1;
> > +}
> > +
> > +/**
> > + * qatzip_send_cleanup: Tear down QATzip session and release private buffers.
> > + *
> > + * @param p Multifd channel params
> > + * @param errp Pointer to error, which will be set in case of error
> > + * @return None
> > + */
> > +static void qatzip_send_cleanup(MultiFDSendParams *p, Error **errp)
> > +{
> > + QatzipData *q = p->compress_data;
> > + const char *err_msg;
> > + int ret;
> > +
> > + ret = qzTeardownSession(&q->sess);
> > + if (ret != QZ_OK) {
> > + err_msg = "qzTeardownSession failed";
> > + goto err;
> > + }
> > +
> > + ret = qzClose(&q->sess);
> > + if (ret != QZ_OK) {
> > + err_msg = "qzClose failed";
> > + goto err;
> > + }
>
> Can qzClose() be called twice on the same session pointer? It's possible
> that we have already failed at multifd_send_setup() and still reach
> here.
>
> And what about qzTeardownSession()? Can it cope with an already closed
> session?
>
> And what about the sessions that never got created because we might have
> exited early at the ops->send_setup() loop?
>
qzTeardownSession() and qzClose() are safe to call on NULL pointers.
But thanks to your comments which corrected my understanding. These
patch was wrote under the impression that when setup() failed,
cleanup() won't be fired. After learning in gdb, apparently I was
wrong. The cleanup() will be called from another thread, which will be
called regardless if setup() returns zero or non-zero. I will rewrite
the setup()/cleanup() logics in my next patchset.
> > +
> > + qzFree(q->in_buf);
> > + q->in_buf = NULL;
> > + qzFree(q->out_buf);
> > + q->out_buf = NULL;
>
> These will double free here if send_setup has already freed.
>
> > + g_free(p->iov);
> > + p->iov = NULL;
> > + g_free(p->compress_data);
> > + p->compress_data = NULL;
> > + return;
> > +
> > +err:
> > + error_setg(errp, "multifd %u: %s", p->id, err_msg);
> > +}
> > +
> > +/**
> > + * qatzip_send_prepare: Compress pages and update IO channel info.
> > + *
> > + * @param p Multifd channel params
> > + * @param errp Pointer to error, which will be set in case of error
> > + * @return 0 on success, -1 on error (and *errp will be set)
> > + */
> > +static int qatzip_send_prepare(MultiFDSendParams *p, Error **errp)
> > +{
> > + MultiFDPages_t *pages = p->pages;
> > + QatzipData *q = p->compress_data;
> > + int ret;
> > + unsigned int in_len, out_len;
> > +
> > + if (!multifd_send_prepare_common(p)) {
> > + goto out;
> > + }
> > +
> > + /* Unlike other multifd compression implementations, we use a
> > + * non-streaming API and place all the data into one buffer, rather than
> > + * sending each page to the compression API at a time. */
> > + for (int i = 0; i < pages->normal_num; i++) {
> > + memcpy(q->in_buf + (i * p->page_size),
> > + p->pages->block->host + pages->offset[i],
>
> pages->block->host
>
I am not sure if I understand your comment here?
> > + p->page_size);
> > + }
> > +
> > + in_len = pages->normal_num * p->page_size;
> > + if (in_len > q->in_len) {
> > + error_setg(errp, "multifd %u: unexpectedly large input", p->id);
> > + return -1;
> > + }
> > + out_len = q->out_len;
> > +
> > + /*
> > + * Unlike other multifd compression implementations, we use a non-streaming
> > + * API and place all the data into one buffer, rather than sending each page
> > + * to the compression API at a time. Based on initial benchmarks, the
> > + * non-streaming API outperforms the streaming API. Plus, the logic in QEMU
> > + * is friendly to using the non-streaming API anyway. If either of these
> > + * statements becomes no longer true, we can revisit adding a streaming
> > + * implementation.
> > + */
> > + ret = qzCompress(&q->sess, q->in_buf, &in_len, q->out_buf, &out_len, 1);
> > + if (ret != QZ_OK) {
> > + error_setg(errp, "multifd %u: QATzip returned %d instead of QZ_OK",
> > + p->id, ret);
> > + return -1;
> > + }
> > + if (in_len != pages->normal_num * p->page_size) {
> > + error_setg(errp, "multifd %u: QATzip failed to compress all input",
> > + p->id);
> > + return -1;
> > + }
> > +
> > + p->iov[p->iovs_num].iov_base = q->out_buf;
> > + p->iov[p->iovs_num].iov_len = out_len;
> > + p->iovs_num++;
> > + p->next_packet_size = out_len;
> > +
> > +out:
> > + p->flags |= MULTIFD_FLAG_QATZIP;
> > + multifd_send_fill_packet(p);
> > + return 0;
> > +}
> > +
> > +/**
> > + * qatzip_recv_setup: Set up QATzip session and allocate private buffers.
> > + *
> > + * @param p Multifd channel params
> > + * @param errp Pointer to error, which will be set in case of error
> > + * @return 0 on success, -1 on error (and *errp will be set)
> > + */
> > +static int qatzip_recv_setup(MultiFDRecvParams *p, Error **errp)
> > +{
> > + QatzipData *q;
> > + QzSessionParamsDeflate_T params;
> > + const char *err_msg;
> > + int ret;
> > +
> > + q = g_new0(QatzipData, 1);
> > + p->compress_data = q;
> > +
> > + /* Prefer without sw_fallback because of bad performance with sw_fallback.
> > + * Warn if sw_fallback needs to be used. */
> > + ret = qzInit(&q->sess, false);
> > + if (ret != QZ_OK && ret != QZ_DUPLICATE) {
> > + /* Warn, and try with sw_fallback. */
> > + warn_report("Initilizing QAT with sw_fallback...");
>
> Same here. Also please add a hint that this is recv and the other one is
> send. It helps with debug.
Will fix in next version.
>
> > + ret = qzInit(&q->sess, true);
> > + if (ret != QZ_OK && ret != QZ_DUPLICATE) {
> > + /* Warn, and try with sw_fallback. */
> > + err_msg = "qzInit failed";
> > + goto err_free_q;
> > + }
> > + }
> > +
> > + ret = qzGetDefaultsDeflate(¶ms);
> > + if (ret != QZ_OK) {
> > + err_msg = "qzGetDefaultsDeflate failed";
> > + goto err_close;
> > + }
> > +
> > + ret = qzSetupSessionDeflate(&q->sess, ¶ms);
> > + if (ret != QZ_OK && ret != QZ_DUPLICATE) {
> > + err_msg = "qzSetupSessionDeflate failed";
> > + goto err_close;
> > + }
> > +
> > + /*
> > + * Mimic multifd-zlib, which reserves extra space for the
> > + * incoming packet.
>
> I'd put the actual rationale here. It will also help in the future to
> spot that this implementation doesn't send uncompressed pages in case
> the compression got too big.
Will fix in next version.
>
> > + */
> > + q->in_len = MULTIFD_PACKET_SIZE * 2;
> > + /* PINNED_MEM is an enum from qatzip headers, which means to use
> > + * kzalloc_node() to allocate memory for QAT DMA purposes. */
> > + q->in_buf = qzMalloc(q->in_len, 0, PINNED_MEM);
> > + if (!q->in_buf) {
> > + err_msg = "qzMalloc failed";
> > + goto err_close;
> > + }
> > +
> > + q->out_len = MULTIFD_PACKET_SIZE;
> > + q->out_buf = qzMalloc(q->out_len, 0, PINNED_MEM);
> > + if (!q->out_buf) {
> > + err_msg = "qzMalloc failed";
> > + goto err_free_inbuf;
> > + }
> > +
> > + return 0;
> > +
> > +err_free_inbuf:
> > + qzFree(q->in_buf);
> > +err_close:
> > + qzClose(&q->sess);
> > +err_free_q:
> > + g_free(q);
> > + p->compress_data = NULL;
> > + error_setg(errp, "multifd %u: %s", p->id, err_msg);
>
> Or maybe put the recv/send information on this string.
>
Will fix in next version.
> > + return -1;
> > +}
> > +
> > +/**
> > + * qatzip_recv_cleanup: Tear down QATzip session and release private buffers.
> > + *
> > + * @param p Multifd channel params
> > + * @return None
> > + */
> > +static void qatzip_recv_cleanup(MultiFDRecvParams *p)
> > +{
> > + QatzipData *q = p->compress_data;
> > +
> > + /* Ignoring return values here due to function signature. */
> > + qzTeardownSession(&q->sess);
> > + qzClose(&q->sess);
> > + qzFree(q->in_buf);
> > + qzFree(q->out_buf);
> > + g_free(p->compress_data);
> > +}
> > +
> > +
> > +/**
> > + * qatzip_recv: Decompress pages and copy them to the appropriate
> > + * locations.
> > + *
> > + * @param p Multifd channel params
> > + * @param errp Pointer to error, which will be set in case of error
> > + * @return 0 on success, -1 on error (and *errp will be set)
> > + */
> > +static int qatzip_recv(MultiFDRecvParams *p, Error **errp)
> > +{
> > + QatzipData *q = p->compress_data;
> > + int ret;
> > + unsigned int in_len, out_len;
> > + uint32_t in_size = p->next_packet_size;
> > + uint32_t expected_size = p->normal_num * p->page_size;
> > + uint32_t flags = p->flags & MULTIFD_FLAG_COMPRESSION_MASK;
> > +
> > + if (in_size > q->in_len) {
> > + error_setg(errp, "multifd %u: received unexpectedly large packet",
> > + p->id);
> > + return -1;
> > + }
> > +
> > + if (flags != MULTIFD_FLAG_QATZIP) {
> > + error_setg(errp, "multifd %u: flags received %x flags expected %x",
> > + p->id, flags, MULTIFD_FLAG_QATZIP);
> > + return -1;
> > + }
> > +
> > + multifd_recv_zero_page_process(p);
> > + if (!p->normal_num) {
> > + assert(in_size == 0);
> > + return 0;
> > + }
> > +
> > + ret = qio_channel_read_all(p->c, (void *)q->in_buf, in_size, errp);
> > + if (ret != 0) {
> > + return ret;
> > + }
> > +
> > + in_len = in_size;
> > + out_len = q->out_len;
> > + ret = qzDecompress(&q->sess, q->in_buf, &in_len, q->out_buf, &out_len);
> > + if (ret != QZ_OK) {
> > + error_setg(errp, "multifd %u: qzDecompress failed", p->id);
> > + return -1;
> > + }
> > + if (out_len != expected_size) {
> > + error_setg(errp, "multifd %u: packet size received %u size expected %u",
> > + p->id, out_len, expected_size);
> > + return -1;
> > + }
> > +
> > + /* Copy each page to its appropriate location. */
> > + for (int i = 0; i < p->normal_num; i++) {
> > + memcpy(p->host + p->normal[i],
> > + q->out_buf + p->page_size * i,
> > + p->page_size);
> > + }
> > + return 0;
> > +}
> > +
> > +static MultiFDMethods multifd_qatzip_ops = {
> > + .send_setup = qatzip_send_setup,
> > + .send_cleanup = qatzip_send_cleanup,
> > + .send_prepare = qatzip_send_prepare,
> > + .recv_setup = qatzip_recv_setup,
> > + .recv_cleanup = qatzip_recv_cleanup,
> > + .recv = qatzip_recv
> > +};
> > +
> > +static void multifd_qatzip_register(void)
> > +{
> > + multifd_register_ops(MULTIFD_COMPRESSION_QATZIP, &multifd_qatzip_ops);
> > +}
> > +
> > +migration_init(multifd_qatzip_register);
> > diff --git a/migration/multifd.h b/migration/multifd.h
> > index 0ecd6f47d7..adceb65050 100644
> > --- a/migration/multifd.h
> > +++ b/migration/multifd.h
> > @@ -34,14 +34,15 @@ MultiFDRecvData *multifd_get_recv_data(void);
> > /* Multifd Compression flags */
> > #define MULTIFD_FLAG_SYNC (1 << 0)
> >
> > -/* We reserve 4 bits for compression methods */
> > -#define MULTIFD_FLAG_COMPRESSION_MASK (0xf << 1)
> > +/* We reserve 5 bits for compression methods */
> > +#define MULTIFD_FLAG_COMPRESSION_MASK (0x1f << 1)
> > /* we need to be compatible. Before compression value was 0 */
> > #define MULTIFD_FLAG_NOCOMP (0 << 1)
> > #define MULTIFD_FLAG_ZLIB (1 << 1)
> > #define MULTIFD_FLAG_ZSTD (2 << 1)
> > #define MULTIFD_FLAG_QPL (4 << 1)
> > #define MULTIFD_FLAG_UADK (8 << 1)
> > +#define MULTIFD_FLAG_QATZIP (16 << 1)
> >
> > /* This value needs to be a multiple of qemu_target_page_size() */
> > #define MULTIFD_PACKET_SIZE (512 * 1024)
> > diff --git a/qapi/migration.json b/qapi/migration.json
> > index cd08f2f710..42b5363449 100644
> > --- a/qapi/migration.json
> > +++ b/qapi/migration.json
> > @@ -558,6 +558,8 @@
> > #
> > # @zstd: use zstd compression method.
> > #
> > +# @qatzip: use qatzip compression method. (Since 9.1)
> > +#
> > # @qpl: use qpl compression method. Query Processing Library(qpl) is
> > # based on the deflate compression algorithm and use the Intel
> > # In-Memory Analytics Accelerator(IAA) accelerated compression
> > @@ -570,6 +572,7 @@
> > { 'enum': 'MultiFDCompression',
> > 'data': [ 'none', 'zlib',
> > { 'name': 'zstd', 'if': 'CONFIG_ZSTD' },
> > + { 'name': 'qatzip', 'if': 'CONFIG_QATZIP'},
> > { 'name': 'qpl', 'if': 'CONFIG_QPL' },
> > { 'name': 'uadk', 'if': 'CONFIG_UADK' } ] }
> >
> > diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
> > index 6508bfb1a2..3068d73e08 100644
> > --- a/tests/qtest/meson.build
> > +++ b/tests/qtest/meson.build
> > @@ -327,6 +327,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'),
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [External] Re: [PATCH v5 4/5] migration: Introduce 'qatzip' compression method
2024-07-14 5:53 ` [External] " Yichen Wang
@ 2024-07-15 16:32 ` Fabiano Rosas
0 siblings, 0 replies; 14+ messages in thread
From: Fabiano Rosas @ 2024-07-15 16:32 UTC (permalink / raw)
To: Yichen Wang
Cc: Peter Xu, Paolo Bonzini, Daniel P. Berrangé, Eduardo Habkost,
Marc-André Lureau, Thomas Huth, Philippe Mathieu-Daudé,
Eric Blake, Markus Armbruster, Laurent Vivier, qemu-devel,
Hao Xiang, Liu, Yuan1, Zou, Nanhai, Ho-Ren (Jack) Chuang,
Bryan Zhang
Yichen Wang <yichen.wang@bytedance.com> writes:
> On Fri, Jul 12, 2024 at 7:17 AM Fabiano Rosas <farosas@suse.de> wrote:
>>
>> Yichen Wang <yichen.wang@bytedance.com> writes:
>>
>> > From: Bryan Zhang <bryan.zhang@bytedance.com>
>> >
>> > Adds support for 'qatzip' as an option for the multifd compression
>> > method parameter, and implements using QAT for 'qatzip' compression and
>> > decompression.
>> >
>> > Signed-off-by: Bryan Zhang <bryan.zhang@bytedance.com>
>> > Signed-off-by: Hao Xiang <hao.xiang@linux.dev>
>> > Signed-off-by: Yichen Wang <yichen.wang@bytedance.com>
>> > ---
>> > hw/core/qdev-properties-system.c | 6 +-
>> > migration/meson.build | 1 +
>> > migration/multifd-qatzip.c | 403 +++++++++++++++++++++++++++++++
>> > migration/multifd.h | 5 +-
>> > qapi/migration.json | 3 +
>> > tests/qtest/meson.build | 4 +
>> > 6 files changed, 419 insertions(+), 3 deletions(-)
>> > create mode 100644 migration/multifd-qatzip.c
>> >
>> > diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
>> > index f13350b4fb..eb50d6ec5b 100644
>> > --- a/hw/core/qdev-properties-system.c
>> > +++ b/hw/core/qdev-properties-system.c
>> > @@ -659,7 +659,11 @@ const PropertyInfo qdev_prop_fdc_drive_type = {
>> > const PropertyInfo qdev_prop_multifd_compression = {
>> > .name = "MultiFDCompression",
>> > .description = "multifd_compression values, "
>> > - "none/zlib/zstd/qpl/uadk",
>> > + "none/zlib/zstd/qpl/uadk"
>> > +#ifdef CONFIG_QATZIP
>> > + "/qatzip"
>> > +#endif
>>
>> It seems the other accelerators don't need the ifdef. What's different
>> here?
>
> Just changed and align to other methods. Will fix in next version.
>
>>
>> > + ,
>> > .enum_table = &MultiFDCompression_lookup,
>> > .get = qdev_propinfo_get_enum,
>> > .set = qdev_propinfo_set_enum,
>> > diff --git a/migration/meson.build b/migration/meson.build
>> > index 5ce2acb41e..c9454c26ae 100644
>> > --- a/migration/meson.build
>> > +++ b/migration/meson.build
>> > @@ -41,6 +41,7 @@ system_ss.add(when: rdma, if_true: files('rdma.c'))
>> > system_ss.add(when: zstd, if_true: files('multifd-zstd.c'))
>> > system_ss.add(when: qpl, if_true: files('multifd-qpl.c'))
>> > system_ss.add(when: uadk, if_true: files('multifd-uadk.c'))
>> > +system_ss.add(when: qatzip, if_true: files('multifd-qatzip.c'))
>> >
>> > specific_ss.add(when: 'CONFIG_SYSTEM_ONLY',
>> > if_true: files('ram.c',
>> > diff --git a/migration/multifd-qatzip.c b/migration/multifd-qatzip.c
>> > new file mode 100644
>> > index 0000000000..d01d51de8f
>> > --- /dev/null
>> > +++ b/migration/multifd-qatzip.c
>> > @@ -0,0 +1,403 @@
>> > +/*
>> > + * Multifd QATzip compression implementation
>> > + *
>> > + * Copyright (c) Bytedance
>> > + *
>> > + * Authors:
>> > + * Bryan Zhang <bryan.zhang@bytedance.com>
>> > + * Hao Xiang <hao.xiang@bytedance.com>
>> > + * Yichen Wang <yichen.wang@bytedance.com>
>> > + *
>> > + * This work is licensed under the terms of the GNU GPL, version 2 or later.
>> > + * See the COPYING file in the top-level directory.
>> > + */
>> > +
>> > +#include "qemu/osdep.h"
>> > +#include "exec/ramblock.h"
>> > +#include "qapi/error.h"
>> > +#include "qemu/error-report.h"
>> > +#include "qapi/qapi-types-migration.h"
>> > +#include "options.h"
>> > +#include "multifd.h"
>> > +#include <qatzip.h>
>> > +
>> > +typedef struct {
>> > + /*
>> > + * Unique session for use with QATzip API
>> > + */
>> > + QzSession_T sess;
>> > +
>> > + /*
>> > + * For compression: Buffer for pages to compress
>> > + * For decompression: Buffer for data to decompress
>> > + */
>> > + uint8_t *in_buf;
>> > + uint32_t in_len;
>> > +
>> > + /*
>> > + * For compression: Output buffer of compressed data
>> > + * For decompression: Output buffer of decompressed data
>> > + */
>> > + uint8_t *out_buf;
>> > + uint32_t out_len;
>> > +} QatzipData;
>> > +
>> > +/**
>> > + * qatzip_send_setup: Set up QATzip session and private buffers.
>> > + *
>> > + * @param p Multifd channel params
>> > + * @param errp Pointer to error, which will be set in case of error
>> > + * @return 0 on success, -1 on error (and *errp will be set)
>> > + */
>> > +static int qatzip_send_setup(MultiFDSendParams *p, Error **errp)
>> > +{
>> > + QatzipData *q;
>> > + QzSessionParamsDeflate_T params;
>> > + const char *err_msg;
>> > + int ret;
>> > +
>> > + q = g_new0(QatzipData, 1);
>> > + p->compress_data = q;
>> > + /* We need one extra place for the packet header */
>> > + p->iov = g_new0(struct iovec, 2);
>> > +
>> > + /* Prefer without sw_fallback because of bad performance with sw_fallback.
>> > + * Warn if sw_fallback needs to be used. */
>>
>> Please run scripts/checkpatch.pl on your series. This style of comments
>> should have been flagged as non-conformant with our guidelines.
>
> Sorry for that. Will fix in next version.
>
>>
>> > + ret = qzInit(&q->sess, false);
>> > + if (ret != QZ_OK && ret != QZ_DUPLICATE) {
>> > + /* Warn, and try with sw_fallback. */
>> > + warn_report("Initilizing QAT with sw_fallback...");
>>
>> This will warn for each multifd channel, maybe use warn_report_once
>> instead. Also s/Initilizing/Initializing/ and let's spell out "software
>> fallback".
>>
>
> Will fix in next version.
>
>> > + ret = qzInit(&q->sess, true);
>> > + if (ret != QZ_OK && ret != QZ_DUPLICATE) {
>> > + /* Warn, and try with sw_fallback. */
>> > + err_msg = "qzInit failed";
>> > + goto err_free_q;
>> > + }
>> > + }
>> > +
>> > + ret = qzGetDefaultsDeflate(¶ms);
>> > + if (ret != QZ_OK) {
>> > + err_msg = "qzGetDefaultsDeflate failed";
>> > + goto err_close;
>> > + }
>> > +
>> > + /* Make sure to use configured QATzip compression level. */
>> > + params.common_params.comp_lvl = migrate_multifd_qatzip_level();
>> > +
>> > + ret = qzSetupSessionDeflate(&q->sess, ¶ms);
>> > + if (ret != QZ_OK && ret != QZ_DUPLICATE) {
>> > + err_msg = "qzSetupSessionDeflate failed";
>> > + goto err_close;
>> > + }
>> > +
>> > + if (MULTIFD_PACKET_SIZE > UINT32_MAX) {
>> > + err_msg = "packet size too large for QAT";
>> > + goto err_close;
>> > + }
>> > +
>> > + q->in_len = MULTIFD_PACKET_SIZE;
>> > + q->in_buf = qzMalloc(q->in_len, 0, PINNED_MEM);
>> > + if (!q->in_buf) {
>> > + err_msg = "qzMalloc failed";
>> > + goto err_close;
>> > + }
>> > +
>> > + q->out_len = qzMaxCompressedLength(MULTIFD_PACKET_SIZE, &q->sess);
>> > + q->out_buf = qzMalloc(q->out_len, 0, PINNED_MEM);
>> > + if (!q->out_buf) {
>> > + err_msg = "qzMalloc failed";
>> > + goto err_free_inbuf;
>> > + }
>> > +
>> > + return 0;
>> > +
>> > +err_free_inbuf:
>> > + qzFree(q->in_buf);
>> > +err_close:
>> > + qzClose(&q->sess);
>> > +err_free_q:
>> > + g_free(q);
>> > + g_free(p->iov);
>> > + p->iov = NULL;
>> > + p->compress_data = NULL;
>> > + error_setg(errp, "multifd %u: %s", p->id, err_msg);
>> > + return -1;
>> > +}
>> > +
>> > +/**
>> > + * qatzip_send_cleanup: Tear down QATzip session and release private buffers.
>> > + *
>> > + * @param p Multifd channel params
>> > + * @param errp Pointer to error, which will be set in case of error
>> > + * @return None
>> > + */
>> > +static void qatzip_send_cleanup(MultiFDSendParams *p, Error **errp)
>> > +{
>> > + QatzipData *q = p->compress_data;
>> > + const char *err_msg;
>> > + int ret;
>> > +
>> > + ret = qzTeardownSession(&q->sess);
>> > + if (ret != QZ_OK) {
>> > + err_msg = "qzTeardownSession failed";
>> > + goto err;
>> > + }
>> > +
>> > + ret = qzClose(&q->sess);
>> > + if (ret != QZ_OK) {
>> > + err_msg = "qzClose failed";
>> > + goto err;
>> > + }
>>
>> Can qzClose() be called twice on the same session pointer? It's possible
>> that we have already failed at multifd_send_setup() and still reach
>> here.
>>
>> And what about qzTeardownSession()? Can it cope with an already closed
>> session?
>>
>> And what about the sessions that never got created because we might have
>> exited early at the ops->send_setup() loop?
>>
>
> qzTeardownSession() and qzClose() are safe to call on NULL pointers.
> But thanks to your comments which corrected my understanding. These
> patch was wrote under the impression that when setup() failed,
> cleanup() won't be fired. After learning in gdb, apparently I was
> wrong. The cleanup() will be called from another thread, which will be
> called regardless if setup() returns zero or non-zero. I will rewrite
> the setup()/cleanup() logics in my next patchset.
>
>> > +
>> > + qzFree(q->in_buf);
>> > + q->in_buf = NULL;
>> > + qzFree(q->out_buf);
>> > + q->out_buf = NULL;
>>
>> These will double free here if send_setup has already freed.
>>
>> > + g_free(p->iov);
>> > + p->iov = NULL;
>> > + g_free(p->compress_data);
>> > + p->compress_data = NULL;
>> > + return;
>> > +
>> > +err:
>> > + error_setg(errp, "multifd %u: %s", p->id, err_msg);
>> > +}
>> > +
>> > +/**
>> > + * qatzip_send_prepare: Compress pages and update IO channel info.
>> > + *
>> > + * @param p Multifd channel params
>> > + * @param errp Pointer to error, which will be set in case of error
>> > + * @return 0 on success, -1 on error (and *errp will be set)
>> > + */
>> > +static int qatzip_send_prepare(MultiFDSendParams *p, Error **errp)
>> > +{
>> > + MultiFDPages_t *pages = p->pages;
>> > + QatzipData *q = p->compress_data;
>> > + int ret;
>> > + unsigned int in_len, out_len;
>> > +
>> > + if (!multifd_send_prepare_common(p)) {
>> > + goto out;
>> > + }
>> > +
>> > + /* Unlike other multifd compression implementations, we use a
>> > + * non-streaming API and place all the data into one buffer, rather than
>> > + * sending each page to the compression API at a time. */
>> > + for (int i = 0; i < pages->normal_num; i++) {
>> > + memcpy(q->in_buf + (i * p->page_size),
>> > + p->pages->block->host + pages->offset[i],
>>
>> pages->block->host
>>
>
> I am not sure if I understand your comment here?
You're holding the pointer to p->pages in the local pages variable. I'm
suggesting you use it instead of p->pages here. In another series[1], we're
looking into changing p->pages into something else and having to change
it all over the code gets bothersome.
1- https://lore.kernel.org/r/20240620212111.29319-2-farosas@suse.de
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2024-07-15 16:34 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-07-11 2:52 [PATCH v5 0/5] Implement QATzip compression method Yichen Wang
2024-07-11 2:52 ` [PATCH v5 1/5] docs/migration: add qatzip compression feature Yichen Wang
2024-07-11 2:52 ` [PATCH v5 2/5] meson: Introduce 'qatzip' feature to the build system Yichen Wang
2024-07-11 2:52 ` [PATCH v5 3/5] migration: Add migration parameters for QATzip Yichen Wang
2024-07-11 2:52 ` [PATCH v5 4/5] migration: Introduce 'qatzip' compression method Yichen Wang
2024-07-12 14:17 ` Fabiano Rosas
2024-07-14 5:53 ` [External] " Yichen Wang
2024-07-15 16:32 ` Fabiano Rosas
2024-07-11 2:52 ` [PATCH v5 5/5] tests/migration: Add integration test for " Yichen Wang
2024-07-11 14:23 ` Peter Xu
2024-07-11 16:45 ` [External] " Yichen Wang
2024-07-11 15:44 ` [PATCH v5 0/5] Implement QATzip " Peter Xu
2024-07-11 16:48 ` [External] " Yichen Wang
2024-07-11 19:36 ` Peter Xu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).