[dpdk-dev] [PATCH v7 06/10] mbuf_offload: library to support attaching offloads to a mbuf

[Olivier MATZ]

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 at intel.com>
> Acked-by: Konstantin Ananyev <konstantin.ananyev at 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