|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v1 1/6] tools/migration: introduce PAGE_DATA_LZ4 stream record type
Le 22/07/2026 à 21:46, Frediano Ziglio a écrit : On Tue, 21 Jul 2026 at 09:20, Teddy Astie <teddy.astie@xxxxxxxxxx> wrote:Le 20/07/2026 à 17:51, Marcus Granado a écrit :Allocate a new migration stream record type REC_TYPE_PAGE_DATA_LZ4 (0x00000013) to transmit PAGE_DATA when payload is LZ4-compressed. The record use the same xc_sr_rec_page_data_header as REC_TYPE_PAGE_DATA but the page data section type changes from page_data to data_lz4. Signed-off-by: Marcus Granado <marcus.granado@xxxxxxxxxx> --- docs/specs/libxc-migration-stream.pandoc | 71 ++++++++++++++++- tools/libs/guest/xg_sr_common.c | 4 + tools/libs/guest/xg_sr_stream_format.h | 13 ++++ tools/python/xen/migration/libxc.py | 90 ++++++++++++++++++++- tools/python/xen/migration/tests.py | 99 +++++++++++++++++++++++- 5 files changed, 271 insertions(+), 6 deletions(-) diff --git a/docs/specs/libxc-migration-stream.pandoc b/docs/specs/libxc-migration-stream.pandoc index 1319ce1f1e..7469c95139 100644 --- a/docs/specs/libxc-migration-stream.pandoc +++ b/docs/specs/libxc-migration-stream.pandoc...Don't we need to bump the version number while we add a new mandatory record? Is there no kind of dialog about the supported version?+PAGE_DATA_LZ4 +------------- + +A PAGE_DATA_LZ4 record carries exactly the same information as a +PAGE_DATA record, but with the page contents LZ4-compressed. The saver +may emit it in place of a PAGE_DATA record when LZ4 compression has been +requested. +Would it be preferable to make this structure more generic, i.e PAGE_DATA_COMPRESSED, as I'm not sure it's wise to restrict to only LZ4 (someone may want to add support for e.g zstd or another algorithm without having to change the format) ?Why not extending the generalization compressing all payload of uncompressed packets (type+body), so to have something like - LZ4 compressed type (uint32) - compressed length (uint32) - compressed data (array of bytes, length field above) Uncompressed data will contain - type (uint32, like PAGE_DATA) - body (array of bytes, not padded) - padding to 8 bytes with zeroes So we could compress any packet, not only PAGE_DATA. Yes, to support more compression types an additional "compression_type" or similar field could help. But what about additional possible options? (you should probably also add uncompressed size, so that decompressors can guard against decompression bomb blocks) In principle, it's preferable as it decouple the compression aspect to the data itself. But it needs a quite significant refactoring to add a middle layer that "transparently" compress/decompress data to the lower layer that perform live migration logic and parse/generate the page_data object. + 0 1 2 3 4 5 6 7 octet + +-----------------------+-------------------------+ + | count (C) | (reserved) | + +-----------------------+-------------------------+ + | pfn[0] | + +-------------------------------------------------+ + ... + +-------------------------------------------------+ + | pfn[C-1] | + +-----------+-------------------------------------+ + | clen[0] | page_data_lz4[0]... | + +-----------+-------------------------------------+ + ... + +-----------+-------------------------------------+ + | clen[N-1] | page_data_lz4[N-1]... | + ... + +-------------------------------------------------+ + +-------------------------------------------------------------------- +Field Description +----------- -------------------------------------------------------- +count Number of pages described in this record. + +pfn An array of count PFNs and their types, with the same + layout and page types as in a PAGE_DATA record. + +data_lz4 The compressed page contents, as one sub-block per page + set as present in the pfn array, in pfn-array order (i.e. + N sub-blocks, with N as in a PAGE_DATA record: N <= C). + Each sub-block is a `uint16` little-endian length `clen` + followed by either an LZ4 block or a raw page. When + `clen > 0`, page_data_lz4 is `clen` octets of a raw LZ4 + block (per the LZ4 block format) whose decompressed output + is one page_size page. When `clen == 0`, page_data_lz4 is + page_size octets of a raw, uncompressed page. + A compressed `clen` is at most page_size - 1, occupying + only the low 12 bits; the top 4 bits are reserved for + future use and must be 0. +-------------------------------------------------------------------- + +The `count` (C) and `pfn` fields are identical in meaning and +constraints to those of a PAGE_DATA record, and N (the number of +present pages, N <= C) is as defined there. Unlike PAGE_DATA, a +PAGE_DATA_LZ4 record always has N >= 1 (at least one sub-block): +the saver emits the LZ4 variant only when there is page data to +compress, and a restoring side rejects a PAGE_DATA_LZ4 record with +N == 0. When a batch has no present pages (all pfns of invalid +types) the saver emits a plain PAGE_DATA record instead. + +PAGE_DATA_LZ4 is a _mandatory_ record: a restoring side that does not +support it must fail the migration. +The inline clen at the beginning of page_data_lz4 feels a bit odd to me, or at least, a bit inconsistent with how pfns are arranged. I think something like i.e 1. count and (reserved) 2. pfn[0] ... pfn[C-1] 3. clen[0] ... clen[N-1] 4. page_data_lz4[0] ... page_data_lz4[N-1] would be better. Though, aside that, I'm not sure having separate compressed block for each page is a good idea. Compression is more efficient when processing larger blocks, LZ4 documents a 64 KB deduplication window (of the past bytes) [1]. In my opinion, it may be preferable to have one large block with all the page datas (covering up to MAX_BATCH_SIZE=1024 pages) rather than having to uncompress individual blocks. Which in the end would remove the need to have individual clen[n].I was thinking the same. Talking with the author today one reason is that bytes could change while compressing them (because they are shared with the guest which is running) but on following updates (dirty pages map is used) they get updated and fixed (otherwise VM will crash). Oh, well, that's (unfortunately) actually worse; while running compression logic to live pages can cause data corruption of the end result, this also causes undefined behavior through the compiler (data race as compiler doesn't expect the pointer content to be modified from elsewhere concurrently). We probably need to copy the memory here to stay safe. Something that could be addressed with xg_foreignmemory_copy_* proposal. That could be considered, but it would make things a bit more complicated to define. I found that along LZ4, there is also LZ4F [1] which can be useful to make it streamable or chunkable in a quite standardized way. Otherwise, we would have to add some arbitrary framing each 32 KB blocks, so it will no longer be simply a LZ4 block.However we could copy the memory to avoid this. Considering the algorithm (in this case) has a window of 64 K we could compress and copy in 64 K chunks to reduce memory usage and increase cache (as processor one) usage (if that's an issue). But that may be too much for just chunking a 4 MB (maximum) block so I'm not really certain of what's best actually. [1] https://github.com/lz4/lz4/blob/dev/doc/lz4_Frame_format.md What do you think ? [1] https://github.com/lz4/lz4/blob/dev/doc/lz4_Block_format.md#compression-techniques ...base-commit: a7fd7d4cbd5e793d31d61c25e08526b330edd7f8TeddyFrediano Teddy Attachment:
OpenPGP_0x660FA9D102CBCFD0.asc Attachment:
OpenPGP_signature.asc
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |