[dpdk-dev] [PATCH v7 06/10] mbuf_offload: library to support attaching offloads to a mbuf
Olivier MATZ
olivier.matz at 6wind.com
Mon Nov 23 10:10:01 CET 2015
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
More information about the dev
mailing list