Re: [Qemu-devel] [PATCH v10 08/13] 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.
>
> It requires visit_next_list() to gain a size parameter, to know
> what size element to allocate; comparable to the size parameter
> of visit_start_struct().
>
> 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>
>
> ---
> v10: hoist earlier in series
> 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
> ---
>  include/qapi/visitor.h       | 14 +++++++-------
>  include/qapi/visitor-impl.h  |  2 +-
>  scripts/qapi-types.py        |  5 +----
>  scripts/qapi-visit.py        |  2 +-
>  qapi/qapi-visit-core.c       |  4 ++--
>  qapi/opts-visitor.c          |  4 ++--
>  qapi/qapi-dealloc-visitor.c  |  3 ++-
>  qapi/qmp-input-visitor.c     |  5 +++--
>  qapi/qmp-output-visitor.c    |  3 ++-
>  qapi/string-input-visitor.c  |  4 ++--
>  qapi/string-output-visitor.c |  2 +-
>  11 files changed, 24 insertions(+), 24 deletions(-)
>
> diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
> index 5e581dc..c131a32 100644
> --- a/include/qapi/visitor.h
> +++ b/include/qapi/visitor.h
> @@ -19,13 +19,13 @@
>  #include "qapi/error.h"
>  #include <stdlib.h>
>
> -typedef struct GenericList
> -{
> -    union {
> -        void *value;
> -        uint64_t padding;
> -    };
> +
> +/* This struct is layout-compatible with all other *List structs
> + * created by the qapi generator.  It is used as a typical
> + * singly-linked list. */
> +typedef struct GenericList {
>      struct GenericList *next;
> +    char padding[];
>  } GenericList;
>
>  void visit_start_struct(Visitor *v, const char *name, void **obj,
> @@ -36,7 +36,7 @@ void visit_start_implicit_struct(Visitor *v, void **obj, 
> size_t size,
>  void visit_end_implicit_struct(Visitor *v);
>
>  void visit_start_list(Visitor *v, const char *name, Error **errp);
> -GenericList *visit_next_list(Visitor *v, GenericList **list);
> +GenericList *visit_next_list(Visitor *v, GenericList **list, size_t size);
>  void visit_end_list(Visitor *v);
>
>  /**
> diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
> index ea252f8..7905a28 100644
> --- a/include/qapi/visitor-impl.h
> +++ b/include/qapi/visitor-impl.h
> @@ -29,7 +29,7 @@ struct Visitor
>
>      void (*start_list)(Visitor *v, const char *name, Error **errp);
>      /* Must be set */
> -    GenericList *(*next_list)(Visitor *v, GenericList **list);
> +    GenericList *(*next_list)(Visitor *v, GenericList **list, size_t size);
>      /* Must be set */
>      void (*end_list)(Visitor *v);
>
> diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> index 7b0dca8..83f230a 100644
> --- a/scripts/qapi-types.py
> +++ b/scripts/qapi-types.py
> @@ -26,11 +26,8 @@ def gen_array(name, element_type):
>      return mcgen('''
>
>  struct %(c_name)s {
> -    union {
> -        %(c_type)s value;
> -        uint64_t padding;
> -    };
>      %(c_name)s *next;
> +    %(c_type)s value;
>  };
>  ''',
>                   c_name=c_name(name), c_type=element_type.c_type())
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index 177dfc4..9ff0337 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -129,7 +129,7 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, 
> %(c_name)s **obj, Error
>      }
>
>      for (prev = (GenericList **)obj;
> -         !err && (i = visit_next_list(v, prev)) != NULL;
> +         !err && (i = visit_next_list(v, prev, sizeof(**obj))) != NULL;
>           prev = &i) {
>          %(c_name)s *native_i = (%(c_name)s *)i;
>          visit_type_%(c_elt_type)s(v, NULL, &native_i->value, &err);
> diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
> index f856286..6fa66f1 100644
> --- a/qapi/qapi-visit-core.c
> +++ b/qapi/qapi-visit-core.c
> @@ -50,9 +50,9 @@ void visit_start_list(Visitor *v, const char *name, Error 
> **errp)
>      v->start_list(v, name, errp);
>  }
>
> -GenericList *visit_next_list(Visitor *v, GenericList **list)
> +GenericList *visit_next_list(Visitor *v, GenericList **list, size_t size)
>  {

Lost since v9: assert(size).  In my review of v9, I suggested tightening
the assertion to size >= GenericList.

> -    return v->next_list(v, list);
> +    return v->next_list(v, list, size);
>  }
>
>  void visit_end_list(Visitor *v)
> diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
> index ae5b955..73e4ace 100644
> --- a/qapi/opts-visitor.c
> +++ b/qapi/opts-visitor.c
> @@ -215,7 +215,7 @@ opts_start_list(Visitor *v, const char *name, Error 
> **errp)
>
>
>  static GenericList *
> -opts_next_list(Visitor *v, GenericList **list)
> +opts_next_list(Visitor *v, GenericList **list, size_t size)
>  {
>      OptsVisitor *ov = to_ov(v);
>      GenericList **link;
> @@ -258,7 +258,7 @@ opts_next_list(Visitor *v, GenericList **list)
>          abort();
>      }
>
> -    *link = g_malloc0(sizeof **link);
> +    *link = g_malloc0(size);
>      return *link;
>  }
>
> diff --git a/qapi/qapi-dealloc-visitor.c b/qapi/qapi-dealloc-visitor.c
> index 2659d3f..6667e8c 100644
> --- a/qapi/qapi-dealloc-visitor.c
> +++ b/qapi/qapi-dealloc-visitor.c
> @@ -100,7 +100,8 @@ static void qapi_dealloc_start_list(Visitor *v, const 
> char *name, Error **errp)
>      qapi_dealloc_push(qov, NULL);
>  }
>
> -static GenericList *qapi_dealloc_next_list(Visitor *v, GenericList **listp)
> +static GenericList *qapi_dealloc_next_list(Visitor *v, GenericList **listp,
> +                                           size_t size)
>  {
>      GenericList *list = *listp;
>      QapiDeallocVisitor *qov = to_qov(v);
> diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
> index 2f48b95..2621660 100644
> --- a/qapi/qmp-input-visitor.c
> +++ b/qapi/qmp-input-visitor.c
> @@ -165,7 +165,8 @@ static void qmp_input_start_list(Visitor *v, const char 
> *name, Error **errp)
>      qmp_input_push(qiv, qobj, errp);
>  }
>
> -static GenericList *qmp_input_next_list(Visitor *v, GenericList **list)
> +static GenericList *qmp_input_next_list(Visitor *v, GenericList **list,
> +                                        size_t size)
>  {
>      QmpInputVisitor *qiv = to_qiv(v);
>      GenericList *entry;
> @@ -184,7 +185,7 @@ static GenericList *qmp_input_next_list(Visitor *v, 
> GenericList **list)
>          return NULL;
>      }
>
> -    entry = g_malloc0(sizeof(*entry));
> +    entry = g_malloc0(size);
>      if (first) {
>          *list = entry;
>      } else {
> diff --git a/qapi/qmp-output-visitor.c b/qapi/qmp-output-visitor.c
> index f47eefa..d44c676 100644
> --- a/qapi/qmp-output-visitor.c
> +++ b/qapi/qmp-output-visitor.c
> @@ -126,7 +126,8 @@ static void qmp_output_start_list(Visitor *v, const char 
> *name, Error **errp)
>      qmp_output_push(qov, list);
>  }
>
> -static GenericList *qmp_output_next_list(Visitor *v, GenericList **listp)
> +static GenericList *qmp_output_next_list(Visitor *v, GenericList **listp,
> +                                         size_t size)
>  {
>      GenericList *list = *listp;
>      QmpOutputVisitor *qov = to_qov(v);
> diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c
> index 18b9339..59eb5dc 100644
> --- a/qapi/string-input-visitor.c
> +++ b/qapi/string-input-visitor.c
> @@ -139,7 +139,7 @@ start_list(Visitor *v, const char *name, Error **errp)
>      }
>  }
>
> -static GenericList *next_list(Visitor *v, GenericList **list)
> +static GenericList *next_list(Visitor *v, GenericList **list, size_t size)
>  {
>      StringInputVisitor *siv = to_siv(v);
>      GenericList **link;
> @@ -173,7 +173,7 @@ static GenericList *next_list(Visitor *v, GenericList 
> **list)
>          link = &(*list)->next;
>      }
>
> -    *link = g_malloc0(sizeof **link);
> +    *link = g_malloc0(size);
>      return *link;
>  }
>
> diff --git a/qapi/string-output-visitor.c b/qapi/string-output-visitor.c
> index b980bd3..c2e5c5b 100644
> --- a/qapi/string-output-visitor.c
> +++ b/qapi/string-output-visitor.c
> @@ -276,7 +276,7 @@ start_list(Visitor *v, const char *name, Error **errp)
>      sov->head = true;
>  }
>
> -static GenericList *next_list(Visitor *v, GenericList **list)
> +static GenericList *next_list(Visitor *v, GenericList **list, size_t size)
>  {
>      StringOutputVisitor *sov = to_sov(v);
>      GenericList *ret = NULL;

Doing this patch earlier in the series halved its size :)