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

[Olivier MATZ <olivier.matz@6wind.com>]

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?

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>