From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail.droids-corp.org (zoll.droids-corp.org [94.23.50.67]) by dpdk.org (Postfix) with ESMTP id 73D6F8DA8 for ; Fri, 20 Nov 2015 16:27:10 +0100 (CET) Received: from was59-1-82-226-113-214.fbx.proxad.net ([82.226.113.214] helo=[192.168.0.10]) by mail.droids-corp.org with esmtpsa (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.84) (envelope-from ) id 1ZznbH-00048o-S9; Fri, 20 Nov 2015 16:27:49 +0100 Message-ID: <564F3BC5.90308@6wind.com> Date: Fri, 20 Nov 2015 16:27:01 +0100 From: Olivier MATZ User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Icedove/31.7.0 MIME-Version: 1.0 To: Declan Doherty , 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> In-Reply-To: <1447441090-8129-7-git-send-email-declan.doherty@intel.com> Content-Type: text/plain; charset=windows-1252 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 15:27:10 -0000 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. >>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. 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? > +/** > + * 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. Here we expect that m->offload_ops == NULL at mbuf initialization. I cannot find where it is initialized. > + > + > +/** 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? 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. 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); } 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); Or even: rx_burst(port, q, crypto_tab, n); with each *cryto_tab pointing to a mbuf 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 Regards, Olivier