Re: [PATCH v1 1/6] tools/migration: introduce PAGE_DATA_LZ4 stream record type

Teddy Astie <[email protected]> Wed, 22 Jul 2026 23:30:33 +0200
Newsgroups	org.xenproject.lists.xen-devel
Message-ID	<1784755834.8631fc262581453bbf619ec5b2062170.19f8bbccffd000edb5@vates.tech>
Le 22/07/2026 à 21:46, Frediano Ziglio a écrit :
> On Tue, 21 Jul 2026 at 09:20, Teddy Astie <[email protected]> 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 <[email protected]>
>>> ---
>>>    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.

> 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).
> 
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.

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: a7fd7d4cbd5e793d31d61c25e08526b330edd7f8
>>
>> Teddy
> 
> Frediano
> 

Teddy
OpenPGP_0x660FA9D102CBCFD0.asc (application/pgp-keys, 2.4 KB)
OpenPGP_signature.asc (application/pgp-signature, 665 B)