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

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

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
  

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

On Mon, Sep 08, 2014 at 02:05:41PM +0200, Olivier MATZ wrote:
> 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?
> 

Things look to work if I just move the definitions en-masse into rte_port.h. Is that the sort of change you would be thinking of? I was wondering about replacing the typed macros here with a generic one to access just beyond the definition of the mbuf structure, but on further thought, I believe that using the buf_addr pointer in the mbuf data structure is probably enough for most applications. [An alternative to moving the definitions into rte_port.h is to move them into rte_table.h and having port_frag.c use the buf_addr pointer instead of a macro to get at the metadata. All other references to the macros apart from two in that port file, are in the tables or in apps that use the tables lib.]
What do you think?

/Bruce
  

Hi Bruce,

On 09/10/2014 05:09 PM, Bruce Richardson wrote:
>> 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?
>>
>
> Things look to work if I just move the definitions en-masse into
> rte_port.h. Is that the sort of change you would be thinking of?
> I was wondering about replacing the typed macros here with a
> generic one to access just beyond the definition of the mbuf
> structure, but on further thought, I believe that using the
> buf_addr pointer in the mbuf data structure is probably enough
> for most applications. [An alternative to moving the definitions
> into rte_port.h is to move them into rte_table.h and having
> port_frag.c use the buf_addr pointer instead of a macro to get at
> the metadata. All other references to the macros apart from two
> in that port file, are in the tables or in apps that use the
> tables lib.]
> What do you think?

Yes, moving the macros in rte_port.h looks good to me, since
the libraries using rte_ports are the users of this specific
metadata format.

Thanks!
Olivier
  

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.
  

Hello Cristian,

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

The rationale is explained in
http://dpdk.org/ml/archives/dev/2014-September/005232.html

"The format of the metadata is up to the application".

The type of data the application stores after the mbuf has not
to be defined in the mbuf. These macros limits the types of
metadata to uint8, uint16, uint32, uint64? What should I do
if I need a void*, a struct foo ? Should we add a macro for
each possible type?

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

Defining a structure in the application which does not pollute
the rte_mbuf structure is "easy and standard(TM)" too.

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

Having the following is clean too:

struct metadata {
     ...
};

struct app_mbuf {
     struct rte_mbuf mbuf;
     struct metadata metadata;
};

There is no need to define anything in the mbuf 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?

Waow. Five occurences of "standard" until now. Could you give a
reference to the standard you're refering to? :)

Our application defines private metadata in mbufs in the way described
above, we never changed that since we're supporting the dpdk. So
I don't understand when you say that each time mbuf is reformatted
it breaks the application.

> Having applications define their optional meta-data is a real
> need. 

Sure. This patch does not prevent this at all. You can continue
to do exactly the same, but in the concerned application, not
in the generic mbuf structure.

> 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

Six :)

I'm not sure these documents define the way to extend a packet
structure with metadata in a C program. Again, Bruce's patch does
not prevent to do what you need, it just moves it at the proper
place.

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

To summarize what I think:

- this patch does not prevent to do what you want to do
- removing the macros help to have a shorter and more comprehensible
  mbuf structure
- the previous approach does not scale because it would require
  a macro per type


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

This is a public mailing list, this disclaimer is invalid.

Regards,
Olivier
  

Hi Olivier,

I agree that your suggested approach for application-dependent metadata makes sense, in fact the two approaches work in exactly the same way (packet metadata immediately after the regular mbuf), there is only a subtle difference, which is related to defining consistent DPDK usage guidelines.

1. Advertising the presence of application-dependent meta-data as supported mechanism
If we explicitly have a metadata zero-size field at the end of the mbuf, we basically tell people that adding their own application meta-data at the end of the mandatory meta-data (mbuf structure) is a mechanism that DPDK allows and supports, and will continue to do so for the foreseeable future. In other words, we guarantee that an application doing so will continue to build successfully with future releases of DPDK, and we will not introduce changes in DPDK that could potentially break this mechanism. It is also a hint to people of where to put their application dependent meta-data.

2. Defining a standard base address for the application-dependent metadata
- There are also libraries in DPDK that work with application dependent meta-data, currently these are the Packet Framework libraries: librte_port, librte_table, librte_pipeline. Of course, the library does not have the knowledge of the application dependent meta-data format, so they treat it as opaque array of bytes, with the offset and size of the array given as arguments. In my opinion, it is safer (and more elegant) if these libraries (and others) can rely on an mbuf API to access the application dependent meta-data (in an opaque way) rather than make an assumption about the mbuf (i.e. the location of custom metadata relative to the mbuf) that is not clearly supported/defined by the mbuf library. 
- By having this API, we basically say: we define the custom meta-data base address (first location where custom metadata _could_ be placed) immediately after the mbuf, so libraries and apps accessing custom meta-data should do so by using a relative offset from this base rather than each application defining its own base: immediately after mbuf, or 128 bytes after mbuf, or 64 bytes before the end of the buffer, or other.

More (minor) comments inline below.

Thanks,
Cristian

-----Original Message-----
From: Olivier MATZ [mailto:olivier.matz@6wind.com] 
Sent: Friday, September 12, 2014 10:02 PM
To: Dumitrescu, Cristian; Richardson, Bruce; dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH 07/13] mbuf: use macros only to access the mbuf metadata

Hello Cristian,

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

The rationale is explained in
http://dpdk.org/ml/archives/dev/2014-September/005232.html

"The format of the metadata is up to the application".

The type of data the application stores after the mbuf has not
to be defined in the mbuf. These macros limits the types of
metadata to uint8, uint16, uint32, uint64? What should I do
if I need a void*, a struct foo ? Should we add a macro for
each possible type?

[Cristian] Actually, this is not correct, as macros to access metadata through pointers (to void or scalar types) are provided as well. This pointer can be converted by the application to the format is defines.

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

Defining a structure in the application which does not pollute
the rte_mbuf structure is "easy and standard(TM)" too.

[Cristian] I agree, both approaches work the same really, it is just the difference in advertising the presence of meta-data as supported mechanism and defining a standard base address for it.

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

Having the following is clean too:

struct metadata {
     ...
};

struct app_mbuf {
     struct rte_mbuf mbuf;
     struct metadata metadata;
};

There is no need to define anything in the mbuf structure.

[Cristian] I agree, both approaches work the same really, it is just the difference in advertising the presence of meta-data as supported mechanism and defining a standard base address for it.

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

Waow. Five occurences of "standard" until now. 
[Cristian] I am sorry :)

Could you give a
reference to the standard you're refering to? :)

[Cristian] See the IEFT Service Function Chaining link below, the environment is different (data center pipeline vs. CPU core-level pipeline), but the concepts are very similar.

Our application defines private metadata in mbufs in the way described
above, we never changed that since we're supporting the dpdk. So
I don't understand when you say that each time mbuf is reformatted
it breaks the application.

> Having applications define their optional meta-data is a real
> need. 

Sure. This patch does not prevent this at all. You can continue
to do exactly the same, but in the concerned application, not
in the generic mbuf structure.


> 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

Six :)

I'm not sure these documents define the way to extend a packet
structure with metadata in a C program. Again, Bruce's patch does
not prevent to do what you need, it just moves it at the proper
place.

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

To summarize what I think:

- this patch does not prevent to do what you want to do
- removing the macros help to have a shorter and more comprehensible
  mbuf structure
- the previous approach does not scale because it would require
  a macro per type


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

This is a public mailing list, this disclaimer is invalid.

Regards,
Olivier

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

I completely agree with Cristian here, instead of leaving to applications where to place their meta data, we can provide a guidance by having this field about placement of application meta while maintaining transparency on the contents of application meta information. 

Regards
Kannan Babu Ramia
Sr.System Architect
Communication Storage Infrastructure Group
DCG
-----Original Message-----
From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Dumitrescu, Cristian
Sent: Wednesday, September 17, 2014 1:37 AM
To: Olivier MATZ; Richardson, Bruce; dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH 07/13] mbuf: use macros only to access the mbuf metadata

Hi Olivier,

I agree that your suggested approach for application-dependent metadata makes sense, in fact the two approaches work in exactly the same way (packet metadata immediately after the regular mbuf), there is only a subtle difference, which is related to defining consistent DPDK usage guidelines.

1. Advertising the presence of application-dependent meta-data as supported mechanism If we explicitly have a metadata zero-size field at the end of the mbuf, we basically tell people that adding their own application meta-data at the end of the mandatory meta-data (mbuf structure) is a mechanism that DPDK allows and supports, and will continue to do so for the foreseeable future. In other words, we guarantee that an application doing so will continue to build successfully with future releases of DPDK, and we will not introduce changes in DPDK that could potentially break this mechanism. It is also a hint to people of where to put their application dependent meta-data.

2. Defining a standard base address for the application-dependent metadata
- There are also libraries in DPDK that work with application dependent meta-data, currently these are the Packet Framework libraries: librte_port, librte_table, librte_pipeline. Of course, the library does not have the knowledge of the application dependent meta-data format, so they treat it as opaque array of bytes, with the offset and size of the array given as arguments. In my opinion, it is safer (and more elegant) if these libraries (and others) can rely on an mbuf API to access the application dependent meta-data (in an opaque way) rather than make an assumption about the mbuf (i.e. the location of custom metadata relative to the mbuf) that is not clearly supported/defined by the mbuf library. 
- By having this API, we basically say: we define the custom meta-data base address (first location where custom metadata _could_ be placed) immediately after the mbuf, so libraries and apps accessing custom meta-data should do so by using a relative offset from this base rather than each application defining its own base: immediately after mbuf, or 128 bytes after mbuf, or 64 bytes before the end of the buffer, or other.

More (minor) comments inline below.

Thanks,
Cristian

-----Original Message-----
From: Olivier MATZ [mailto:olivier.matz@6wind.com]
Sent: Friday, September 12, 2014 10:02 PM
To: Dumitrescu, Cristian; Richardson, Bruce; dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH 07/13] mbuf: use macros only to access the mbuf metadata

Hello Cristian,

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

The rationale is explained in
http://dpdk.org/ml/archives/dev/2014-September/005232.html

"The format of the metadata is up to the application".

The type of data the application stores after the mbuf has not to be defined in the mbuf. These macros limits the types of metadata to uint8, uint16, uint32, uint64? What should I do if I need a void*, a struct foo ? Should we add a macro for each possible type?

[Cristian] Actually, this is not correct, as macros to access metadata through pointers (to void or scalar types) are provided as well. This pointer can be converted by the application to the format is defines.

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

Defining a structure in the application which does not pollute the rte_mbuf structure is "easy and standard(TM)" too.

[Cristian] I agree, both approaches work the same really, it is just the difference in advertising the presence of meta-data as supported mechanism and defining a standard base address for it.

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

Having the following is clean too:

struct metadata {
     ...
};

struct app_mbuf {
     struct rte_mbuf mbuf;
     struct metadata metadata;
};

There is no need to define anything in the mbuf structure.

[Cristian] I agree, both approaches work the same really, it is just the difference in advertising the presence of meta-data as supported mechanism and defining a standard base address for it.

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

Waow. Five occurences of "standard" until now. 
[Cristian] I am sorry :)

Could you give a
reference to the standard you're refering to? :)

[Cristian] See the IEFT Service Function Chaining link below, the environment is different (data center pipeline vs. CPU core-level pipeline), but the concepts are very similar.

Our application defines private metadata in mbufs in the way described above, we never changed that since we're supporting the dpdk. So I don't understand when you say that each time mbuf is reformatted it breaks the application.

> Having applications define their optional meta-data is a real need.

Sure. This patch does not prevent this at all. You can continue to do exactly the same, but in the concerned application, not in the generic mbuf structure.


> 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

Six :)

I'm not sure these documents define the way to extend a packet structure with metadata in a C program. Again, Bruce's patch does not prevent to do what you need, it just moves it at the proper place.

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

To summarize what I think:

- this patch does not prevent to do what you want to do
- removing the macros help to have a shorter and more comprehensible
  mbuf structure
- the previous approach does not scale because it would require
  a macro per type


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

This is a public mailing list, this disclaimer is invalid.

Regards,
Olivier

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

> -----Original Message-----
> From: Ramia, Kannan Babu
> Sent: Tuesday, September 16, 2014 11:06 PM
> To: Dumitrescu, Cristian; Olivier MATZ; Richardson, Bruce; dev@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH 07/13] mbuf: use macros only to access the
> mbuf metadata
> 
> I completely agree with Cristian here, instead of leaving to applications where to
> place their meta data, we can provide a guidance by having this field about
> placement of application meta while maintaining transparency on the contents
> of application meta information.
> 
My opinion on this is that this is better served via documentation or a comment in the code. The reason is that this approach is not going to be suitable for all applications. The mbuf headroom being used by the metadata is actually designed to be used for any additional headers to be added to the packet - though other things can obviously be stored in it too. Therefore the amount of metadata that can be stored in it will depend from application to application, as any apps doing e.g. tunnelling will need the headroom for tunnelling headers and may only be able to store a small amount of metadata - potentially none. For larger amounts of metadata - I would feel that anything over 64-bytes or so - I have proposed adding in a separate userdata pointer in the mbuf structure so that apps have the option of storing the metadata externally e.g. pointing to a flow table entry or similar. [Please see mbuf rework patch set 3 proposal].
Because of this, I think it's better to put in a comment in the code indicating that metadata can go in the headroom, document this properly - including caveats and limitations - in the documentation, and provide an example of doing such - something we already have in the packet framework.

All that being said, and while I think this is a good patch, I don't feel too strongly about it. I'm happy enough if this particular patch does not get merged in for 1.8, as it's incidental to the overall mbuf changes.

Regards,
/Bruce


> Regards
> Kannan Babu Ramia
> Sr.System Architect
> Communication Storage Infrastructure Group
> DCG
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Dumitrescu, Cristian
> Sent: Wednesday, September 17, 2014 1:37 AM
> To: Olivier MATZ; Richardson, Bruce; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH 07/13] mbuf: use macros only to access the
> mbuf metadata
> 
> Hi Olivier,
> 
> I agree that your suggested approach for application-dependent metadata
> makes sense, in fact the two approaches work in exactly the same way (packet
> metadata immediately after the regular mbuf), there is only a subtle difference,
> which is related to defining consistent DPDK usage guidelines.
> 
> 1. Advertising the presence of application-dependent meta-data as supported
> mechanism If we explicitly have a metadata zero-size field at the end of the
> mbuf, we basically tell people that adding their own application meta-data at
> the end of the mandatory meta-data (mbuf structure) is a mechanism that DPDK
> allows and supports, and will continue to do so for the foreseeable future. In
> other words, we guarantee that an application doing so will continue to build
> successfully with future releases of DPDK, and we will not introduce changes in
> DPDK that could potentially break this mechanism. It is also a hint to people of
> where to put their application dependent meta-data.
> 
> 2. Defining a standard base address for the application-dependent metadata
> - There are also libraries in DPDK that work with application dependent meta-
> data, currently these are the Packet Framework libraries: librte_port,
> librte_table, librte_pipeline. Of course, the library does not have the knowledge
> of the application dependent meta-data format, so they treat it as opaque array
> of bytes, with the offset and size of the array given as arguments. In my opinion,
> it is safer (and more elegant) if these libraries (and others) can rely on an mbuf
> API to access the application dependent meta-data (in an opaque way) rather
> than make an assumption about the mbuf (i.e. the location of custom metadata
> relative to the mbuf) that is not clearly supported/defined by the mbuf library.
> - By having this API, we basically say: we define the custom meta-data base
> address (first location where custom metadata _could_ be placed) immediately
> after the mbuf, so libraries and apps accessing custom meta-data should do so
> by using a relative offset from this base rather than each application defining its
> own base: immediately after mbuf, or 128 bytes after mbuf, or 64 bytes before
> the end of the buffer, or other.
> 
> More (minor) comments inline below.
> 
> Thanks,
> Cristian
> 
> -----Original Message-----
> From: Olivier MATZ [mailto:olivier.matz@6wind.com]
> Sent: Friday, September 12, 2014 10:02 PM
> To: Dumitrescu, Cristian; Richardson, Bruce; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH 07/13] mbuf: use macros only to access the
> mbuf metadata
> 
> Hello Cristian,
> 
> > What is the reason to remove this field? Please explain the rationale
> > of removing this field.
> 
> The rationale is explained in
> http://dpdk.org/ml/archives/dev/2014-September/005232.html
> 
> "The format of the metadata is up to the application".
> 
> The type of data the application stores after the mbuf has not to be defined in
> the mbuf. These macros limits the types of metadata to uint8, uint16, uint32,
> uint64? What should I do if I need a void*, a struct foo ? Should we add a macro
> for each possible type?
> 
> [Cristian] Actually, this is not correct, as macros to access metadata through
> pointers (to void or scalar types) are provided as well. This pointer can be
> converted by the application to the format is defines.
> 
> > 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.
> 
> Defining a structure in the application which does not pollute the rte_mbuf
> structure is "easy and standard(TM)" too.
> 
> [Cristian] I agree, both approaches work the same really, it is just the difference
> in advertising the presence of meta-data as supported mechanism and defining a
> standard base address for it.
> 
> > 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.
> 
> Having the following is clean too:
> 
> struct metadata {
>      ...
> };
> 
> struct app_mbuf {
>      struct rte_mbuf mbuf;
>      struct metadata metadata;
> };
> 
> There is no need to define anything in the mbuf structure.
> 
> [Cristian] I agree, both approaches work the same really, it is just the difference
> in advertising the presence of meta-data as supported mechanism and defining a
> standard base address for it.
> 
> >
> > 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?
> 
> Waow. Five occurences of "standard" until now.
> [Cristian] I am sorry :)
> 
> Could you give a
> reference to the standard you're refering to? :)
> 
> [Cristian] See the IEFT Service Function Chaining link below, the environment is
> different (data center pipeline vs. CPU core-level pipeline), but the concepts are
> very similar.
> 
> Our application defines private metadata in mbufs in the way described above,
> we never changed that since we're supporting the dpdk. So I don't understand
> when you say that each time mbuf is reformatted it breaks the application.
> 
> > Having applications define their optional meta-data is a real need.
> 
> Sure. This patch does not prevent this at all. You can continue to do exactly the
> same, but in the concerned application, not in the generic mbuf structure.
> 
> 
> > 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
> 
> Six :)
> 
> I'm not sure these documents define the way to extend a packet structure with
> metadata in a C program. Again, Bruce's patch does not prevent to do what you
> need, it just moves it at the proper place.
> 
> > 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.
> 
> To summarize what I think:
> 
> - this patch does not prevent to do what you want to do
> - removing the macros help to have a shorter and more comprehensible
>   mbuf structure
> - the previous approach does not scale because it would require
>   a macro per type
> 
> 
> > --------------------------------------------------------------
> > 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.
> 
> This is a public mailing list, this disclaimer is invalid.
> 
> Regards,
> Olivier
> 
> --------------------------------------------------------------
> 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.
>
  

2014-09-17 10:31, Richardson, Bruce:
> > From: Ramia, Kannan Babu
> > 
> > I completely agree with Cristian here, instead of leaving to applications
> > where to place their meta data, we can provide a guidance by having this
> > field about placement of application meta while maintaining transparency
> > on the contents of application meta information.
> 
> My opinion on this is that this is better served via documentation or a
> comment in the code. The reason is that this approach is not going to be
> suitable for all applications. The mbuf headroom being used by the metadata
> is actually designed to be used for any additional headers to be added to
> the packet - though other things can obviously be stored in it too.
> Therefore the amount of metadata that can be stored in it will depend from
> application to application, as any apps doing e.g. tunnelling will need the
> headroom for tunnelling headers and may only be able to store a small
> amount of metadata - potentially none. For larger amounts of metadata - I
> would feel that anything over 64-bytes or so - I have proposed adding in a
> separate userdata pointer in the mbuf structure so that apps have the
> option of storing the metadata externally e.g. pointing to a flow table
> entry or similar. [Please see mbuf rework patch set 3 proposal]. Because of
> this, I think it's better to put in a comment in the code indicating that
> metadata can go in the headroom, document this properly - including caveats
> and limitations - in the documentation, and provide an example of doing
> such - something we already have in the packet framework.

I agree that replacing these markers by documentation give more accurate
informations and (bonus) is simpler.
When documentation will be embedded in the git repository, I'd like to see
a patch to document these mbuf usages.

> All that being said, and while I think this is a good patch, I don't feel
> too strongly about it. I'm happy enough if this particular patch does not
> get merged in for 1.8, as it's incidental to the overall mbuf changes.

I think also it's a good patch so I keep it.