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

["Dumitrescu, Cristian" <cristian.dumitrescu@intel.com>]

Bruce, Olivier, 

What is the reason to remove this field? Please explain the rationale of removing this field.

We previously agreed we need to provide an easy and standard mechanism for applications to extend the mandatory per buffer metadata (mbuf) with optional application-dependent metadata. This field just provides a clean way for the apps to know where is the end of the mandatory metadata, i.e. the first location in the packet buffer where the app can add its own metadata (of course, the app has to manage the headroom space before the first byte of packet data). A zero-size field is the standard mechanism that DPDK uses extensively in pretty much every library to access memory immediately after a header structure.

The impact of removing this field is that there is no standard way to identify where the end of the mandatory metadata is, so each application will have to reinvent this. With no clear convention, we will end up with a lot of non-standard ways. Every time the format of the mbuf structure is going to be changed, this can potentially break applications that use custom metadata, while using this simple standard mechanism would prevent this. So why remove this?

Having applications define their optional meta-data is a real need. Please take a look at the Service Chaining IEFT emerging protocols (https://datatracker.ietf.org/wg/sfc/documents/), which provide standard mechanisms for applications to define their own packet meta-data and share it between the elements of the processing pipeline (for Service Chaining, these are typically virtual machines scattered amongst the data center).

And, in my opinion, there is no negative impact/cost associated with keeping this field.

Regards,
Cristian


-----Original Message-----
From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Richardson, Bruce
Sent: Tuesday, September 9, 2014 10:01 AM
To: Olivier MATZ; dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH 07/13] mbuf: use macros only to access the mbuf metadata

> -----Original Message-----
> From: Olivier MATZ [mailto:olivier.matz@6wind.com]
> Sent: Monday, September 08, 2014 1:06 PM
> To: Richardson, Bruce; dev@dpdk.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:
> > 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
> 
Yes, I'll look into that.

/Bruce
--------------------------------------------------------------
Intel Shannon Limited
Registered in Ireland
Registered Office: Collinstown Industrial Park, Leixlip, County Kildare
Registered Number: 308263
Business address: Dromore House, East Park, Shannon, Co. Clare

This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies.