DPDK patches and discussions
 help / color / mirror / Atom feed
From: "Dumitrescu, Cristian" <cristian.dumitrescu@intel.com>
To: Olivier MATZ <olivier.matz@6wind.com>,
	"Richardson, Bruce" <bruce.richardson@intel.com>,
	"dev@dpdk.org" <dev@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH 07/13] mbuf: use macros only to access the mbuf metadata
Date: Tue, 16 Sep 2014 20:07:27 +0000
Message-ID: <3EB4FA525960D640B5BDFFD6A3D891262E071FE6@IRSMSX108.ger.corp.intel.com> (raw)
In-Reply-To: <54135F63.2090401@6wind.com>

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.

  reply	other threads:[~2014-09-16 20:02 UTC|newest]

Thread overview: 62+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-09-03 15:49 [dpdk-dev] [PATCH 00/13] Mbuf Structure Rework, part 2 Bruce Richardson
2014-09-03 15:49 ` [dpdk-dev] [PATCH 01/13] mbuf: replace data pointer by an offset Bruce Richardson
2014-09-08  9:52   ` Olivier MATZ
2014-09-08  9:55     ` Olivier MATZ
2014-09-03 15:49 ` [dpdk-dev] [PATCH 02/13] mbuf: reorder fields by time of use Bruce Richardson
2014-09-08 10:17   ` Olivier MATZ
2014-09-03 15:49 ` [dpdk-dev] [PATCH 03/13] mbuf: add packet_type field Bruce Richardson
2014-09-08 10:17   ` Olivier MATZ
2014-09-08 10:33     ` Yerden Zhumabekov
2014-09-08 11:17       ` Olivier MATZ
2014-09-09  3:59         ` Zhang, Helin
     [not found]           ` <540EB428.9060706@6wind.com>
2014-09-09  8:45             ` Zhang, Helin
2014-09-09  9:47             ` Richardson, Bruce
2014-09-09 15:05         ` Jim Thompson
2014-09-03 15:49 ` [dpdk-dev] [PATCH 04/13] mbuf: expand ol_flags field to 64-bits Bruce Richardson
2014-09-08 10:25   ` Olivier MATZ
2014-09-09  9:00     ` Richardson, Bruce
2014-09-03 15:49 ` [dpdk-dev] [PATCH 05/13] mbuf: introduce a flag to indicate a control mbuf Bruce Richardson
2014-09-08 11:53   ` Olivier MATZ
2014-09-03 15:49 ` [dpdk-dev] [PATCH 06/13] mbuf: minor changes for readability Bruce Richardson
2014-09-08 12:03   ` Olivier MATZ
2014-09-03 15:49 ` [dpdk-dev] [PATCH 07/13] mbuf: use macros only to access the mbuf metadata Bruce Richardson
2014-09-08 12:05   ` Olivier MATZ
2014-09-09  9:01     ` Richardson, Bruce
2014-09-12 16:56       ` Dumitrescu, Cristian
2014-09-12 21:02         ` Olivier MATZ
2014-09-16 20:07           ` Dumitrescu, Cristian [this message]
2014-09-16 22:06             ` Ramia, Kannan Babu
2014-09-17 10:31               ` Richardson, Bruce
2014-09-17 14:01                 ` Thomas Monjalon
2014-09-10 15:09     ` Bruce Richardson
2014-09-10 15:31       ` Olivier MATZ
2014-09-03 15:49 ` [dpdk-dev] [PATCH 08/13] mbuf: add named points inside the mbuf structure Bruce Richardson
2014-09-08 12:08   ` Olivier MATZ
2014-09-03 15:49 ` [dpdk-dev] [PATCH 09/13] ixgbe: rework vector pmd following mbuf changes Bruce Richardson
2014-09-03 15:49 ` [dpdk-dev] [PATCH 10/13] mbuf: split mbuf across two cache lines Bruce Richardson
2014-09-08 12:10   ` Olivier MATZ
2014-09-03 15:49 ` [dpdk-dev] [PATCH 11/13] mbuf: move l2_len and l3_len to second cache line Bruce Richardson
2014-09-04  5:08   ` Yerden Zhumabekov
2014-09-04 10:27     ` Bruce Richardson
2014-09-04 11:00       ` Yerden Zhumabekov
2014-09-04 11:55         ` Bruce Richardson
2014-09-03 15:49 ` [dpdk-dev] [PATCH 12/13] ixgbe: Fix perf regression due to moved pool ptr Bruce Richardson
2014-09-03 15:49 ` [dpdk-dev] [PATCH 13/13] ixgbe: Improve slow-path perf: vector scattered RX Bruce Richardson
2014-09-11 13:15 ` [dpdk-dev] [PATCH v2 00/13] Mbuf Structure Rework, part 2 Bruce Richardson
2014-09-11 13:15   ` [dpdk-dev] [PATCH v2 01/13] mbuf: replace data pointer by an offset Bruce Richardson
2014-09-11 13:15   ` [dpdk-dev] [PATCH v2 02/13] mbuf: reorder fields by time of use Bruce Richardson
2014-09-15  7:11     ` Liu, Jijiang
2014-09-15  8:19       ` Richardson, Bruce
2014-09-11 13:15   ` [dpdk-dev] [PATCH v2 03/13] mbuf: expand ol_flags field to 64-bits Bruce Richardson
2014-09-11 13:15   ` [dpdk-dev] [PATCH v2 04/13] mbuf: introduce a flag to indicate a control mbuf Bruce Richardson
2014-09-11 13:15   ` [dpdk-dev] [PATCH v2 05/13] mbuf: minor changes for readability Bruce Richardson
2014-09-11 13:15   ` [dpdk-dev] [PATCH v2 06/13] mbuf: use macros only to access the mbuf metadata Bruce Richardson
2014-09-11 13:15   ` [dpdk-dev] [PATCH v2 07/13] mbuf: move metadata macros to rte_port library Bruce Richardson
2014-09-11 13:15   ` [dpdk-dev] [PATCH v2 08/13] mbuf: add named points inside the mbuf structure Bruce Richardson
2014-09-11 13:15   ` [dpdk-dev] [PATCH v2 09/13] ixgbe: rework vector pmd following mbuf changes Bruce Richardson
2014-09-11 13:15   ` [dpdk-dev] [PATCH v2 10/13] mbuf: split mbuf across two cache lines Bruce Richardson
2014-09-11 13:15   ` [dpdk-dev] [PATCH v2 11/13] mbuf: move l2_len and l3_len to second cache line Bruce Richardson
2014-09-11 13:15   ` [dpdk-dev] [PATCH v2 12/13] ixgbe: Fix perf regression due to moved pool ptr Bruce Richardson
2014-09-15 16:20     ` [dpdk-dev] [PATCH v3 " Bruce Richardson
2014-09-11 13:15   ` [dpdk-dev] [PATCH v2 13/13] ixgbe: Improve slow-path perf: vector scattered RX Bruce Richardson
2014-09-17 22:35   ` [dpdk-dev] [PATCH v2 00/13] Mbuf Structure Rework, part 2 Thomas Monjalon

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=3EB4FA525960D640B5BDFFD6A3D891262E071FE6@IRSMSX108.ger.corp.intel.com \
    --to=cristian.dumitrescu@intel.com \
    --cc=bruce.richardson@intel.com \
    --cc=dev@dpdk.org \
    --cc=olivier.matz@6wind.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git