Re: [dpdk-dev] [PATCH 01/10] eal: add and use unaligned integer types

[Cyril Chemparathy <cchemparathy@ezchip.com>]

On Fri, 19 Jun 2015 17:58:57 +0200
Olivier MATZ <olivier.matz@6wind.com> wrote:

> Hello Cyril,
> 
> First, sorry commenting that late. My first intent was to benchmark
> with your modifications, but I did not find the time for it.
> 
> So, please find some comments on your series below so it can be pushed
> for 2.1.
> 
> On 04/29/2015 06:15 PM, Cyril Chemparathy wrote:
> > On machines that are strict on pointer alignment, current code
> > breaks on GCC's -Wcast-align checks on casts from narrower to wider
> > types.  This patch introduces new unaligned_uint(16|32|64)_t types,
> > which correctly retain alignment in such cases.
> > 
> > This is currently unimplemented on ICC and clang, and equivalents
> > will need to be plugged in.
> > 
> > Signed-off-by: Cyril Chemparathy <cchemparathy@ezchip.com>
> > ---
> >  app/test-pmd/flowgen.c                     |  4 ++--
> >  app/test-pmd/txonly.c                      |  2 +-
> >  app/test/packet_burst_generator.c          |  4 ++--
> >  app/test/test_mbuf.c                       | 16 ++++++++--------
> >  lib/librte_eal/common/include/rte_common.h | 10 ++++++++++
> >  lib/librte_ether/rte_ether.h               |  2 +-
> >  lib/librte_ip_frag/rte_ipv4_reassembly.c   |  4 ++--
> >  7 files changed, 26 insertions(+), 16 deletions(-)
> > 
> > [...]
> > diff --git a/lib/librte_eal/common/include/rte_common.h
> > b/lib/librte_eal/common/include/rte_common.h index c0ab8b4..3bb97d1
> > 100644 --- a/lib/librte_eal/common/include/rte_common.h
> > +++ b/lib/librte_eal/common/include/rte_common.h
> > @@ -61,6 +61,16 @@ extern "C" {
> >  
> >  /*********** Macros to eliminate unused variable warnings ********/
> >  
> > +#if defined(__GNUC__)
> > +typedef uint64_t unaligned_uint64_t __attribute__ ((aligned(1)));
> > +typedef uint32_t unaligned_uint32_t __attribute__ ((aligned(1)));
> > +typedef uint16_t unaligned_uint16_t __attribute__ ((aligned(1)));
> > +#else
> > +typedef uint64_t unaligned_uint64_t;
> > +typedef uint32_t unaligned_uint32_t;
> > +typedef uint16_t unaligned_uint16_t;
> > +#endif
> > +
> 
> Shouldn't we only define these unaligned types for architectures that
> have strict alignment constraints ? I am a bit puzzled by only
> defining it when compiling with gcc: is it because only gcc triggers
> a warning? If yes, I'm not sure it's a good reason.
> 
> Maybe we could have a compile-time option to enable these types, and
> this option would be set for architectures that require it only. The
> advantage would be to ensure there's no modification on x86.
> 
> What do you think?
> 

Fair enough.  I like that better than keying off of GCC or anything
like that.  I will change this patch accordingly for the next revision.

> cosmetic: the definitions should go above the comment line that refers
> to the macro below.
> 
> For the rest of the series (except a small comment on patch 8/10,
> see the other mail):
> Acked-by: Olivier Matz <olivier.matz@6wind.com>
> 
>