[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-ppc] [Qemu-devel] [PATCH v9 36/37] RFC: qapi: Adjust layout of
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-ppc] [Qemu-devel] [PATCH v9 36/37] RFC: qapi: Adjust layout of FooList types |
Date: |
Thu, 28 Jan 2016 16:34:00 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) |
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.