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

[Declan Doherty <declan.doherty@intel.com>]

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 <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.

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 <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?

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
>