Re: [PATCH V1 19/26] physmem: preserve ram blocks for cpr

[Steven Sistare]

On 5/29/2024 3:25 PM, Peter Xu wrote:
> On Wed, May 29, 2024 at 01:31:53PM -0400, Steven Sistare wrote:
> > On 5/28/2024 5:44 PM, Peter Xu wrote:
> > > On Mon, Apr 29, 2024 at 08:55:28AM -0700, Steve Sistare wrote:
> > > > Preserve fields of RAMBlocks that allocate their host memory during CPR so
> > > > the RAM allocation can be recovered.
> > >
> > > This sentence itself did not explain much, IMHO.  QEMU can share memory
> > > using fd based memory already of all kinds, as long as the memory backend
> > > is path-based it can be shared by sharing the same paths to dst.
> > >
> > > This reads very confusing as a generic concept.  I mean, QEMU migration
> > > relies on so many things to work right.  We mostly asks the users to "use
> > > exactly the same cmdline for src/dst QEMU unless you know what you're
> > > doing", otherwise many things can break.  That should also include ramblock
> > > being matched between src/dst due to the same cmdlines provided on both
> > > sides.  It'll be confusing to mention this when we thought the ramblocks
> > > also rely on that fact.
> > >
> > > So IIUC this sentence should be dropped in the real patch, and I'll try to
> > > guess the real reason with below..
> >
> > The properties of the implicitly created ramblocks must be preserved.
> > The defaults can and do change between qemu releases, even when the command-line
> > parameters do not change for the explicit objects that cause these implicit
> > ramblocks to be created.
>
> AFAIU, QEMU relies on ramblocks to be the same before this series.  Do you
> have an example?  Would that already cause issue when migrate?

Alignment has changed, and used_length vs max_length changed when
resizeable ramblocks were introduced.  I have dealt with these issues
while supporting cpr for our internal use, and the learned lesson is to
explicitly communicate the creation-time parameters to new qemu.

These are not an issue for migration because the ramblock is re-created
and the data copied into the new memory.

> > > > Mirror the mr->align field in the RAMBlock to simplify the vmstate.
> > > > Preserve the old host address, even though it is immediately discarded,
> > > > as it will be needed in the future for CPR with iommufd.  Preserve
> > > > guest_memfd, even though CPR does not yet support it, to maintain vmstate
> > > > compatibility when it becomes supported.
> > >
> > > .. It could be about the vfio vaddr update feature that you mentioned and
> > > only for iommufd (as IIUC vfio still relies on iova ranges, then it won't
> > > help here)?
> > >
> > > If so, IMHO we should have this patch (or any variance form) to be there
> > > for your upcoming vfio support.  Keeping this around like this will make
> > > the series harder to review.  Or is it needed even before VFIO?
> >
> > This patch is needed independently of vfio or iommufd.
> >
> > guest_memfd is independent of vfio or iommufd.  It is a recent addition
> > which I have not tried to support, but I added this placeholder field
> > to it can be supported in the future without adding a new field later
> > and maintaining backwards compatibility.
>
> Is guest_memfd the only user so far, then?  If so, would it be possible we
> split it as a separate effort on top of the base cpr-exec support?

I don't understand the question.  I am indeed deferring support for guest_memfd
to a future time.  For now, I am adding a blocker, and reserving a field for
it in the preserved ramblock attributes, to avoid adding a subsection later.

- Steve