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

[Frediano Ziglio <freddy77@xxxxxxxxx>]

On Wed, 22 Jul 2026 at 22:30, Teddy Astie <teddy.astie@xxxxxxxxxx> wrote:
>
> 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)
>

Yes, apparently that field disappeared between thinking and writing the reply.
It's also useful for different purposes like preallocating.

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

A similar change was done (not by me) in another project I worked on previously.
I thought it was more complicated too but it turned out the change was
pretty clean, not requiring much.
More or less during the process of the package you decompress the
package and call the process again on the decompressed package.
Similar for the sending side.

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

About compressing all together and considering also the issue of
memory changing while sending I would vote to copy the memory in a
temporary buffer to avoid this. One advantage is that it simplified
the format. The current LZ4 implementation seems to cope with data
changes but nothing guarantees it in the future, the buffer you are
passing is not supposed to change while you compress it.

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