Re: [PATCH RFC v2 09/11] vmstate: Implement VMS_ARRAY_OF_POINTER_AUTO_ALLOC

[Fabiano Rosas <farosas@suse.de>]

Peter Xu <peterx@redhat.com> writes:

> Introduce a new flag, VMS_ARRAY_OF_POINTER_AUTO_ALLOC, for VMSD field.  It
> must be used together with VMS_ARRAY_OF_POINTER.
>
> It can be used to allow migration of an array of pointers where the
> pointers may point to NULLs.
>
> Note that we used to allow migration of a NULL pointer within an array that
> is being migrated. That corresponds to the code around vmstate_info_nullptr
> where we may get/put one byte showing that the element of an array is NULL.
>
> That usage is fine but very limited, it's because even if it will migrate a
> NULL pointer with a marker, it still works in a way that both src and dest
> QEMUs must know exactly which elements of the array are non-NULL, so
> instead of dynamically loading an array (which can have NULL pointers), it
> actually only verifies the known NULL pointers are still NULL pointers
> after migration.
>
> Also, in that case since dest QEMU knows exactly which element is NULL,
> which is not NULL, dest QEMU's device code will manage all allocations for
> the elements before invoking vmstate_load_vmsd().
>
> That's not enough per evolving needs of new device states that may want to
> provide real dynamic array of pointers, like what Alexander proposed here
> with the NVMe device migration:
>
> https://lore.kernel.org/r/20260317102708.126725-1-alexander@mihalicyn.com
>
> This patch is an alternative approach to address the problem.
>
> Along with the flag, introduce two new macros:
>
>   VMSTATE_VARRAY_OF_POINTER_TO_STRUCT_UINT{8|32}_ALLOC()
>
> Which will be used very soon in the NVMe series.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  include/migration/vmstate.h |  51 +++++++++++++-
>  migration/savevm.c          |  27 ++++++-
>  migration/vmstate.c         | 136 ++++++++++++++++++++++++++++++------
>  3 files changed, 190 insertions(+), 24 deletions(-)
>
> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> index 2e51b5ea04..d844b46e63 100644
> --- a/include/migration/vmstate.h
> +++ b/include/migration/vmstate.h
> @@ -161,8 +161,21 @@ enum VMStateFlags {
>       * structure we are referencing to use. */
>      VMS_VSTRUCT           = 0x8000,
>  
> +    /*
> +     * This is a sub-flag for VMS_ARRAY_OF_POINTER.  When this flag is set,
> +     * VMS_ARRAY_OF_POINTER must also be set.  When set, it means array
> +     * elements can contain either valid or NULL pointers, vmstate core
> +     * will be responsible for synchronizing the pointer status, providing
> +     * proper memory allocations on the pointer when it is populated on the
> +     * source QEMU.  It also means the user of the field must make sure all
> +     * the elements in the array are NULL pointers before loading.  This
> +     * should also work with VMS_ALLOC when the array itself also needs to
> +     * be allocated.
> +     */
> +    VMS_ARRAY_OF_POINTER_AUTO_ALLOC = 0x10000,
> +
>      /* Marker for end of list */
> -    VMS_END = 0x10000
> +    VMS_END                         = 0x20000,
>  };
>  
>  typedef enum {
> @@ -580,6 +593,42 @@ extern const VMStateInfo vmstate_info_qlist;
>      .offset     = vmstate_offset_array(_s, _f, _type*, _n),          \
>  }
>  
> +/*
> + * For migrating a dynamically allocated uint{8,32}-indexed array of
> + * pointers to structures (with NULL entries and with auto memory
> + * allocation).
> + *
> + * _type: type of structure pointed to
> + * _vmsd: VMSD for structure _type (when VMS_STRUCT is set)
> + * _info: VMStateInfo for _type (when VMS_STRUCT is not set)
> + * start: size of (_type) pointed to (for auto memory allocation)
> + */
> +#define VMSTATE_VARRAY_OF_POINTER_TO_STRUCT_UINT8_ALLOC(\
> +    _field, _state, _field_num, _version, _vmsd, _type) {            \
> +    .name       = (stringify(_field)),                               \
> +    .version_id = (_version),                                        \
> +    .num_offset = vmstate_offset_value(_state, _field_num, uint8_t), \
> +    .vmsd       = &(_vmsd),                                          \
> +    .size       = sizeof(_type),                                     \
> +    .flags      = VMS_POINTER | VMS_VARRAY_UINT8 |                   \
> +                  VMS_ARRAY_OF_POINTER | VMS_STRUCT |                \
> +                  VMS_ARRAY_OF_POINTER_AUTO_ALLOC,                   \
> +    .offset     = vmstate_offset_pointer(_state, _field, _type *),   \
> +}
> +
> +#define VMSTATE_VARRAY_OF_POINTER_TO_STRUCT_UINT32_ALLOC(\
> +    _field, _state, _field_num, _version, _vmsd, _type) {             \
> +    .name       = (stringify(_field)),                                \
> +    .version_id = (_version),                                         \
> +    .num_offset = vmstate_offset_value(_state, _field_num, uint32_t), \
> +    .vmsd       = &(_vmsd),                                           \
> +    .size       = sizeof(_type),                                      \
> +    .flags      = VMS_POINTER | VMS_VARRAY_UINT32 |                   \
> +                  VMS_ARRAY_OF_POINTER | VMS_STRUCT |                 \
> +                  VMS_ARRAY_OF_POINTER_AUTO_ALLOC,                    \
> +    .offset     = vmstate_offset_pointer(_state, _field, _type *),    \
> +}
> +
>  #define VMSTATE_VARRAY_OF_POINTER_UINT32(_field, _state, _field_num, _version, _info, _type) { \
>      .name       = (stringify(_field)),                                    \
>      .version_id = (_version),                                             \
> diff --git a/migration/savevm.c b/migration/savevm.c
> index f5a6fd0c66..765df8ce2d 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -869,8 +869,33 @@ static void vmstate_check(const VMStateDescription *vmsd)
>      if (field) {
>          while (field->name) {
>              if (field->flags & VMS_ARRAY_OF_POINTER) {
> -                assert(field->size == 0);
> +                if (field->flags & VMS_ARRAY_OF_POINTER_AUTO_ALLOC) {
> +                    /*
> +                     * Size must be provided because dest QEMU needs that
> +                     * info to know what to allocate
> +                     */
> +                    assert(field->size || field->size_offset);
> +                } else {
> +                    /*
> +                     * Otherwise size info isn't useful (because it's
> +                     * always the size of host pointer), detect accidental
> +                     * setup of sizes in this case.
> +                     */
> +                    assert(field->size == 0 && field->size_offset == 0);
> +                }
> +                /*
> +                 * VMS_ARRAY_OF_POINTER must be used only together with one
> +                 * of VMS_(V)ARRAY* flags.
> +                 */
> +                assert(field->flags & (VMS_ARRAY | VMS_VARRAY_INT32 |
> +                                       VMS_VARRAY_UINT16 | VMS_VARRAY_UINT8 |
> +                                       VMS_VARRAY_UINT32));
>              }
> +
> +            if (field->flags & VMS_ARRAY_OF_POINTER_AUTO_ALLOC) {
> +                assert(field->flags & VMS_ARRAY_OF_POINTER);
> +            }
> +
>              if (field->flags & (VMS_STRUCT | VMS_VSTRUCT)) {
>                  /* Recurse to sub structures */
>                  vmstate_check(field->vmsd);
> diff --git a/migration/vmstate.c b/migration/vmstate.c
> index 47812eb882..9cd0a88ce9 100644
> --- a/migration/vmstate.c
> +++ b/migration/vmstate.c
> @@ -153,6 +153,12 @@ static bool vmstate_ptr_marker_load(QEMUFile *f, bool *load_field,
>          return true;
>      }
>  
> +    if (byte == VMS_MARKER_PTR_VALID) {
> +        /* We need to load the field right after the marker */
> +        *load_field = true;
> +        return true;
> +    }
> +
>      error_setg(errp, "Unexpected ptr marker: %d", byte);

just checking: is this error always the right thing to do? IOW, an array
of pointers member should never be NULL unless wrapped by PTR_NULL or
PTR_VALID.

>      return false;
>  }
> @@ -234,6 +240,67 @@ static bool vmstate_post_load(const VMStateDescription *vmsd,
>      return true;
>  }
>  
> +/*
> + * Try to prepare loading the next element, the object pointer to be put
> + * into @next_elem.  When @next_elem is NULL, it means we should skip
> + * loading this element.
> + *
> + * Returns false for errors, in which case *errp will be set, migration
> + * must be aborted.
> + */
> +static bool vmstate_load_next(QEMUFile *f, const VMStateField *field,
> +                              void *first_elem, void **next_elem,
> +                              int size, int i, Error **errp)
> +{
> +    bool auto_alloc = field->flags & VMS_ARRAY_OF_POINTER_AUTO_ALLOC;
> +    void *ptr = first_elem + size * i, **pptr;
> +    bool load_field;
> +
> +    if (!(field->flags & VMS_ARRAY_OF_POINTER)) {
> +        /* Simplest case, no pointer involved */
> +        *next_elem = ptr;
> +        return true;
> +    }
> +
> +    /*
> +     * We're loading an array of pointers, switch to use pptr to make it
> +     * easier to read later
> +     */
> +    pptr = (void **)ptr;
> +
> +    /*
> +     * Some special cases use pointer markers: (1) _AUTO_ALLOC implies a
> +     * ptr marker will always exist, or (2) the element on destination is
> +     * NULL, which expects the src to send a NULL-only marker.
> +     */
> +    if (auto_alloc || !*pptr) {

If auto_alloc && load_field, then !*pptr must be NULL. And if            [1]
!load_field, then *pptr must also be NULL. So this auto_alloc check is
not needed.

> +        if (!vmstate_ptr_marker_load(f, &load_field, errp)) {

If !*pptr && !auto_alloc, we'll load the marker, which must be           [2]
VMS_MARKER_PTR_NULL, but the vmstate_ptr_marker_load function will also
happilly accept VMS_MARKER_PTR_VALID. I guess that's what the assert
down below is for.

> +            trace_vmstate_load_field_error(field->name, -EINVAL);
> +            return false;
> +        }
> +
> +        if (load_field) {
> +            /*
> +             * When reaching here, it means we received a non-NULL ptr
> +             * marker, so we need to populate the field before loading it.
> +             *
> +             * NOTE: do not use vmstate_size() here, because we need the
> +             * object size, not entry size of the array.
> +             */
> +            assert(auto_alloc);
> +            *pptr = g_malloc0(field->size);
> +        } else {
> +            /* Clear the pointer to imply a skip */
> +            *next_elem = NULL;

nit: if !*pptr then there's no need to set *next_elem to NULL.    [3]

> +            return true;
> +        }
> +    }

What about this version:

[1] if (!*pptr) {         
        int byte = qemu_get_byte(f);

        if (byte == VMS_MARKER_PTR_NULL) {
            /* When it's a null ptr marker, do not continue the load */
[3]         goto out;
        }

[2]     if (auto_alloc && byte == VMS_MARKER_PTR_VALID) {
            /*
             * When reaching here, it means we received a non-NULL ptr
             * marker, so we need to populate the field before loading it.
             *
             * NOTE: do not use vmstate_size() here, because we need the
             * object size, not entry size of the array.
             */
            *pptr = g_malloc0(field->size);
        } else {
            error_setg(errp, "Unexpected ptr marker: %d", byte);
            return false;
        }
    }

out:
    /* Move the cursor to the next element for loading */
    *next_elem = *pptr;
    return true;

> +
> +    /* Move the cursor to the next element for loading */
> +    *next_elem = *pptr;
> +    return true;
> +}
> +
>  bool vmstate_load_vmsd(QEMUFile *f, const VMStateDescription *vmsd,
>                         void *opaque, int version_id, Error **errp)
>  {
> @@ -279,27 +346,22 @@ bool vmstate_load_vmsd(QEMUFile *f, const VMStateDescription *vmsd,
>              }
>  
>              for (i = 0; i < n_elems; i++) {
> -                /* If we will process the load of field? */
> -                bool load_field = true;
> -                bool ok = true;
> -                void *curr_elem = first_elem + size * i;
> +                void *curr_elem;
> +                bool ok;
>  
> -                if (field->flags & VMS_ARRAY_OF_POINTER) {
> -                    curr_elem = *(void **)curr_elem;
> -                    if (!curr_elem) {
> -                        /* Read the marker instead of VMSD itself */
> -                        if (!vmstate_ptr_marker_load(f, &load_field, errp)) {
> -                            trace_vmstate_load_field_error(field->name,
> -                                                           -EINVAL);
> -                            return false;
> -                        }
> -                    }
> +                ok = vmstate_load_next(f, field, first_elem, &curr_elem,
> +                                       size, i, errp);
> +                if (!ok) {
> +                    return false;
>                  }
>  
> -                if (load_field) {
> -                    ok = vmstate_load_field(f, curr_elem, size, field, errp);
> +                if (!curr_elem) {
> +                    /* Implies a skip */
> +                    continue;
>                  }
>  
> +                ok = vmstate_load_field(f, curr_elem, size, field, errp);
> +
>                  if (ok) {
>                      int ret = qemu_file_get_error(f);
>                      if (ret < 0) {
> @@ -397,6 +459,16 @@ static bool vmsd_can_compress(const VMStateField *field)
>          return false;
>      }
>  
> +    if (field->flags & VMS_ARRAY_OF_POINTER_AUTO_ALLOC) {
> +        /*
> +         * This may involve two VMSD fields to be saved, one for the
> +         * marker to show if the pointer is NULL, followed by the real
> +         * vmstate object.  To make it simple at least for now, skip
> +         * compression for this one.
> +         */
> +        return false;
> +    }
> +
>      if (field->flags & VMS_STRUCT) {
>          const VMStateField *sfield = field->vmsd->fields;
>          while (sfield->name) {
> @@ -583,6 +655,12 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
>              int size = vmstate_size(opaque, field);
>              JSONWriter *vmdesc_loop = vmdesc;
>              bool is_prev_null = false;
> +            /*
> +             * When this is enabled, it means we will always push a ptr
> +             * marker first for each element saying if it's populated.
> +             */
> +            bool use_dynamic_array =
> +                field->flags & VMS_ARRAY_OF_POINTER_AUTO_ALLOC;
>  
>              trace_vmstate_save_state_loop(vmsd->name, field->name, n_elems);
>              if (field->flags & VMS_POINTER) {
> @@ -603,14 +681,9 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
>                  }
>  
>                  is_null = !curr_elem && size;
> -                use_marker_field = is_null;
> +                use_marker_field = use_dynamic_array || is_null;
>  
>                  if (use_marker_field) {
> -                    /*
> -                     * If null pointer found (which should only happen in
> -                     * an array of pointers), use null placeholder and do
> -                     * not follow.
> -                     */
>                      inner_field = vmsd_create_ptr_marker_field(field);
>                  } else {
>                      inner_field = field;
> @@ -657,6 +730,25 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
>                      goto out;
>                  }
>  
> +                /*
> +                 * If we're using dynamic array and the element is
> +                 * populated, save the real object right after the marker.
> +                 */
> +                if (use_dynamic_array && curr_elem) {
> +                    /*
> +                     * NOTE: do not use vmstate_size() here because we want
> +                     * to save the real VMSD object now.
> +                     */
> +                    ok = vmstate_save_field_with_vmdesc(f, curr_elem,
> +                                                        field->size, vmsd,
> +                                                        field, vmdesc_loop,
> +                                                        i, max_elems, errp);
> +
> +                    if (!ok) {
> +                        goto out;
> +                    }
> +                }
> +
>                  /* Compressed arrays only care about the first element */
>                  if (vmdesc_loop && max_elems > 1) {
>                      vmdesc_loop = NULL;