From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by dpdk.org (Postfix) with ESMTP id 6085E333 for ; Wed, 17 Sep 2014 12:26:57 +0200 (CEST) Received: from azsmga001.ch.intel.com ([10.2.17.19]) by orsmga101.jf.intel.com with ESMTP; 17 Sep 2014 03:32:36 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.04,540,1406617200"; d="scan'208";a="478095754" Received: from irsmsx102.ger.corp.intel.com ([163.33.3.155]) by azsmga001.ch.intel.com with ESMTP; 17 Sep 2014 03:32:34 -0700 Received: from irsmsx103.ger.corp.intel.com ([169.254.3.112]) by IRSMSX102.ger.corp.intel.com ([169.254.2.24]) with mapi id 14.03.0195.001; Wed, 17 Sep 2014 11:31:36 +0100 From: "Richardson, Bruce" To: "Ramia, Kannan Babu" , "Dumitrescu, Cristian" , Olivier MATZ , "dev@dpdk.org" Thread-Topic: [dpdk-dev] [PATCH 07/13] mbuf: use macros only to access the mbuf metadata Thread-Index: AQHPx46wQcWhgcKKB0epuTO3xpr155v3GoqAgAFviACABTV2wIAAOk6AgAY59YCAACFCgIAAziWw Date: Wed, 17 Sep 2014 10:31:35 +0000 Message-ID: <59AF69C657FD0841A61C55336867B5B0343F2BD2@IRSMSX103.ger.corp.intel.com> References: <1409759378-10113-1-git-send-email-bruce.richardson@intel.com> <1409759378-10113-8-git-send-email-bruce.richardson@intel.com> <540D9B95.3020504@6wind.com> <59AF69C657FD0841A61C55336867B5B0343EFAA3@IRSMSX103.ger.corp.intel.com> <3EB4FA525960D640B5BDFFD6A3D891262E070D42@IRSMSX108.ger.corp.intel.com> <54135F63.2090401@6wind.com> <3EB4FA525960D640B5BDFFD6A3D891262E071FE6@IRSMSX108.ger.corp.intel.com> <682698A055A0F44AA47192B20141149711B1FFE6@BGSMSX102.gar.corp.intel.com> In-Reply-To: <682698A055A0F44AA47192B20141149711B1FFE6@BGSMSX102.gar.corp.intel.com> Accept-Language: en-GB, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [163.33.239.180] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH 07/13] mbuf: use macros only to access the mbuf metadata X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 17 Sep 2014 10:26:58 -0000 > -----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 >=20 > 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 abo= ut > placement of application meta while maintaining transparency on the conte= nts > of application meta information. >=20 My opinion on this is that this is better served via documentation or a com= ment in the code. The reason is that this approach is not going to be suita= ble for all applications. The mbuf headroom being used by the metadata is a= ctually designed to be used for any additional headers to be added to the p= acket - though other things can obviously be stored in it too. Therefore th= e 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 fo= r tunnelling headers and may only be able to store a small amount of metada= ta - potentially none. For larger amounts of metadata - I would feel that a= nything 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 indica= ting that metadata can go in the headroom, document this properly - includi= ng caveats and limitations - in the documentation, and provide an example o= f 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 t= oo strongly about it. I'm happy enough if this particular patch does not ge= t 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 >=20 > Hi Olivier, >=20 > I agree that your suggested approach for application-dependent metadata > makes sense, in fact the two approaches work in exactly the same way (pac= ket > metadata immediately after the regular mbuf), there is only a subtle diff= erence, > which is related to defining consistent DPDK usage guidelines. >=20 > 1. Advertising the presence of application-dependent meta-data as support= ed > 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-dat= a at > the end of the mandatory meta-data (mbuf structure) is a mechanism that D= PDK > allows and supports, and will continue to do so for the foreseeable futur= e. In > other words, we guarantee that an application doing so will continue to b= uild > successfully with future releases of DPDK, and we will not introduce chan= ges in > DPDK that could potentially break this mechanism. It is also a hint to pe= ople of > where to put their application dependent meta-data. >=20 > 2. Defining a standard base address for the application-dependent metadat= a > - There are also libraries in DPDK that work with application dependent m= eta- > data, currently these are the Packet Framework libraries: librte_port, > librte_table, librte_pipeline. Of course, the library does not have the k= nowledge > 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 o= n an mbuf > API to access the application dependent meta-data (in an opaque way) rath= er > than make an assumption about the mbuf (i.e. the location of custom metad= ata > relative to the mbuf) that is not clearly supported/defined by the mbuf l= ibrary. > - By having this API, we basically say: we define the custom meta-data ba= se > address (first location where custom metadata _could_ be placed) immediat= ely > after the mbuf, so libraries and apps accessing custom meta-data should d= o so > by using a relative offset from this base rather than each application de= fining its > own base: immediately after mbuf, or 128 bytes after mbuf, or 64 bytes be= fore > the end of the buffer, or other. >=20 > More (minor) comments inline below. >=20 > Thanks, > Cristian >=20 > -----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 >=20 > Hello Cristian, >=20 > > What is the reason to remove this field? Please explain the rationale > > of removing this field. >=20 > The rationale is explained in > http://dpdk.org/ml/archives/dev/2014-September/005232.html >=20 > "The format of the metadata is up to the application". >=20 > The type of data the application stores after the mbuf has not to be defi= ned in > the mbuf. These macros limits the types of metadata to uint8, uint16, uin= t32, > uint64? What should I do if I need a void*, a struct foo ? Should we add = a macro > for each possible type? >=20 > [Cristian] Actually, this is not correct, as macros to access metadata th= rough > pointers (to void or scalar types) are provided as well. This pointer can= be > converted by the application to the format is defines. >=20 > > 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. >=20 > Defining a structure in the application which does not pollute the rte_mb= uf > structure is "easy and standard(TM)" too. >=20 > [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 defin= ing a > standard base address for it. >=20 > > 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. >=20 > Having the following is clean too: >=20 > struct metadata { > ... > }; >=20 > struct app_mbuf { > struct rte_mbuf mbuf; > struct metadata metadata; > }; >=20 > There is no need to define anything in the mbuf structure. >=20 > [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 defin= ing a > standard base address for it. >=20 > > > > 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? >=20 > Waow. Five occurences of "standard" until now. > [Cristian] I am sorry :) >=20 > Could you give a > reference to the standard you're refering to? :) >=20 > [Cristian] See the IEFT Service Function Chaining link below, the environ= ment is > different (data center pipeline vs. CPU core-level pipeline), but the con= cepts are > very similar. >=20 > Our application defines private metadata in mbufs in the way described ab= ove, > we never changed that since we're supporting the dpdk. So I don't underst= and > when you say that each time mbuf is reformatted it breaks the application= . >=20 > > Having applications define their optional meta-data is a real need. >=20 > Sure. This patch does not prevent this at all. You can continue to do exa= ctly the > same, but in the concerned application, not in the generic mbuf structure= . >=20 >=20 > > 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 >=20 > Six :) >=20 > 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. >=20 > > 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. >=20 > To summarize what I think: >=20 > - 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 >=20 >=20 > > -------------------------------------------------------------- > > 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 t= he 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 sen= der and > delete all copies. >=20 > This is a public mailing list, this disclaimer is invalid. >=20 > Regards, > Olivier >=20 > -------------------------------------------------------------- > Intel Shannon Limited > Registered in Ireland > Registered Office: Collinstown Industrial Park, Leixlip, County Kildare R= egistered > Number: 308263 Business address: Dromore House, East Park, Shannon, Co. > Clare >=20 > 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 sen= der and > delete all copies. >=20