Re: [PATCH 07/13] mbuf: use macros only to access the mbuf metadata

-----Original Message-----
From: Olivier MATZ [mailto:olivier.matz-pdR9zngts4EAvxtiuMwx3w@public.gmane.org]
Sent: Monday, September 08, 2014 1:06 PM
To: Richardson, Bruce; dev-VfR2kkLFssw@public.gmane.org
Subject: Re: [dpdk-dev] [PATCH 07/13] mbuf: use macros only to access the
mbuf metadata

Hi Bruce,

On 09/03/2014 05:49 PM, Bruce Richardson wrote:

quoted

Removed the explicit zero-sized metadata definition at the end of the
mbuf data structure. Updated the metadata macros to take account of this
change so that all existing code which uses those macros still works.

Signed-off-by: Bruce Richardson <redacted>
---
 lib/librte_mbuf/rte_mbuf.h | 22 ++++++++--------------
 1 file changed, 8 insertions(+), 14 deletions(-)

diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index 5260001..ca66d9a 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h

@@ -166,31 +166,25 @@ struct rte_mbuf {
 	struct rte_mempool *pool; /**< Pool from which mbuf was allocated.

*/

quoted

 	struct rte_mbuf *next;    /**< Next segment of scattered packet. */

-	union {
-		uint8_t metadata[0];
-		uint16_t metadata16[0];
-		uint32_t metadata32[0];
-		uint64_t metadata64[0];
-	} __rte_cache_aligned;
 } __rte_cache_aligned;

 #define RTE_MBUF_METADATA_UINT8(mbuf, offset)              \
-	(mbuf->metadata[offset])
+	(((uint8_t *)&(mbuf)[1])[offset])
 #define RTE_MBUF_METADATA_UINT16(mbuf, offset)             \
-	(mbuf->metadata16[offset/sizeof(uint16_t)])
+	(((uint16_t *)&(mbuf)[1])[offset/sizeof(uint16_t)])
 #define RTE_MBUF_METADATA_UINT32(mbuf, offset)             \
-	(mbuf->metadata32[offset/sizeof(uint32_t)])
+	(((uint32_t *)&(mbuf)[1])[offset/sizeof(uint32_t)])
 #define RTE_MBUF_METADATA_UINT64(mbuf, offset)             \
-	(mbuf->metadata64[offset/sizeof(uint64_t)])
+	(((uint64_t *)&(mbuf)[1])[offset/sizeof(uint64_t)])

 #define RTE_MBUF_METADATA_UINT8_PTR(mbuf, offset)          \
-	(&mbuf->metadata[offset])
+	(&RTE_MBUF_METADATA_UINT8(mbuf, offset))
 #define RTE_MBUF_METADATA_UINT16_PTR(mbuf, offset)         \
-	(&mbuf->metadata16[offset/sizeof(uint16_t)])
+	(&RTE_MBUF_METADATA_UINT16(mbuf, offset))
 #define RTE_MBUF_METADATA_UINT32_PTR(mbuf, offset)         \
-	(&mbuf->metadata32[offset/sizeof(uint32_t)])
+	(&RTE_MBUF_METADATA_UINT32(mbuf, offset))
 #define RTE_MBUF_METADATA_UINT64_PTR(mbuf, offset)         \
-	(&mbuf->metadata64[offset/sizeof(uint64_t)])
+	(&RTE_MBUF_METADATA_UINT64(mbuf, offset))

 /**
  * Given the buf_addr returns the pointer to corresponding mbuf.

I think it goes in the good direction. So:
Acked-by: Olivier Matz <redacted>

Just one question: why not removing RTE_MBUF_METADATA*() macros?
I'd just provide one macro that gives a (void*) to the first byte
after the mbuf structure.

The format of the metadata is up to the application, that usually
casts (m + 1) into a private structure, making the macros not very
useful. I suggest to move these macros outside rte_mbuf.h, in the
application-specific or library-specific header, what do you think?

Regards,
Olivier

Yes, I'll look into that.

/Bruce