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

[Peter Xu <peterx@redhat.com>]

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?

Thanks,

-- 
Peter Xu