diff mbox

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

Message ID 1430324134-25654-2-git-send-email-cchemparathy@ezchip.com (mailing list archive)
State Superseded, archived
Headers

Commit Message

Cyril Chemparathy April 29, 2015, 4:15 p.m. UTC 
  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(-)

Comments

Olivier Matz June 19, 2015, 3:58 p.m. UTC | #1 
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>
Cyril Chemparathy June 19, 2015, 5:18 p.m. UTC | #2 
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>
> 
>
diff mbox

Patch

diff --git a/app/test-pmd/flowgen.c b/app/test-pmd/flowgen.c
index 72016c9..174c003 100644
--- a/app/test-pmd/flowgen.c
+++ b/app/test-pmd/flowgen.c
@@ -101,7 +101,7 @@  tx_mbuf_alloc(struct rte_mempool *mp)
 
 
 static inline uint16_t
-ip_sum(const uint16_t *hdr, int hdr_len)
+ip_sum(const unaligned_uint16_t *hdr, int hdr_len)
 {
 	uint32_t sum = 0;
 
@@ -193,7 +193,7 @@  pkt_burst_flow_gen(struct fwd_stream *fs)
 							   next_flow);
 		ip_hdr->total_length	= RTE_CPU_TO_BE_16(pkt_size -
 							   sizeof(*eth_hdr));
-		ip_hdr->hdr_checksum	= ip_sum((uint16_t *)ip_hdr,
+		ip_hdr->hdr_checksum	= ip_sum((unaligned_uint16_t *)ip_hdr,
 						 sizeof(*ip_hdr));
 
 		/* Initialize UDP header. */
diff --git a/app/test-pmd/txonly.c b/app/test-pmd/txonly.c
index ca32c85..9e66552 100644
--- a/app/test-pmd/txonly.c
+++ b/app/test-pmd/txonly.c
@@ -167,7 +167,7 @@  setup_pkt_udp_ip_headers(struct ipv4_hdr *ip_hdr,
 	/*
 	 * Compute IP header checksum.
 	 */
-	ptr16 = (uint16_t*) ip_hdr;
+	ptr16 = (unaligned_uint16_t*) ip_hdr;
 	ip_cksum = 0;
 	ip_cksum += ptr16[0]; ip_cksum += ptr16[1];
 	ip_cksum += ptr16[2]; ip_cksum += ptr16[3];
diff --git a/app/test/packet_burst_generator.c b/app/test/packet_burst_generator.c
index b46eed7..06762c6 100644
--- a/app/test/packet_burst_generator.c
+++ b/app/test/packet_burst_generator.c
@@ -154,7 +154,7 @@  initialize_ipv4_header(struct ipv4_hdr *ip_hdr, uint32_t src_addr,
 		uint32_t dst_addr, uint16_t pkt_data_len)
 {
 	uint16_t pkt_len;
-	uint16_t *ptr16;
+	unaligned_uint16_t *ptr16;
 	uint32_t ip_cksum;
 
 	/*
@@ -175,7 +175,7 @@  initialize_ipv4_header(struct ipv4_hdr *ip_hdr, uint32_t src_addr,
 	/*
 	 * Compute IP header checksum.
 	 */
-	ptr16 = (uint16_t *)ip_hdr;
+	ptr16 = (unaligned_uint16_t *)ip_hdr;
 	ip_cksum = 0;
 	ip_cksum += ptr16[0]; ip_cksum += ptr16[1];
 	ip_cksum += ptr16[2]; ip_cksum += ptr16[3];
diff --git a/app/test/test_mbuf.c b/app/test/test_mbuf.c
index 5e8a377..d9beb29 100644
--- a/app/test/test_mbuf.c
+++ b/app/test/test_mbuf.c
@@ -333,7 +333,7 @@  testclone_testupdate_testdetach(void)
 	struct rte_mbuf *m = NULL;
 	struct rte_mbuf *clone = NULL;
 	struct rte_mbuf *clone2 = NULL;
-	uint32_t *data;
+	unaligned_uint32_t *data;
 
 	/* alloc a mbuf */
 	m = rte_pktmbuf_alloc(pktmbuf_pool);
@@ -344,7 +344,7 @@  testclone_testupdate_testdetach(void)
 		GOTO_FAIL("Bad length");
 
 	rte_pktmbuf_append(m, sizeof(uint32_t));
-	data = rte_pktmbuf_mtod(m, uint32_t *);
+	data = rte_pktmbuf_mtod(m, unaligned_uint32_t *);
 	*data = MAGIC_DATA;
 
 	/* clone the allocated mbuf */
@@ -352,7 +352,7 @@  testclone_testupdate_testdetach(void)
 	if (clone == NULL)
 		GOTO_FAIL("cannot clone data\n");
 
-	data = rte_pktmbuf_mtod(clone, uint32_t *);
+	data = rte_pktmbuf_mtod(clone, unaligned_uint32_t *);
 	if (*data != MAGIC_DATA)
 		GOTO_FAIL("invalid data in clone\n");
 
@@ -369,18 +369,18 @@  testclone_testupdate_testdetach(void)
 		GOTO_FAIL("Next Pkt Null\n");
 
 	rte_pktmbuf_append(m->next, sizeof(uint32_t));
-	data = rte_pktmbuf_mtod(m->next, uint32_t *);
+	data = rte_pktmbuf_mtod(m->next, unaligned_uint32_t *);
 	*data = MAGIC_DATA;
 
 	clone = rte_pktmbuf_clone(m, pktmbuf_pool);
 	if (clone == NULL)
 		GOTO_FAIL("cannot clone data\n");
 
-	data = rte_pktmbuf_mtod(clone, uint32_t *);
+	data = rte_pktmbuf_mtod(clone, unaligned_uint32_t *);
 	if (*data != MAGIC_DATA)
 		GOTO_FAIL("invalid data in clone\n");
 
-	data = rte_pktmbuf_mtod(clone->next, uint32_t *);
+	data = rte_pktmbuf_mtod(clone->next, unaligned_uint32_t *);
 	if (*data != MAGIC_DATA)
 		GOTO_FAIL("invalid data in clone->next\n");
 
@@ -396,11 +396,11 @@  testclone_testupdate_testdetach(void)
 	if (clone2 == NULL)
 		GOTO_FAIL("cannot clone the clone\n");
 
-	data = rte_pktmbuf_mtod(clone2, uint32_t *);
+	data = rte_pktmbuf_mtod(clone2, unaligned_uint32_t *);
 	if (*data != MAGIC_DATA)
 		GOTO_FAIL("invalid data in clone2\n");
 
-	data = rte_pktmbuf_mtod(clone2->next, uint32_t *);
+	data = rte_pktmbuf_mtod(clone2->next, unaligned_uint32_t *);
 	if (*data != MAGIC_DATA)
 		GOTO_FAIL("invalid data in clone2->next\n");
 
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
+
 /**
  * short definition to mark a function parameter unused
  */
diff --git a/lib/librte_ether/rte_ether.h b/lib/librte_ether/rte_ether.h
index 49f4576..da25cb3 100644
--- a/lib/librte_ether/rte_ether.h
+++ b/lib/librte_ether/rte_ether.h
@@ -175,7 +175,7 @@  static inline int is_multicast_ether_addr(const struct ether_addr *ea)
  */
 static inline int is_broadcast_ether_addr(const struct ether_addr *ea)
 {
-	const uint16_t *ea_words = (const uint16_t *)ea;
+	const unaligned_uint16_t *ea_words = (const unaligned_uint16_t *)ea;
 
 	return (ea_words[0] == 0xFFFF && ea_words[1] == 0xFFFF &&
 		ea_words[2] == 0xFFFF);
diff --git a/lib/librte_ip_frag/rte_ipv4_reassembly.c b/lib/librte_ip_frag/rte_ipv4_reassembly.c
index 841ac14..279be6c 100644
--- a/lib/librte_ip_frag/rte_ipv4_reassembly.c
+++ b/lib/librte_ip_frag/rte_ipv4_reassembly.c
@@ -120,7 +120,7 @@  rte_ipv4_frag_reassemble_packet(struct rte_ip_frag_tbl *tbl,
 {
 	struct ip_frag_pkt *fp;
 	struct ip_frag_key key;
-	const uint64_t *psd;
+	const unaligned_uint64_t *psd;
 	uint16_t ip_len;
 	uint16_t flag_offset, ip_ofs, ip_flag;
 
@@ -128,7 +128,7 @@  rte_ipv4_frag_reassemble_packet(struct rte_ip_frag_tbl *tbl,
 	ip_ofs = (uint16_t)(flag_offset & IPV4_HDR_OFFSET_MASK);
 	ip_flag = (uint16_t)(flag_offset & IPV4_HDR_MF_FLAG);
 
-	psd = (uint64_t *)&ip_hdr->src_addr;
+	psd = (unaligned_uint64_t *)&ip_hdr->src_addr;
 	/* use first 8 bytes only */
 	key.src_dst[0] = psd[0];
 	key.id = ip_hdr->packet_id;