From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by dpdk.org (Postfix) with ESMTP id 0E6048E8C for ; Mon, 23 Nov 2015 12:53:07 +0100 (CET) Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by orsmga102.jf.intel.com with ESMTP; 23 Nov 2015 03:52:46 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.20,336,1444719600"; d="scan'208";a="845229374" Received: from irsmsx110.ger.corp.intel.com ([163.33.3.25]) by fmsmga001.fm.intel.com with ESMTP; 23 Nov 2015 03:52:46 -0800 Received: from irsmsx112.ger.corp.intel.com (10.108.20.5) by irsmsx110.ger.corp.intel.com (163.33.3.25) with Microsoft SMTP Server (TLS) id 14.3.248.2; Mon, 23 Nov 2015 11:52:44 +0000 Received: from irsmsx105.ger.corp.intel.com ([169.254.7.203]) by irsmsx112.ger.corp.intel.com ([169.254.1.8]) with mapi id 14.03.0248.002; Mon, 23 Nov 2015 11:52:44 +0000 From: "Ananyev, Konstantin" To: Olivier MATZ , "Doherty, Declan" , "dev@dpdk.org" Thread-Topic: [dpdk-dev] [PATCH v7 06/10] mbuf_offload: library to support attaching offloads to a mbuf Thread-Index: AQHRI6gBCXzz8XZ44EqLRtiqD9k+xp6lKekAgAQsPoCAACu10A== Date: Mon, 23 Nov 2015 11:52:44 +0000 Message-ID: <2601191342CEEE43887BDE71AB97725836ACB2DA@irsmsx105.ger.corp.intel.com> References: <1447176763-19303-1-git-send-email-declan.doherty@intel.com> <1447441090-8129-1-git-send-email-declan.doherty@intel.com> <1447441090-8129-7-git-send-email-declan.doherty@intel.com> <564F3BC5.90308@6wind.com> <564F57CE.2030203@intel.com> <5652D7E9.7050202@6wind.com> In-Reply-To: <5652D7E9.7050202@6wind.com> Accept-Language: en-IE, 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 v7 06/10] mbuf_offload: library to support attaching offloads to a mbuf 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: Mon, 23 Nov 2015 11:53:08 -0000 Hi Olivier, > On 11/20/2015 06:26 PM, Declan Doherty wrote: > >> The new files are called rte_mbuf_offload, but from what I understand, > >> it is more like a mbuf metadata api. What you call "offload operation" > >> is not called because an offload is attached, but because you call > >> rte_cryptodev_enqueue_burst() on it. > > > > Maybe rte_mbuf_offload_metadata would be a better name, if not a bit > > more long winded :) The idea of this API set is to give a generic > > framework for attaching the the offload operation meta data to a mbuf > > which will be retrieved at a later point, when the particular offload > > burst function is called. I guess as we only have a single offload > > device at the moment the API may look a little over the top! >=20 > Indeed, not sure rte_mbuf_offload_metadata is better. > I'm still not convinced that offload should appear in the name, it > is a bit confusing with hardware offloads (ol_flags). Also, it > suggests that a work is delegated to another entity, but for instance > in this case it is just used as a storage area: >=20 > ol =3D rte_pktmbuf_offload_alloc(pool, RTE_PKTMBUF_OL_CRYPTO); > rte_crypto_op_attach_session(&ol->op.crypto, session); > ol->op.crypto.digest.data =3D rte_pktmbuf_append(m, digest_len); > ol->op.crypto.digest.phys_addr =3D ...; > /* ... */ > rte_pktmbuf_offload_attach(m, ol); > ret =3D rte_cryptodev_enqueue_burst(dev_id, qp_id, &m, 1); >=20 > Do you have some other examples in mind that would use this API? >=20 > >>> +/** Rearms rte_mbuf_offload default parameters */ > >>> +static inline void > >>> +__rte_pktmbuf_offload_reset(struct rte_mbuf_offload *ol, > >>> + enum rte_mbuf_ol_op_type type) > >>> +{ > >>> + ol->m =3D NULL; > >>> + ol->type =3D type; > >>> + > >>> + switch (type) { > >>> + case RTE_PKTMBUF_OL_CRYPTO: > >>> + __rte_crypto_op_reset(&ol->op.crypto); break; > >>> + default: > >>> + break; > >>> + } > >>> +} > >> > >> Would it work if several OL are registered? > > > > I can't see any reason why it wouldn't >=20 > Sorry, I read it to quickly. I thought it was a > rte_pktmbuf_offload_detach() function. By the way there is no > such function? >=20 >=20 > >> Also, what is not clear to me is how the offload structure is freed. > >> For instance, I think that calling rte_pktmbuf_free(m) on a mbuf > >> that has a offload attached would result in a leak. > >> > >> It would mean that it is not allowed to call any function that free or > >> reassemble a mbuf when an offload is attached. > > > > We just need to walk the chain of offloads calling > > rte_pktmbuf_offload_free(), before freeing the mbuf, which will be an > > issue with any externally attached meta data. In the case of > > reassembling I don't see why we would just move the chain to the head m= buf. >=20 > Walking the chain of offload + adding the initialization will probably > have a little cost that should be evaluated. >=20 > The freeing is probably not the only problem: > - packet duplication: are the offload infos copied? If no, it means that > the copy is not exactly a copy > - if you chain 2 mbufs that both have offload info attached, how does it > behave? > - if you prepend a segment to your mbuf, you need to copy the mbuf > offload pointer, and also parse the list of offload to update the > ol->m pointer of each element. >=20 > >> It seems that these offload structures are only used to pass crypto > >> info to the cryptodev. Would it be a problem to have an API like this? > >> > >> rx_burst(port, q, mbuf_tab, crypto_tab, n); > >> > > > > I really dislike this option, there's no direct linkage between mbuf an= d > > offload operation. > > > >> Or even: > >> > >> rx_burst(port, q, crypto_tab, n); > >> > >> with each *cryto_tab pointing to a mbuf > >> > > > > I looked at this but it would really hamstring any pipelining > > applications which might want to attach multiple offloads to a mbuf at = a > > point in the pipeline for processing at later steps. >=20 > As far as I can see, there is already a way to chain several crypto > operations in the crypto structure. >=20 > Another option is to use the mbuf offload API (or the m->userdata > pointer which already works for that) only in the application: >=20 > - the field is completely ignored by the mbuf api and the dpdk driver > - it is up to the application to initialize it and free it >=20 > -> it can only be used when passing mbuf from a part of the app > to another, so it perfectly matches the pipeline use case. I don't think we should start to re-use userdata. Userdata was intended for the upper layer app to pass/store it's private data associated with mbuf, and we probably should keep it this way. While mbuf_offload (or whatever we'll name it) supposed to keep data necessary for crypto (and other future type of devices) to operate with mbu= f. All your comments above about that this new field is just ignored by most m= buf operations (copy/attach/append/free, etc) are valid. By I suppose for now we can just state that for mbuf_offload is not support= ed by all these mbuf ops. I understand that current API is probably not perfect and might need to be = revised in future. Again, as it is completely new feature, I suspect it would be a lot of chan= ge requests for it anyway.=20 But we need some generic way to pass other (not ethdev) specific informatio= n within mbuf between user-level and PMDs for non-ethernet devices (and probably between = different PMDs too). Konstantin