[dpdk-dev] [PATCH 00/10] Improve cast alignment for strict aligned machines

[dpdk-dev] [PATCH 00/10] Improve cast alignment for strict aligned machines

[Cyril Chemparathy]

This series contains a few improvements that allow the DPDK code base to build
properly on machines that enforce strict pointer cast alignment constraints.

When dealing with packet data which could be arbitrarily aligned, we get the
compiler to do the right thing by (a) making sure that header types are
packed, and (b) introducing and using unaligned_uint(16|32|64)_t types when
upcasting from byte pointers.

In a few other instances, we know apriori that the pointer cast cannot
possibly break alignment.  This applies to the changes in mempool, hash, mbuf,
and the ethdev stats code.  Here, we simply silence the compiler by casting
through (void *) using the RTE_PTR_(ADD|SUB) macros.

Finally, we introduce a new rte_pktmbuf_mtod_offset() helper to return a type
casted pointer to an offset within the packet data.  This replaces the
following commonly used pattern:
	(struct foo *)(rte_pktmbuf_mtod(m, char *) + offset)
with:
	rte_pktmbuf_mtod_offset(m, struct foo *, offset)
To ensure consistency, the above transform was applied throughout the code
base using the coccinelle semantic patching tool.

Cyril Chemparathy (10):
  eal: add and use unaligned integer types
  mempool: silence -Wcast-align on pointer arithmetic
  hash: silence -Wcast-align on pointer arithmetic
  mbuf: silence -Wcast-align on pointer arithmetic
  ethdev: silence -Wcast-align on pointer arithmetic
  app/test-pmd: pack simple_gre_hdr
  app/test: use struct ether_addr instead of a byte array cast
  librte_mbuf: Add rte_pktmbuf_mtod_offset()
  librte_mbuf: Add transform for rte_pktmbuf_mtod_offset()
  librte_mbuf: Apply mtod-offset.cocci transform

 app/test-pmd/csumonly.c                    |  2 +-
 app/test-pmd/flowgen.c                     |  4 +-
 app/test-pmd/ieee1588fwd.c                 |  4 +-
 app/test-pmd/rxonly.c                      | 21 +++++----
 app/test-pmd/txonly.c                      |  6 +--
 app/test/packet_burst_generator.c          |  9 ++--
 app/test/test_mbuf.c                       | 16 +++----
 app/test/test_pmd_perf.c                   | 10 ++--
 examples/dpdk_qat/crypto.c                 |  8 ++--
 examples/dpdk_qat/main.c                   |  5 +-
 examples/l3fwd-acl/main.c                  | 20 ++++----
 examples/l3fwd-power/main.c                |  8 ++--
 examples/l3fwd-vf/main.c                   |  4 +-
 examples/l3fwd/main.c                      | 71 ++++++++++++----------------
 examples/load_balancer/runtime.c           |  4 +-
 examples/vhost_xen/main.c                  |  4 +-
 lib/librte_eal/common/include/rte_common.h | 10 ++++
 lib/librte_ether/rte_ethdev.c              | 24 +++++-----
 lib/librte_ether/rte_ether.h               |  2 +-
 lib/librte_hash/rte_hash.c                 | 10 ++--
 lib/librte_ip_frag/rte_ipv4_reassembly.c   |  7 ++-
 lib/librte_ip_frag/rte_ipv6_reassembly.c   |  5 +-
 lib/librte_mbuf/rte_mbuf.h                 | 28 +++++++----
 lib/librte_mempool/rte_mempool.h           |  4 +-
 lib/librte_pmd_mlx4/mlx4.c                 |  9 ++--
 lib/librte_port/rte_port_ras.c             |  3 +-
 lib/librte_vhost/vhost_rxtx.c              |  4 +-
 scripts/cocci.sh                           | 64 +++++++++++++++++++++++++
 scripts/cocci/mtod-offset.cocci            | 76 ++++++++++++++++++++++++++++++
 29 files changed, 298 insertions(+), 144 deletions(-)
 create mode 100755 scripts/cocci.sh
 create mode 100644 scripts/cocci/mtod-offset.cocci

-- 
2.1.2

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

[Cyril Chemparathy]

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/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;
-- 
2.1.2

[dpdk-dev] [PATCH 02/10] mempool: silence -Wcast-align on pointer arithmetic

[Cyril Chemparathy]

Translating from a mempool object to the mempool pointer does not break
alignment constraints.  However, the compiler is unaware of this fact and
complains on -Wcast-align.  This patch modifies the code to use RTE_PTR_SUB(),
thereby silencing the compiler by casting through (void *).

Signed-off-by: Cyril Chemparathy <cchemparathy@ezchip.com>
---
 lib/librte_mempool/rte_mempool.h | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/lib/librte_mempool/rte_mempool.h b/lib/librte_mempool/rte_mempool.h
index 9001312..1fb4184 100644
--- a/lib/librte_mempool/rte_mempool.h
+++ b/lib/librte_mempool/rte_mempool.h
@@ -236,15 +236,13 @@ struct rte_mempool {
  */
 static inline struct rte_mempool **__mempool_from_obj(void *obj)
 {
-	struct rte_mempool **mpp;
 	unsigned off;
 
 	off = sizeof(struct rte_mempool *);
 #ifdef RTE_LIBRTE_MEMPOOL_DEBUG
 	off += sizeof(uint64_t);
 #endif
-	mpp = (struct rte_mempool **)((char *)obj - off);
-	return mpp;
+	return RTE_PTR_SUB(obj, off);
 }
 
 /**
-- 
2.1.2

[dpdk-dev] [PATCH 03/10] hash: silence -Wcast-align on pointer arithmetic

[Cyril Chemparathy]

Since sig_tbl_bucket_size and key_tbl_key_size are explicitly aligned at
initialization, offset dereferences in the hash table code cannot possibly be
unaligned.  However, the compiler is unaware of this fact and complains on
-Wcast-align.  This patch modifies the code to use RTE_PTR_ADD(), thereby
silencing the compiler by casting through (void *).

Signed-off-by: Cyril Chemparathy <cchemparathy@ezchip.com>
---
 lib/librte_hash/rte_hash.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/lib/librte_hash/rte_hash.c b/lib/librte_hash/rte_hash.c
index 9245716..67dff5b 100644
--- a/lib/librte_hash/rte_hash.c
+++ b/lib/librte_hash/rte_hash.c
@@ -96,23 +96,23 @@ EAL_REGISTER_TAILQ(rte_hash_tailq)
 static inline hash_sig_t *
 get_sig_tbl_bucket(const struct rte_hash *h, uint32_t bucket_index)
 {
-	return (hash_sig_t *)
-			&(h->sig_tbl[bucket_index * h->sig_tbl_bucket_size]);
+	return RTE_PTR_ADD(h->sig_tbl, (bucket_index *
+					h->sig_tbl_bucket_size));
 }
 
 /* Returns a pointer to the first key in specified bucket. */
 static inline uint8_t *
 get_key_tbl_bucket(const struct rte_hash *h, uint32_t bucket_index)
 {
-	return (uint8_t *) &(h->key_tbl[bucket_index * h->bucket_entries *
-				     h->key_tbl_key_size]);
+	return RTE_PTR_ADD(h->key_tbl, (bucket_index * h->bucket_entries *
+					h->key_tbl_key_size));
 }
 
 /* Returns a pointer to a key at a specific position in a specified bucket. */
 static inline void *
 get_key_from_bucket(const struct rte_hash *h, uint8_t *bkt, uint32_t pos)
 {
-	return (void *) &bkt[pos * h->key_tbl_key_size];
+	return RTE_PTR_ADD(bkt, pos * h->key_tbl_key_size);
 }
 
 /* Does integer division with rounding-up of result. */
-- 
2.1.2

[dpdk-dev] [PATCH 04/10] mbuf: silence -Wcast-align on pointer arithmetic

[Cyril Chemparathy]

Translating from an mbuf element to the mbuf pointer does not break alignment
constraints.  However, the compiler is unaware of this fact and complains on
-Wcast-align.  This patch modifies the code to use RTE_PTR_SUB(), thereby
silencing the compiler by casting through (void *).

Signed-off-by: Cyril Chemparathy <cchemparathy@ezchip.com>
---
 lib/librte_mbuf/rte_mbuf.h | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index 70b0987..5ddd8dd 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -337,13 +337,7 @@ static inline uint16_t rte_pktmbuf_priv_size(struct rte_mempool *mp);
 static inline struct rte_mbuf *
 rte_mbuf_from_indirect(struct rte_mbuf *mi)
 {
-	struct rte_mbuf *md;
-
-	/* mi->buf_addr and mi->priv_size correspond to buffer and
-	 * private size of the direct mbuf */
-	md = (struct rte_mbuf *)((char *)mi->buf_addr - sizeof(*mi) -
-		mi->priv_size);
-	return md;
+	return RTE_PTR_SUB(mi->buf_addr, sizeof(*mi) + mi->priv_size);
 }
 
 /**
-- 
2.1.2

[dpdk-dev] [PATCH 05/10] ethdev: silence -Wcast-align on pointer arithmetic

[Cyril Chemparathy]

Statistics offsets in the rte_stats_strings[] array are always 64-bit aligned.
However, the compiler is unaware of this fact and complains on -Wcast-align.
This patch modifies the code to use RTE_PTR_ADD(), thereby silencing the
compiler by casting through (void *).

Signed-off-by: Cyril Chemparathy <cchemparathy@ezchip.com>
---
 lib/librte_ether/rte_ethdev.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index 024fe8b..d91068b 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -1742,8 +1742,7 @@ rte_eth_xstats_get(uint8_t port_id, struct rte_eth_xstats *xstats,
 	struct rte_eth_stats eth_stats;
 	struct rte_eth_dev *dev;
 	unsigned count, i, q;
-	uint64_t val;
-	char *stats_ptr;
+	uint64_t val, *stats_ptr;
 
 	if (!rte_eth_dev_is_valid_port(port_id)) {
 		PMD_DEBUG_TRACE("Invalid port_id=%d\n", port_id);
@@ -1771,8 +1770,9 @@ rte_eth_xstats_get(uint8_t port_id, struct rte_eth_xstats *xstats,
 
 	/* global stats */
 	for (i = 0; i < RTE_NB_STATS; i++) {
-		stats_ptr = (char *)&eth_stats + rte_stats_strings[i].offset;
-		val = *(uint64_t *)stats_ptr;
+		stats_ptr = RTE_PTR_ADD(&eth_stats,
+					rte_stats_strings[i].offset);
+		val = *stats_ptr;
 		snprintf(xstats[count].name, sizeof(xstats[count].name),
 			"%s", rte_stats_strings[i].name);
 		xstats[count++].value = val;
@@ -1781,10 +1781,10 @@ rte_eth_xstats_get(uint8_t port_id, struct rte_eth_xstats *xstats,
 	/* per-rxq stats */
 	for (q = 0; q < dev->data->nb_rx_queues; q++) {
 		for (i = 0; i < RTE_NB_RXQ_STATS; i++) {
-			stats_ptr = (char *)&eth_stats;
-			stats_ptr += rte_rxq_stats_strings[i].offset;
-			stats_ptr += q * sizeof(uint64_t);
-			val = *(uint64_t *)stats_ptr;
+			stats_ptr = RTE_PTR_ADD(&eth_stats,
+					rte_rxq_stats_strings[i].offset +
+					q * sizeof(uint64_t));
+			val = *stats_ptr;
 			snprintf(xstats[count].name, sizeof(xstats[count].name),
 				"rx_queue_%u_%s", q,
 				rte_rxq_stats_strings[i].name);
@@ -1795,10 +1795,10 @@ rte_eth_xstats_get(uint8_t port_id, struct rte_eth_xstats *xstats,
 	/* per-txq stats */
 	for (q = 0; q < dev->data->nb_tx_queues; q++) {
 		for (i = 0; i < RTE_NB_TXQ_STATS; i++) {
-			stats_ptr = (char *)&eth_stats;
-			stats_ptr += rte_txq_stats_strings[i].offset;
-			stats_ptr += q * sizeof(uint64_t);
-			val = *(uint64_t *)stats_ptr;
+			stats_ptr = RTE_PTR_ADD(&eth_stats,
+					rte_txq_stats_strings[i].offset +
+					q * sizeof(uint64_t));
+			val = *stats_ptr;
 			snprintf(xstats[count].name, sizeof(xstats[count].name),
 				"tx_queue_%u_%s", q,
 				rte_txq_stats_strings[i].name);
-- 
2.1.2

[dpdk-dev] [PATCH 06/10] app/test-pmd: pack simple_gre_hdr

[Cyril Chemparathy]

Not packing this causes -Wcast-align breakage on machines that are strict on
alignment.  This patch fixes this bug.

Signed-off-by: Cyril Chemparathy <cchemparathy@ezchip.com>
---
 app/test-pmd/csumonly.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c
index c180ff2..eadfd2b 100644
--- a/app/test-pmd/csumonly.c
+++ b/app/test-pmd/csumonly.c
@@ -108,7 +108,7 @@ struct testpmd_offload_info {
 struct simple_gre_hdr {
 	uint16_t flags;
 	uint16_t proto;
-};
+} __attribute((packed));
 
 static uint16_t
 get_psd_sum(void *l3_hdr, uint16_t ethertype, uint64_t ol_flags)
-- 
2.1.2

[dpdk-dev] [PATCH 07/10] app/test: use struct ether_addr instead of a byte array cast

[Cyril Chemparathy]

This patch instantiates struct ether_addr instead of type casting a uint8_t
pointer to the same structure.  The type cast breaks on machines that enforce
strict alignment.

Signed-off-by: Cyril Chemparathy <cchemparathy@ezchip.com>
---
 app/test/test_pmd_perf.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/app/test/test_pmd_perf.c b/app/test/test_pmd_perf.c
index 49a494d..5810e7f 100644
--- a/app/test/test_pmd_perf.c
+++ b/app/test/test_pmd_perf.c
@@ -229,13 +229,13 @@ init_traffic(struct rte_mempool *mp,
 	struct ipv4_hdr pkt_ipv4_hdr;
 	struct udp_hdr pkt_udp_hdr;
 	uint32_t pktlen;
-	static uint8_t src_mac[] = { 0x00, 0xFF, 0xAA, 0xFF, 0xAA, 0xFF };
-	static uint8_t dst_mac[] = { 0x00, 0xAA, 0xFF, 0xAA, 0xFF, 0xAA };
-
+	static struct ether_addr src_mac =
+		{ { 0x00, 0xFF, 0xAA, 0xFF, 0xAA, 0xFF } };
+	static struct ether_addr dst_mac =
+		{ { 0x00, 0xAA, 0xFF, 0xAA, 0xFF, 0xAA } };
 
 	initialize_eth_header(&pkt_eth_hdr,
-		(struct ether_addr *)src_mac,
-		(struct ether_addr *)dst_mac, ETHER_TYPE_IPv4, 0, 0);
+		&src_mac, &dst_mac, ETHER_TYPE_IPv4, 0, 0);
 
 	pktlen = initialize_ipv4_header(&pkt_ipv4_hdr,
 					IPV4_ADDR(10, 0, 0, 1),
-- 
2.1.2

[dpdk-dev] [PATCH 08/10] librte_mbuf: Add rte_pktmbuf_mtod_offset()

[Cyril Chemparathy]

There are a number of instances in the code where rte_pktmbuf_mtod() is used
to get the mbuf data pointer, only to add an offset before casting the result
to some other header type.  This patch adds a new rte_pktmbuf_mtod_offset()
macro to eliminate these awful double cast situations.

Signed-off-by: Cyril Chemparathy <cchemparathy@ezchip.com>
---
 lib/librte_mbuf/rte_mbuf.h | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index 5ddd8dd..e8f1bf4 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -54,6 +54,7 @@
  */
 
 #include <stdint.h>
+#include <rte_common.h>
 #include <rte_mempool.h>
 #include <rte_memory.h>
 #include <rte_atomic.h>
@@ -1071,6 +1072,23 @@ static inline struct rte_mbuf *rte_pktmbuf_lastseg(struct rte_mbuf *m)
 }
 
 /**
+ * A macro that points to an offset into the data in the mbuf.
+ *
+ * The returned pointer is cast to type t. Before using this
+ * function, the user must ensure that m_headlen(m) is large enough to
+ * read its data.
+ *
+ * @param m
+ *   The packet mbuf.
+ * @param o
+ *   The offset into the mbuf data.
+ * @param t
+ *   The type to cast the result into.
+ */
+#define rte_pktmbuf_mtod_offset(m, t, o)	\
+	((t)((char *)(m)->buf_addr + (m)->data_off + (o)))
+
+/**
  * A macro that points to the start of the data in the mbuf.
  *
  * The returned pointer is cast to type t. Before using this
@@ -1082,7 +1100,7 @@ static inline struct rte_mbuf *rte_pktmbuf_lastseg(struct rte_mbuf *m)
  * @param t
  *   The type to cast the result into.
  */
-#define rte_pktmbuf_mtod(m, t) ((t)((char *)(m)->buf_addr + (m)->data_off))
+#define rte_pktmbuf_mtod(m, t) rte_pktmbuf_mtod_offset(m, t, 0)
 
 /**
  * A macro that returns the length of the packet.
-- 
2.1.2

[dpdk-dev] [PATCH 09/10] librte_mbuf: Add transform for rte_pktmbuf_mtod_offset()

[Cyril Chemparathy]

This patch adds a coccinelle (see http://coccinelle.lip6.fr/) transform to use
the newly added rte_pktmbuf_mtod_offset() helper.  In addition, we add a
simple script to apply all available transforms to a codebase.

Signed-off-by: Cyril Chemparathy <cchemparathy@ezchip.com>
---
 scripts/cocci.sh                | 64 ++++++++++++++++++++++++++++++++++
 scripts/cocci/mtod-offset.cocci | 76 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 140 insertions(+)
 create mode 100755 scripts/cocci.sh
 create mode 100644 scripts/cocci/mtod-offset.cocci

diff --git a/scripts/cocci.sh b/scripts/cocci.sh
new file mode 100755
index 0000000..3a70223
--- /dev/null
+++ b/scripts/cocci.sh
@@ -0,0 +1,64 @@
+#! /bin/sh
+
+# BSD LICENSE
+#
+# Copyright 2015 EZchip Semiconductor.
+#
+# Redistribution and use in source and binary forms, with or without
+# modification, are permitted provided that the following conditions
+# are met:
+#
+#   * Redistributions of source code must retain the above copyright
+#     notice, this list of conditions and the following disclaimer.
+#   * Redistributions in binary form must reproduce the above copyright
+#     notice, this list of conditions and the following disclaimer in
+#     the documentation and/or other materials provided with the
+#     distribution.
+#   * Neither the name of EZchip Semiconductor nor the names of its
+#     contributors may be used to endorse or promote products derived
+#     from this software without specific prior written permission.
+#
+# THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+# "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+# LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+# A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+# OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+# DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+# THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+# (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+
+# Apply coccinelle transforms.
+
+SRCTREE=$(readlink -f $(dirname $0)/..)
+COCCI=$SRCTREE/scripts/cocci
+[ -n "$SPATCH" ] || SPATCH=$(which spatch)
+
+PATCH_LIST="$@"
+[ -n "$PATCH_LIST" ] || PATCH_LIST=$(echo $COCCI/*.cocci)
+
+[ -x "$SPATCH" ] || (
+	echo "Coccinelle tools not installed."
+	exit 1
+)
+
+tmp=$(mktemp)
+
+for c in $PATCH_LIST; do
+	while true; do
+		echo -n "Applying $c..."
+		$SPATCH --sp-file $c -c --linux-spacing --very-quiet	\
+			--include-headers --preprocess			\
+			--in-place --dir $SRCTREE > $tmp
+		if [ -s $tmp ]; then
+			echo " changes applied, retrying."
+		else
+			echo " no change."
+			break;
+		fi
+	done
+done
+
+rm -f $tmp
diff --git a/scripts/cocci/mtod-offset.cocci b/scripts/cocci/mtod-offset.cocci
new file mode 100644
index 0000000..13134e9
--- /dev/null
+++ b/scripts/cocci/mtod-offset.cocci
@@ -0,0 +1,76 @@
+//
+// Replace explicit packet offset computations with rte_pktmbuf_mtod_offset().
+//
+@disable paren@
+typedef uint8_t;
+expression M, O;
+@@
+(
+- rte_pktmbuf_mtod(M, char *) + O
++ rte_pktmbuf_mtod_offset(M, char *, O)
+|
+- rte_pktmbuf_mtod(M, char *) - O
++ rte_pktmbuf_mtod_offset(M, char *, -O)
+|
+- rte_pktmbuf_mtod(M, unsigned char *) + O
++ rte_pktmbuf_mtod_offset(M, unsigned char *, O)
+|
+- rte_pktmbuf_mtod(M, unsigned char *) - O
++ rte_pktmbuf_mtod_offset(M, unsigned char *, -O)
+|
+- rte_pktmbuf_mtod(M, uint8_t *) + O
++ rte_pktmbuf_mtod_offset(M, uint8_t *, O)
+|
+- rte_pktmbuf_mtod(M, uint8_t *) - O
++ rte_pktmbuf_mtod_offset(M, uint8_t *, -O)
+)
+
+
+//
+// Fold subsequent offset terms into pre-existing offset used in
+// rte_pktmbuf_mtod_offset().
+//
+@disable paren@
+expression M, O1, O2;
+@@
+(
+- rte_pktmbuf_mtod_offset(M, char *, O1) + O2
++ rte_pktmbuf_mtod_offset(M, char *, O1 + O2)
+|
+- rte_pktmbuf_mtod_offset(M, char *, O1) - O2
++ rte_pktmbuf_mtod_offset(M, char *, O1 - O2)
+|
+- rte_pktmbuf_mtod_offset(M, unsigned char *, O1) + O2
++ rte_pktmbuf_mtod_offset(M, unsigned char *, O1 + O2)
+|
+- rte_pktmbuf_mtod_offset(M, unsigned char *, O1) - O2
++ rte_pktmbuf_mtod_offset(M, unsigned char *, O1 - O2)
+|
+- rte_pktmbuf_mtod_offset(M, uint8_t *, O1) + O2
++ rte_pktmbuf_mtod_offset(M, uint8_t *, O1 + O2)
+|
+- rte_pktmbuf_mtod_offset(M, uint8_t *, O1) - O2
++ rte_pktmbuf_mtod_offset(M, uint8_t *, O1 - O2)
+)
+
+
+//
+// Cleanup rules.  Fold in double casts, remove unnecessary paranthesis, etc.
+//
+@disable paren@
+expression M, O;
+type C, T;
+@@
+(
+- (C)rte_pktmbuf_mtod_offset(M, T, O)
++ rte_pktmbuf_mtod_offset(M, C, O)
+|
+- (rte_pktmbuf_mtod_offset(M, T, O))
++ rte_pktmbuf_mtod_offset(M, T, O)
+|
+- (C)rte_pktmbuf_mtod(M, T)
++ rte_pktmbuf_mtod(M, C)
+|
+- (rte_pktmbuf_mtod(M, T))
++ rte_pktmbuf_mtod(M, T)
+)
-- 
2.1.2

[dpdk-dev] [PATCH 10/10] librte_mbuf: Apply mtod-offset.cocci transform

[Cyril Chemparathy]

This patch simply applies the transform previously committed in
scripts/cocci/mtod-offset.cocci.  No other modifications have been made here.

This patch applies on commit 9f0bf3f4a3eabd7ab5af13b4ed793c36e7615b35.  This
patch may need to be regenerated by rerunning scripts/cocci.sh instead of
simply merging in the change.

Signed-off-by: Cyril Chemparathy <cchemparathy@ezchip.com>
---
 app/test-pmd/ieee1588fwd.c               |  4 +-
 app/test-pmd/rxonly.c                    | 21 ++++++----
 app/test-pmd/txonly.c                    |  4 +-
 app/test/packet_burst_generator.c        |  5 ++-
 examples/dpdk_qat/crypto.c               |  8 ++--
 examples/dpdk_qat/main.c                 |  5 ++-
 examples/l3fwd-acl/main.c                | 20 ++++-----
 examples/l3fwd-power/main.c              |  8 ++--
 examples/l3fwd-vf/main.c                 |  4 +-
 examples/l3fwd/main.c                    | 71 ++++++++++++++------------------
 examples/load_balancer/runtime.c         |  4 +-
 examples/vhost_xen/main.c                |  4 +-
 lib/librte_ip_frag/rte_ipv4_reassembly.c |  3 +-
 lib/librte_ip_frag/rte_ipv6_reassembly.c |  5 +--
 lib/librte_pmd_mlx4/mlx4.c               |  9 ++--
 lib/librte_port/rte_port_ras.c           |  3 +-
 lib/librte_vhost/vhost_rxtx.c            |  4 +-
 17 files changed, 88 insertions(+), 94 deletions(-)

diff --git a/app/test-pmd/ieee1588fwd.c b/app/test-pmd/ieee1588fwd.c
index 84237c1..dfbb185 100644
--- a/app/test-pmd/ieee1588fwd.c
+++ b/app/test-pmd/ieee1588fwd.c
@@ -573,8 +573,8 @@ ieee1588_packet_fwd(struct fwd_stream *fs)
 	 * Check that the received PTP packet is a PTP V2 packet of type
 	 * PTP_SYNC_MESSAGE.
 	 */
-	ptp_hdr = (struct ptpv2_msg *) (rte_pktmbuf_mtod(mb, char *) +
-					sizeof(struct ether_hdr));
+	ptp_hdr = rte_pktmbuf_mtod_offset(mb, struct ptpv2_msg *,
+					  sizeof(struct ether_hdr));
 	if (ptp_hdr->version != 0x02) {
 		printf("Port %u Received PTP V2 Ethernet frame with wrong PTP"
 		       " protocol version 0x%x (should be 0x02)\n",
diff --git a/app/test-pmd/rxonly.c b/app/test-pmd/rxonly.c
index ac56090..b8e35ab 100644
--- a/app/test-pmd/rxonly.c
+++ b/app/test-pmd/rxonly.c
@@ -175,22 +175,25 @@ pkt_burst_receive(struct fwd_stream *fs)
 			 /* Do not support ipv4 option field */
 			if (ol_flags & PKT_RX_TUNNEL_IPV4_HDR) {
 				l3_len = sizeof(struct ipv4_hdr);
-				ipv4_hdr = (struct ipv4_hdr *) (rte_pktmbuf_mtod(mb,
-						unsigned char *) + l2_len);
+				ipv4_hdr = rte_pktmbuf_mtod_offset(mb,
+								   struct ipv4_hdr *,
+								   l2_len);
 				l4_proto = ipv4_hdr->next_proto_id;
 			} else {
 				l3_len = sizeof(struct ipv6_hdr);
-				ipv6_hdr = (struct ipv6_hdr *) (rte_pktmbuf_mtod(mb,
-						unsigned char *) + l2_len);
+				ipv6_hdr = rte_pktmbuf_mtod_offset(mb,
+								   struct ipv6_hdr *,
+								   l2_len);
 				l4_proto = ipv6_hdr->proto;
 			}
 			if (l4_proto == IPPROTO_UDP) {
-				udp_hdr = (struct udp_hdr *) (rte_pktmbuf_mtod(mb,
-						unsigned char *) + l2_len + l3_len);
+				udp_hdr = rte_pktmbuf_mtod_offset(mb,
+								  struct udp_hdr *,
+								  l2_len + l3_len);
 				l4_len = sizeof(struct udp_hdr);
-				vxlan_hdr = (struct vxlan_hdr *) (rte_pktmbuf_mtod(mb,
-						unsigned char *) + l2_len + l3_len
-						 + l4_len);
+				vxlan_hdr = rte_pktmbuf_mtod_offset(mb,
+								    struct vxlan_hdr *,
+								    l2_len + l3_len + l4_len);
 
 				printf(" - VXLAN packet: packet type =%d, "
 					"Destination UDP port =%d, VNI = %d",
diff --git a/app/test-pmd/txonly.c b/app/test-pmd/txonly.c
index 9e66552..f8027f1 100644
--- a/app/test-pmd/txonly.c
+++ b/app/test-pmd/txonly.c
@@ -110,7 +110,7 @@ copy_buf_to_pkt_segs(void* buf, unsigned len, struct rte_mbuf *pkt,
 		seg = seg->next;
 	}
 	copy_len = seg->data_len - offset;
-	seg_buf = (rte_pktmbuf_mtod(seg, char *) + offset);
+	seg_buf = rte_pktmbuf_mtod_offset(seg, char *, offset);
 	while (len > copy_len) {
 		rte_memcpy(seg_buf, buf, (size_t) copy_len);
 		len -= copy_len;
@@ -125,7 +125,7 @@ static inline void
 copy_buf_to_pkt(void* buf, unsigned len, struct rte_mbuf *pkt, unsigned offset)
 {
 	if (offset + len <= pkt->data_len) {
-		rte_memcpy((rte_pktmbuf_mtod(pkt, char *) + offset),
+		rte_memcpy(rte_pktmbuf_mtod_offset(pkt, char *, offset),
 			buf, (size_t) len);
 		return;
 	}
diff --git a/app/test/packet_burst_generator.c b/app/test/packet_burst_generator.c
index 06762c6..28d9e25 100644
--- a/app/test/packet_burst_generator.c
+++ b/app/test/packet_burst_generator.c
@@ -59,7 +59,7 @@ copy_buf_to_pkt_segs(void *buf, unsigned len, struct rte_mbuf *pkt,
 		seg = seg->next;
 	}
 	copy_len = seg->data_len - offset;
-	seg_buf = rte_pktmbuf_mtod(seg, char *) + offset;
+	seg_buf = rte_pktmbuf_mtod_offset(seg, char *, offset);
 	while (len > copy_len) {
 		rte_memcpy(seg_buf, buf, (size_t) copy_len);
 		len -= copy_len;
@@ -74,7 +74,8 @@ static inline void
 copy_buf_to_pkt(void *buf, unsigned len, struct rte_mbuf *pkt, unsigned offset)
 {
 	if (offset + len <= pkt->data_len) {
-		rte_memcpy(rte_pktmbuf_mtod(pkt, char *) + offset, buf, (size_t) len);
+		rte_memcpy(rte_pktmbuf_mtod_offset(pkt, char *, offset), buf,
+			   (size_t) len);
 		return;
 	}
 	copy_buf_to_pkt_segs(buf, len, pkt, offset);
diff --git a/examples/dpdk_qat/crypto.c b/examples/dpdk_qat/crypto.c
index 00e0eb5..c03ea78 100644
--- a/examples/dpdk_qat/crypto.c
+++ b/examples/dpdk_qat/crypto.c
@@ -772,8 +772,8 @@ enum crypto_result
 crypto_encrypt(struct rte_mbuf *rte_buff, enum cipher_alg c, enum hash_alg h)
 {
 	CpaCySymDpOpData *opData =
-			(CpaCySymDpOpData *) (rte_pktmbuf_mtod(rte_buff, char *)
-					+ CRYPTO_OFFSET_TO_OPDATA);
+			rte_pktmbuf_mtod_offset(rte_buff, CpaCySymDpOpData *,
+						CRYPTO_OFFSET_TO_OPDATA);
 	uint32_t lcore_id;
 
 	if (unlikely(c >= NUM_CRYPTO || h >= NUM_HMAC))
@@ -847,8 +847,8 @@ enum crypto_result
 crypto_decrypt(struct rte_mbuf *rte_buff, enum cipher_alg c, enum hash_alg h)
 {
 
-	CpaCySymDpOpData *opData = (void*) (rte_pktmbuf_mtod(rte_buff, char *)
-			+ CRYPTO_OFFSET_TO_OPDATA);
+	CpaCySymDpOpData *opData = rte_pktmbuf_mtod_offset(rte_buff, void *,
+							   CRYPTO_OFFSET_TO_OPDATA);
 	uint32_t lcore_id;
 
 	if (unlikely(c >= NUM_CRYPTO || h >= NUM_HMAC))
diff --git a/examples/dpdk_qat/main.c b/examples/dpdk_qat/main.c
index 053be91..7c2f628 100644
--- a/examples/dpdk_qat/main.c
+++ b/examples/dpdk_qat/main.c
@@ -326,8 +326,9 @@ main_loop(__attribute__((unused)) void *dummy)
 			continue;
 		/* Send packet to either QAT encrypt, QAT decrypt or NIC TX */
 		if (pkt_from_nic_rx) {
-			struct ipv4_hdr *ip  = (struct ipv4_hdr *) (rte_pktmbuf_mtod(pkt, unsigned char *) +
-					sizeof(struct ether_hdr));
+			struct ipv4_hdr *ip  = rte_pktmbuf_mtod_offset(pkt,
+								       struct ipv4_hdr *,
+								       sizeof(struct ether_hdr));
 			if (ip->src_addr & rte_cpu_to_be_32(ACTION_ENCRYPT)) {
 				if (CRYPTO_RESULT_FAIL == crypto_encrypt(pkt,
 					(enum cipher_alg)((ip->src_addr >> 16) & 0xFF),
diff --git a/examples/l3fwd-acl/main.c b/examples/l3fwd-acl/main.c
index 1a04004..3944843 100644
--- a/examples/l3fwd-acl/main.c
+++ b/examples/l3fwd-acl/main.c
@@ -218,9 +218,9 @@ send_single_packet(struct rte_mbuf *m, uint8_t port);
 #define OFF_IPV42PROTO (offsetof(struct ipv4_hdr, next_proto_id))
 #define OFF_IPV62PROTO (offsetof(struct ipv6_hdr, proto))
 #define MBUF_IPV4_2PROTO(m)	\
-	(rte_pktmbuf_mtod((m), uint8_t *) + OFF_ETHHEAD + OFF_IPV42PROTO)
+	rte_pktmbuf_mtod_offset((m), uint8_t *, OFF_ETHHEAD + OFF_IPV42PROTO)
 #define MBUF_IPV6_2PROTO(m)	\
-	(rte_pktmbuf_mtod((m), uint8_t *) + OFF_ETHHEAD + OFF_IPV62PROTO)
+	rte_pktmbuf_mtod_offset((m), uint8_t *, OFF_ETHHEAD + OFF_IPV62PROTO)
 
 #define GET_CB_FIELD(in, fd, base, lim, dlm)	do {            \
 	unsigned long val;                                      \
@@ -566,9 +566,9 @@ dump_acl4_rule(struct rte_mbuf *m, uint32_t sig)
 {
 	uint32_t offset = sig & ~ACL_DENY_SIGNATURE;
 	unsigned char a, b, c, d;
-	struct ipv4_hdr *ipv4_hdr = (struct ipv4_hdr *)
-					(rte_pktmbuf_mtod(m, unsigned char *) +
-					sizeof(struct ether_hdr));
+	struct ipv4_hdr *ipv4_hdr = rte_pktmbuf_mtod_offset(m,
+							    struct ipv4_hdr *,
+							    sizeof(struct ether_hdr));
 
 	uint32_t_to_char(rte_bswap32(ipv4_hdr->src_addr), &a, &b, &c, &d);
 	printf("Packet Src:%hhu.%hhu.%hhu.%hhu ", a, b, c, d);
@@ -590,9 +590,9 @@ dump_acl6_rule(struct rte_mbuf *m, uint32_t sig)
 {
 	unsigned i;
 	uint32_t offset = sig & ~ACL_DENY_SIGNATURE;
-	struct ipv6_hdr *ipv6_hdr = (struct ipv6_hdr *)
-					(rte_pktmbuf_mtod(m, unsigned char *) +
-					sizeof(struct ether_hdr));
+	struct ipv6_hdr *ipv6_hdr = rte_pktmbuf_mtod_offset(m,
+							    struct ipv6_hdr *,
+							    sizeof(struct ether_hdr));
 
 	printf("Packet Src");
 	for (i = 0; i < RTE_DIM(ipv6_hdr->src_addr); i += sizeof(uint16_t))
@@ -651,8 +651,8 @@ prepare_one_packet(struct rte_mbuf **pkts_in, struct acl_search_t *acl,
 
 	if (type == PKT_RX_IPV4_HDR) {
 
-		ipv4_hdr = (struct ipv4_hdr *)(rte_pktmbuf_mtod(pkt,
-			unsigned char *) + sizeof(struct ether_hdr));
+		ipv4_hdr = rte_pktmbuf_mtod_offset(pkt, struct ipv4_hdr *,
+						   sizeof(struct ether_hdr));
 
 		/* Check to make sure the packet is valid (RFC1812) */
 		if (is_valid_ipv4_pkt(ipv4_hdr, pkt->pkt_len) >= 0) {
diff --git a/examples/l3fwd-power/main.c b/examples/l3fwd-power/main.c
index bb0b66f..b0748cf 100644
--- a/examples/l3fwd-power/main.c
+++ b/examples/l3fwd-power/main.c
@@ -640,8 +640,8 @@ l3fwd_simple_forward(struct rte_mbuf *m, uint8_t portid,
 	if (m->ol_flags & PKT_RX_IPV4_HDR) {
 		/* Handle IPv4 headers.*/
 		ipv4_hdr =
-			(struct ipv4_hdr *)(rte_pktmbuf_mtod(m, unsigned char*)
-						+ sizeof(struct ether_hdr));
+			rte_pktmbuf_mtod_offset(m, struct ipv4_hdr *,
+						sizeof(struct ether_hdr));
 
 #ifdef DO_RFC_1812_CHECKS
 		/* Check to make sure the packet is valid (RFC1812) */
@@ -679,8 +679,8 @@ l3fwd_simple_forward(struct rte_mbuf *m, uint8_t portid,
 		struct ipv6_hdr *ipv6_hdr;
 
 		ipv6_hdr =
-			(struct ipv6_hdr *)(rte_pktmbuf_mtod(m, unsigned char*)
-						+ sizeof(struct ether_hdr));
+			rte_pktmbuf_mtod_offset(m, struct ipv6_hdr *,
+						sizeof(struct ether_hdr));
 
 		dst_port = get_ipv6_dst_port(ipv6_hdr, portid,
 					qconf->ipv6_lookup_struct);
diff --git a/examples/l3fwd-vf/main.c b/examples/l3fwd-vf/main.c
index f007bc1..32ff4d5 100644
--- a/examples/l3fwd-vf/main.c
+++ b/examples/l3fwd-vf/main.c
@@ -461,8 +461,8 @@ l3fwd_simple_forward(struct rte_mbuf *m, uint8_t portid, lookup_struct_t * l3fwd
 
 	eth_hdr = rte_pktmbuf_mtod(m, struct ether_hdr *);
 
-	ipv4_hdr = (struct ipv4_hdr *)(rte_pktmbuf_mtod(m, unsigned char *) +
-				sizeof(struct ether_hdr));
+	ipv4_hdr = rte_pktmbuf_mtod_offset(m, struct ipv4_hdr *,
+					   sizeof(struct ether_hdr));
 
 #ifdef DO_RFC_1812_CHECKS
 	/* Check to make sure the packet is valid (RFC1812) */
diff --git a/examples/l3fwd/main.c b/examples/l3fwd/main.c
index 7871038..2d06d95 100644
--- a/examples/l3fwd/main.c
+++ b/examples/l3fwd/main.c
@@ -752,14 +752,14 @@ simple_ipv4_fwd_4pkts(struct rte_mbuf* m[4], uint8_t portid, struct lcore_conf *
 	eth_hdr[3] = rte_pktmbuf_mtod(m[3], struct ether_hdr *);
 
 	/* Handle IPv4 headers.*/
-	ipv4_hdr[0] = (struct ipv4_hdr *)(rte_pktmbuf_mtod(m[0], unsigned char *) +
-			sizeof(struct ether_hdr));
-	ipv4_hdr[1] = (struct ipv4_hdr *)(rte_pktmbuf_mtod(m[1], unsigned char *) +
-			sizeof(struct ether_hdr));
-	ipv4_hdr[2] = (struct ipv4_hdr *)(rte_pktmbuf_mtod(m[2], unsigned char *) +
-			sizeof(struct ether_hdr));
-	ipv4_hdr[3] = (struct ipv4_hdr *)(rte_pktmbuf_mtod(m[3], unsigned char *) +
-			sizeof(struct ether_hdr));
+	ipv4_hdr[0] = rte_pktmbuf_mtod_offset(m[0], struct ipv4_hdr *,
+					      sizeof(struct ether_hdr));
+	ipv4_hdr[1] = rte_pktmbuf_mtod_offset(m[1], struct ipv4_hdr *,
+					      sizeof(struct ether_hdr));
+	ipv4_hdr[2] = rte_pktmbuf_mtod_offset(m[2], struct ipv4_hdr *,
+					      sizeof(struct ether_hdr));
+	ipv4_hdr[3] = rte_pktmbuf_mtod_offset(m[3], struct ipv4_hdr *,
+					      sizeof(struct ether_hdr));
 
 #ifdef DO_RFC_1812_CHECKS
 	/* Check to make sure the packet is valid (RFC1812) */
@@ -795,14 +795,10 @@ simple_ipv4_fwd_4pkts(struct rte_mbuf* m[4], uint8_t portid, struct lcore_conf *
 	}
 #endif // End of #ifdef DO_RFC_1812_CHECKS
 
-	data[0] = _mm_loadu_si128((__m128i*)(rte_pktmbuf_mtod(m[0], unsigned char *) +
-		sizeof(struct ether_hdr) + offsetof(struct ipv4_hdr, time_to_live)));
-	data[1] = _mm_loadu_si128((__m128i*)(rte_pktmbuf_mtod(m[1], unsigned char *) +
-		sizeof(struct ether_hdr) + offsetof(struct ipv4_hdr, time_to_live)));
-	data[2] = _mm_loadu_si128((__m128i*)(rte_pktmbuf_mtod(m[2], unsigned char *) +
-		sizeof(struct ether_hdr) + offsetof(struct ipv4_hdr, time_to_live)));
-	data[3] = _mm_loadu_si128((__m128i*)(rte_pktmbuf_mtod(m[3], unsigned char *) +
-		sizeof(struct ether_hdr) + offsetof(struct ipv4_hdr, time_to_live)));
+	data[0] = _mm_loadu_si128(rte_pktmbuf_mtod_offset(m[0], __m128i *, sizeof(struct ether_hdr) + offsetof(struct ipv4_hdr, time_to_live)));
+	data[1] = _mm_loadu_si128(rte_pktmbuf_mtod_offset(m[1], __m128i *, sizeof(struct ether_hdr) + offsetof(struct ipv4_hdr, time_to_live)));
+	data[2] = _mm_loadu_si128(rte_pktmbuf_mtod_offset(m[2], __m128i *, sizeof(struct ether_hdr) + offsetof(struct ipv4_hdr, time_to_live)));
+	data[3] = _mm_loadu_si128(rte_pktmbuf_mtod_offset(m[3], __m128i *, sizeof(struct ether_hdr) + offsetof(struct ipv4_hdr, time_to_live)));
 
 	key[0].xmm = _mm_and_si128(data[0], mask0);
 	key[1].xmm = _mm_and_si128(data[1], mask0);
@@ -863,14 +859,9 @@ simple_ipv4_fwd_4pkts(struct rte_mbuf* m[4], uint8_t portid, struct lcore_conf *
 static inline void get_ipv6_5tuple(struct rte_mbuf* m0, __m128i mask0, __m128i mask1,
 				 union ipv6_5tuple_host * key)
 {
-        __m128i tmpdata0 = _mm_loadu_si128((__m128i*)(rte_pktmbuf_mtod(m0, unsigned char *)
-			+ sizeof(struct ether_hdr) + offsetof(struct ipv6_hdr, payload_len)));
-        __m128i tmpdata1 = _mm_loadu_si128((__m128i*)(rte_pktmbuf_mtod(m0, unsigned char *)
-			+ sizeof(struct ether_hdr) + offsetof(struct ipv6_hdr, payload_len)
-			+  sizeof(__m128i)));
-        __m128i tmpdata2 = _mm_loadu_si128((__m128i*)(rte_pktmbuf_mtod(m0, unsigned char *)
-			+ sizeof(struct ether_hdr) + offsetof(struct ipv6_hdr, payload_len)
-			+ sizeof(__m128i) + sizeof(__m128i)));
+        __m128i tmpdata0 = _mm_loadu_si128(rte_pktmbuf_mtod_offset(m0, __m128i *, sizeof(struct ether_hdr) + offsetof(struct ipv6_hdr, payload_len)));
+        __m128i tmpdata1 = _mm_loadu_si128(rte_pktmbuf_mtod_offset(m0, __m128i *, sizeof(struct ether_hdr) + offsetof(struct ipv6_hdr, payload_len) + sizeof(__m128i)));
+        __m128i tmpdata2 = _mm_loadu_si128(rte_pktmbuf_mtod_offset(m0, __m128i *, sizeof(struct ether_hdr) + offsetof(struct ipv6_hdr, payload_len) + sizeof(__m128i) + sizeof(__m128i)));
         key->xmm[0] = _mm_and_si128(tmpdata0, mask0);
         key->xmm[1] = tmpdata1;
         key->xmm[2] = _mm_and_si128(tmpdata2, mask1);
@@ -893,14 +884,14 @@ simple_ipv6_fwd_4pkts(struct rte_mbuf* m[4], uint8_t portid, struct lcore_conf *
 	eth_hdr[3] = rte_pktmbuf_mtod(m[3], struct ether_hdr *);
 
 	/* Handle IPv6 headers.*/
-	ipv6_hdr[0] = (struct ipv6_hdr *)(rte_pktmbuf_mtod(m[0], unsigned char *) +
-			sizeof(struct ether_hdr));
-	ipv6_hdr[1] = (struct ipv6_hdr *)(rte_pktmbuf_mtod(m[1], unsigned char *) +
-			sizeof(struct ether_hdr));
-	ipv6_hdr[2] = (struct ipv6_hdr *)(rte_pktmbuf_mtod(m[2], unsigned char *) +
-			sizeof(struct ether_hdr));
-	ipv6_hdr[3] = (struct ipv6_hdr *)(rte_pktmbuf_mtod(m[3], unsigned char *) +
-			sizeof(struct ether_hdr));
+	ipv6_hdr[0] = rte_pktmbuf_mtod_offset(m[0], struct ipv6_hdr *,
+					      sizeof(struct ether_hdr));
+	ipv6_hdr[1] = rte_pktmbuf_mtod_offset(m[1], struct ipv6_hdr *,
+					      sizeof(struct ether_hdr));
+	ipv6_hdr[2] = rte_pktmbuf_mtod_offset(m[2], struct ipv6_hdr *,
+					      sizeof(struct ether_hdr));
+	ipv6_hdr[3] = rte_pktmbuf_mtod_offset(m[3], struct ipv6_hdr *,
+					      sizeof(struct ether_hdr));
 
 	get_ipv6_5tuple(m[0], mask1, mask2, &key[0]);
 	get_ipv6_5tuple(m[1], mask1, mask2, &key[1]);
@@ -959,8 +950,8 @@ l3fwd_simple_forward(struct rte_mbuf *m, uint8_t portid, struct lcore_conf *qcon
 
 	if (m->ol_flags & PKT_RX_IPV4_HDR) {
 		/* Handle IPv4 headers.*/
-		ipv4_hdr = (struct ipv4_hdr *)(rte_pktmbuf_mtod(m, unsigned char *) +
-				sizeof(struct ether_hdr));
+		ipv4_hdr = rte_pktmbuf_mtod_offset(m, struct ipv4_hdr *,
+						   sizeof(struct ether_hdr));
 
 #ifdef DO_RFC_1812_CHECKS
 		/* Check to make sure the packet is valid (RFC1812) */
@@ -996,8 +987,8 @@ l3fwd_simple_forward(struct rte_mbuf *m, uint8_t portid, struct lcore_conf *qcon
 		/* Handle IPv6 headers.*/
 		struct ipv6_hdr *ipv6_hdr;
 
-		ipv6_hdr = (struct ipv6_hdr *)(rte_pktmbuf_mtod(m, unsigned char *) +
-				sizeof(struct ether_hdr));
+		ipv6_hdr = rte_pktmbuf_mtod_offset(m, struct ipv6_hdr *,
+						   sizeof(struct ether_hdr));
 
 		dst_port = get_ipv6_dst_port(ipv6_hdr, portid, qconf->ipv6_lookup_struct);
 
@@ -1188,10 +1179,10 @@ processx4_step3(struct rte_mbuf *pkt[FWDSTEP], uint16_t dst_port[FWDSTEP])
 	__m128i ve[FWDSTEP];
 	__m128i *p[FWDSTEP];
 
-	p[0] = (rte_pktmbuf_mtod(pkt[0], __m128i *));
-	p[1] = (rte_pktmbuf_mtod(pkt[1], __m128i *));
-	p[2] = (rte_pktmbuf_mtod(pkt[2], __m128i *));
-	p[3] = (rte_pktmbuf_mtod(pkt[3], __m128i *));
+	p[0] = rte_pktmbuf_mtod(pkt[0], __m128i *);
+	p[1] = rte_pktmbuf_mtod(pkt[1], __m128i *);
+	p[2] = rte_pktmbuf_mtod(pkt[2], __m128i *);
+	p[3] = rte_pktmbuf_mtod(pkt[3], __m128i *);
 
 	ve[0] = val_eth[dst_port[0]];
 	te[0] = _mm_load_si128(p[0]);
diff --git a/examples/load_balancer/runtime.c b/examples/load_balancer/runtime.c
index adec4ba..2b265c2 100644
--- a/examples/load_balancer/runtime.c
+++ b/examples/load_balancer/runtime.c
@@ -535,7 +535,9 @@ app_lcore_worker(
 			}
 
 			pkt = lp->mbuf_in.array[j];
-			ipv4_hdr = (struct ipv4_hdr *)(rte_pktmbuf_mtod(pkt, unsigned char *) + sizeof(struct ether_hdr));
+			ipv4_hdr = rte_pktmbuf_mtod_offset(pkt,
+							   struct ipv4_hdr *,
+							   sizeof(struct ether_hdr));
 			ipv4_dst = rte_be_to_cpu_32(ipv4_hdr->dst_addr);
 
 			if (unlikely(rte_lpm_lookup(lp->lpm_table, ipv4_dst, &port) != 0)) {
diff --git a/examples/vhost_xen/main.c b/examples/vhost_xen/main.c
index b672bf3..ad24079 100644
--- a/examples/vhost_xen/main.c
+++ b/examples/vhost_xen/main.c
@@ -874,8 +874,8 @@ virtio_tx_route(struct virtio_net* dev, struct rte_mbuf *m, struct rte_mempool *
 	vlan_hdr->h_vlan_TCI = htons(vlan_tag);
 
 	/* Copy the remaining packet contents to the mbuf. */
-	rte_memcpy((void *)(rte_pktmbuf_mtod(mbuf, uint8_t *) + VLAN_ETH_HLEN),
-		(const void *)(rte_pktmbuf_mtod(m, uint8_t *) + ETH_HLEN),
+	rte_memcpy(rte_pktmbuf_mtod_offset(mbuf, void *, VLAN_ETH_HLEN),
+		rte_pktmbuf_mtod_offset(m, const void *, ETH_HLEN),
 		(m->data_len - ETH_HLEN));
 	tx_q->m_table[len] = mbuf;
 	len++;
diff --git a/lib/librte_ip_frag/rte_ipv4_reassembly.c b/lib/librte_ip_frag/rte_ipv4_reassembly.c
index 279be6c..14bab91 100644
--- a/lib/librte_ip_frag/rte_ipv4_reassembly.c
+++ b/lib/librte_ip_frag/rte_ipv4_reassembly.c
@@ -85,8 +85,7 @@ ipv4_frag_reassemble(const struct ip_frag_pkt *fp)
 	m->ol_flags |= PKT_TX_IP_CKSUM;
 
 	/* update ipv4 header for the reassmebled packet */
-	ip_hdr = (struct ipv4_hdr*)(rte_pktmbuf_mtod(m, uint8_t *) +
-		m->l2_len);
+	ip_hdr = rte_pktmbuf_mtod_offset(m, struct ipv4_hdr *, m->l2_len);
 
 	ip_hdr->total_length = rte_cpu_to_be_16((uint16_t)(fp->total_size +
 		m->l3_len));
diff --git a/lib/librte_ip_frag/rte_ipv6_reassembly.c b/lib/librte_ip_frag/rte_ipv6_reassembly.c
index 71cf721..1f1c172 100644
--- a/lib/librte_ip_frag/rte_ipv6_reassembly.c
+++ b/lib/librte_ip_frag/rte_ipv6_reassembly.c
@@ -108,8 +108,7 @@ ipv6_frag_reassemble(const struct ip_frag_pkt *fp)
 	m->ol_flags |= PKT_TX_IP_CKSUM;
 
 	/* update ipv6 header for the reassembled datagram */
-	ip_hdr = (struct ipv6_hdr *) (rte_pktmbuf_mtod(m, uint8_t *) +
-								  m->l2_len);
+	ip_hdr = rte_pktmbuf_mtod_offset(m, struct ipv6_hdr *, m->l2_len);
 
 	ip_hdr->payload_len = rte_cpu_to_be_16(payload_len);
 
@@ -124,7 +123,7 @@ ipv6_frag_reassemble(const struct ip_frag_pkt *fp)
 	frag_hdr = (struct ipv6_extension_fragment *) (ip_hdr + 1);
 	ip_hdr->proto = frag_hdr->next_header;
 
-	ip_frag_memmove(rte_pktmbuf_mtod(m, char*) + sizeof(*frag_hdr),
+	ip_frag_memmove(rte_pktmbuf_mtod_offset(m, char *, sizeof(*frag_hdr)),
 			rte_pktmbuf_mtod(m, char*), move_len);
 
 	rte_pktmbuf_adj(m, sizeof(*frag_hdr));
diff --git a/lib/librte_pmd_mlx4/mlx4.c b/lib/librte_pmd_mlx4/mlx4.c
index f915bc1..e6b90ec 100644
--- a/lib/librte_pmd_mlx4/mlx4.c
+++ b/lib/librte_pmd_mlx4/mlx4.c
@@ -1103,10 +1103,10 @@ mlx4_tx_burst(void *dpdk_txq, struct rte_mbuf **pkts, uint16_t pkts_n)
 			linearize = 1;
 		}
 		/* Set WR fields. */
-		assert(((uintptr_t)rte_pktmbuf_mtod(buf, char *) -
+		assert((rte_pktmbuf_mtod(buf, uintptr_t) -
 			(uintptr_t)buf) <= 0xffff);
 		WR_ID(wr->wr_id).offset =
-			((uintptr_t)rte_pktmbuf_mtod(buf, char *) -
+			(rte_pktmbuf_mtod(buf, uintptr_t) -
 			 (uintptr_t)buf);
 		wr->num_sge = segs;
 		/* Register segments as SGEs. */
@@ -1141,7 +1141,7 @@ mlx4_tx_burst(void *dpdk_txq, struct rte_mbuf **pkts, uint16_t pkts_n)
 			assert(sge->length == 0);
 			assert(sge->lkey == 0);
 			/* Update SGE. */
-			sge->addr = (uintptr_t)rte_pktmbuf_mtod(buf, char *);
+			sge->addr = rte_pktmbuf_mtod(buf, uintptr_t);
 			if (txq->priv->vf)
 				rte_prefetch0((volatile void *)sge->addr);
 			sge->length = DATA_LEN(buf);
@@ -1591,8 +1591,7 @@ rxq_alloc_elts_sp(struct rxq *rxq, unsigned int elts_n,
 			assert(sizeof(sge->addr) >= sizeof(uintptr_t));
 			if (j == 0) {
 				/* The first SGE keeps its headroom. */
-				sge->addr = (uintptr_t)rte_pktmbuf_mtod(buf,
-									char *);
+				sge->addr = rte_pktmbuf_mtod(buf, uintptr_t);
 				sge->length = (buf->buf_len -
 					       RTE_PKTMBUF_HEADROOM);
 			} else {
diff --git a/lib/librte_port/rte_port_ras.c b/lib/librte_port/rte_port_ras.c
index b6ab67a..5f60e93 100644
--- a/lib/librte_port/rte_port_ras.c
+++ b/lib/librte_port/rte_port_ras.c
@@ -136,8 +136,7 @@ static inline void
 process_one(struct rte_port_ring_writer_ipv4_ras *p, struct rte_mbuf *pkt)
 {
 	/* Assume there is no ethernet header */
-	struct ipv4_hdr *pkt_hdr = (struct ipv4_hdr *)
-			(rte_pktmbuf_mtod(pkt, unsigned char *));
+	struct ipv4_hdr *pkt_hdr = rte_pktmbuf_mtod(pkt, struct ipv4_hdr *);
 
 	/* Get "Do not fragment" flag and fragment offset */
 	uint16_t frag_field = rte_be_to_cpu_16(pkt_hdr->fragment_offset);
diff --git a/lib/librte_vhost/vhost_rxtx.c b/lib/librte_vhost/vhost_rxtx.c
index 510ffe8..ef13213 100644
--- a/lib/librte_vhost/vhost_rxtx.c
+++ b/lib/librte_vhost/vhost_rxtx.c
@@ -272,7 +272,7 @@ copy_from_mbuf_to_vring(struct virtio_net *dev, uint16_t res_base_idx,
 	while (cpy_len > 0) {
 		/* Copy mbuf data to vring buffer */
 		rte_memcpy((void *)(uintptr_t)(vb_addr + vb_offset),
-			(const void *)(rte_pktmbuf_mtod(pkt, char*) + seg_offset),
+			rte_pktmbuf_mtod_offset(pkt, const void *, seg_offset),
 			cpy_len);
 
 		PRINT_PACKET(dev,
@@ -621,7 +621,7 @@ rte_vhost_dequeue_burst(struct virtio_net *dev, uint16_t queue_id,
 		cur = m;
 		prev = m;
 		while (cpy_len != 0) {
-			rte_memcpy((void *)(rte_pktmbuf_mtod(cur, char *) + seg_offset),
+			rte_memcpy(rte_pktmbuf_mtod_offset(cur, void *, seg_offset),
 				(void *)((uintptr_t)(vb_addr + vb_offset)),
 				cpy_len);
 
-- 
2.1.2

Re: [dpdk-dev] [PATCH 00/10] Improve cast alignment for strict aligned machines

[Olivier MATZ]

Hi Cyril,

On 04/29/2015 06:15 PM, Cyril Chemparathy wrote:
> This series contains a few improvements that allow the DPDK code base to build
> properly on machines that enforce strict pointer cast alignment constraints.
> 
> When dealing with packet data which could be arbitrarily aligned, we get the
> compiler to do the right thing by (a) making sure that header types are
> packed, and (b) introducing and using unaligned_uint(16|32|64)_t types when
> upcasting from byte pointers.
> 
> In a few other instances, we know apriori that the pointer cast cannot
> possibly break alignment.  This applies to the changes in mempool, hash, mbuf,
> and the ethdev stats code.  Here, we simply silence the compiler by casting
> through (void *) using the RTE_PTR_(ADD|SUB) macros.
> 
> Finally, we introduce a new rte_pktmbuf_mtod_offset() helper to return a type
> casted pointer to an offset within the packet data.  This replaces the
> following commonly used pattern:
> 	(struct foo *)(rte_pktmbuf_mtod(m, char *) + offset)
> with:
> 	rte_pktmbuf_mtod_offset(m, struct foo *, offset)
> To ensure consistency, the above transform was applied throughout the code
> base using the coccinelle semantic patching tool.
> 

Before diving into the patches, I'm wondering if adding aligned(1) or
(packed) attribute at some places would have a performance impact on
supported architectures (Intel or IBM Power). Did you manage to test
it?

Regards,
Olivier

Re: [dpdk-dev] [PATCH 00/10] Improve cast alignment for strict aligned machines

[Cyril Chemparathy]

On Mon, 4 May 2015 11:50:09 +0200
Olivier MATZ <olivier.matz@6wind.com> wrote:

> Hi Cyril,
> 
> On 04/29/2015 06:15 PM, Cyril Chemparathy wrote:
> > This series contains a few improvements that allow the DPDK code
> > base to build properly on machines that enforce strict pointer cast
> > alignment constraints.
> > 
> > When dealing with packet data which could be arbitrarily aligned,
> > we get the compiler to do the right thing by (a) making sure that
> > header types are packed, and (b) introducing and using
> > unaligned_uint(16|32|64)_t types when upcasting from byte pointers.
> > 
> > In a few other instances, we know apriori that the pointer cast
> > cannot possibly break alignment.  This applies to the changes in
> > mempool, hash, mbuf, and the ethdev stats code.  Here, we simply
> > silence the compiler by casting through (void *) using the
> > RTE_PTR_(ADD|SUB) macros.
> > 
> > Finally, we introduce a new rte_pktmbuf_mtod_offset() helper to
> > return a type casted pointer to an offset within the packet data.
> > This replaces the following commonly used pattern:
> > 	(struct foo *)(rte_pktmbuf_mtod(m, char *) + offset)
> > with:
> > 	rte_pktmbuf_mtod_offset(m, struct foo *, offset)
> > To ensure consistency, the above transform was applied throughout
> > the code base using the coccinelle semantic patching tool.
> > 
> 
> Before diving into the patches, I'm wondering if adding aligned(1) or
> (packed) attribute at some places would have a performance impact on
> supported architectures (Intel or IBM Power). Did you manage to test
> it?
> 

I don't expect to see a performance impact on x86.  I don't really
have a way of trying this out on PPC, and I could use some help here.

Overall, I think that the few places where we've injected aligned(1) or
packed really warrant the insertion for correctness.  These are places
where packet data of unknown alignment is being typecast and
dereferenced.  In such cases, _not_ representing the unaligned
nature of these accesses breaks machines with strict alignment.

Thanks
-- Cyril.

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

[Olivier MATZ]

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>

Re: [dpdk-dev] [PATCH 08/10] librte_mbuf: Add rte_pktmbuf_mtod_offset()

[Olivier MATZ]

On 04/29/2015 06:15 PM, Cyril Chemparathy wrote:
> There are a number of instances in the code where rte_pktmbuf_mtod() is used
> to get the mbuf data pointer, only to add an offset before casting the result
> to some other header type.  This patch adds a new rte_pktmbuf_mtod_offset()
> macro to eliminate these awful double cast situations.
> 
> Signed-off-by: Cyril Chemparathy <cchemparathy@ezchip.com>
> ---
>  lib/librte_mbuf/rte_mbuf.h | 20 +++++++++++++++++++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> index 5ddd8dd..e8f1bf4 100644
> --- a/lib/librte_mbuf/rte_mbuf.h
> +++ b/lib/librte_mbuf/rte_mbuf.h
> @@ -54,6 +54,7 @@
>   */
>  
>  #include <stdint.h>
> +#include <rte_common.h>
>  #include <rte_mempool.h>
>  #include <rte_memory.h>
>  #include <rte_atomic.h>
> @@ -1071,6 +1072,23 @@ static inline struct rte_mbuf *rte_pktmbuf_lastseg(struct rte_mbuf *m)
>  }
>  
>  /**
> + * A macro that points to an offset into the data in the mbuf.
> + *
> + * The returned pointer is cast to type t. Before using this
> + * function, the user must ensure that m_headlen(m) is large enough to
> + * read its data.

Small comment here: m_headlen(m) should be replaced by "the length of
the first segment". Indeed, there is no m_headlen() function (I see that
it is coming from a copy/paste of the mtod() comment). Could you
also fix it by the way?




> + *
> + * @param m
> + *   The packet mbuf.
> + * @param o
> + *   The offset into the mbuf data.
> + * @param t
> + *   The type to cast the result into.
> + */
> +#define rte_pktmbuf_mtod_offset(m, t, o)	\
> +	((t)((char *)(m)->buf_addr + (m)->data_off + (o)))
> +
> +/**
>   * A macro that points to the start of the data in the mbuf.
>   *
>   * The returned pointer is cast to type t. Before using this
> @@ -1082,7 +1100,7 @@ static inline struct rte_mbuf *rte_pktmbuf_lastseg(struct rte_mbuf *m)
>   * @param t
>   *   The type to cast the result into.
>   */
> -#define rte_pktmbuf_mtod(m, t) ((t)((char *)(m)->buf_addr + (m)->data_off))
> +#define rte_pktmbuf_mtod(m, t) rte_pktmbuf_mtod_offset(m, t, 0)
>  
>  /**
>   * A macro that returns the length of the packet.
> 

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

[Cyril Chemparathy]

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>
> 
> 

Re: [dpdk-dev] [PATCH 08/10] librte_mbuf: Add rte_pktmbuf_mtod_offset()

[Cyril Chemparathy]

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

> Small comment here: m_headlen(m) should be replaced by "the length of
> the first segment". Indeed, there is no m_headlen() function (I see
> that it is coming from a copy/paste of the mtod() comment). Could you
> also fix it by the way?

Will do.  Thanks.