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

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

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