Re: [dpdk-dev] [PATCH 1/2] cryptodev: add min headroom and tailroom requirement

["Joseph, Anoob" <Anoob.Joseph@caviumnetworks.com>]

Hi Pablo,

On 28-06-2018 17:11, De Lara Guarch, Pablo wrote:
> External Email
>
> Hi Anoob,
>
>> -----Original Message-----
>> From: Joseph, Anoob [mailto:Anoob.Joseph@caviumnetworks.com]
>> Sent: Thursday, June 28, 2018 3:56 AM
>> To: Doherty, Declan <declan.doherty@intel.com>; De Lara Guarch, Pablo
>> <pablo.de.lara.guarch@intel.com>
>> Cc: Akhil Goyal <akhil.goyal@nxp.com>; Ankur Dwivedi
>> <ankur.dwivedi@caviumnetworks.com>; Jerin Jacob
>> <jerin.jacob@caviumnetworks.com>; Narayana Prasad
>> <narayanaprasad.athreya@caviumnetworks.com>; dev@dpdk.org
>> Subject: Re: [PATCH 1/2] cryptodev: add min headroom and tailroom
>> requirement
>>
>> Hi Declan,
>>
>> Please see inline.
>>
>> Thanks,
>>
>> Anoob
>>
>>
>> On 26-06-2018 15:42, Doherty, Declan wrote:
>>> External Email
>>>
>>> On 19/06/2018 7:26 AM, Anoob Joseph wrote:
>>>> Enabling crypto devs to specify the minimum headroom and tailroom it
>>>> expects in the mbuf. For net PMDs, standard headroom has to be
>>>> honoured by applications, which is not strictly followed for crypto
>>>> devs. This
>>> How is this done for NET PMDs, I don't see anything explicit in the
>>> ehtdev API for specification of headroom requirements.
>> In rte_mbuf.h, the minimum size required for packets involved in rx/tx is
>> specified and that considers headroom also. Applications usually use these
>> default macros while creating mbufs which are involved in rx/tx.
>> https://git.dpdk.org/dpdk/tree/lib/librte_mbuf/rte_mbuf.h#n411
>>>> prevents crypto devs from using free space in mbuf (available as
>>>> head/tailroom) for internal requirements in crypto operations.
>>>> Addition of head/tailroom requirement will help PMDs to communicate
>>>> such requirements to the application.
>>>>
>>>> The availability and use of head/tailroom is an optimization if the
>>>> hardware supports use of head/tailroom for crypto-op info. For
>>>> devices that do not support using the head/tailroom, they can
>>>> continue to operate without any performance-drop.
>>>>
>>> Is there any variations in requirements for terms headroom/tailroom on
>>> a per algorithmic basis or is it purely for the device?
>> It is purely per device basis. The device can specify upper bounds for the
>> head/tailroom. A device that even specified the room, may not even use the
>> entire room in all cases. So it doesn't have to be algo specific.
>>>> Signed-off-by: Anoob Joseph <anoob.joseph@caviumnetworks.com>
>>>> ---
>>>>    doc/guides/rel_notes/deprecation.rst | 4 ++++
>>>>    lib/librte_cryptodev/rte_cryptodev.h | 6 ++++++
>>>>    2 files changed, 10 insertions(+)
>>>>
>>>> diff --git a/doc/guides/rel_notes/deprecation.rst
>>>> b/doc/guides/rel_notes/deprecation.rst
>>>> index 1ce692e..a547289 100644
>>>> --- a/doc/guides/rel_notes/deprecation.rst
>>>> +++ b/doc/guides/rel_notes/deprecation.rst
>>>> @@ -122,3 +122,7 @@ Deprecation Notices
>>>>      - Function ``rte_cryptodev_get_private_session_size()`` will be
>>>> deprecated
>>>>        in 18.05, and it gets replaced with
>>>> ``rte_cryptodev_sym_get_private_session_size()``.
>>>>        It will be removed in 18.08.
>>>> +  - New field, ``min_headroom_req``, added in ``rte_cryptodev_info``
>>>> structure. It will be
>>>> +    added in 18.11.
>>>> +  - New field, ``min_tailroom_req``, added in ``rte_cryptodev_info``
>>>> structure. It will be
>>>> +    added in 18.11.
>>>> diff --git a/lib/librte_cryptodev/rte_cryptodev.h
>>>> b/lib/librte_cryptodev/rte_cryptodev.h
>>>> index 92ce6d4..fa944b8 100644
>>>> --- a/lib/librte_cryptodev/rte_cryptodev.h
>>>> +++ b/lib/librte_cryptodev/rte_cryptodev.h
>>>> @@ -382,6 +382,12 @@ struct rte_cryptodev_info {
>>>>        unsigned max_nb_queue_pairs;
>>>>        /**< Maximum number of queues pairs supported by device. */
>>>>
>>>> +     uint32_t min_headroom_req;
>>>> +     /**< Minimum mbuf headroom required by device */
>>>> +
>>>> +     uint32_t min_tailroom_req;
>>>> +     /**< Minimum mbuf tailroom required by device */
> I would add the word "mbuf" here, in the variable names (e.g. min_mbuf_headroom_req),
> to be more explicit.
Will revise the patch with this change.
>
> Also, just let you know that we are currently modifying the info structure in this release.
> Therefore, I think we could make these changes in now and then you don't need to add deprecation notices on this,
> but better to add the API Change in release notes.
Will do so.

Thanks,
Anoob