Re: [PATCH v2 03/21] migration: Add documentation for SaveVMHandlers

["Cédric Le Goater" <clg@redhat.com>]

On 3/4/24 10:05, Avihai Horon wrote:
> Hi Cedric,
> 
> A few nits below.

Just in time for v3 I was about to send ! I will include these
suggestions.

Thanks,

C.




> 
> On 27/02/2024 20:03, Cédric Le Goater wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> The SaveVMHandlers structure is still in use for complex subsystems
>> and devices. Document the handlers since we are going to modify a few
>> later.
>>
>> Signed-off-by: Cédric Le Goater <clg@redhat.com>
>> ---
>>   include/migration/register.h | 257 +++++++++++++++++++++++++++++++----
>>   1 file changed, 231 insertions(+), 26 deletions(-)
>>
>> diff --git a/include/migration/register.h b/include/migration/register.h
>> index 2e6a7d766e62f64940086b7b511249c9ff21fa62..2cc71ec45f65bf2884c9e7a823d2968752f15c20 100644
>> --- a/include/migration/register.h
>> +++ b/include/migration/register.h
>> @@ -16,30 +16,129 @@
>>
>>   #include "hw/vmstate-if.h"
>>
>> +/**
>> + * struct SaveVMHandlers: handler structure to finely control
>> + * migration of complex subsystems and devices, such as RAM, block and
>> + * VFIO.
>> + */
>>   typedef struct SaveVMHandlers {
>> -    /* This runs inside the BQL.  */
>> +
>> +    /* The following handlers runs inside the BQL. */
> 
> s/runs/run
> 
>> +
>> +    /**
>> +     * @save_state
>> +     *
>> +     * Saves state section on the source using the latest state format
>> +     * version.
>> +     *
>> +     * Legacy method. Should be deprecated when all users are ported
>> +     * to VMState.
>> +     *
>> +     * @f: QEMUFile where to send the data
>> +     * @opaque: data pointer passed to register_savevm_live()
>> +     */
>>       void (*save_state)(QEMUFile *f, void *opaque);
>>
>> -    /*
>> -     * save_prepare is called early, even before migration starts, and can be
>> -     * used to perform early checks.
>> +    /**
>> +     * @save_prepare
>> +     *
>> +     * Called early, even before migration starts, and can be used to
>> +     * perform early checks.
>> +     *
>> +     * @opaque: data pointer passed to register_savevm_live()
>> +     * @errp: pointer to Error*, to store an error if it happens.
>> +     *
>> +     * Returns zero to indicate success and negative for error
>>        */
>>       int (*save_prepare)(void *opaque, Error **errp);
>> +
>> +    /**
>> +     * @save_setup
>> +     *
>> +     * Initializes the data structures on the source and transmits
>> +     * first section containing information on the device
>> +     *
>> +     * @f: QEMUFile where to send the data
>> +     * @opaque: data pointer passed to register_savevm_live()
>> +     *
>> +     * Returns zero to indicate success and negative for error
>> +     */
>>       int (*save_setup)(QEMUFile *f, void *opaque);
>> +
>> +    /**
>> +     * @save_cleanup
>> +     *
>> +     * Performs save related cleanup
> 
> Use save_setup phrasing?
> Uninitializes the data structures on the source.
> 
>> +     *
>> +     * @opaque: data pointer passed to register_savevm_live()
>> +     *
>> +     * Returns zero to indicate success and negative for error
> 
> This can be dropped as it's a void function.
> 
>> +     */
>>       void (*save_cleanup)(void *opaque);
>> +
>> +    /**
>> +     * @save_live_complete_postcopy
>> +     *
>> +     * Called at the end of postcopy for all postcopyiable devices.
> 
> s/postcopyiable/postcopyable> 
>> +     *
>> +     * @f: QEMUFile where to send the data
>> +     * @opaque: data pointer passed to register_savevm_live()
>> +     *
>> +     * Returns zero to indicate success and negative for error
>> +     */
>>       int (*save_live_complete_postcopy)(QEMUFile *f, void *opaque);
>> +
>> +    /**
>> +     * @save_live_complete_precopy
>> +     *
>> +     * Transmits the last section for the device containing any
>> +     * remaining data.
>> +     *
>> +     * @f: QEMUFile where to send the data
>> +     * @opaque: data pointer passed to register_savevm_live()
>> +     *
>> +     * Returns zero to indicate success and negative for error
>> +     */
>>       int (*save_live_complete_precopy)(QEMUFile *f, void *opaque);
>>
>>       /* This runs both outside and inside the BQL.  */
>> +
>> +    /**
>> +     * @is_active
>> +     *
>> +     * Will skip a state section if not active
>> +     *
>> +     * @opaque: data pointer passed to register_savevm_live()
>> +     *
>> +     * Returns true if state section is active else false
>> +     */
>>       bool (*is_active)(void *opaque);
>> +
>> +    /**
>> +     * @has_postcopy
>> +     *
>> +     * checks if a device supports postcopy
> 
> s/checks/Checks
> 
>> +     *
>> +     * @opaque: data pointer passed to register_savevm_live()
>> +     *
>> +     * Returns true for postcopy support else false
>> +     */
>>       bool (*has_postcopy)(void *opaque);
>>
>> -    /* is_active_iterate
>> -     * If it is not NULL then qemu_savevm_state_iterate will skip iteration if
>> -     * it returns false. For example, it is needed for only-postcopy-states,
>> -     * which needs to be handled by qemu_savevm_state_setup and
>> -     * qemu_savevm_state_pending, but do not need iterations until not in
>> -     * postcopy stage.
>> +    /**
>> +     * @is_active_iterate
>> +     *
>> +     * As #SaveVMHandlers.is_active(), will skip an inactive state
>> +     * section in qemu_savevm_state_iterate.
>> +     *
>> +     * For example, it is needed for only-postcopy-states, which needs
>> +     * to be handled by qemu_savevm_state_setup() and
>> +     * qemu_savevm_state_pending(), but do not need iterations until
>> +     * not in postcopy stage.
>> +     *
>> +     * @opaque: data pointer passed to register_savevm_live()
>> +     *
>> +     * Returns true if state section is active else false
>>        */
>>       bool (*is_active_iterate)(void *opaque);
>>
>> @@ -48,44 +147,150 @@ typedef struct SaveVMHandlers {
>>        * use data that is local to the migration thread or protected
>>        * by other locks.
>>        */
>> +
>> +    /**
>> +     * @save_live_iterate
>> +     *
>> +     * Should send a chunk of data until the point that stream
>> +     * bandwidth limits tell it to stop. Each call generates one
>> +     * section.
>> +     *
>> +     * @f: QEMUFile where to send the data
>> +     * @opaque: data pointer passed to register_savevm_live()
>> +     *
>> +     * Returns zero to indicate success and negative for error
> 
> 0 indicates that there is still more data to send, 1 indicates that there is no more data to send and negative indicates error.
> 
>> +     */
>>       int (*save_live_iterate)(QEMUFile *f, void *opaque);
>>
>>       /* This runs outside the BQL!  */
>> -    /* Note for save_live_pending:
>> -     * must_precopy:
>> -     * - must be migrated in precopy or in stopped state
>> -     * - i.e. must be migrated before target start
>> -     *
>> -     * can_postcopy:
>> -     * - can migrate in postcopy or in stopped state
>> -     * - i.e. can migrate after target start
>> -     * - some can also be migrated during precopy (RAM)
>> -     * - some must be migrated after source stops (block-dirty-bitmap)
>> -     *
>> -     * Sum of can_postcopy and must_postcopy is the whole amount of
>> +
>> +    /**
>> +     * @state_pending_estimate
>> +     *
>> +     * This estimates the remaining data to transfer
>> +     *
>> +     * Sum of @can_postcopy and @must_postcopy is the whole amount of
>>        * pending data.
>> +     *
>> +     * @opaque: data pointer passed to register_savevm_live()
>> +     * @must_precopy: amount of data that must be migrated in precopy
>> +     *                or in stopped state, i.e. that must be migrated
>> +     *                before target start.
>> +     * @can_postcopy: amount of data that can be migrated in postcopy
>> +     *                or in stopped state, i.e. after target start.
>> +     *                Some can also be migrated during precopy (RAM).
>> +     *                Some must be migrated after source stops
>> +     *                (block-dirty-bitmap)
>>        */
>> -    /* This estimates the remaining data to transfer */
>>       void (*state_pending_estimate)(void *opaque, uint64_t *must_precopy,
>>                                      uint64_t *can_postcopy);
>> -    /* This calculate the exact remaining data to transfer */
>> +
>> +    /**
>> +     * @state_pending_exact
>> +     *
>> +     * This calculate the exact remaining data to transfer
> 
> s/calculate/calculates
> 
>> +     *
>> +     * Sum of @can_postcopy and @must_postcopy is the whole amount of
>> +     * pending data.
>> +     *
>> +     * @opaque: data pointer passed to register_savevm_live()
>> +     * @must_precopy: amount of data that must be migrated in precopy
>> +     *                or in stopped state, i.e. that must be migrated
>> +     *                before target start.
>> +     * @can_postcopy: amount of data that can be migrated in postcopy
>> +     *                or in stopped state, i.e. after target start.
>> +     *                Some can also be migrated during precopy (RAM).
>> +     *                Some must be migrated after source stops
>> +     *                (block-dirty-bitmap)
>> +     */
>>       void (*state_pending_exact)(void *opaque, uint64_t *must_precopy,
>>                                   uint64_t *can_postcopy);
>> +
>> +    /**
>> +     * @load_state
>> +     *
>> +     * Load sections generated by any of the save functions that
>> +     * generate sections.
>> +     *
>> +     * Legacy method. Should be deprecated when all users are ported
>> +     * to VMState.
>> +     *
>> +     * @f: QEMUFile where to receive the data
>> +     * @opaque: data pointer passed to register_savevm_live()
>> +     * @version_id: the maximum version_id supported
>> +     *
>> +     * Returns zero to indicate success and negative for error
>> +     */
>>       int (*load_state)(QEMUFile *f, void *opaque, int version_id);
>> +
>> +    /**
>> +     * @load_setup
>> +     *
>> +     * Initializes the data structures on the destination.
>> +     *
>> +     * @f: QEMUFile where to receive the data
>> +     * @opaque: data pointer passed to register_savevm_live()
>> +     *
>> +     * Returns zero to indicate success and negative for error
>> +     */
>>       int (*load_setup)(QEMUFile *f, void *opaque);
>> +
>> +    /**
>> +     * @load_cleanup
>> +     *
>> +     * Performs cleanup of load data structures
> 
> Use load_setup phrasing?
> Uninitializes the data structures on the destination.
> 
>> +     *
>> +     * @opaque: data pointer passed to register_savevm_live()
>> +     *
>> +     * Returns zero to indicate success and negative for error
>> +     */
>>       int (*load_cleanup)(void *opaque);
>> -    /* Called when postcopy migration wants to resume from failure */
>> +
>> +    /**
>> +     * @resume_prepare
>> +     *
>> +     * Called when postcopy migration wants to resume from failure
>> +     *
>> +     * @s: Current migration state
>> +     * @opaque: data pointer passed to register_savevm_live()
>> +     *
>> +     * Returns zero to indicate success and negative for error
>> +     */
>>       int (*resume_prepare)(MigrationState *s, void *opaque);
>> -    /* Checks if switchover ack should be used. Called only in dest */
>> +
>> +    /**
>> +     * @switchover_ack_needed
>> +     *
>> +     * Checks if switchover ack should be used. Called only on
>> +     * destination.
>> +     *
>> +     * @opaque: data pointer passed to register_savevm_live()
> 
> Add:
> Returns true if switchover ack should be used and false otherwise
> 
>> +     */
>>       bool (*switchover_ack_needed)(void *opaque);
>>   } SaveVMHandlers;
>>
>> +/**
>> + * register_savevm_live: Register a set of custom migration handlers
>> + *
>> + * @idstr: state section identifier
>> + * @instance_id: instance id
>> + * @version_id: version id supported
>> + * @ops: SaveVMHandlers structurex
> 
> s/structurex/structure
> 
>> + * @opaque: data pointer passed SaveVMHandlers handlers
> 
> s/passed SaveVMHandlers/passed to SaveVMHandlers
> 
> Thanks.
> 
>> + */
>>   int register_savevm_live(const char *idstr,
>>                            uint32_t instance_id,
>>                            int version_id,
>>                            const SaveVMHandlers *ops,
>>                            void *opaque);
>>
>> +/**
>> + * unregister_savevm: Unregister custom migration handlers
>> + *
>> + * @obj: object associated with state section
>> + * @idstr:  state section identifier
>> + * @opaque: data pointer passed to register_savevm_live()
>> + */
>>   void unregister_savevm(VMStateIf *obj, const char *idstr, void *opaque);
>>
>>   #endif
>> -- 
>> 2.43.2
>>
>