Re: [PATCH v3 1/6] migration: Allow immutable device state to be migrated early (i.e., before RAM)

[Peter Xu]

On Mon, Jan 09, 2023 at 03:34:48PM +0100, David Hildenbrand wrote:
> On 05.01.23 18:15, Peter Xu wrote:
> > On Thu, Jan 05, 2023 at 09:35:54AM +0100, David Hildenbrand wrote:
> > > On 04.01.23 18:23, Peter Xu wrote:
> > > > On Thu, Dec 22, 2022 at 12:02:10PM +0100, David Hildenbrand wrote:
> > > > > Migrating device state before we start iterating is currently 
> > > > > impossible.
> > > > > Introduce and use qemu_savevm_state_start_precopy(), and use
> > > > > a new special migration priority -- MIG_PRI_POST_SETUP -- to decide 
> > > > > whether
> > > > > state will be saved in qemu_savevm_state_start_precopy() or in
> > > > > qemu_savevm_state_complete_precopy_*().
> > > > 
> > > > Can something like this be done in qemu_savevm_state_setup()?
> > > 
> > > Hi Peter,
> > 
> > Hi, David,
> > 
> > > 
> > > Do you mean
> > > 
> > > (a) Moving qemu_savevm_state_start_precopy() effectively into
> > >      qemu_savevm_state_setup()
> > > 
> > > (b) Using se->ops->save_setup()
> > 
> > I meant (b).
> > 
> > > 
> > > I first tried going via (b), but decided to go the current way of using a
> > > proper vmstate with properties (instead of e.g., filling the stream
> > > manually), which also made vmdesc handling possible (and significantly
> > > cleaner).
> > > 
> > > Regarding (a), I decided to not move logic of
> > > qemu_savevm_state_start_precopy() into qemu_savevm_state_setup(), because 
> > > it
> > > looked cleaner to save device state with the BQL held and for background
> > > snapshots, the VM has been stopped. To decouple device state saving from 
> > > the
> > > setup path, just like we do it right now for all vmstates.
> > 
> > Is BQL required or optional?  IIUC it's at least still not taken in the
> > migration thread path, only in savevm path.
> > 
> > > 
> > > Having that said, for virtio-mem, it would still work because that state 
> > > is
> > > immutable once migration starts, but it felt cleaner to separate the 
> > > setup()
> > > phase from actual device state saving.
> > 
> > I get the point.  My major concerns are:
> > 
> >    (1) The new migration priority is changing the semantic of original,
> >        making it over-complicated
> > 
> >    (2) The new precopy-start routine added one more step to the migration
> >        framework, while it's somehow overlapping (if not to say, mostly the
> >        same as..) save_setup().
> > 
> > For (1): the old priority was only deciding the order of save entries in
> > the global list, nothing more than that.  Even if we want to have a
> > precopy-start phase, I'd suggest we use something else and keep the
> > migration priority simple.  Otherwise we really need serious documentation
> > for MigrationPriority and if so I'd rather don't bother and not reuse the
> > priority field.
> > 
> > For (2), if you see there're a bunch of save_setup() that already does
> > things like transferring static data besides the device states.  Besides
> > the notorious ram_save_setup() there's also dirty_bitmap_save_setup() which
> > also sends a bitmap during save_setup() and some others.  It looks clean to
> > me to do it in the same way as we used to.
> > 
> > Reusing vmstate_save() and vmsd structures are useful too which I totally
> > agree.  So.. can we just call vmstate_save_state() in the save_setup() of
> > the other new vmsd of virtio-mem?
> 
> 
> I went halfway that way, by moving stuff into qemu_savevm_state_setup()
> and avoiding using a new migration priority. Seems to work:

The whole point of my suggestion is not moving things into
qemu_savevm_state_setup(), but avoid introducing more complexity to the
migration framework if unnecessary, so keep the generic framework as simple
as possible.

> 
> I think we could go one step further and perform it from a save_setup() 
> callback,
> however, I'm not convinced that this gets particularly cleaner (vmdesc 
> handling
> eventually).

What I wanted to suggest is exactly trying to avoid vmsd handling.  To be
more explicit, I mean: besides vmstate_virtio_mem_device_early, virtio-mem
can register with another new SaveVMHandlers with both save_setup and
load_setup registered, then e.g. in its save_setup(), one simply calls:

  vmstate_save_state(f, &vmstate_virtio_mem_device_early, virtio_mem_dev,
                     NULL);

I'm not sure whether the JSONWriter* is required in this case, maybe not
yet to at least make it work.

We'll need to impl the load part, but then IIUC we don't need to touch the
migration framework at all, and we keep all similar things (like other
devices I mentioned) to be inside save_setup().

Would that work?

-- 
Peter Xu