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
