Re: [RFC PATCH v1 05/17] vmstate: Remove vmdesc_loop

[Peter Xu <peterx@redhat.com>]

On Wed, Mar 25, 2026 at 03:11:25PM -0300, Fabiano Rosas wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > On Tue, Mar 24, 2026 at 04:43:20PM -0300, Fabiano Rosas wrote:
> >> The vmdesc_loop variable is being used as a hacky way of writing the
> >> JSON description only for the first element of a compressed
> >> array. There are also a few implicit uses, such as writing the JSON
> >> for the first non-compressed element after a compressed portion.
> >> 
> >> Future work will have trouble with this. Use a simple boolean to be
> >> explicit now.
> >> 
> >> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> >> ---
> >>  migration/vmstate.c | 25 ++++++++++++++-----------
> >>  1 file changed, 14 insertions(+), 11 deletions(-)
> >> 
> >> diff --git a/migration/vmstate.c b/migration/vmstate.c
> >> index dec9cf920b..7a12245d36 100644
> >> --- a/migration/vmstate.c
> >> +++ b/migration/vmstate.c
> >> @@ -622,8 +622,9 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
> >>              void *first_elem = opaque + field->offset;
> >>              int i, n_elems = vmstate_n_elems(opaque, field);
> >>              int size = vmstate_size(opaque, field);
> >> -            JSONWriter *vmdesc_loop = vmdesc;
> >>              bool is_null_prev = false;
> >> +            bool use_vmdesc = true;
> >> +
> >>              /*
> >>               * When this is enabled, it means we will always push a ptr
> >>               * marker first for each element saying if it's populated.
> >> @@ -673,7 +674,7 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
> >>                      is_null != is_null_prev) {
> >>  
> >>                      is_null_prev = is_null;
> >> -                    vmdesc_loop = vmdesc;
> >> +                    use_vmdesc = true;
> >>  
> >>                      for (int j = i + 1; j < n_elems; j++) {
> >>                          void *elem = *(void **)(first_elem + size * j);
> >> @@ -686,8 +687,13 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
> >>                      }
> >>                  }
> >>  
> >> +                if (use_dynamic_array) {
> >> +                    use_vmdesc = true;
> >> +                }
> >
> > [1]
> >
> >> +
> >>                  ok = vmstate_save_field_with_vmdesc(f, curr_elem, size, vmsd,
> >> -                                                    inner_field, vmdesc_loop,
> >> +                                                    inner_field,
> >> +                                                    use_vmdesc ? vmdesc : NULL,
> >>                                                      i, max_elems, errp);
> >>  
> >>                  /* If we used a fake temp field.. free it now */
> >> @@ -708,19 +714,16 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
> >>                       * NOTE: do not use vmstate_size() here because we want
> >>                       * to dump the real VMSD object now.
> >>                       */
> >> -                    ok = vmstate_save_field_with_vmdesc(f, curr_elem,
> >> -                                                        field->size, vmsd,
> >> -                                                        field, vmdesc_loop,
> >> -                                                        i, max_elems, errp);
> >> +                    ok = vmstate_save_field_with_vmdesc(
> >> +                        f, curr_elem, field->size, vmsd, field,
> >> +                        use_vmdesc ? vmdesc : NULL, i, max_elems, errp);
> >> +
> >>                      if (!ok) {
> >>                          goto out;
> >>                      }
> >>                  }
> >>  
> >> -                /* Compressed arrays only care about the first element */
> >> -                if (vmdesc_loop && vmsd_can_compress(field)) {
> >> -                    vmdesc_loop = NULL;
> >> -                }
> >> +                use_vmdesc = false;
> >
> > Hmm...
> >
> > vmsd_can_compress() can return arbitrary things depending on the field
> > itself, now we always do the dedup almost for everything.
> >
> > The patch did add above [1] to make AUTO_ALLOC be ok, by enforce setting
> > TRUE for it for every loop, but what about other cases where
> > vmsd_can_compress() may return false (where the field does not support
> > compression)?
> >
> 
> Hmm, I think you're right. But what I'm doing by setting
> use_vmdesc=false here is just clearing it for the next iteration, where
> it should be set according to necessity. So I'd leave this down here,
> but up there change to:
> 
>          if (vmdesc && vmsd_can_compress(field)) {
>              if (is_null != is_null_prev) {
>                  use_vmdesc = true;
>                  ...
>              }
> -        }
> -
> -        if (use_dynamic_array) {
> +        } else {
>              use_vmdesc = true;
>          }

I think this may still cause some confusion and inefficiency.

For example, for uncompressed fields, we'll update this field twice for
each of the elements (setting true here, clearing to false at the end).

The old approach looks still better in that it only flips the pointer
whenever needed.  Said that, it doesn't always need to be a pointer, we can
still use a bool.

> 
> In words: all fields should appear in the JSON, unless its part of the
> NULL portion of a compressed array. For compressed arrays only the first
> element appears in the JSON. If a stream of NULLs is followed by a
> non-NULL member, then that member should appear in the JSON after the
> compressed entry.
> 
> These two catch all these corner-cases I think:
> 
> PYTHON=$(pwd)/pyvenv/bin/python3.11
> QTEST_QEMU_BINARY=./qemu-system-s390x ./tests/qtest/migration-test -p
> /s390x/migration/analyze-script
> 
> PYTHON=$(pwd)/pyvenv/bin/python3.11
> QTEST_QEMU_BINARY=./qemu-system-ppc64 ./tests/qtest/migration-test-p
> /ppc64/migration/analyze-script
> 
> >>              }
> >>          } else {
> >>              if (field->flags & VMS_MUST_EXIST) {
> >> -- 
> >> 2.51.0
> >> 
> 

-- 
Peter Xu