Re: [Qemu-devel] [PATCH v9 36/37] RFC: qapi: Adjust layout of FooList types

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> By sticking the next pointer first, we don't need a union with
> 64-bit padding for smaller types.  On 32-bit platforms, this
> can reduce the size of uint8List from 16 bytes (or 12, depending
> on whether 64-bit ints can tolerate 4-byte alignment) down to 8.
> It has no effect on 64-bit platforms (where alignment still
> dictates a 16-byte struct); but fewer anonymous unions is still
> a win in my book.
>
> However, this requires visit_start_list() and visit_next_list()
> to gain a size parameter, to know what size element to allocate.
>
> I debated about going one step further, to allow for fewer casts,
> by doing:
>     typedef GenericList GenericList;
>     struct GenericList {
>         GenericList *next;
>     };
>     struct FooList {
>         GenericList base;
>         Foo value;
>     };
> so that you convert to 'GenericList *' by '&foolist->base', and
> back by 'container_of(generic, GenericList, base)' (as opposed to
> the existing '(GenericList *)foolist' and '(FooList *)generic').
> But doing that would require hoisting the declaration of
> GenericList prior to inclusion of qapi-types.h, rather than its
> current spot in visitor.h; it also makes iteration a bit more
> verbose through 'foolist->base.next' instead of 'foolist->next'.
>
> Signed-off-by: Eric Blake <address@hidden>
>
> ---
> v9: no change
> v8: rebase to earlier changes
> v7: new patch; probably too invasive to be worth it, especially if
> we can't prove that our current size for uint8List is a bottleneck
> ---
>  hw/ppc/spapr_drc.c           |  2 +-
>  include/qapi/visitor-impl.h  |  5 +++--
>  include/qapi/visitor.h       | 39 +++++++++++++++++++--------------------
>  qapi/opts-visitor.c          |  9 +++++----
>  qapi/qapi-dealloc-visitor.c  |  5 +++--
>  qapi/qapi-visit-core.c       | 14 +++++++++-----
>  qapi/qmp-input-visitor.c     |  8 ++++----
>  qapi/qmp-output-visitor.c    |  5 +++--
>  qapi/string-input-visitor.c  |  9 +++++----
>  qapi/string-output-visitor.c |  5 +++--
>  scripts/qapi-types.py        |  5 +----
>  scripts/qapi-visit.py        |  4 ++--
>  12 files changed, 58 insertions(+), 52 deletions(-)
>
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index 41f2da0..0eba901 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -299,7 +299,7 @@ static void prop_get_fdt(Object *obj, Visitor *v, const 
> char *name,
>              int i;
>              prop = fdt_get_property_by_offset(fdt, fdt_offset, &prop_len);
>              name = fdt_string(fdt, fdt32_to_cpu(prop->nameoff));
> -            visit_start_list(v, name, NULL, &err);
> +            visit_start_list(v, name, NULL, 0, &err);
>              if (err) {
>                  error_propagate(errp, err);
>                  return;
> diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
> index 8df4ba1..dbbbcdb 100644
> --- a/include/qapi/visitor-impl.h
> +++ b/include/qapi/visitor-impl.h
> @@ -41,9 +41,10 @@ struct Visitor
>
>      /* Must be set */
>      bool (*start_list)(Visitor *v, const char *name, GenericList **list,
> -                       Error **errp);
> +                       size_t size, Error **errp);
>      /* Must be set */
> -    GenericList *(*next_list)(Visitor *v, GenericList *element, Error 
> **errp);
> +    GenericList *(*next_list)(Visitor *v, GenericList *element, size_t size,
> +                              Error **errp);
>      /* Must be set */
>      void (*end_list)(Visitor *v);
>
> diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
> index 4eae633..c446726 100644
> --- a/include/qapi/visitor.h
> +++ b/include/qapi/visitor.h
> @@ -56,11 +56,8 @@
>   * created by the qapi generator. */
>  typedef struct GenericList
>  {
> -    union {
> -        void *value;
> -        uint64_t padding;
> -    };
>      struct GenericList *next;
> +    char padding[];
>  } GenericList;

Less trickery, I like it.

Member padding appears to be unused.

>
>  /**
> @@ -130,19 +127,19 @@ void visit_end_implicit_struct(Visitor *v);
>  /**
>   * Prepare to visit a list tied to an object key @name.
>   * @name will be NULL if this is visited as part of another list.
> - * Input visitors malloc a qapi List struct into address@hidden, or set it to
> - * NULL if there are no elements in the list; and output visitors
> - * expect address@hidden to point to the start of the list, if any.  On
> - * return, if address@hidden is non-NULL, the caller should enter a loop
> + * Input visitors malloc a qapi List struct into address@hidden of size 
> @size,
> + * or set it to NULL if there are no elements in the list; and output
> + * visitors expect address@hidden to point to the start of the list, if any.
> + * On return, if address@hidden is non-NULL, the caller should enter a loop
>   * visiting the current element, then using visit_next_list() to
>   * advance to the next element, until that returns NULL; then
>   * visit_end_list() must be used to complete the visit.
>   *
> - * If supported by a visitor, @list can be NULL to indicate that there
> - * is no qapi List struct, and that the upcoming visit calls are
> - * parsing input to or creating output from some other representation;
> - * in this case, visit_next_list() will not be needed, but
> - * visit_end_list() is still mandatory.
> + * If supported by a visitor, @list can be NULL (and @size 0) to
> + * indicate that there is no qapi List struct, and that the upcoming
> + * visit calls are parsing input to or creating output from some other
> + * representation; in this case, visit_next_list() will not be needed,
> + * but visit_end_list() is still mandatory.
>   *
>   * Returns true if address@hidden was allocated; if that happens, and an 
> error
>   * occurs any time before the matching visit_end_list(), then the
> @@ -150,17 +147,19 @@ void visit_end_implicit_struct(Visitor *v);
>   * allocation before returning control further.
>   */
>  bool visit_start_list(Visitor *v, const char *name, GenericList **list,
> -                      Error **errp);
> +                      size_t size, Error **errp);
>  /**
>   * Iterate over a GenericList during a list visit.
>   * Before calling this function, the caller should use the appropriate
> - * visit_type_FOO() for the current list element at @element->value, and
> - * check for errors. @element must not be NULL; on the first iteration,
> - * it should be the value in *list after visit_start_list(); on other
> - * calls it should be the previous return value.  This function
> - * returns NULL once there are no further list elements.
> + * visit_type_FOO() for the current list element at @element->value,
> + * and check for errors. @element must not be NULL; on the first
> + * iteration, it should be the value in *list after
> + * visit_start_list(); on other calls it should be the previous return
> + * value.  @size should be the same as for visit_start_list().  This
> + * function returns NULL once there are no further list elements.
>   */
> -GenericList *visit_next_list(Visitor *v, GenericList *element, Error **errp);
> +GenericList *visit_next_list(Visitor *v, GenericList *element, size_t size,
> +                             Error **errp);
>  /**
>   * Complete the list started earlier.
>   * Must be called after any successful use of visit_start_list(),
> diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
> index 38d1e68..28f9a8a 100644
> --- a/qapi/opts-visitor.c
> +++ b/qapi/opts-visitor.c
> @@ -214,7 +214,8 @@ lookup_distinct(const OptsVisitor *ov, const char *name, 
> Error **errp)
>
>
>  static bool
> -opts_start_list(Visitor *v, const char *name, GenericList **list, Error 
> **errp)
> +opts_start_list(Visitor *v, const char *name, GenericList **list, size_t 
> size,
> +                Error **errp)
>  {
>      OptsVisitor *ov = to_ov(v);
>
> @@ -225,7 +226,7 @@ opts_start_list(Visitor *v, const char *name, GenericList 
> **list, Error **errp)
>      ov->repeated_opts = lookup_distinct(ov, name, errp);
>      if (ov->repeated_opts) {
>          ov->list_mode = LM_IN_PROGRESS;
> -        *list = g_new0(GenericList, 1);
> +        *list = g_malloc0(size);
>          return true;
>      } else {
>          *list = NULL;
> @@ -235,7 +236,7 @@ opts_start_list(Visitor *v, const char *name, GenericList 
> **list, Error **errp)
>
>
>  static GenericList *
> -opts_next_list(Visitor *v, GenericList *list, Error **errp)
> +opts_next_list(Visitor *v, GenericList *list, size_t size, Error **errp)
>  {
>      OptsVisitor *ov = to_ov(v);
>
> @@ -269,7 +270,7 @@ opts_next_list(Visitor *v, GenericList *list, Error 
> **errp)
>          abort();
>      }
>
> -    list->next = g_new0(GenericList, 1);
> +    list->next = g_malloc0(size);
>      return list->next;
>  }
>
> diff --git a/qapi/qapi-dealloc-visitor.c b/qapi/qapi-dealloc-visitor.c
> index 0990199..d498f29 100644
> --- a/qapi/qapi-dealloc-visitor.c
> +++ b/qapi/qapi-dealloc-visitor.c
> @@ -90,7 +90,8 @@ static void qapi_dealloc_end_implicit_struct(Visitor *v)
>  }
>
>  static bool qapi_dealloc_start_list(Visitor *v, const char *name,
> -                                    GenericList **list, Error **errp)
> +                                    GenericList **list, size_t size,
> +                                    Error **errp)
>  {
>      QapiDeallocVisitor *qov = to_qov(v);
>      qapi_dealloc_push(qov, NULL);
> @@ -98,7 +99,7 @@ static bool qapi_dealloc_start_list(Visitor *v, const char 
> *name,
>  }
>
>  static GenericList *qapi_dealloc_next_list(Visitor *v, GenericList *list,
> -                                           Error **errp)
> +                                           size_t size, Error **errp)
>  {
>      GenericList *next = list->next;
>      g_free(list);
> diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
> index 259e0cb..ed4de71 100644
> --- a/qapi/qapi-visit-core.c
> +++ b/qapi/qapi-visit-core.c
> @@ -80,19 +80,23 @@ void visit_end_implicit_struct(Visitor *v)
>  }
>
>  bool visit_start_list(Visitor *v, const char *name, GenericList **list,
> -                      Error **errp)
> +                      size_t size, Error **errp)
>  {
> -    bool result = v->start_list(v, name, list, errp);
> +    bool result;
> +
> +    assert(list ? size : !size);

Tighter than size != 0 would be size >= GenericList.  Same elsewhere.

> +    result = v->start_list(v, name, list, size, errp);
>      if (result) {
>          assert(list && *list);
>      }
>      return result;
>  }
>
> -GenericList *visit_next_list(Visitor *v, GenericList *list, Error **errp)
> +GenericList *visit_next_list(Visitor *v, GenericList *list, size_t size,
> +                             Error **errp)
>  {
> -    assert(list);
> -    return v->next_list(v, list, errp);
> +    assert(list && size);
> +    return v->next_list(v, list, size, errp);
>  }
>
>  void visit_end_list(Visitor *v)
[...]

Rest looks good.  Didn't look as closely as for the previous patches
(getting tired), but so far I like the idea.