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

["De Lara Guarch, Pablo" <pablo.de.lara.guarch@intel.com>]

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.

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.