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

[Olivier MATZ <olivier.matz@6wind.com>]

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