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

[Declan Doherty]

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
>