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

[Frediano Ziglio <freddy77@xxxxxxxxx>]

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?

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

> What do you think ?
>
> [1]
> https://github.com/lz4/lz4/blob/dev/doc/lz4_Block_format.md#compression-techniques
> ...
>
> >
> > base-commit: a7fd7d4cbd5e793d31d61c25e08526b330edd7f8
>
> Teddy

Frediano