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

[Teddy Astie <teddy.astie@xxxxxxxxxx>]

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

> +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) ?
> +     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].
What do you think ?

[1] 
https://github.com/lz4/lz4/blob/dev/doc/lz4_Block_format.md#compression-techniques
...

> base-commit: a7fd7d4cbd5e793d31d61c25e08526b330edd7f8
Teddy

Attachment: OpenPGP_0x660FA9D102CBCFD0.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature