Re: SPAM? Re: [lwip-users] about alignment issues.

[Pedro Alves]

Jan Ulvesten wrote:

(converted top post to bottom post to keep context)

> Derek Guerdon wrote:
>  
>
> > On Fri, 14 Apr 2006 09:30:46 -0700, Curt McDowell wrote:
> >
> >    
> >
> > > Still, a union is not called for:
> > >
> > > struct {
> > > struct pbuf pbuf;
> > > u64_t payload[MEM_ALIGN_SIZE(PBUF_POOL_BUFSIZE) / sizeof(u64_t)];
> > > } pbuf_pool_memory[PBUF_POOL_SIZE];
> > >
> > > Or, if it's more pleasing, we could define a "maximal" type somewhere
> > > and use that for payload[].  "struct mem" does that in mem.c, except
> > >      
> it
>  
>
> > > should be defined in a header file to clean it up.
> > >   
> > >
> > >      
> > I think the latter is preferable for three reasons. First, even u64_t
> > isn't required to be maximally aligned under ANSI-C (although in
> > practice I've never seen a case where it wasn't). Second, there are
> > some compilers without native unsigned 64-bit integer. Third,
> > allocating an array of 'memory aligned types' allows targets with
> > limited memory to avoid allocating unused memory.
> >
> > The declaration needs to be reworked a bit as the one given won't
> > allocate enough space when MEM_ALIGNMENT is less than 8 (e.g. if
> > MEM_ALIGNMENT is 4 and PBUF_POOL_SIZE is 1540, only 1536 bytes are
> > allocated in payload).
> >
> > Something along these lines should work:
> > MAX_ALIGN_T
> > payload[(PBUF_POOL_BUFSIZE+sizeof(MAX_ALIGN_T)-1)/sizeof(MAX_ALIGN_T)];
> >
> >
> > I'd like to offer my thanks to you for addressing these issues. I
> > think they consumed the majority of the time I spent porting lwIP.
> > Getting the alignment issues fixed will be a major improvement to the
> > code base.
> >
> >
> >    
>
> Although I don't belong to the "unions are ugly school", I'm ok with a 
> solution like this.
> Actually, I think that flattening the pool some more would be even
> better.
>
> static struct pbuf mem[PBUF_POOL_SIZE];
> typedef u32_t MAX_ALIGN_T;
> typedef MAX_ALIGN_T 
> pbuf_pool_payload_t[(PBUF_POOL_BUFSIZE+sizeof(MAX_ALIGN_T)-1)/sizeof(MAX
> _ALIGN_T)];
> pbuf_pool_payload_t pbuf_pool_payload[PBUF_POOL_SIZE];
>
> void
> pbuf_init_dummy_just_to_show(void)
> {
>  int i;
>  for(i = 0; i < PBUF_POOL_SIZE; ++i) {
>    mem[i].next = mem[i+1].next;
>    mem[i].len = mem[i].tot_len = sizeof(pbuf_pool_payload_t);
>    mem[i].payload = pbuf_pool_payload[i];
>    mem[i].flags = PBUF_FLAG_POOL;
>  }
> }
>  
> From Alteras lwIP port:

> static u8_t ip_reassbuf[IP_HLEN + IP_REASS_BUFSIZE] __attribute__
> ((aligned (MEM_ALIGNMENT)));
>
> Memory alignment can be forced on specific variables, at least for GNU
> compilers. I guess other compilers have a similar syntax?
>
> Why not use that like below?
>
> #define MEM_ALIGNMENT_DIRECTIVE  __attribute__ ((aligned
> (MEM_ALIGNMENT)));
> static u8_t ip_reassbuf[IP_HLEN + IP_REASS_BUFSIZE]
> MEM_ALIGNMENT_DIRECTIVE;
>
> MEM _ALIGNMENT_DIRECTIVE can be blank if not supported.
The problem with this is that when the directive is blank, the will not have 
enough space if the array ends up unaligned.
Do we have a list of supported compilers?

If we go the LWIP_MAX_ALIGN_T route, I suggest to have something like thisin 
some lwip global include, maybe in opt.h:

#ifndef LWIP_MAX_ALIGN_T

#if MEM_ALIGNMENT == 1
typedef u8_t lwip_align_t;
#elif MEM_ALIGNMENT == 2
typedef u16_t lwip_align_t;
#elif MEM_ALIGNMENT == 4
typedef u32_t lwip_align_t;
#elif MEM_ALIGNMENT == 8
typedef u64_t lwip_align_t;
#endif

struct lwip_alignof_test_t
{
        char dummy;
        lwip_align_t x;
};

#if offsetof(lwip_alignof_test_t, x) != MEM_ALIGNMENT
# error must define MEM_ALIGNMENT
#else
# define LWIP_MAX_ALIGN_T lwip_align_t
#endif

#endif

This way, most users won't need to declare LWIP_MAX_ALIGN_T, and all will stay 
as it is today.

...

Humm, how about making a big union with all the native types and use that as 
LWIP_MAX_ALIGN_T?

#ifndef LWIP_MAX_ALIGN_T
typedef union
{
        u8_t dum1;
        u8_t dum2;
        u16_t dum3;
        s16_t dum4;
        u32_t dum5;
        s32_t dum6;
        void (*ptrfun)(void);
        (etc...)
} lwip_align_t;
# define LWIP_MAX_ALIGN_T lwip_align_t
#endif

What do you think?

Cheers,
Pedro Alves