Hi Anoob, > -----Original Message----- > From: Joseph, Anoob [mailto:anoob.jos...@caviumnetworks.com] > Sent: Thursday, June 28, 2018 3:56 AM > To: Doherty, Declan <declan.dohe...@intel.com>; De Lara Guarch, Pablo > <pablo.de.lara.gua...@intel.com> > Cc: Akhil Goyal <akhil.go...@nxp.com>; Ankur Dwivedi > <ankur.dwiv...@caviumnetworks.com>; Jerin Jacob > <jerin.ja...@caviumnetworks.com>; Narayana Prasad > <narayanaprasad.athr...@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.jos...@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.
