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

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

Hi Bruce,

On 09/03/2014 05:49 PM, Bruce Richardson wrote:
> 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 <bruce.richardson@intel.com>
> ---
>  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. */
>  	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 <olivier.matz@6wind.com>

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