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 1BE435A6C for ; Fri, 20 Nov 2015 18:27:38 +0100 (CET) Received: from orsmga003.jf.intel.com ([10.7.209.27]) by orsmga101.jf.intel.com with ESMTP; 20 Nov 2015 09:27:39 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.20,323,1444719600"; d="scan'208";a="690662867" Received: from dwdohert-dpdk.ir.intel.com ([163.33.213.167]) by orsmga003.jf.intel.com with ESMTP; 20 Nov 2015 09:27:37 -0800 To: Olivier MATZ , dev@dpdk.org 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> From: Declan Doherty Message-ID: <564F57CE.2030203@intel.com> Date: Fri, 20 Nov 2015 17:26:38 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 MIME-Version: 1.0 In-Reply-To: <564F3BC5.90308@6wind.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit 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: Fri, 20 Nov 2015 17:27:40 -0000 Hey Oliver, see reply inline. On 20/11/15 15:27, Olivier MATZ wrote: > Hi Declan, > > Please find some comments below. > > On 11/13/2015 07:58 PM, Declan Doherty wrote: >> This library add support for adding a chain of offload operations to a >> mbuf. It contains the definition of the rte_mbuf_offload structure as >> well as helper functions for attaching offloads to mbufs and a mempool >> management functions. >> >> This initial implementation supports attaching multiple offload >> operations to a single mbuf, but only a single offload operation of a >> specific type can be attach to that mbuf. >> >> Signed-off-by: Declan Doherty >> Acked-by: Konstantin Ananyev >> --- >> MAINTAINERS | 4 + >> config/common_bsdapp | 6 + >> config/common_linuxapp | 6 + >> doc/api/doxy-api-index.md | 1 + >> doc/api/doxy-api.conf | 1 + >> lib/Makefile | 1 + >> lib/librte_mbuf/rte_mbuf.h | 6 + >> lib/librte_mbuf_offload/Makefile | 52 ++++ >> lib/librte_mbuf_offload/rte_mbuf_offload.c | 100 +++++++ >> lib/librte_mbuf_offload/rte_mbuf_offload.h | 302 +++++++++++++++++++++ >> .../rte_mbuf_offload_version.map | 7 + >> mk/rte.app.mk | 1 + >> 12 files changed, 487 insertions(+) >> create mode 100644 lib/librte_mbuf_offload/Makefile >> create mode 100644 lib/librte_mbuf_offload/rte_mbuf_offload.c >> create mode 100644 lib/librte_mbuf_offload/rte_mbuf_offload.h >> create mode 100644 lib/librte_mbuf_offload/rte_mbuf_offload_version.map > > 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! > > From what I see, it is not said in the help of > rte_cryptodev_enqueue_burst() that the offload structure has to be > set in the given mbufs. > I need to update the API documentation to make that explicit. > Is my understanding correct? > > >> +/** >> + * @file >> + * RTE mbuf offload >> + * >> + * The rte_mbuf_offload library provides the ability to specify a device generic >> + * off-load operation independent of the current Rx/Tx Ethernet offloads >> + * supported within the rte_mbuf structure, and add supports for multiple >> + * off-load operations and offload device types. >> + * >> + * The rte_mbuf_offload specifies the particular off-load operation type, such >> + * as a crypto operation, and provides a container for the operations >> + * parameter's inside the op union. These parameters are then used by the >> + * device which supports that operation to perform the specified offload. >> + * >> + * This library provides an API to create pre-allocated mempool of offload >> + * operations, with supporting allocate and free functions. It also provides >> + * APIs for attaching an offload to a mbuf, as well as an API to retrieve a >> + * specified offload type from an mbuf offload chain. >> + */ >> + >> +#include >> +#include >> + >> + >> +/** packet mbuf offload operation types */ >> +enum rte_mbuf_ol_op_type { >> + RTE_PKTMBUF_OL_NOT_SPECIFIED = 0, >> + /**< Off-load not specified */ >> + RTE_PKTMBUF_OL_CRYPTO >> + /**< Crypto offload operation */ >> +}; > > Here, the mbuf offload API is presented as a generic API for offload. > But it includes rte_crypto. Actually, it means that at the end this > API needs to be aware of any offload type. > > Wouldn't it be possible to hide the knowledge of the metadata structure > to this API? The design makes the offload API aware of the underlying offload operations, but I don't see this as a problem, the main idea was to have a no dependencies other than the structure pointer added to the rte_mbuf, while also making the offload extensible in the future. Also this approach makes the management of the offload elements very straight forward, with very simplified pool management functions and also there isn't the need for another level of indirection to the actual offload operation. > > >> +/** >> + * Attach a rte_mbuf_offload to a mbuf. We only support a single offload of any >> + * one type in our chain of offloads. >> + * >> + * @param m packet mbuf. >> + * @param ol rte_mbuf_offload strucutre to be attached >> + * >> + * @returns >> + * - On success returns the pointer to the offload we just added >> + * - On failure returns NULL >> + */ >> +static inline struct rte_mbuf_offload * >> +rte_pktmbuf_offload_attach(struct rte_mbuf *m, struct rte_mbuf_offload *ol) >> +{ >> + struct rte_mbuf_offload **ol_last; >> + >> + for (ol_last = &m->offload_ops; ol_last[0] != NULL; >> + ol_last = &ol_last[0]->next) >> + if (ol_last[0]->type == ol->type) >> + return NULL; >> + >> + ol_last[0] = ol; >> + ol_last[0]->m = m; >> + ol_last[0]->next = NULL; >> + >> + return ol_last[0]; >> +} > > *ol_last would be clearer than ol_last[0] as it's not a table. Just a personal preference, but I can submit a change later. > Here we expect that m->offload_ops == NULL at mbuf initialization. > I cannot find where it is initialized. > I'll need to add the initialization to rte_pktmbuf_reset() >> + >> + >> +/** 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 = NULL; >> + ol->type = 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 > > 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 mbuf. > > I'm wondering if there is such a leak in l2fwd_crypto_send_burst(): > > /* <<<<<<< here packets have the offload attached */ > ret = rte_cryptodev_enqueue_burst(cparams->dev_id, > cparams->qp_id, pkt_buffer, (uint16_t) n); > crypto_statistics[cparams->dev_id].enqueued += ret; > if (unlikely(ret < n)) { > crypto_statistics[cparams->dev_id].errors += (n - ret); > do { > /* <<<<<<<<< offload struct is lost? */ > rte_pktmbuf_free(pkt_buffer[ret]); > } while (++ret < n); > } > > I'll push a fix for this leak, I just noticed it myself this morning. > 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 and 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. > > If we really want to use mbufs to store the crypto info (is it > something we want? for pipelining?), another idea would be to use > the usual metadata after the mbuf. It may require to enhance this > metadata framework to allow to better register room for metadata, > because today anyone using metadata expects that its metadata are > the only ones. > > Pros: > - no additional allocation for crypto offload struct > - no risk of leak > Cons: > - room is reserved for crypto in all mbufs even if no crypto > is done on them > I have some further API to add in R2.3 which would allow the offload to allocated in the private data of the mbuf but they are not fully tested as yet. Also I don't think it's practical from a memory management point of view to make the assumption that there will always be enough space in the mbufs private data, especially if a number of offloads were to be attached to a single mbuf. > > Regards, > Olivier >