Peter Xu <[email protected]> writes:
> On Wed, Jan 28, 2026 at 04:35:32PM -0300, Fabiano Rosas wrote:
>> Peter Xu <[email protected]> writes:
>>
>> > Split it into two smaller chunks:
>> >
>> > - Dump of early_setup VMSDs
>> > - Dump of save_setup() sections
>> >
>> > They're mutual exclusive, hence we can run two loops and do them
>> > sequentially. This will cause migration thread to loop one more time, but
>> > it should be fine when migration just started and only do it once. It's
>> > needed because we will need to reuse the early_vmsd helper later to
>> > deduplicate code elsewhere.
>> >
>> > QEMU almost sticks with qemu_savevm_state_XXX() to represent the dump of
>> > vmstates's section XXX.
>> > With that in mind, this patch renamed the original
>> > qemu_savevm_state_setup() to qemu_savevm_state_do_setup() instead.
>> >
>> > So after this patch:
>> >
>> > - qemu_savevm_state_non_iterable_early() dumps early_vmsds only,
>> > - qemu_savevm_state_setup() dumps save_setup() sections only,
>> > - qemu_savevm_state_do_setup() does all things needed during setup
>> > phase (including migration SETUP notifies)
>> >
>> > Signed-off-by: Peter Xu <[email protected]>
>>
>> Reviewed-by: Fabiano Rosas <[email protected]>
>>
>> Not for this series, but I think we could try to standardize the naming
>> in savevm.c a bit. Here's a list of functions from savevm.c plus the
>> main function called by them. You'll see that there's room from
>> improvement.
>>
>> Removing this "qemu" prefix and choosing when to put "state" in the name
>> would already be a win.
>
> True.
>
> IMHO if we choose it again we can use prefix vmstate_. The current
> savevm_state_ is pretty vague on its own when loadvm needs to use it. Then
> it can be vmstate_save_* on save side, and vmstate_load_* on load side (and
> vmstate_XXX) when valid on both.
>
It's slightly annoying that vmstate is also used in the macros and
virtually every single VMStateDescription all over QEMU. I wish we had a
separate name for the implementation. But vmstate_ is definitely better
than qemu_savevm_state_.
Or perhaps something like migstate, migvm, etc.
I think we could see great benefit from a small-ish change doing:
s/qemu_savevm_state_/new_name_/ and s/SaveStateEntry/NewNameEntry/
Because then it becomes clear there's a high level thing that deals with
SaveVMHandlers and VMStateDescription which are two different ways of
migrating state.
>>
>> Also maybe making more clear what calls vmstate code and what deals with
>> se->ops only.
>
> Yes we can try, but there'll be some tricky ones, inline below (assuming we
> can shorten the prefix to "vmstate_").
>
Yeah, I see.
>>
>> ---
>> qemu_loadvm_state_setup { se->ops->load_setup }
>
> Taking this one as example.
>
> We could rename this to vmstate_ops_load_setup(), however "load_setup" is
> actually a concept that is higher than the impl. IOW it'll be also
> reasonable if we want to add a load_setup() hook to VMSDs some day. Maybe
> still good to rename it to vmstate_load_setup().
>
>> qemu_savevm_state_active { se->ops->is_active }
>
> Similarly, here I'd think vmstate_active() (it applies to both save/load)
> better than vmstate_ops_active() because active describes the vmstate
> instance, and it's fine one day we want to introduce VMSD.active.
>
> ...
>
>> qemu_savevm_state_blocked { se->vmsd->unmigratable }
>> qemu_savevm_state_cleanup { se->ops->save_cleanup }
>> qemu_savevm_state_guest_unplug_pending { se->vmsd->dev_unplug_pending }
>> qemu_savevm_state_iterate { se->ops->save_live_iterate }
>> qemu_savevm_state_non_iterable_early { se->vmsd->early_setup;
>> vmstate_save }
>> qemu_savevm_state_pending_estimate { se->ops->state_pending_estimate }
>> qemu_savevm_state_pending_exact { se->ops->state_pending_exact }
>> qemu_savevm_state_postcopy_prepare { se->ops->save_postcopy_prepare }
>> qemu_savevm_state_prepare { se->ops->save_prepare }
>> qemu_savevm_state_resume_prepare { se->ops->resume_prepare }
>> loadvm_postcopy_handle_switchover_start { se->ops->switchover_start }
>> qemu_loadvm_load_state_buffer { se->ops->load_state_buffer }
>> qemu_loadvm_state_switchover_ack_needed { se->ops->switchover_ack_needed }
>> qemu_savevm_complete { se->ops->save_complete }
>> qemu_savevm_complete_exists { se->ops->save_complete }
>> qemu_savevm_non_migratable_list { se->vmsd->unmigratable }
>> qemu_savevm_se_iterable { se->ops->save_setup }
>> save_state_priority { se->vmsd->priority }
>> vmstate_load { se->ops->load_state }
>
> This one (and vmstate_save()) is special because it handles both impls
> (VMSD and OPS). The current names are actually suitable.
>
>> vmstate_save_old_style { se->ops->save_state }
>>
>> loadvm_handle_cmd_packaged { qemu_loadvm_state_main }
>> qemu_load_device_state { qemu_loadvm_state_main }
>> qemu_savevm_maybe_send_switchover_start {
>> qemu_savevm_send_switchover_start }
>> qemu_savevm_send_* { qemu_savevm_command_send }
>> qemu_savevm_state_complete_postcopy { qemu_savevm_complete }
>> qemu_savevm_state_complete_precopy_iterable { qemu_savevm_complete }
>> qemu_savevm_state_end_precopy { qemu_savevm_state_end }
>> qemu_savevm_state_header { qemu_savevm_send_header }
>
> ...
>
>>
>> qemu_savevm_state_do_setup {
>> qemu_savevm_state_non_iterable_early;
>> qemu_savevm_state_setup }
>>
>> qemu_loadvm_state { qemu_loadvm_state_header;
>> qemu_loadvm_state_setup;
>>
>> qemu_loadvm_state_switchover_ack_needed;
>> qemu_loadvm_state_main;
>> qemu_loadvm_thread_pool_wait }
>>
>> qemu_loadvm_state_main { qemu_loadvm_section_start_full;
>> qemu_loadvm_section_part_end;
>> loadvm_process_command }
>>
>> qemu_savevm_state { qemu_savevm_state_header;
>> qemu_savevm_state_do_setup;
>> qemu_savevm_state_iterate;
>> qemu_savevm_state_complete_precopy;
>> qemu_savevm_state_cleanup }
>>
>> qemu_savevm_state_complete_precopy {
>> qemu_savevm_state_complete_precopy_iterable;
>> qemu_savevm_state_non_iterable;
>> qemu_savevm_state_end_precopy }
>>
>> qemu_save_device_state {
>> qemu_savevm_state_non_iterable_early;
>> qemu_savevm_state_non_iterable;
>> qemu_savevm_state_end }
>
> Most of above are special functions higher than vmstate alone, hence IMHO
> not vmstate_*() APIs. We can name it whatever way we like, or leave them
> be.
>
They definitely need a better name, it's only a question of "when".
>>
>> loadvm_handle_recv_bitmap { migrate_send_rp_recv_bitmap }
>> loadvm_postcopy_handle_advise { ram_postcopy_incoming_init }
>> loadvm_postcopy_handle_listen { postcopy_incoming_setup }
>> loadvm_postcopy_handle_resume { mis->postcopy_pause_sem_fault }
>> loadvm_postcopy_handle_run { loadvm_postcopy_handle_run_bh }
>> loadvm_postcopy_handle_run_bh { vm_start }
>> loadvm_postcopy_ram_handle_discard { postcopy_ram_prepare_discard }
>> loadvm_process_command { switch (cmd) }
>> loadvm_process_enable_colo { colo_init_ram_cache }
>
> These are almost not vmstate relevant but about MIG_CMD*. We can likely
> leave them alone. Currently the loadvm_ prefix actually sounds not too
> bad.
>
I agree.
> ...
>
> [will skip most of below; we could start with vmstate API first]
>
>> qemu_loadvm_approve_switchover { migrate_send_rp_switchover_ack }
>> qemu_loadvm_load_thread { data->function }
>> qemu_loadvm_section_part_end { vmstate_load }
>> qemu_loadvm_section_start_full { vmstate_load }
>> qemu_loadvm_start_load_thread { thread_pool_submit }
>> qemu_loadvm_state_cleanup { se->ops->load_cleanup }
>> qemu_loadvm_state_header { QEMU_VM_FILE_MAGIC }
>> qemu_loadvm_thread_pool_create { thread_pool_new }
>> qemu_loadvm_thread_pool_destroy { thread_pool_free }
>> qemu_loadvm_thread_pool_wait { thread_pool_wait }
>> qemu_savevm_command_send { QEMU_VM_COMMAND }
>> qemu_savevm_send_colo_enable { MIG_CMD_ENABLE_COLO }
>> qemu_savevm_send_configuration {
>> vmstate_save_state(&vmstate_configuration) }
>> qemu_savevm_send_header { QEMU_VM_FILE_MAGIC }
>> qemu_savevm_state_end { QEMU_VM_EOF }
>> qemu_savevm_state_non_iterable { vmstate_save }
>> qemu_savevm_state_setup { se->ops->save_setup }
>> qemu_savevm_state_vm_desc { QEMU_VM_VMDESCRIPTION }
>> register_savevm_live { savevm_state_handler_insert }
>> save_section_footer { QEMU_VM_SECTION_FOOTER }
>> save_section_header { section_type }
>> savevm_state_handler_insert { QTAILQ_INSERT_TAIL }
>> savevm_state_handler_remove { QTAILQ_REMOVE }
>> unregister_savevm { savevm_state_handler_remove }
>> vmstate_register_with_alias_id { savevm_state_handler_insert }
>> vmstate_save { vmstate_save_old_style;
>> vmstate_save_state }
>> vmstate_unregister { savevm_state_handler_remove }
>>
