| Message ID | 20200915174635.2333553-2-richard.henderson@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | qom: Allow object to be aligned | expand |
On Tue, Sep 15, 2020 at 10:46:31AM -0700, Richard Henderson wrote: > It turns out that some hosts have a default malloc alignment less > than that required for vectors. > > We assume that, with compiler annotation on CPUArchState, that we > can properly align the vector portion of the guest state. Fix the > alignment of the allocation by using qemu_memalloc when required. > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > Cc: Paolo Bonzini <pbonzini@redhat.com> > Cc: "Daniel P. Berrangé" <berrange@redhat.com> > Cc: Eduardo Habkost <ehabkost@redhat.com> > --- > include/qom/object.h | 4 ++++ > qom/object.c | 16 +++++++++++++--- > 2 files changed, 17 insertions(+), 3 deletions(-) > > diff --git a/include/qom/object.h b/include/qom/object.h > index 056f67ab3b..d52d0781a3 100644 > --- a/include/qom/object.h > +++ b/include/qom/object.h > @@ -770,6 +770,9 @@ struct Object > * @instance_size: The size of the object (derivative of #Object). If > * @instance_size is 0, then the size of the object will be the size of the > * parent object. > + * @instance_align: The required alignment of the object. If @instance_align > + * is 0, then normal malloc alignment is sufficient; if non-zero, then we > + * must use qemu_memalign for allocation. > * @instance_init: This function is called to initialize an object. The parent > * class will have already been initialized so the type is only responsible > * for initializing its own members. > @@ -807,6 +810,7 @@ struct TypeInfo > const char *parent; > > size_t instance_size; > + size_t instance_align; > void (*instance_init)(Object *obj); > void (*instance_post_init)(Object *obj); > void (*instance_finalize)(Object *obj); > diff --git a/qom/object.c b/qom/object.c > index 387efb25eb..2e53cb44a6 100644 > --- a/qom/object.c > +++ b/qom/object.c > @@ -50,6 +50,7 @@ struct TypeImpl > size_t class_size; > > size_t instance_size; > + size_t instance_align; > > void (*class_init)(ObjectClass *klass, void *data); > void (*class_base_init)(ObjectClass *klass, void *data); > @@ -114,6 +115,7 @@ static TypeImpl *type_new(const TypeInfo *info) > > ti->class_size = info->class_size; > ti->instance_size = info->instance_size; > + ti->instance_align = info->instance_align; > > ti->class_init = info->class_init; > ti->class_base_init = info->class_base_init; > @@ -691,13 +693,21 @@ static void object_finalize(void *data) > static Object *object_new_with_type(Type type) > { > Object *obj; > + size_t size, align; > > g_assert(type != NULL); > type_initialize(type); > > - obj = g_malloc(type->instance_size); > - object_initialize_with_type(obj, type->instance_size, type); > - obj->free = g_free; > + size = type->instance_size; > + align = type->instance_align; > + if (align) { If we check for (align > G_MEM_ALIGN) instead, we will be able to set instance_align automatically at OBJECT_DEFINE_TYPE*. > + obj = qemu_memalign(align, size); > + } else { > + obj = g_malloc(size); > + } > + > + object_initialize_with_type(obj, size, type); > + obj->free = (align ? qemu_vfree : g_free); > > return obj; > } > -- > 2.25.1 > -- Eduardo
On 9/15/20 11:07 AM, Eduardo Habkost wrote: > On Tue, Sep 15, 2020 at 10:46:31AM -0700, Richard Henderson wrote: >> It turns out that some hosts have a default malloc alignment less >> than that required for vectors. >> >> We assume that, with compiler annotation on CPUArchState, that we >> can properly align the vector portion of the guest state. Fix the >> alignment of the allocation by using qemu_memalloc when required. >> >> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> >> --- >> Cc: Paolo Bonzini <pbonzini@redhat.com> >> Cc: "Daniel P. Berrangé" <berrange@redhat.com> >> Cc: Eduardo Habkost <ehabkost@redhat.com> >> --- >> include/qom/object.h | 4 ++++ >> qom/object.c | 16 +++++++++++++--- >> 2 files changed, 17 insertions(+), 3 deletions(-) >> >> diff --git a/include/qom/object.h b/include/qom/object.h >> index 056f67ab3b..d52d0781a3 100644 >> --- a/include/qom/object.h >> +++ b/include/qom/object.h >> @@ -770,6 +770,9 @@ struct Object >> * @instance_size: The size of the object (derivative of #Object). If >> * @instance_size is 0, then the size of the object will be the size of the >> * parent object. >> + * @instance_align: The required alignment of the object. If @instance_align >> + * is 0, then normal malloc alignment is sufficient; if non-zero, then we >> + * must use qemu_memalign for allocation. >> * @instance_init: This function is called to initialize an object. The parent >> * class will have already been initialized so the type is only responsible >> * for initializing its own members. >> @@ -807,6 +810,7 @@ struct TypeInfo >> const char *parent; >> >> size_t instance_size; >> + size_t instance_align; >> void (*instance_init)(Object *obj); >> void (*instance_post_init)(Object *obj); >> void (*instance_finalize)(Object *obj); >> diff --git a/qom/object.c b/qom/object.c >> index 387efb25eb..2e53cb44a6 100644 >> --- a/qom/object.c >> +++ b/qom/object.c >> @@ -50,6 +50,7 @@ struct TypeImpl >> size_t class_size; >> >> size_t instance_size; >> + size_t instance_align; >> >> void (*class_init)(ObjectClass *klass, void *data); >> void (*class_base_init)(ObjectClass *klass, void *data); >> @@ -114,6 +115,7 @@ static TypeImpl *type_new(const TypeInfo *info) >> >> ti->class_size = info->class_size; >> ti->instance_size = info->instance_size; >> + ti->instance_align = info->instance_align; >> >> ti->class_init = info->class_init; >> ti->class_base_init = info->class_base_init; >> @@ -691,13 +693,21 @@ static void object_finalize(void *data) >> static Object *object_new_with_type(Type type) >> { >> Object *obj; >> + size_t size, align; >> >> g_assert(type != NULL); >> type_initialize(type); >> >> - obj = g_malloc(type->instance_size); >> - object_initialize_with_type(obj, type->instance_size, type); >> - obj->free = g_free; >> + size = type->instance_size; >> + align = type->instance_align; >> + if (align) { > > If we check for (align > G_MEM_ALIGN) instead, we will be able to > set instance_align automatically at OBJECT_DEFINE_TYPE*. I agree a value check would be good here, as well as setting this by default. As for the value check itself... I see that G_MEM_ALIGN isn't actually defined in an interesting or even correct way. E.g. it doesn't take the long double type into account. The usual mechanism is struct s { char pad; union { long l; void *p; double d; long double ld; } u; }; offsetof(s, u) since all of these types are required to be taken into account by the system malloc. E.g it doesn't take other host guarantees into account, e.g. i386-linux guarantees 16-byte alignment. This possibly dubious ABI change was made 20+ years ago with the introduction of SSE and is now set in stone. Glibc has a "malloc-alignment.h" internal header that defaults to MIN(2 * sizeof(size_t), __alignof__(long double)) and is overridden for i386. Sadly, it doesn't export MALLOC_ALIGNMENT. Musl has two different malloc implementations. One has UNIT = 16; the other has SIZE_ALIGN = 4*sizeof(size_t). Both have a minimum value of 16, and this is not target-specific. Any further comments on the subject, or should I put together something that computes the MAX of the above? r~
On Tue, Sep 15, 2020 at 12:09:59PM -0700, Richard Henderson wrote: > On 9/15/20 11:07 AM, Eduardo Habkost wrote: > > On Tue, Sep 15, 2020 at 10:46:31AM -0700, Richard Henderson wrote: > >> It turns out that some hosts have a default malloc alignment less > >> than that required for vectors. > >> > >> We assume that, with compiler annotation on CPUArchState, that we > >> can properly align the vector portion of the guest state. Fix the > >> alignment of the allocation by using qemu_memalloc when required. > >> > >> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > >> --- > >> Cc: Paolo Bonzini <pbonzini@redhat.com> > >> Cc: "Daniel P. Berrangé" <berrange@redhat.com> > >> Cc: Eduardo Habkost <ehabkost@redhat.com> > >> --- > >> include/qom/object.h | 4 ++++ > >> qom/object.c | 16 +++++++++++++--- > >> 2 files changed, 17 insertions(+), 3 deletions(-) > >> > >> diff --git a/include/qom/object.h b/include/qom/object.h > >> index 056f67ab3b..d52d0781a3 100644 > >> --- a/include/qom/object.h > >> +++ b/include/qom/object.h > >> @@ -770,6 +770,9 @@ struct Object > >> * @instance_size: The size of the object (derivative of #Object). If > >> * @instance_size is 0, then the size of the object will be the size of the > >> * parent object. > >> + * @instance_align: The required alignment of the object. If @instance_align > >> + * is 0, then normal malloc alignment is sufficient; if non-zero, then we > >> + * must use qemu_memalign for allocation. > >> * @instance_init: This function is called to initialize an object. The parent > >> * class will have already been initialized so the type is only responsible > >> * for initializing its own members. > >> @@ -807,6 +810,7 @@ struct TypeInfo > >> const char *parent; > >> > >> size_t instance_size; > >> + size_t instance_align; > >> void (*instance_init)(Object *obj); > >> void (*instance_post_init)(Object *obj); > >> void (*instance_finalize)(Object *obj); > >> diff --git a/qom/object.c b/qom/object.c > >> index 387efb25eb..2e53cb44a6 100644 > >> --- a/qom/object.c > >> +++ b/qom/object.c > >> @@ -50,6 +50,7 @@ struct TypeImpl > >> size_t class_size; > >> > >> size_t instance_size; > >> + size_t instance_align; > >> > >> void (*class_init)(ObjectClass *klass, void *data); > >> void (*class_base_init)(ObjectClass *klass, void *data); > >> @@ -114,6 +115,7 @@ static TypeImpl *type_new(const TypeInfo *info) > >> > >> ti->class_size = info->class_size; > >> ti->instance_size = info->instance_size; > >> + ti->instance_align = info->instance_align; > >> > >> ti->class_init = info->class_init; > >> ti->class_base_init = info->class_base_init; > >> @@ -691,13 +693,21 @@ static void object_finalize(void *data) > >> static Object *object_new_with_type(Type type) > >> { > >> Object *obj; > >> + size_t size, align; > >> > >> g_assert(type != NULL); > >> type_initialize(type); > >> > >> - obj = g_malloc(type->instance_size); > >> - object_initialize_with_type(obj, type->instance_size, type); > >> - obj->free = g_free; > >> + size = type->instance_size; > >> + align = type->instance_align; > >> + if (align) { > > > > If we check for (align > G_MEM_ALIGN) instead, we will be able to > > set instance_align automatically at OBJECT_DEFINE_TYPE*. > > I agree a value check would be good here, as well as setting this by default. > > As for the value check itself... > > I see that G_MEM_ALIGN isn't actually defined in an interesting or even correct > way. E.g. it doesn't take the long double type into account. > > The usual mechanism is > > struct s { > char pad; > union { > long l; > void *p; > double d; > long double ld; > } u; > }; > > offsetof(s, u) > > since all of these types are required to be taken into account by the system > malloc. Due to the existence of G_MEM_ALIGN, I thought GLib alignment guarantees could be weaker than malloc(). I've just learned that GLib uses system malloc() since 2.46. > > E.g it doesn't take other host guarantees into account, e.g. i386-linux > guarantees 16-byte alignment. This possibly dubious ABI change was made 20+ > years ago with the introduction of SSE and is now set in stone. > > Glibc has a "malloc-alignment.h" internal header that defaults to > > MIN(2 * sizeof(size_t), __alignof__(long double)) > > and is overridden for i386. Sadly, it doesn't export MALLOC_ALIGNMENT. > > Musl has two different malloc implementations. One has UNIT = 16; the other > has SIZE_ALIGN = 4*sizeof(size_t). Both have a minimum value of 16, and this > is not target-specific. > > Any further comments on the subject, or should I put together something that > computes the MAX of the above? Once we move to C11, we can just use max_align_t. While we don't move to C11, why not just use __alignof__(union { long l; void *p; double d; long double ld;}) ? -- Eduardo
On 9/15/20 1:19 PM, Eduardo Habkost wrote: > Once we move to C11, we can just use max_align_t. Yes. > While we don't move to C11, why not just use > __alignof__(union { long l; void *p; double d; long double ld;}) > ? For i386, this is 4. r~
On Tue, Sep 15, 2020 at 01:51:48PM -0700, Richard Henderson wrote: > On 9/15/20 1:19 PM, Eduardo Habkost wrote: > > Once we move to C11, we can just use max_align_t. > > Yes. > > > While we don't move to C11, why not just use > > __alignof__(union { long l; void *p; double d; long double ld;}) > > ? > > For i386, this is 4. Is i386-linux the only case where there are additional alignment guarantees not covered by C99? I would prefer a i386-linux-specific #ifdef for that case instead of guessing based on undocumented libc internals. -- Eduardo
On 9/15/20 2:27 PM, Eduardo Habkost wrote: > On Tue, Sep 15, 2020 at 01:51:48PM -0700, Richard Henderson wrote: >> On 9/15/20 1:19 PM, Eduardo Habkost wrote: >>> Once we move to C11, we can just use max_align_t. >> >> Yes. >> >>> While we don't move to C11, why not just use >>> __alignof__(union { long l; void *p; double d; long double ld;}) >>> ? >> >> For i386, this is 4. > > Is i386-linux the only case where there are additional alignment > guarantees not covered by C99? I think so. > I would prefer a i386-linux-specific #ifdef for that case instead > of guessing based on undocumented libc internals. I was thinking abi, not internals. r~
On Tue, Sep 15, 2020 at 02:30:34PM -0700, Richard Henderson wrote: > On 9/15/20 2:27 PM, Eduardo Habkost wrote: > > On Tue, Sep 15, 2020 at 01:51:48PM -0700, Richard Henderson wrote: > >> On 9/15/20 1:19 PM, Eduardo Habkost wrote: > >>> Once we move to C11, we can just use max_align_t. > >> > >> Yes. > >> > >>> While we don't move to C11, why not just use > >>> __alignof__(union { long l; void *p; double d; long double ld;}) > >>> ? > >> > >> For i386, this is 4. > > > > Is i386-linux the only case where there are additional alignment > > guarantees not covered by C99? > > I think so. > > > I would prefer a i386-linux-specific #ifdef for that case instead > > of guessing based on undocumented libc internals. > > I was thinking abi, not internals. I see. As long as we can point to the specification backing the assumptions in the code, I'm OK with that. To me, it seems simpler to start with something that works on all hosts (the alignment guarantees provided by C99), and add arch-specific optimizations later. -- Eduardo
diff --git a/include/qom/object.h b/include/qom/object.h index 056f67ab3b..d52d0781a3 100644 --- a/include/qom/object.h +++ b/include/qom/object.h @@ -770,6 +770,9 @@ struct Object * @instance_size: The size of the object (derivative of #Object). If * @instance_size is 0, then the size of the object will be the size of the * parent object. + * @instance_align: The required alignment of the object. If @instance_align + * is 0, then normal malloc alignment is sufficient; if non-zero, then we + * must use qemu_memalign for allocation. * @instance_init: This function is called to initialize an object. The parent * class will have already been initialized so the type is only responsible * for initializing its own members. @@ -807,6 +810,7 @@ struct TypeInfo const char *parent; size_t instance_size; + size_t instance_align; void (*instance_init)(Object *obj); void (*instance_post_init)(Object *obj); void (*instance_finalize)(Object *obj); diff --git a/qom/object.c b/qom/object.c index 387efb25eb..2e53cb44a6 100644 --- a/qom/object.c +++ b/qom/object.c @@ -50,6 +50,7 @@ struct TypeImpl size_t class_size; size_t instance_size; + size_t instance_align; void (*class_init)(ObjectClass *klass, void *data); void (*class_base_init)(ObjectClass *klass, void *data); @@ -114,6 +115,7 @@ static TypeImpl *type_new(const TypeInfo *info) ti->class_size = info->class_size; ti->instance_size = info->instance_size; + ti->instance_align = info->instance_align; ti->class_init = info->class_init; ti->class_base_init = info->class_base_init; @@ -691,13 +693,21 @@ static void object_finalize(void *data) static Object *object_new_with_type(Type type) { Object *obj; + size_t size, align; g_assert(type != NULL); type_initialize(type); - obj = g_malloc(type->instance_size); - object_initialize_with_type(obj, type->instance_size, type); - obj->free = g_free; + size = type->instance_size; + align = type->instance_align; + if (align) { + obj = qemu_memalign(align, size); + } else { + obj = g_malloc(size); + } + + object_initialize_with_type(obj, size, type); + obj->free = (align ? qemu_vfree : g_free); return obj; }
It turns out that some hosts have a default malloc alignment less than that required for vectors. We assume that, with compiler annotation on CPUArchState, that we can properly align the vector portion of the guest state. Fix the alignment of the allocation by using qemu_memalloc when required. Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- Cc: Paolo Bonzini <pbonzini@redhat.com> Cc: "Daniel P. Berrangé" <berrange@redhat.com> Cc: Eduardo Habkost <ehabkost@redhat.com> --- include/qom/object.h | 4 ++++ qom/object.c | 16 +++++++++++++--- 2 files changed, 17 insertions(+), 3 deletions(-)