From: Olivier MATZ <olivier.matz@6wind.com>
To: Declan Doherty <declan.doherty@intel.com>, dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH v7 06/10] mbuf_offload: library to support attaching offloads to a mbuf
Date: Fri, 20 Nov 2015 16:27:01 +0100 [thread overview]
Message-ID: <564F3BC5.90308@6wind.com> (raw)
In-Reply-To: <1447441090-8129-7-git-send-email-declan.doherty@intel.com>
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 <declan.doherty@intel.com>
> Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> ---
> 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 <rte_mbuf.h>
> +#include <rte_crypto.h>
> +
> +
> +/** 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
next prev parent reply other threads:[~2015-11-20 15:27 UTC|newest]
Thread overview: 115+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-10-02 23:01 [dpdk-dev] [PATCH 0/6] Crypto API and device framework Declan Doherty
2015-10-02 23:01 ` [dpdk-dev] [PATCH 1/6] cryptodev: Initial DPDK Crypto APIs and device framework release Declan Doherty
2015-10-21 9:24 ` Thomas Monjalon
2015-10-21 11:16 ` Declan Doherty
2015-10-02 23:01 ` [dpdk-dev] [PATCH 2/6] qat_crypto_pmd: Addition of a new QAT DPDK PMD Declan Doherty
2015-10-02 23:01 ` [dpdk-dev] [PATCH 3/6] aesni_mb_pmd: Initial implementation of multi buffer based crypto device Declan Doherty
2015-10-02 23:01 ` [dpdk-dev] [PATCH 4/6] docs: add getting started guides for multi-buffer pmd and qat pmd Declan Doherty
2015-10-21 11:34 ` Thomas Monjalon
2015-10-02 23:01 ` [dpdk-dev] [PATCH 5/6] app/test: add cryptodev unit and performance tests Declan Doherty
2015-10-02 23:01 ` [dpdk-dev] [PATCH 6/6] l2fwd-crypto: crypto Declan Doherty
2015-10-21 9:11 ` [dpdk-dev] [PATCH 0/6] Crypto API and device framework Declan Doherty
2015-10-30 12:59 ` [dpdk-dev] [PATCH v2 " Declan Doherty
2015-10-30 12:59 ` [dpdk-dev] [PATCH v2 1/6] cryptodev: Initial DPDK Crypto APIs and device framework release Declan Doherty
2015-10-30 12:59 ` [dpdk-dev] [PATCH v2 2/6] mbuf_offload: library to support attaching offloads to a mbuf Declan Doherty
2015-10-30 12:59 ` [dpdk-dev] [PATCH v2 3/6] qat_crypto_pmd: Addition of a new QAT DPDK PMD Declan Doherty
2015-10-30 12:59 ` [dpdk-dev] [PATCH v2 4/6] aesni_mb_pmd: Initial implementation of multi buffer based crypto device Declan Doherty
2015-10-30 12:59 ` [dpdk-dev] [PATCH v2 5/6] app/test: add cryptodev unit and performance tests Declan Doherty
2015-10-30 12:59 ` [dpdk-dev] [PATCH v2 6/6] l2fwd-crypto: crypto Declan Doherty
2015-10-30 16:08 ` [dpdk-dev] [PATCH v3 0/6] Crypto API and device framework Declan Doherty
2015-10-30 16:08 ` [dpdk-dev] [PATCH v3 1/6] cryptodev: Initial DPDK Crypto APIs and device framework release Declan Doherty
2015-10-30 16:08 ` [dpdk-dev] [PATCH v3 2/6] mbuf_offload: library to support attaching offloads to a mbuf Declan Doherty
2015-10-30 16:34 ` Ananyev, Konstantin
2015-10-30 16:08 ` [dpdk-dev] [PATCH v3 3/6] qat_crypto_pmd: Addition of a new QAT DPDK PMD Declan Doherty
2015-10-30 16:08 ` [dpdk-dev] [PATCH v3 4/6] aesni_mb_pmd: Initial implementation of multi buffer based crypto device Declan Doherty
2015-10-30 16:08 ` [dpdk-dev] [PATCH v3 5/6] app/test: add cryptodev unit and performance tests Declan Doherty
2015-10-30 16:08 ` [dpdk-dev] [PATCH v3 6/6] l2fwd-crypto: crypto Declan Doherty
2015-11-03 17:45 ` [dpdk-dev] [PATCH v4 0/6] Crypto API and device framework Declan Doherty
2015-11-03 17:45 ` [dpdk-dev] [PATCH v4 1/6] cryptodev: Initial DPDK Crypto APIs and device framework release Declan Doherty
2015-11-03 17:45 ` [dpdk-dev] [PATCH v4 2/6] mbuf_offload: library to support attaching offloads to a mbuf Declan Doherty
2015-11-03 17:45 ` [dpdk-dev] [PATCH v4 3/6] qat_crypto_pmd: Addition of a new QAT DPDK PMD Declan Doherty
2015-11-03 17:45 ` [dpdk-dev] [PATCH v4 4/6] aesni_mb_pmd: Initial implementation of multi buffer based crypto device Declan Doherty
2015-11-03 17:45 ` [dpdk-dev] [PATCH v4 5/6] app/test: add cryptodev unit and performance tests Declan Doherty
2015-11-03 17:45 ` [dpdk-dev] [PATCH v4 6/6] l2fwd-crypto: crypto Declan Doherty
2015-11-03 21:20 ` [dpdk-dev] [PATCH v4 0/6] Crypto API and device framework Sergio Gonzalez Monroy
2015-11-09 20:34 ` [dpdk-dev] [PATCH v5 00/10] " Declan Doherty
2015-11-09 20:34 ` [dpdk-dev] [PATCH v5 01/10] ethdev: rename macros to have RTE_ prefix Declan Doherty
2015-11-10 10:30 ` Bruce Richardson
2015-11-09 20:34 ` [dpdk-dev] [PATCH v5 02/10] ethdev: make error checking macros public Declan Doherty
2015-11-10 10:32 ` Bruce Richardson
2015-11-10 15:50 ` Adrien Mazarguil
2015-11-10 17:00 ` Declan Doherty
2015-11-09 20:34 ` [dpdk-dev] [PATCH v5 03/10] eal: add __rte_packed /__rte_aligned macros Declan Doherty
2015-11-09 20:34 ` [dpdk-dev] [PATCH v5 04/10] mbuf: add new marcos to get the physical address of data Declan Doherty
2015-11-09 20:34 ` [dpdk-dev] [PATCH v5 05/10] cryptodev: Initial DPDK Crypto APIs and device framework release Declan Doherty
2015-11-09 20:34 ` [dpdk-dev] [PATCH v5 06/10] mbuf_offload: library to support attaching offloads to a mbuf Declan Doherty
2015-11-09 20:34 ` [dpdk-dev] [PATCH v5 07/10] qat_crypto_pmd: Addition of a new QAT DPDK PMD Declan Doherty
2015-11-09 20:34 ` [dpdk-dev] [PATCH v5 08/10] aesni_mb_pmd: Initial implementation of multi buffer based crypto device Declan Doherty
2015-11-09 20:34 ` [dpdk-dev] [PATCH v5 09/10] app/test: add cryptodev unit and performance tests Declan Doherty
2015-11-09 20:34 ` [dpdk-dev] [PATCH v5 10/10] l2fwd-crypto: crypto Declan Doherty
2015-11-10 17:32 ` [dpdk-dev] [PATCH v6 00/10] Crypto API and device framework Declan Doherty
2015-11-10 17:32 ` [dpdk-dev] [PATCH v6 01/10] ethdev: rename macros to have RTE_ prefix Declan Doherty
2015-11-10 17:32 ` [dpdk-dev] [PATCH v6 02/10] ethdev: make error checking macros public Declan Doherty
2015-11-10 17:38 ` Adrien Mazarguil
2015-11-10 17:32 ` [dpdk-dev] [PATCH v6 03/10] eal: add __rte_packed /__rte_aligned macros Declan Doherty
2015-11-13 15:35 ` Thomas Monjalon
2015-11-13 15:41 ` Declan Doherty
2015-11-10 17:32 ` [dpdk-dev] [PATCH v6 04/10] mbuf: add new marcos to get the physical address of data Declan Doherty
2015-11-10 17:32 ` [dpdk-dev] [PATCH v6 05/10] cryptodev: Initial DPDK Crypto APIs and device framework release Declan Doherty
2015-11-13 15:44 ` Thomas Monjalon
2015-11-10 17:32 ` [dpdk-dev] [PATCH v6 06/10] mbuf_offload: library to support attaching offloads to a mbuf Declan Doherty
2015-11-13 15:59 ` Thomas Monjalon
2015-11-13 16:11 ` Thomas Monjalon
2015-11-10 17:32 ` [dpdk-dev] [PATCH v6 07/10] qat_crypto_pmd: Addition of a new QAT DPDK PMD Declan Doherty
2015-11-13 16:00 ` Thomas Monjalon
2015-11-13 16:25 ` Declan Doherty
2015-11-10 17:32 ` [dpdk-dev] [PATCH v6 08/10] aesni_mb_pmd: Initial implementation of multi buffer based crypto device Declan Doherty
2015-11-10 17:32 ` [dpdk-dev] [PATCH v6 09/10] app/test: add cryptodev unit and performance tests Declan Doherty
2015-11-10 17:32 ` [dpdk-dev] [PATCH v6 10/10] l2fwd-crypto: crypto Declan Doherty
2015-11-13 16:03 ` Thomas Monjalon
2015-11-13 18:58 ` [dpdk-dev] [PATCH v7 00/10] Crypto API and device framework Declan Doherty
2015-11-13 18:58 ` [dpdk-dev] [PATCH v7 01/10] ethdev: rename macros to have RTE_ prefix Declan Doherty
2015-11-17 14:44 ` Declan Doherty
2015-11-17 15:39 ` Thomas Monjalon
2015-11-17 16:04 ` [dpdk-dev] [PATCH v7.1 " Declan Doherty
2015-11-13 18:58 ` [dpdk-dev] [PATCH v7 02/10] ethdev: make error checking macros public Declan Doherty
2015-11-13 18:58 ` [dpdk-dev] [PATCH v7 03/10] eal: add __rte_packed /__rte_aligned macros Declan Doherty
2015-11-13 18:58 ` [dpdk-dev] [PATCH v7 04/10] mbuf: add new marcos to get the physical address of data Declan Doherty
2015-11-25 0:25 ` Thomas Monjalon
2015-11-13 18:58 ` [dpdk-dev] [PATCH v7 05/10] cryptodev: Initial DPDK Crypto APIs and device framework release Declan Doherty
2015-11-25 0:32 ` Thomas Monjalon
2015-11-13 18:58 ` [dpdk-dev] [PATCH v7 06/10] mbuf_offload: library to support attaching offloads to a mbuf Declan Doherty
2015-11-20 15:27 ` Olivier MATZ [this message]
2015-11-20 17:26 ` Declan Doherty
2015-11-23 9:10 ` Olivier MATZ
2015-11-23 11:52 ` Ananyev, Konstantin
2015-11-23 12:16 ` Declan Doherty
2015-11-23 13:08 ` Olivier MATZ
2015-11-23 14:17 ` Thomas Monjalon
2015-11-23 14:46 ` Thomas Monjalon
2015-11-23 15:47 ` Declan Doherty
2015-11-23 14:33 ` Declan Doherty
2015-11-13 18:58 ` [dpdk-dev] [PATCH v7 07/10] qat_crypto_pmd: Addition of a new QAT DPDK PMD Declan Doherty
2015-11-25 1:00 ` Thomas Monjalon
2015-11-25 9:16 ` Mcnamara, John
2015-11-25 10:34 ` Thomas Monjalon
2015-11-25 10:49 ` Thomas Monjalon
2015-11-25 10:59 ` Declan Doherty
2015-11-25 12:01 ` Mcnamara, John
2015-11-13 18:58 ` [dpdk-dev] [PATCH v7 08/10] aesni_mb_pmd: Initial implementation of multi buffer based crypto device Declan Doherty
2015-11-25 10:32 ` Thomas Monjalon
2015-11-13 18:58 ` [dpdk-dev] [PATCH v7 09/10] app/test: add cryptodev unit and performance tests Declan Doherty
2015-11-13 18:58 ` [dpdk-dev] [PATCH v7 10/10] l2fwd-crypto: crypto Declan Doherty
2015-11-25 1:03 ` Thomas Monjalon
2015-11-25 13:25 ` [dpdk-dev] [PATCH v8 00/10] Crypto API and device framework Declan Doherty
2015-11-25 13:25 ` [dpdk-dev] [PATCH v8 01/10] ethdev: rename macros to have RTE_ prefix Declan Doherty
2015-11-25 13:25 ` [dpdk-dev] [PATCH v8 02/10] ethdev: make error checking macros public Declan Doherty
2015-11-25 13:25 ` [dpdk-dev] [PATCH v8 03/10] eal: add __rte_packed /__rte_aligned macros Declan Doherty
2015-11-25 13:25 ` [dpdk-dev] [PATCH v8 04/10] mbuf: add new marcos to get the physical address of data Declan Doherty
2015-11-25 13:25 ` [dpdk-dev] [PATCH v8 05/10] cryptodev: Initial DPDK Crypto APIs and device framework release Declan Doherty
2015-11-25 13:25 ` [dpdk-dev] [PATCH v8 06/10] mbuf_offload: library to support attaching offloads to a mbuf Declan Doherty
2015-11-25 13:25 ` [dpdk-dev] [PATCH v8 07/10] qat_crypto_pmd: Addition of a new QAT DPDK PMD Declan Doherty
2015-11-25 13:25 ` [dpdk-dev] [PATCH v8 08/10] aesni_mb_pmd: Initial implementation of multi buffer based crypto device Declan Doherty
2015-11-25 13:25 ` [dpdk-dev] [PATCH v8 09/10] app/test: add cryptodev unit and performance tests Declan Doherty
2015-11-25 13:25 ` [dpdk-dev] [PATCH v8 10/10] l2fwd-crypto: crypto Declan Doherty
2015-11-25 17:44 ` [dpdk-dev] [PATCH v8 00/10] Crypto API and device framework 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=564F3BC5.90308@6wind.com \
--to=olivier.matz@6wind.com \
--cc=declan.doherty@intel.com \
--cc=dev@dpdk.org \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).