On Fri, 19 Jun 2015 17:58:57 +0200
Olivier MATZ <olivier.matz at 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 at 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 at 6wind.com>
>
>
