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

  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
  

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
  

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
>
  

Hi Declan,

On 11/20/2015 06:26 PM, Declan Doherty wrote:
>> 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!

Indeed, not sure rte_mbuf_offload_metadata is better.
I'm still not convinced that offload should appear in the name, it
is a bit confusing with hardware offloads (ol_flags). Also, it
suggests that a work is delegated to another entity, but for instance
in this case it is just used as a storage area:

	ol = rte_pktmbuf_offload_alloc(pool, RTE_PKTMBUF_OL_CRYPTO);
	rte_crypto_op_attach_session(&ol->op.crypto, session);
	ol->op.crypto.digest.data = rte_pktmbuf_append(m, digest_len);
	ol->op.crypto.digest.phys_addr = ...;
	/* ... */
	rte_pktmbuf_offload_attach(m, ol);
	ret = rte_cryptodev_enqueue_burst(dev_id, qp_id, &m, 1);

Do you have some other examples in mind that would use this API?

>>> +/** 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

Sorry, I read it to quickly. I thought it was a
rte_pktmbuf_offload_detach() function. By the way there is no
such function?


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

Walking the chain of offload + adding the initialization will probably
have a little cost that should be evaluated.

The freeing is probably not the only problem:
- packet duplication: are the offload infos copied? If no, it means that
  the copy is not exactly a copy
- if you chain 2 mbufs that both have offload info attached, how does it
  behave?
- if you prepend a segment to your mbuf, you need to copy the mbuf
  offload pointer, and also parse the list of offload to update the
  ol->m pointer of each element.

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

As far as I can see, there is already a way to chain several crypto
operations in the crypto structure.

Another option is to use the mbuf offload API (or the m->userdata
pointer which already works for that) only in the application:

- the field is completely ignored by the mbuf api and the dpdk driver
- it is up to the application to initialize it and free it

-> it can only be used when passing mbuf from a part of the app
   to another, so it perfectly matches the pipeline use case.

Example:

app_core1:

  /* receive a mbuf */
  crypto = alloc()
  crypto->xxx = yyy:
  /* ... */
  m->userdata = crypto;
  enqueue(m, app_core2);

app_core2:

  m = dequeue();
  crypto = m->userdata;
  m->userdata = NULL;
  /* api is tx_burst(port, q, mbuf_tab, crypto_tab, n) */
  tx_burst(port, q, &m, &crypto, 1);



Regards,
Olivier
  

Hi Olivier,

> On 11/20/2015 06:26 PM, Declan Doherty wrote:
> >> 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!
> 
> Indeed, not sure rte_mbuf_offload_metadata is better.
> I'm still not convinced that offload should appear in the name, it
> is a bit confusing with hardware offloads (ol_flags). Also, it
> suggests that a work is delegated to another entity, but for instance
> in this case it is just used as a storage area:
> 
> 	ol = rte_pktmbuf_offload_alloc(pool, RTE_PKTMBUF_OL_CRYPTO);
> 	rte_crypto_op_attach_session(&ol->op.crypto, session);
> 	ol->op.crypto.digest.data = rte_pktmbuf_append(m, digest_len);
> 	ol->op.crypto.digest.phys_addr = ...;
> 	/* ... */
> 	rte_pktmbuf_offload_attach(m, ol);
> 	ret = rte_cryptodev_enqueue_burst(dev_id, qp_id, &m, 1);
> 
> Do you have some other examples in mind that would use this API?
> 
> >>> +/** 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
> 
> Sorry, I read it to quickly. I thought it was a
> rte_pktmbuf_offload_detach() function. By the way there is no
> such function?
> 
> 
> >> 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.
> 
> Walking the chain of offload + adding the initialization will probably
> have a little cost that should be evaluated.
> 
> The freeing is probably not the only problem:
> - packet duplication: are the offload infos copied? If no, it means that
>   the copy is not exactly a copy
> - if you chain 2 mbufs that both have offload info attached, how does it
>   behave?
> - if you prepend a segment to your mbuf, you need to copy the mbuf
>   offload pointer, and also parse the list of offload to update the
>   ol->m pointer of each element.
> 
> >> 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.
> 
> As far as I can see, there is already a way to chain several crypto
> operations in the crypto structure.
> 
> Another option is to use the mbuf offload API (or the m->userdata
> pointer which already works for that) only in the application:
> 
> - the field is completely ignored by the mbuf api and the dpdk driver
> - it is up to the application to initialize it and free it
> 
> -> it can only be used when passing mbuf from a part of the app
>    to another, so it perfectly matches the pipeline use case.

I don't think we should start to re-use userdata.
Userdata was intended for the upper layer app to pass/store it's
private data associated with mbuf, and we probably should keep it this way.
While mbuf_offload (or whatever we'll name it) supposed to keep data
necessary for crypto (and other future type of devices) to operate with mbuf.

All your comments above about that this new field is just ignored by most mbuf
operations (copy/attach/append/free, etc) are valid.
By I suppose for now we can just state that for mbuf_offload is not supported by
all these mbuf ops.
I understand that current API is probably not perfect and might need to be revised in future.
Again, as it is completely new feature, I suspect it would be a lot of change requests for it anyway. 
But we need some generic way to pass other (not ethdev) specific information within mbuf
between user-level and PMDs for non-ethernet devices (and probably between different PMDs too).

Konstantin
  

On 23/11/15 11:52, Ananyev, Konstantin wrote:
> Hi Olivier,
>
>> On 11/20/2015 06:26 PM, Declan Doherty wrote:
>>>> 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!
>>
>> Indeed, not sure rte_mbuf_offload_metadata is better.
>> I'm still not convinced that offload should appear in the name, it
>> is a bit confusing with hardware offloads (ol_flags). Also, it
>> suggests that a work is delegated to another entity, but for instance
>> in this case it is just used as a storage area:
>>
>> 	ol = rte_pktmbuf_offload_alloc(pool, RTE_PKTMBUF_OL_CRYPTO);
>> 	rte_crypto_op_attach_session(&ol->op.crypto, session);
>> 	ol->op.crypto.digest.data = rte_pktmbuf_append(m, digest_len);
>> 	ol->op.crypto.digest.phys_addr = ...;
>> 	/* ... */
>> 	rte_pktmbuf_offload_attach(m, ol);
>> 	ret = rte_cryptodev_enqueue_burst(dev_id, qp_id, &m, 1);
>>
>> Do you have some other examples in mind that would use this API?
>>
>>>>> +/** 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
>>
>> Sorry, I read it to quickly. I thought it was a
>> rte_pktmbuf_offload_detach() function. By the way there is no
>> such function?
>>
>>
>>>> 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.
>>
>> Walking the chain of offload + adding the initialization will probably
>> have a little cost that should be evaluated.
>>
>> The freeing is probably not the only problem:
>> - packet duplication: are the offload infos copied? If no, it means that
>>    the copy is not exactly a copy
>> - if you chain 2 mbufs that both have offload info attached, how does it
>>    behave?
>> - if you prepend a segment to your mbuf, you need to copy the mbuf
>>    offload pointer, and also parse the list of offload to update the
>>    ol->m pointer of each element.
>>
>>>> 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.
>>
>> As far as I can see, there is already a way to chain several crypto
>> operations in the crypto structure.
>>
>> Another option is to use the mbuf offload API (or the m->userdata
>> pointer which already works for that) only in the application:
>>
>> - the field is completely ignored by the mbuf api and the dpdk driver
>> - it is up to the application to initialize it and free it
>>
>> -> it can only be used when passing mbuf from a part of the app
>>     to another, so it perfectly matches the pipeline use case.
>
> I don't think we should start to re-use userdata.
> Userdata was intended for the upper layer app to pass/store it's
> private data associated with mbuf, and we probably should keep it this way.
> While mbuf_offload (or whatever we'll name it) supposed to keep data
> necessary for crypto (and other future type of devices) to operate with mbuf.
>
> All your comments above about that this new field is just ignored by most mbuf
> operations (copy/attach/append/free, etc) are valid.
> By I suppose for now we can just state that for mbuf_offload is not supported by
> all these mbuf ops.
> I understand that current API is probably not perfect and might need to be revised in future.
> Again, as it is completely new feature, I suspect it would be a lot of change requests for it anyway.
> But we need some generic way to pass other (not ethdev) specific information within mbuf
> between user-level and PMDs for non-ethernet devices (and probably between different PMDs too).
>
> Konstantin
>


Just to re-iterate Konstantin's point above. I'm aware that a number of 
mbuf operations currently are not supported and are not aware of the 
rte_mbuf_offload, but we will be continuing development throughout 2.3 
and beyond to address this. Also I hope we will get much more community 
feedback and interaction so we can get a more definite feature set for 
the library, and  by minimizing the features in this release, it will 
allow more flexibility on how we can develop this part of handling 
offloaded operations and it will limit the ABI issues we will face if it 
turns out we need to change this going forwards.

Thanks
Declan
  

Hi,

On 11/23/2015 01:16 PM, Declan Doherty wrote:
>> I don't think we should start to re-use userdata.
>> Userdata was intended for the upper layer app to pass/store it's
>> private data associated with mbuf, and we probably should keep it this
>> way.

If the crypto API PMD takes both mbuf and crypto instead of just the
mbuf, the mbuf_offload and the userdata wouldn't be very different,
as the metadata would stay inside the application.

So, if it's application private (the dpdk does not know how to handle
it in case of mbuf free, dup, ...), the content of it should be
app-specific.

>> While mbuf_offload (or whatever we'll name it) supposed to keep data
>> necessary for crypto (and other future type of devices) to operate
>> with mbuf.

Any idea of future devices?

>> All your comments above about that this new field is just ignored by
>> most mbuf
>> operations (copy/attach/append/free, etc) are valid.
>> By I suppose for now we can just state that for mbuf_offload is not
>> supported by
>> all these mbuf ops.

So, who is responsible of freeing the mbuf offload metadata?
The crypto pmd ?

It means that as soon as the mbuf offload is added to the mbuf, it is
not possible to call any other dpdk function that would process the
mbuf... if we cannot call anything else before, why not just passing
the crypto argument as a parameter?

Managing offload data would even be more complex in the future if there
are more than one mbuf_offload attached to the mbuf.

>> I understand that current API is probably not perfect and might need
>> to be revised in future.

The problem is that it's not easy to change the dpdk API now.

>> Again, as it is completely new feature, I suspect it would be a lot of
>> change requests for it anyway.
>> But we need some generic way to pass other (not ethdev) specific
>> information within mbuf
>> between user-level and PMDs for non-ethernet devices (and probably
>> between different PMDs too).

If a crypto PMD needs a crypto info, why not adding a crypto argument?
I feel it's clearer from a user point of view.

About PMD to PMD metadata, do you have any use case?


> Just to re-iterate Konstantin's point above. I'm aware that a number of
> mbuf operations currently are not supported and are not aware of the
> rte_mbuf_offload, but we will be continuing development throughout 2.3
> and beyond to address this. Also I hope we will get much more community
> feedback and interaction so we can get a more definite feature set for
> the library, and  by minimizing the features in this release, it will
> allow more flexibility on how we can develop this part of handling
> offloaded operations and it will limit the ABI issues we will face if it
> turns out we need to change this going forwards.

I can see the amount of work you've done for making the cryptodev
happen in dpdk. I also recognize that I didn't make comments very early.

If we really want to have this feature in the next release, maybe there
is a way to mark it as experimental, meaning that the API is subject to
change? What do you think?

Thomas, any comment?

Regards,
Olivier
  

2015-11-23 14:08, Olivier MATZ:
> 2015-11-23 12:16, Declan Doherty:
> > 2015-11-23 11:52, Ananyev, Konstantin:
> >> I understand that current API is probably not perfect and might need
> >> to be revised in future.
> 
> The problem is that it's not easy to change the dpdk API now.
[...]
> > Just to re-iterate Konstantin's point above. I'm aware that a number of
> > mbuf operations currently are not supported and are not aware of the
> > rte_mbuf_offload, but we will be continuing development throughout 2.3
> > and beyond to address this. Also I hope we will get much more community
> > feedback and interaction so we can get a more definite feature set for
> > the library, and  by minimizing the features in this release, it will
> > allow more flexibility on how we can develop this part of handling
> > offloaded operations and it will limit the ABI issues we will face if it
> > turns out we need to change this going forwards.
> 
> I can see the amount of work you've done for making the cryptodev
> happen in dpdk. I also recognize that I didn't make comments very early.
> 
> If we really want to have this feature in the next release, maybe there
> is a way to mark it as experimental, meaning that the API is subject to
> change? What do you think?
> 
> Thomas, any comment?

Yes, it is a totally new work and it probably needs more time to have a
design working well for most of use cases.
As I already discussed with Olivier, I think it should be considered as
experimental. It means we can try it but do not consider it as a stable
API. So the deprecation process would not apply until the experimental
flag is removed.

For the release 2.2, it would be better to remove the crypto dependency
in mbuf. Do you think it is possible?
  

On 23/11/15 13:08, Olivier MATZ wrote:
> Hi,
>
> On 11/23/2015 01:16 PM, Declan Doherty wrote:
>>> I don't think we should start to re-use userdata.
>>> Userdata was intended for the upper layer app to pass/store it's
>>> private data associated with mbuf, and we probably should keep it this
>>> way.
>
> If the crypto API PMD takes both mbuf and crypto instead of just the
> mbuf, the mbuf_offload and the userdata wouldn't be very different,
> as the metadata would stay inside the application.
>
> So, if it's application private (the dpdk does not know how to handle
> it in case of mbuf free, dup, ...), the content of it should be
> app-specific.
>
>>> While mbuf_offload (or whatever we'll name it) supposed to keep data
>>> necessary for crypto (and other future type of devices) to operate
>>> with mbuf.
>
> Any idea of future devices?

Well the QAT hardware supports compression, so I guess that might be a 
likely candidate for future work.

>
>>> All your comments above about that this new field is just ignored by
>>> most mbuf
>>> operations (copy/attach/append/free, etc) are valid.
>>> By I suppose for now we can just state that for mbuf_offload is not
>>> supported by
>>> all these mbuf ops.
>
> So, who is responsible of freeing the mbuf offload metadata?
> The crypto pmd ?
>
> It means that as soon as the mbuf offload is added to the mbuf, it is
> not possible to call any other dpdk function that would process the
> mbuf... if we cannot call anything else before, why not just passing
> the crypto argument as a parameter?
>
> Managing offload data would even be more complex in the future if there
> are more than one mbuf_offload attached to the mbuf.
>
>>> I understand that current API is probably not perfect and might need
>>> to be revised in future.
>
> The problem is that it's not easy to change the dpdk API now.
>
>>> Again, as it is completely new feature, I suspect it would be a lot of
>>> change requests for it anyway.
>>> But we need some generic way to pass other (not ethdev) specific
>>> information within mbuf
>>> between user-level and PMDs for non-ethernet devices (and probably
>>> between different PMDs too).
>
> If a crypto PMD needs a crypto info, why not adding a crypto argument?
> I feel it's clearer from a user point of view.
>
> About PMD to PMD metadata, do you have any use case?

One use case that comes to mind would be the enablement of inline IPsec. 
To enable this you would need a mechanism to attach the IPsec metadata, 
ESP header offsets etc to an mbuf which the PMD can then use to 
correctly complete the tx descriptor.


>
>
>> Just to re-iterate Konstantin's point above. I'm aware that a number of
>> mbuf operations currently are not supported and are not aware of the
>> rte_mbuf_offload, but we will be continuing development throughout 2.3
>> and beyond to address this. Also I hope we will get much more community
>> feedback and interaction so we can get a more definite feature set for
>> the library, and  by minimizing the features in this release, it will
>> allow more flexibility on how we can develop this part of handling
>> offloaded operations and it will limit the ABI issues we will face if it
>> turns out we need to change this going forwards.
>
> I can see the amount of work you've done for making the cryptodev
> happen in dpdk. I also recognize that I didn't make comments very early.
>
> If we really want to have this feature in the next release, maybe there
> is a way to mark it as experimental, meaning that the API is subject to
> change? What do you think?

I wouldn't be against this idea. I think a mechanism for marking new 
libraries / APIs as experimental for their initial release could be very 
useful to allow new features to be introduced with the flexibility to 
make further API changes on user feedback without the overhead of ABI 
management in the subsequent release.

>
> Thomas, any comment?
>
> Regards,
> Olivier
>
  

2015-11-23 15:17, Thomas Monjalon:
> Yes, it is a totally new work and it probably needs more time to have a
> design working well for most of use cases.
> As I already discussed with Olivier, I think it should be considered as
> experimental. It means we can try it but do not consider it as a stable
> API. So the deprecation process would not apply until the experimental
> flag is removed.

If nobody complains, I'll apply this v7 and will send a patch to add some
experimental markers.

> For the release 2.2, it would be better to remove the crypto dependency
> in mbuf. Do you think it is possible?

Sorry, forget it, the dependency is in mbuf_offload.
  

On 23/11/15 14:46, Thomas Monjalon wrote:
> 2015-11-23 15:17, Thomas Monjalon:
>> Yes, it is a totally new work and it probably needs more time to have a
>> design working well for most of use cases.
>> As I already discussed with Olivier, I think it should be considered as
>> experimental. It means we can try it but do not consider it as a stable
>> API. So the deprecation process would not apply until the experimental
>> flag is removed.
>
> If nobody complains, I'll apply this v7 and will send a patch to add some
> experimental markers.
>
>> For the release 2.2, it would be better to remove the crypto dependency
>> in mbuf. Do you think it is possible?
>
> Sorry, forget it, the dependency is in mbuf_offload.
>

That sounds good to me, thanks Thomas!