[Mesa-dev] [PATCH 2/2] util/ralloc: Make sizeof(linear_header) a multiple of 8

[Gustaw Smolarczyk]

Wt., 13 lis 2018, 06:03: Matt Turner <mattst88 at gmail.com> napisał(a):

> On Mon, Nov 12, 2018 at 3:07 PM Eric Anholt <eric at anholt.net> wrote:
> >
> > Matt Turner <mattst88 at gmail.com> writes:
> >
> > > Prior to this patch sizeof(linear_header) was 20 bytes in a
> > > non-debug build on 32-bit platforms. We do some pointer arithmetic to
> > > calculate the next available location with
> > >
> > >    ptr = (linear_size_chunk *)((char *)&latest[1] + latest->offset);
> > >
> > > in linear_alloc_child(). The &latest[1] adds 20 bytes, so an allocation
> > > would only be 4-byte aligned.
> > >
> > > On 32-bit SPARC a 'sttw' instruction (which stores a consecutive pair
> of
> > > 4-byte registers to memory) requires an 8-byte aligned address. Such an
> > > instruction is used to store to an 8-byte integer type, like intmax_t
> > > which is used in glcpp's expression_value_t struct.
> > >
> > > As a result of the 4-byte alignment returned by linear_alloc_child() we
> > > would generate a SIGBUS (unaligned exception) on SPARC.
> > >
> > > According to the GNU libc manual malloc() always returns memory that
> has
> > > at least an alignment of 8-bytes [1]. I think our allocator should do
> > > the same.
> > >
> > > So, simple fix with two parts:
> > >
> > >    (1) Increase SUBALLOC_ALIGNMENT to 8 unconditionally.
> > >    (2) Mark linear_header with an aligned attribute, which will cause
> > >        its sizeof to be rounded up to that alignment. (We already do
> > >        this for ralloc_header)
> > >
> > > With this done, all Mesa's unit tests now pass on SPARC.
> > >
> > > [1]
> https://www.gnu.org/software/libc/manual/html_node/Aligned-Memory-Blocks.html
> > >
> > > Fixes: 47e17586924f ("glcpp: use the linear allocator for most
> objects")
> > > Bug: https://bugs.gentoo.org/636326
> > > ---
> > >  src/util/ralloc.c | 14 ++++++++++++--
> > >  1 file changed, 12 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/src/util/ralloc.c b/src/util/ralloc.c
> > > index 745b4cf1226..fc35661996d 100644
> > > --- a/src/util/ralloc.c
> > > +++ b/src/util/ralloc.c
> > > @@ -552,10 +552,18 @@ ralloc_vasprintf_rewrite_tail(char **str, size_t
> *start, const char *fmt,
> > >   */
> > >
> > >  #define MIN_LINEAR_BUFSIZE 2048
> > > -#define SUBALLOC_ALIGNMENT sizeof(uintptr_t)
> > > +#define SUBALLOC_ALIGNMENT 8
> > >  #define LMAGIC 0x87b9c7d3
> > >
> > > -struct linear_header {
> > > +struct
> > > +#ifdef _MSC_VER
> > > + __declspec(align(8))
> > > +#elif defined(__LP64__)
> > > + __attribute__((aligned(16)))
> > > +#else
> > > + __attribute__((aligned(8)))
> > > +#endif
> > > +   linear_header {
> > >  #ifndef NDEBUG
> > >     unsigned magic;   /* for debugging */
> > >  #endif
> > > @@ -647,6 +655,8 @@ linear_alloc_child(void *parent, unsigned size)
> > >     ptr = (linear_size_chunk *)((char*)&latest[1] + latest->offset);
> > >     ptr->size = size;
> > >     latest->offset += full_size;
> > > +
> > > +   assert((uintptr_t)&ptr[1] % SUBALLOC_ALIGNMENT == 0);
> > >     return &ptr[1];
> > >  }
> >
> > These patches are:
> >
> > Reviewed-by: Eric Anholt <eric at anholt.net>
>
> Thanks a bunch! I hope this is useful for you on arm as well.
>
> > However, shouldn't we also bump SUBALLOC_ALIGNMENT to 16 on LP64, too,
> > if that's what glibc is doing for malloc?
>
> 16-byte alignment is necessary for SSE aligned vector load/store
> instructions. I suppose we're not getting any vectorized SSE
> load/store instructions to memory allocated by linear_alloc_* and
> that's why we haven't seen problems?
>

FWIW, at least clang on x86 assumes malloc/new return pointers aligned to
16 bytes, though it probably doesn't detect linear_alloc_* as such.

Regards,
Gustaw Smolarczyk


> Seems reasonable to bump it to 16-bytes.
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20181113/4f45dd2f/attachment-0001.html>