[dpdk-dev] [PATCH 0/2] add head/tailroom requirement for crypto PMDs

[dpdk-dev] [PATCH 0/2] add head/tailroom requirement for crypto PMDs

[Anoob Joseph]

This series adds the ability, for crypto PMDs, to communicate the
minimum head/tailroom requirement it may have, using the existing
cryptodev_info framework.

The availability and use of head/tailroom is an optimisation if the
hardware supports its use for crypto-op info. Devices that do not
support using the head/tailroom, can continue to operate without
any performance-drop.

Cavium's CPT hardware supports this feature and would use headroom and
tailroom for submitting crypto-ops to the hardware.

Anoob Joseph (2):
  cryptodev: add min headroom and tailroom requirement
  app/crypto-perf: honour cryptodev's min headroom/tailroom

 app/test-crypto-perf/cperf_options.h     |  2 ++
 app/test-crypto-perf/cperf_test_common.c | 33 +++++++++++++++++++++-----------
 app/test-crypto-perf/main.c              | 17 ++++++++++++++++
 doc/guides/rel_notes/deprecation.rst     |  4 ++++
 lib/librte_cryptodev/rte_cryptodev.h     |  6 ++++++
 5 files changed, 51 insertions(+), 11 deletions(-)

-- 
2.7.4

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

[Anoob Joseph]

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
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.

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 */
+
 	struct {
 		unsigned max_nb_sessions;
 		/**< Maximum number of sessions supported by device. */
-- 
2.7.4

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

[Akhil Goyal]

On 6/19/2018 11:56 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
> 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.
>
> 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.

Is this targeted for 18.08 or 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 */
> +
>   	struct {
>   		unsigned max_nb_sessions;
>   		/**< Maximum number of sessions supported by device. */

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

[Joseph, Anoob]

Hi Akhil,

The change adds couple of entries in cryptodev_info structure. I was 
under the impression this will be an ABI change and would need one 
release cycle. If we can get this through in 18.08, it would be great. 
Do you want me to revise the patch stating it that way?

Thanks,
Anoob
On 21-06-2018 19:54, Akhil Goyal wrote:
>
> On 6/19/2018 11:56 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
>> 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.
>>
>> 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.
>
> Is this targeted for 18.08 or 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 */
>> +
>>       struct {
>>               unsigned max_nb_sessions;
>>               /**< Maximum number of sessions supported by device. */
>

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

[Akhil Goyal]

Hi Anoob,


On 6/22/2018 12:22 PM, Joseph, Anoob wrote:
> Hi Akhil,
>
> The change adds couple of entries in cryptodev_info structure. I was 
> under the impression this will be an ABI change and would need one 
> release cycle. If we can get this through in 18.08, it would be great. 
> Do you want me to revise the patch stating it that way?

If you are adding a deprecation notice for next release, then you cannot add code in that release.

To add this in current release, I think Pablo can take a decision on this.


>
> Thanks,
> Anoob
> On 21-06-2018 19:54, Akhil Goyal wrote:
>>
>> On 6/19/2018 11:56 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
>>> 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.
>>>
>>> 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.
>>
>> Is this targeted for 18.08 or 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 */
>>> +
>>>       struct {
>>>               unsigned max_nb_sessions;
>>>               /**< Maximum number of sessions supported by device. */
>>
>

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

[Doherty, Declan]

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.

> 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?

> 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 */
> +
>   	struct {
>   		unsigned max_nb_sessions;
>   		/**< Maximum number of sessions supported by device. */
> 

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

[Joseph, Anoob]

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 */
>> +
>>       struct {
>>               unsigned max_nb_sessions;
>>               /**< Maximum number of sessions supported by device. */
>>
>

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

[De Lara Guarch, Pablo]

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.

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

[Joseph, Anoob]

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

[dpdk-dev] [PATCH 2/2] app/crypto-perf: honour cryptodev's min headroom/tailroom

[Anoob Joseph]

Crypto dev would specify its headroom and tailroom requirement and the
application is expected to honour this while creating buffers.

Signed-off-by: Anoob Joseph <anoob.joseph@caviumnetworks.com>
---
 app/test-crypto-perf/cperf_options.h     |  2 ++
 app/test-crypto-perf/cperf_test_common.c | 33 +++++++++++++++++++++-----------
 app/test-crypto-perf/main.c              | 17 ++++++++++++++++
 3 files changed, 41 insertions(+), 11 deletions(-)

diff --git a/app/test-crypto-perf/cperf_options.h b/app/test-crypto-perf/cperf_options.h
index 350ad7e..f5bf03c 100644
--- a/app/test-crypto-perf/cperf_options.h
+++ b/app/test-crypto-perf/cperf_options.h
@@ -76,6 +76,8 @@ struct cperf_options {
 
 	uint32_t pool_sz;
 	uint32_t total_ops;
+	uint32_t headroom_sz;
+	uint32_t tailroom_sz;
 	uint32_t segment_sz;
 	uint32_t test_buffer_size;
 	uint32_t *imix_buffer_sizes;
diff --git a/app/test-crypto-perf/cperf_test_common.c b/app/test-crypto-perf/cperf_test_common.c
index 423782c..e803dc1 100644
--- a/app/test-crypto-perf/cperf_test_common.c
+++ b/app/test-crypto-perf/cperf_test_common.c
@@ -11,12 +11,15 @@ struct obj_params {
 	uint32_t src_buf_offset;
 	uint32_t dst_buf_offset;
 	uint16_t segment_sz;
+	uint16_t headroom_sz;
+	uint16_t data_len;
 	uint16_t segments_nb;
 };
 
 static void
 fill_single_seg_mbuf(struct rte_mbuf *m, struct rte_mempool *mp,
-		void *obj, uint32_t mbuf_offset, uint16_t segment_sz)
+		void *obj, uint32_t mbuf_offset, uint16_t segment_sz,
+		uint16_t headroom, uint16_t data_len)
 {
 	uint32_t mbuf_hdr_size = sizeof(struct rte_mbuf);
 
@@ -26,10 +29,10 @@ fill_single_seg_mbuf(struct rte_mbuf *m, struct rte_mempool *mp,
 	m->buf_iova = rte_mempool_virt2iova(obj) +
 		mbuf_offset + mbuf_hdr_size;
 	m->buf_len = segment_sz;
-	m->data_len = segment_sz;
+	m->data_len = data_len;
 
-	/* No headroom needed for the buffer */
-	m->data_off = 0;
+	/* Use headroom specified for the buffer */
+	m->data_off = headroom;
 
 	/* init some constant fields */
 	m->pool = mp;
@@ -42,7 +45,7 @@ fill_single_seg_mbuf(struct rte_mbuf *m, struct rte_mempool *mp,
 static void
 fill_multi_seg_mbuf(struct rte_mbuf *m, struct rte_mempool *mp,
 		void *obj, uint32_t mbuf_offset, uint16_t segment_sz,
-		uint16_t segments_nb)
+		uint16_t headroom, uint16_t data_len, uint16_t segments_nb)
 {
 	uint16_t mbuf_hdr_size = sizeof(struct rte_mbuf);
 	uint16_t remaining_segments = segments_nb;
@@ -57,10 +60,10 @@ fill_multi_seg_mbuf(struct rte_mbuf *m, struct rte_mempool *mp,
 		m->buf_iova = next_seg_phys_addr;
 		next_seg_phys_addr += mbuf_hdr_size + segment_sz;
 		m->buf_len = segment_sz;
-		m->data_len = segment_sz;
+		m->data_len = data_len;
 
-		/* No headroom needed for the buffer */
-		m->data_off = 0;
+		/* Use headroom specified for the buffer */
+		m->data_off = headroom;
 
 		/* init some constant fields */
 		m->pool = mp;
@@ -99,10 +102,12 @@ mempool_obj_init(struct rte_mempool *mp,
 	op->sym->m_src = m;
 	if (params->segments_nb == 1)
 		fill_single_seg_mbuf(m, mp, obj, params->src_buf_offset,
-				params->segment_sz);
+				params->segment_sz, params->headroom_sz,
+				params->data_len);
 	else
 		fill_multi_seg_mbuf(m, mp, obj, params->src_buf_offset,
-				params->segment_sz, params->segments_nb);
+				params->segment_sz, params->headroom_sz,
+				params->data_len, params->segments_nb);
 
 
 	/* Set destination buffer */
@@ -110,7 +115,8 @@ mempool_obj_init(struct rte_mempool *mp,
 		m = (struct rte_mbuf *) ((uint8_t *) obj +
 				params->dst_buf_offset);
 		fill_single_seg_mbuf(m, mp, obj, params->dst_buf_offset,
-				params->segment_sz);
+				params->segment_sz, params->headroom_sz,
+				params->data_len);
 		op->sym->m_dst = m;
 	} else
 		op->sym->m_dst = NULL;
@@ -172,6 +178,11 @@ cperf_alloc_common_memory(const struct cperf_options *options,
 
 	struct obj_params params = {
 		.segment_sz = options->segment_sz,
+		.headroom_sz = options->headroom_sz,
+		/* Data len = segment size - (headroom + tailroom) */
+		.data_len = options->segment_sz -
+			    options->headroom_sz -
+			    options->tailroom_sz,
 		.segments_nb = segments_nb,
 		.src_buf_offset = crypto_op_total_size_padded,
 		.dst_buf_offset = 0
diff --git a/app/test-crypto-perf/main.c b/app/test-crypto-perf/main.c
index 4ae1439..9d9154e 100644
--- a/app/test-crypto-perf/main.c
+++ b/app/test-crypto-perf/main.c
@@ -149,6 +149,23 @@ cperf_initialize_cryptodev(struct cperf_options *opts, uint8_t *enabled_cdevs,
 			.nb_descriptors = opts->nb_descriptors
 		};
 
+		/**
+		 * Device info specifies the min headroom and tailroom
+		 * requirement for the crypto PMD. This need to be honoured
+		 * by the application, while creating mbuf.
+		 */
+		if (opts->headroom_sz < cdev_info.min_headroom_req) {
+			/* Update headroom */
+			opts->headroom_sz = cdev_info.min_headroom_req;
+		}
+		if (opts->tailroom_sz < cdev_info.min_tailroom_req) {
+			/* Update tailroom */
+			opts->tailroom_sz = cdev_info.min_tailroom_req;
+		}
+
+		/* Update segment size to include headroom & tailroom */
+		opts->segment_sz += (opts->headroom_sz + opts->tailroom_sz);
+
 		if (session_pool_socket[socket_id] == NULL) {
 			char mp_name[RTE_MEMPOOL_NAMESIZE];
 			struct rte_mempool *sess_mp;
-- 
2.7.4

Re: [dpdk-dev] [PATCH 2/2] app/crypto-perf: honour cryptodev's min headroom/tailroom

[De Lara Guarch, Pablo]

> -----Original Message-----
> From: Anoob Joseph [mailto:anoob.joseph@caviumnetworks.com]
> Sent: Tuesday, June 19, 2018 7:26 AM
> To: Doherty, Declan <declan.doherty@intel.com>; De Lara Guarch, Pablo
> <pablo.de.lara.guarch@intel.com>
> Cc: Anoob Joseph <anoob.joseph@caviumnetworks.com>; 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: [PATCH 2/2] app/crypto-perf: honour cryptodev's min
> headroom/tailroom
> 
> Crypto dev would specify its headroom and tailroom requirement and the
> application is expected to honour this while creating buffers.
> 
> Signed-off-by: Anoob Joseph <anoob.joseph@caviumnetworks.com>

I would say that this change should be done in all our sample apps, including the test app, right?

Thanks,
Pablo

[dpdk-dev] [PATCH v1 0/3] add head/tailroom requirement for crypto PMDs

[Anoob Joseph]

This series adds the ability, for crypto PMDs, to communicate the
minimum head/tailroom requirement it may have, using the existing
cryptodev_info framework.

The availability and use of head/tailroom is an optimisation if the
hardware supports its use for crypto-op info. Devices that do not
support using the head/tailroom, can continue to operate without
any performance-drop.

Cavium's OcteonTX crypto hardware supports this feature and would use
headroom and tailroom for submitting crypto-ops to the hardware.

v1:
* Removed deprecation notice and updated release notes
* Added corresponding change in test-cryptodev

Anoob Joseph (3):
  cryptodev: add min headroom and tailroom requirement
  app/crypto-perf: honour cryptodev's min headroom/tailroom
  test/crypto: skip validation of head/tailroom used by PMD

 app/test-crypto-perf/cperf_options.h     |  2 ++
 app/test-crypto-perf/cperf_test_common.c | 33 ++++++++++++------
 app/test-crypto-perf/main.c              | 17 +++++++++
 doc/guides/rel_notes/release_18_08.rst   |  6 ++++
 lib/librte_cryptodev/rte_cryptodev.h     |  6 ++++
 test/test/test_cryptodev_blockcipher.c   | 60 ++++++++++++++++++++++++++++----
 6 files changed, 107 insertions(+), 17 deletions(-)

-- 
2.7.4

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

[Anoob Joseph]

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
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.

Signed-off-by: Anoob Joseph <anoob.joseph@caviumnetworks.com>
---
v1:
* Removed deprecation notice
* Updated release note
* Renamed new fields to have 'mbuf' in the name
* Changed the type of new fields to uint16_t (instead of uint32_t)

 doc/guides/rel_notes/release_18_08.rst | 6 ++++++
 lib/librte_cryptodev/rte_cryptodev.h   | 6 ++++++
 2 files changed, 12 insertions(+)

diff --git a/doc/guides/rel_notes/release_18_08.rst b/doc/guides/rel_notes/release_18_08.rst
index 5bc23c5..fae0d26 100644
--- a/doc/guides/rel_notes/release_18_08.rst
+++ b/doc/guides/rel_notes/release_18_08.rst
@@ -70,6 +70,12 @@ ABI Changes
    Also, make sure to start the actual text at the margin.
    =========================================================
 
+* cryptodev: Additional fields in rte_cryptodev_info.
+
+  Two new fields of type ``uint16_t`` added in ``rte_cryptodev_info``
+  structure: ``min_mbuf_headroom_req`` and ``min_mbuf_tailroom_req``. These
+  parameters specify the recommended headroom and tailroom for mbufs to be
+  processed by the PMD.
 
 Removed Items
 -------------
diff --git a/lib/librte_cryptodev/rte_cryptodev.h b/lib/librte_cryptodev/rte_cryptodev.h
index 92ce6d4..4e5b5b4 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. */
 
+	uint16_t min_mbuf_headroom_req;
+	/**< Minimum mbuf headroom required by device */
+
+	uint16_t min_mbuf_tailroom_req;
+	/**< Minimum mbuf tailroom required by device */
+
 	struct {
 		unsigned max_nb_sessions;
 		/**< Maximum number of sessions supported by device. */
-- 
2.7.4

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

[De Lara Guarch, Pablo]

Hi Anoob,

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Anoob Joseph
> Sent: Wednesday, July 4, 2018 2:56 PM
> To: Doherty, Declan <declan.doherty@intel.com>; De Lara Guarch, Pablo
> <pablo.de.lara.guarch@intel.com>
> Cc: Anoob Joseph <anoob.joseph@caviumnetworks.com>; 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: [dpdk-dev] [PATCH v1 1/3] cryptodev: add min headroom and tailroom
> requirement
> 
> 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 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.
> 
> Signed-off-by: Anoob Joseph <anoob.joseph@caviumnetworks.com>
> ---
> v1:
> * Removed deprecation notice
> * Updated release note
> * Renamed new fields to have 'mbuf' in the name
> * Changed the type of new fields to uint16_t (instead of uint32_t)
> 
>  doc/guides/rel_notes/release_18_08.rst | 6 ++++++
>  lib/librte_cryptodev/rte_cryptodev.h   | 6 ++++++
>  2 files changed, 12 insertions(+)
> 
> diff --git a/doc/guides/rel_notes/release_18_08.rst
> b/doc/guides/rel_notes/release_18_08.rst
> index 5bc23c5..fae0d26 100644
> --- a/doc/guides/rel_notes/release_18_08.rst
> +++ b/doc/guides/rel_notes/release_18_08.rst
> @@ -70,6 +70,12 @@ ABI Changes
>     Also, make sure to start the actual text at the margin.
>     =========================================================
> 
> +* cryptodev: Additional fields in rte_cryptodev_info.
> +
> +  Two new fields of type ``uint16_t`` added in ``rte_cryptodev_info``
> +  structure: ``min_mbuf_headroom_req`` and ``min_mbuf_tailroom_req``.
> + These  parameters specify the recommended headroom and tailroom for
> + mbufs to be  processed by the PMD.

I think the "cryptodev scheduler PMD" needs changes to take these new parameters into consideration.
Scheduler_pmd_info_get should return the maximum number of these two fields on all the slaves
(like what's done with max number of sessions).

We need to close the subtree today, with all API changes done. Will you have time to make this change today?

Thanks!
Pablo

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

[Anoob Joseph]

Hi Pablo,

I'll look into this and will give you an updated patch.

Thanks,
Anoob

On 10-07-2018 15:56, De Lara Guarch, Pablo wrote:
> External Email
>
> Hi Anoob,
>
>> -----Original Message-----
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Anoob Joseph
>> Sent: Wednesday, July 4, 2018 2:56 PM
>> To: Doherty, Declan <declan.doherty@intel.com>; De Lara Guarch, Pablo
>> <pablo.de.lara.guarch@intel.com>
>> Cc: Anoob Joseph <anoob.joseph@caviumnetworks.com>; 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: [dpdk-dev] [PATCH v1 1/3] cryptodev: add min headroom and tailroom
>> requirement
>>
>> 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 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.
>>
>> Signed-off-by: Anoob Joseph <anoob.joseph@caviumnetworks.com>
>> ---
>> v1:
>> * Removed deprecation notice
>> * Updated release note
>> * Renamed new fields to have 'mbuf' in the name
>> * Changed the type of new fields to uint16_t (instead of uint32_t)
>>
>>   doc/guides/rel_notes/release_18_08.rst | 6 ++++++
>>   lib/librte_cryptodev/rte_cryptodev.h   | 6 ++++++
>>   2 files changed, 12 insertions(+)
>>
>> diff --git a/doc/guides/rel_notes/release_18_08.rst
>> b/doc/guides/rel_notes/release_18_08.rst
>> index 5bc23c5..fae0d26 100644
>> --- a/doc/guides/rel_notes/release_18_08.rst
>> +++ b/doc/guides/rel_notes/release_18_08.rst
>> @@ -70,6 +70,12 @@ ABI Changes
>>      Also, make sure to start the actual text at the margin.
>>      =========================================================
>>
>> +* cryptodev: Additional fields in rte_cryptodev_info.
>> +
>> +  Two new fields of type ``uint16_t`` added in ``rte_cryptodev_info``
>> +  structure: ``min_mbuf_headroom_req`` and ``min_mbuf_tailroom_req``.
>> + These  parameters specify the recommended headroom and tailroom for
>> + mbufs to be  processed by the PMD.
> I think the "cryptodev scheduler PMD" needs changes to take these new parameters into consideration.
> Scheduler_pmd_info_get should return the maximum number of these two fields on all the slaves
> (like what's done with max number of sessions).
>
> We need to close the subtree today, with all API changes done. Will you have time to make this change today?
>
> Thanks!
> Pablo
>

[dpdk-dev] [PATCH v1 2/3] app/crypto-perf: honour cryptodev's min headroom/tailroom

[Anoob Joseph]

Crypto dev would specify its headroom and tailroom requirement and the
application is expected to honour this while creating buffers.

Signed-off-by: Anoob Joseph <anoob.joseph@caviumnetworks.com>
---
v1:
* Accomodated the name change of new fields

 app/test-crypto-perf/cperf_options.h     |  2 ++
 app/test-crypto-perf/cperf_test_common.c | 33 +++++++++++++++++++++-----------
 app/test-crypto-perf/main.c              | 17 ++++++++++++++++
 3 files changed, 41 insertions(+), 11 deletions(-)

diff --git a/app/test-crypto-perf/cperf_options.h b/app/test-crypto-perf/cperf_options.h
index 350ad7e..f5bf03c 100644
--- a/app/test-crypto-perf/cperf_options.h
+++ b/app/test-crypto-perf/cperf_options.h
@@ -76,6 +76,8 @@ struct cperf_options {
 
 	uint32_t pool_sz;
 	uint32_t total_ops;
+	uint32_t headroom_sz;
+	uint32_t tailroom_sz;
 	uint32_t segment_sz;
 	uint32_t test_buffer_size;
 	uint32_t *imix_buffer_sizes;
diff --git a/app/test-crypto-perf/cperf_test_common.c b/app/test-crypto-perf/cperf_test_common.c
index 423782c..e803dc1 100644
--- a/app/test-crypto-perf/cperf_test_common.c
+++ b/app/test-crypto-perf/cperf_test_common.c
@@ -11,12 +11,15 @@ struct obj_params {
 	uint32_t src_buf_offset;
 	uint32_t dst_buf_offset;
 	uint16_t segment_sz;
+	uint16_t headroom_sz;
+	uint16_t data_len;
 	uint16_t segments_nb;
 };
 
 static void
 fill_single_seg_mbuf(struct rte_mbuf *m, struct rte_mempool *mp,
-		void *obj, uint32_t mbuf_offset, uint16_t segment_sz)
+		void *obj, uint32_t mbuf_offset, uint16_t segment_sz,
+		uint16_t headroom, uint16_t data_len)
 {
 	uint32_t mbuf_hdr_size = sizeof(struct rte_mbuf);
 
@@ -26,10 +29,10 @@ fill_single_seg_mbuf(struct rte_mbuf *m, struct rte_mempool *mp,
 	m->buf_iova = rte_mempool_virt2iova(obj) +
 		mbuf_offset + mbuf_hdr_size;
 	m->buf_len = segment_sz;
-	m->data_len = segment_sz;
+	m->data_len = data_len;
 
-	/* No headroom needed for the buffer */
-	m->data_off = 0;
+	/* Use headroom specified for the buffer */
+	m->data_off = headroom;
 
 	/* init some constant fields */
 	m->pool = mp;
@@ -42,7 +45,7 @@ fill_single_seg_mbuf(struct rte_mbuf *m, struct rte_mempool *mp,
 static void
 fill_multi_seg_mbuf(struct rte_mbuf *m, struct rte_mempool *mp,
 		void *obj, uint32_t mbuf_offset, uint16_t segment_sz,
-		uint16_t segments_nb)
+		uint16_t headroom, uint16_t data_len, uint16_t segments_nb)
 {
 	uint16_t mbuf_hdr_size = sizeof(struct rte_mbuf);
 	uint16_t remaining_segments = segments_nb;
@@ -57,10 +60,10 @@ fill_multi_seg_mbuf(struct rte_mbuf *m, struct rte_mempool *mp,
 		m->buf_iova = next_seg_phys_addr;
 		next_seg_phys_addr += mbuf_hdr_size + segment_sz;
 		m->buf_len = segment_sz;
-		m->data_len = segment_sz;
+		m->data_len = data_len;
 
-		/* No headroom needed for the buffer */
-		m->data_off = 0;
+		/* Use headroom specified for the buffer */
+		m->data_off = headroom;
 
 		/* init some constant fields */
 		m->pool = mp;
@@ -99,10 +102,12 @@ mempool_obj_init(struct rte_mempool *mp,
 	op->sym->m_src = m;
 	if (params->segments_nb == 1)
 		fill_single_seg_mbuf(m, mp, obj, params->src_buf_offset,
-				params->segment_sz);
+				params->segment_sz, params->headroom_sz,
+				params->data_len);
 	else
 		fill_multi_seg_mbuf(m, mp, obj, params->src_buf_offset,
-				params->segment_sz, params->segments_nb);
+				params->segment_sz, params->headroom_sz,
+				params->data_len, params->segments_nb);
 
 
 	/* Set destination buffer */
@@ -110,7 +115,8 @@ mempool_obj_init(struct rte_mempool *mp,
 		m = (struct rte_mbuf *) ((uint8_t *) obj +
 				params->dst_buf_offset);
 		fill_single_seg_mbuf(m, mp, obj, params->dst_buf_offset,
-				params->segment_sz);
+				params->segment_sz, params->headroom_sz,
+				params->data_len);
 		op->sym->m_dst = m;
 	} else
 		op->sym->m_dst = NULL;
@@ -172,6 +178,11 @@ cperf_alloc_common_memory(const struct cperf_options *options,
 
 	struct obj_params params = {
 		.segment_sz = options->segment_sz,
+		.headroom_sz = options->headroom_sz,
+		/* Data len = segment size - (headroom + tailroom) */
+		.data_len = options->segment_sz -
+			    options->headroom_sz -
+			    options->tailroom_sz,
 		.segments_nb = segments_nb,
 		.src_buf_offset = crypto_op_total_size_padded,
 		.dst_buf_offset = 0
diff --git a/app/test-crypto-perf/main.c b/app/test-crypto-perf/main.c
index 4ae1439..2c46525 100644
--- a/app/test-crypto-perf/main.c
+++ b/app/test-crypto-perf/main.c
@@ -149,6 +149,23 @@ cperf_initialize_cryptodev(struct cperf_options *opts, uint8_t *enabled_cdevs,
 			.nb_descriptors = opts->nb_descriptors
 		};
 
+		/**
+		 * Device info specifies the min headroom and tailroom
+		 * requirement for the crypto PMD. This need to be honoured
+		 * by the application, while creating mbuf.
+		 */
+		if (opts->headroom_sz < cdev_info.min_mbuf_headroom_req) {
+			/* Update headroom */
+			opts->headroom_sz = cdev_info.min_mbuf_headroom_req;
+		}
+		if (opts->tailroom_sz < cdev_info.min_mbuf_tailroom_req) {
+			/* Update tailroom */
+			opts->tailroom_sz = cdev_info.min_mbuf_tailroom_req;
+		}
+
+		/* Update segment size to include headroom & tailroom */
+		opts->segment_sz += (opts->headroom_sz + opts->tailroom_sz);
+
 		if (session_pool_socket[socket_id] == NULL) {
 			char mp_name[RTE_MEMPOOL_NAMESIZE];
 			struct rte_mempool *sess_mp;
-- 
2.7.4

Re: [dpdk-dev] [PATCH v1 2/3] app/crypto-perf: honour cryptodev's min headroom/tailroom

[De Lara Guarch, Pablo]

> -----Original Message-----
> From: Anoob Joseph [mailto:anoob.joseph@caviumnetworks.com]
> Sent: Wednesday, July 4, 2018 2:56 PM
> To: Doherty, Declan <declan.doherty@intel.com>; De Lara Guarch, Pablo
> <pablo.de.lara.guarch@intel.com>
> Cc: Anoob Joseph <anoob.joseph@caviumnetworks.com>; 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: [PATCH v1 2/3] app/crypto-perf: honour cryptodev's min
> headroom/tailroom
> 
> Crypto dev would specify its headroom and tailroom requirement and the
> application is expected to honour this while creating buffers.
> 
> Signed-off-by: Anoob Joseph <anoob.joseph@caviumnetworks.com>

...

> --- a/app/test-crypto-perf/cperf_test_common.c
> +++ b/app/test-crypto-perf/cperf_test_common.c

...

> fill_multi_seg_mbuf(struct rte_mbuf *m, struct rte_mempool *mp,
>  		m->buf_iova = next_seg_phys_addr;
>  		next_seg_phys_addr += mbuf_hdr_size + segment_sz;
>  		m->buf_len = segment_sz;
> -		m->data_len = segment_sz;
> +		m->data_len = data_len;
> 
> -		/* No headroom needed for the buffer */
> -		m->data_off = 0;
> +		/* Use headroom specified for the buffer */
> +		m->data_off = headroom;

Headroom is only applicable for the first segment/s.
This is adding headroom in all the segments, which looks wrong.

Re: [dpdk-dev] [PATCH v1 2/3] app/crypto-perf: honour cryptodev's min headroom/tailroom

[De Lara Guarch, Pablo]

> -----Original Message-----
> From: De Lara Guarch, Pablo
> Sent: Tuesday, July 10, 2018 12:08 PM
> To: 'Anoob Joseph' <anoob.joseph@caviumnetworks.com>; Doherty, Declan
> <declan.doherty@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 v1 2/3] app/crypto-perf: honour cryptodev's min
> headroom/tailroom
> 
> 
> 
> > -----Original Message-----
> > From: Anoob Joseph [mailto:anoob.joseph@caviumnetworks.com]
> > Sent: Wednesday, July 4, 2018 2:56 PM
> > To: Doherty, Declan <declan.doherty@intel.com>; De Lara Guarch, Pablo
> > <pablo.de.lara.guarch@intel.com>
> > Cc: Anoob Joseph <anoob.joseph@caviumnetworks.com>; 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: [PATCH v1 2/3] app/crypto-perf: honour cryptodev's min
> > headroom/tailroom
> >
> > Crypto dev would specify its headroom and tailroom requirement and the
> > application is expected to honour this while creating buffers.
> >
> > Signed-off-by: Anoob Joseph <anoob.joseph@caviumnetworks.com>
> 
> ...
> 
> > --- a/app/test-crypto-perf/cperf_test_common.c
> > +++ b/app/test-crypto-perf/cperf_test_common.c
> 
> ...
> 
> > fill_multi_seg_mbuf(struct rte_mbuf *m, struct rte_mempool *mp,
> >  		m->buf_iova = next_seg_phys_addr;
> >  		next_seg_phys_addr += mbuf_hdr_size + segment_sz;
> >  		m->buf_len = segment_sz;
> > -		m->data_len = segment_sz;
> > +		m->data_len = data_len;
> >
> > -		/* No headroom needed for the buffer */
> > -		m->data_off = 0;
> > +		/* Use headroom specified for the buffer */
> > +		m->data_off = headroom;
> 
> Headroom is only applicable for the first segment/s.
> This is adding headroom in all the segments, which looks wrong.
> 

I think "max_size" needs to be recalculated in "cperf_alloc_common_memory",
adding headroom and tailroom size, which will potentially increase the number of segments required.
Then, headroom size needs to be checked in case it is bigger than segment size, so data might need to start in the next segment.
Similar thing for tailroom.

Thanks,
Pablo

Re: [dpdk-dev] [PATCH v1 2/3] app/crypto-perf: honour cryptodev's min headroom/tailroom

[De Lara Guarch, Pablo]

> -----Original Message-----
> From: De Lara Guarch, Pablo
> Sent: Tuesday, July 10, 2018 12:17 PM
> To: 'Anoob Joseph' <anoob.joseph@caviumnetworks.com>; Doherty, Declan
> <declan.doherty@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'
> <dev@dpdk.org>
> Subject: RE: [PATCH v1 2/3] app/crypto-perf: honour cryptodev's min
> headroom/tailroom
> 
> 
> 
> > -----Original Message-----
> > From: De Lara Guarch, Pablo
> > Sent: Tuesday, July 10, 2018 12:08 PM
> > To: 'Anoob Joseph' <anoob.joseph@caviumnetworks.com>; Doherty, Declan
> > <declan.doherty@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 v1 2/3] app/crypto-perf: honour cryptodev's min
> > headroom/tailroom
> >
> >
> >
> > > -----Original Message-----
> > > From: Anoob Joseph [mailto:anoob.joseph@caviumnetworks.com]
> > > Sent: Wednesday, July 4, 2018 2:56 PM
> > > To: Doherty, Declan <declan.doherty@intel.com>; De Lara Guarch,
> > > Pablo <pablo.de.lara.guarch@intel.com>
> > > Cc: Anoob Joseph <anoob.joseph@caviumnetworks.com>; 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: [PATCH v1 2/3] app/crypto-perf: honour cryptodev's min
> > > headroom/tailroom
> > >
> > > Crypto dev would specify its headroom and tailroom requirement and
> > > the application is expected to honour this while creating buffers.
> > >
> > > Signed-off-by: Anoob Joseph <anoob.joseph@caviumnetworks.com>
> >
> > ...
> >
> > > --- a/app/test-crypto-perf/cperf_test_common.c
> > > +++ b/app/test-crypto-perf/cperf_test_common.c
> >
> > ...
> >
> > > fill_multi_seg_mbuf(struct rte_mbuf *m, struct rte_mempool *mp,
> > >  		m->buf_iova = next_seg_phys_addr;
> > >  		next_seg_phys_addr += mbuf_hdr_size + segment_sz;
> > >  		m->buf_len = segment_sz;
> > > -		m->data_len = segment_sz;
> > > +		m->data_len = data_len;
> > >
> > > -		/* No headroom needed for the buffer */
> > > -		m->data_off = 0;
> > > +		/* Use headroom specified for the buffer */
> > > +		m->data_off = headroom;
> >
> > Headroom is only applicable for the first segment/s.
> > This is adding headroom in all the segments, which looks wrong.
> >
> 
> I think "max_size" needs to be recalculated in "cperf_alloc_common_memory",
> adding headroom and tailroom size, which will potentially increase the number
> of segments required.
> Then, headroom size needs to be checked in case it is bigger than segment size,
> so data might need to start in the next segment.
> Similar thing for tailroom.

Actually, forget about this. I have been thinking about it, and it looks artificial to do this.
Generally, in a mbuf pool, headroom is the same for all mbufs/segments.

In any case, I have a concern though about this. Headroom size is got from a compile time option:
CONFIG_RTE_PKTMBUF_HEADROOM=128. PMDs generally use this value to set "data_off",
but they could use another different value.
So what happens if min_mbuf_headroom is more than this value?
since this is not configurable, this won't work.

Also, generally, headroom and tailroom are used for encapsulation, so I am not sure if this is the best place.
What about using the private size of the mbuf? That is actually configurable, even though that data is not necessarily contiguous
to the mbuf data.

Sorry for the confusion and this last minute concern.

Thanks,
Pablo


> 
> Thanks,
> Pablo
> 
> 

Re: [dpdk-dev] [PATCH v1 2/3] app/crypto-perf: honour cryptodev's min headroom/tailroom

[Anoob Joseph]

Hi Pablo,

Please see inline.

Thanks,
Anoob
On 10-07-2018 17:18, De Lara Guarch, Pablo wrote:
> External Email
>
>> -----Original Message-----
>> From: De Lara Guarch, Pablo
>> Sent: Tuesday, July 10, 2018 12:17 PM
>> To: 'Anoob Joseph' <anoob.joseph@caviumnetworks.com>; Doherty, Declan
>> <declan.doherty@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'
>> <dev@dpdk.org>
>> Subject: RE: [PATCH v1 2/3] app/crypto-perf: honour cryptodev's min
>> headroom/tailroom
>>
>>
>>
>>> -----Original Message-----
>>> From: De Lara Guarch, Pablo
>>> Sent: Tuesday, July 10, 2018 12:08 PM
>>> To: 'Anoob Joseph' <anoob.joseph@caviumnetworks.com>; Doherty, Declan
>>> <declan.doherty@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 v1 2/3] app/crypto-perf: honour cryptodev's min
>>> headroom/tailroom
>>>
>>>
>>>
>>>> -----Original Message-----
>>>> From: Anoob Joseph [mailto:anoob.joseph@caviumnetworks.com]
>>>> Sent: Wednesday, July 4, 2018 2:56 PM
>>>> To: Doherty, Declan <declan.doherty@intel.com>; De Lara Guarch,
>>>> Pablo <pablo.de.lara.guarch@intel.com>
>>>> Cc: Anoob Joseph <anoob.joseph@caviumnetworks.com>; 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: [PATCH v1 2/3] app/crypto-perf: honour cryptodev's min
>>>> headroom/tailroom
>>>>
>>>> Crypto dev would specify its headroom and tailroom requirement and
>>>> the application is expected to honour this while creating buffers.
>>>>
>>>> Signed-off-by: Anoob Joseph <anoob.joseph@caviumnetworks.com>
>>> ...
>>>
>>>> --- a/app/test-crypto-perf/cperf_test_common.c
>>>> +++ b/app/test-crypto-perf/cperf_test_common.c
>>> ...
>>>
>>>> fill_multi_seg_mbuf(struct rte_mbuf *m, struct rte_mempool *mp,
>>>>            m->buf_iova = next_seg_phys_addr;
>>>>            next_seg_phys_addr += mbuf_hdr_size + segment_sz;
>>>>            m->buf_len = segment_sz;
>>>> -         m->data_len = segment_sz;
>>>> +         m->data_len = data_len;
>>>>
>>>> -         /* No headroom needed for the buffer */
>>>> -         m->data_off = 0;
>>>> +         /* Use headroom specified for the buffer */
>>>> +         m->data_off = headroom;
>>> Headroom is only applicable for the first segment/s.
>>> This is adding headroom in all the segments, which looks wrong.
>>>
>> I think "max_size" needs to be recalculated in "cperf_alloc_common_memory",
>> adding headroom and tailroom size, which will potentially increase the number
>> of segments required.
>> Then, headroom size needs to be checked in case it is bigger than segment size,
>> so data might need to start in the next segment.
>> Similar thing for tailroom.
> Actually, forget about this. I have been thinking about it, and it looks artificial to do this.
> Generally, in a mbuf pool, headroom is the same for all mbufs/segments.
Do I need to revisit this patch? Or is this fine?
>
> In any case, I have a concern though about this. Headroom size is got from a compile time option:
> CONFIG_RTE_PKTMBUF_HEADROOM=128. PMDs generally use this value to set "data_off",
> but they could use another different value.
> So what happens if min_mbuf_headroom is more than this value?
> since this is not configurable, this won't work.
Since this is a PMD specific issue, we can have an extra check in the 
driver to make sure "CONFIG_RTE_PKTMBUF_HEADROOM">= min_mbuf_headroom 
for the PMD. If this check isn't satisfied, the driver probe would fail. 
Is this approach fine?

If application chooses to ignore CONFIG_RTE_PKTMBUF_HEADROOM altogether, 
then it will be a problem for most PMDs. With protocol offloads etc, 
headroom would be used internally, right?
> Also, generally, headroom and tailroom are used for encapsulation, so I am not sure if this is the best place.
Is your concern about whether there is enough space in headroom for 
encapsulation & PMD's usage? Application can probe the individual values 
and see if there is enough space, right? In our use case, the headroom 
requirement is 24 bytes & tailroom requirement is 8 bytes.
> What about using the private size of the mbuf? That is actually configurable, even though that data is not necessarily contiguous
> to the mbuf data.
That memory being non contiguous is the problem. We use the headroom to 
specify the command so that one single buffer can be sent to the h/w for 
processing. If there is a gap of 128 bytes (headroom which lies in 
between private space & data), it will not work.
>
> Sorry for the confusion and this last minute concern.
>
> Thanks,
> Pablo
>
>
>> Thanks,
>> Pablo
>>
>>

Re: [dpdk-dev] [PATCH v1 2/3] app/crypto-perf: honour cryptodev's min headroom/tailroom

[De Lara Guarch, Pablo]

> -----Original Message-----
> From: Anoob Joseph [mailto:anoob.joseph@caviumnetworks.com]
> Sent: Tuesday, July 10, 2018 1:23 PM
> To: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>; Doherty, Declan
> <declan.doherty@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 v1 2/3] app/crypto-perf: honour cryptodev's min
> headroom/tailroom
> 
> Hi Pablo,
> 
> Please see inline.
> 
> Thanks,
> Anoob
> On 10-07-2018 17:18, De Lara Guarch, Pablo wrote:
> > External Email
> >
> >> -----Original Message-----
> >> From: De Lara Guarch, Pablo
> >> Sent: Tuesday, July 10, 2018 12:17 PM
> >> To: 'Anoob Joseph' <anoob.joseph@caviumnetworks.com>; Doherty, Declan
> >> <declan.doherty@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'
> >> <dev@dpdk.org>
> >> Subject: RE: [PATCH v1 2/3] app/crypto-perf: honour cryptodev's min
> >> headroom/tailroom
> >>
> >>
> >>
> >>> -----Original Message-----
> >>> From: De Lara Guarch, Pablo
> >>> Sent: Tuesday, July 10, 2018 12:08 PM
> >>> To: 'Anoob Joseph' <anoob.joseph@caviumnetworks.com>; Doherty,
> >>> Declan <declan.doherty@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 v1 2/3] app/crypto-perf: honour cryptodev's min
> >>> headroom/tailroom
> >>>
> >>>
> >>>
> >>>> -----Original Message-----
> >>>> From: Anoob Joseph [mailto:anoob.joseph@caviumnetworks.com]
> >>>> Sent: Wednesday, July 4, 2018 2:56 PM
> >>>> To: Doherty, Declan <declan.doherty@intel.com>; De Lara Guarch,
> >>>> Pablo <pablo.de.lara.guarch@intel.com>
> >>>> Cc: Anoob Joseph <anoob.joseph@caviumnetworks.com>; 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: [PATCH v1 2/3] app/crypto-perf: honour cryptodev's min
> >>>> headroom/tailroom
> >>>>
> >>>> Crypto dev would specify its headroom and tailroom requirement and
> >>>> the application is expected to honour this while creating buffers.
> >>>>
> >>>> Signed-off-by: Anoob Joseph <anoob.joseph@caviumnetworks.com>
> >>> ...
> >>>
> >>>> --- a/app/test-crypto-perf/cperf_test_common.c
> >>>> +++ b/app/test-crypto-perf/cperf_test_common.c
> >>> ...
> >>>
> >>>> fill_multi_seg_mbuf(struct rte_mbuf *m, struct rte_mempool *mp,
> >>>>            m->buf_iova = next_seg_phys_addr;
> >>>>            next_seg_phys_addr += mbuf_hdr_size + segment_sz;
> >>>>            m->buf_len = segment_sz;
> >>>> -         m->data_len = segment_sz;
> >>>> +         m->data_len = data_len;
> >>>>
> >>>> -         /* No headroom needed for the buffer */
> >>>> -         m->data_off = 0;
> >>>> +         /* Use headroom specified for the buffer */
> >>>> +         m->data_off = headroom;
> >>> Headroom is only applicable for the first segment/s.
> >>> This is adding headroom in all the segments, which looks wrong.
> >>>
> >> I think "max_size" needs to be recalculated in
> >> "cperf_alloc_common_memory", adding headroom and tailroom size, which
> >> will potentially increase the number of segments required.
> >> Then, headroom size needs to be checked in case it is bigger than
> >> segment size, so data might need to start in the next segment.
> >> Similar thing for tailroom.
> > Actually, forget about this. I have been thinking about it, and it looks artificial
> to do this.
> > Generally, in a mbuf pool, headroom is the same for all mbufs/segments.
> Do I need to revisit this patch? Or is this fine?

I'd say it is ok then. I might rework the app in the future, to deal better with the pool creation
(without needing to set the mbuf parameters manually).

> >
> > In any case, I have a concern though about this. Headroom size is got from a
> compile time option:
> > CONFIG_RTE_PKTMBUF_HEADROOM=128. PMDs generally use this value to
> set
> > "data_off", but they could use another different value.
> > So what happens if min_mbuf_headroom is more than this value?
> > since this is not configurable, this won't work.
> Since this is a PMD specific issue, we can have an extra check in the driver to
> make sure "CONFIG_RTE_PKTMBUF_HEADROOM">= min_mbuf_headroom for
> the PMD. If this check isn't satisfied, the driver probe would fail.
> Is this approach fine?

Probably ok, although eventually, a check in the actual headroom, per operation, will be required.

> 
> If application chooses to ignore CONFIG_RTE_PKTMBUF_HEADROOM
> altogether, then it will be a problem for most PMDs. With protocol offloads etc,
> headroom would be used internally, right?

I am not sure what can be done here. Headroom availability depends on the network driver,
but then the application can prepend data and make the headroom smaller.

> > Also, generally, headroom and tailroom are used for encapsulation, so I am
> not sure if this is the best place.
> Is your concern about whether there is enough space in headroom for
> encapsulation & PMD's usage? Application can probe the individual values and
> see if there is enough space, right? In our use case, the headroom requirement is
> 24 bytes & tailroom requirement is 8 bytes.

Right, although this will have to be done in data path, right?
Headroom and tailroom can only be known once packets are received.

> > What about using the private size of the mbuf? That is actually
> > configurable, even though that data is not necessarily contiguous to the mbuf
> data.
> That memory being non contiguous is the problem. We use the headroom to
> specify the command so that one single buffer can be sent to the h/w for
> processing. If there is a gap of 128 bytes (headroom which lies in between
> private space & data), it will not work.

Ok, I understand. Then I'd say this is the only way to do it.

> >
> > Sorry for the confusion and this last minute concern.
> >
> > Thanks,
> > Pablo
> >
> >
> >> Thanks,
> >> Pablo
> >>
> >>

Re: [dpdk-dev] [PATCH v1 2/3] app/crypto-perf: honour cryptodev's min headroom/tailroom

[Anoob Joseph]

Hi Pablo,


On 10-07-2018 18:57, De Lara Guarch, Pablo wrote:
> External Email
>
>> -----Original Message-----
>> From: Anoob Joseph [mailto:anoob.joseph@caviumnetworks.com]
>> Sent: Tuesday, July 10, 2018 1:23 PM
>> To: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>; Doherty, Declan
>> <declan.doherty@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 v1 2/3] app/crypto-perf: honour cryptodev's min
>> headroom/tailroom
>>
>> Hi Pablo,
>>
>> Please see inline.
>>
>> Thanks,
>> Anoob
>> On 10-07-2018 17:18, De Lara Guarch, Pablo wrote:
>>> External Email
>>>
>>>> -----Original Message-----
>>>> From: De Lara Guarch, Pablo
>>>> Sent: Tuesday, July 10, 2018 12:17 PM
>>>> To: 'Anoob Joseph' <anoob.joseph@caviumnetworks.com>; Doherty, Declan
>>>> <declan.doherty@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'
>>>> <dev@dpdk.org>
>>>> Subject: RE: [PATCH v1 2/3] app/crypto-perf: honour cryptodev's min
>>>> headroom/tailroom
>>>>
>>>>
>>>>
>>>>> -----Original Message-----
>>>>> From: De Lara Guarch, Pablo
>>>>> Sent: Tuesday, July 10, 2018 12:08 PM
>>>>> To: 'Anoob Joseph' <anoob.joseph@caviumnetworks.com>; Doherty,
>>>>> Declan <declan.doherty@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 v1 2/3] app/crypto-perf: honour cryptodev's min
>>>>> headroom/tailroom
>>>>>
>>>>>
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Anoob Joseph [mailto:anoob.joseph@caviumnetworks.com]
>>>>>> Sent: Wednesday, July 4, 2018 2:56 PM
>>>>>> To: Doherty, Declan <declan.doherty@intel.com>; De Lara Guarch,
>>>>>> Pablo <pablo.de.lara.guarch@intel.com>
>>>>>> Cc: Anoob Joseph <anoob.joseph@caviumnetworks.com>; 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: [PATCH v1 2/3] app/crypto-perf: honour cryptodev's min
>>>>>> headroom/tailroom
>>>>>>
>>>>>> Crypto dev would specify its headroom and tailroom requirement and
>>>>>> the application is expected to honour this while creating buffers.
>>>>>>
>>>>>> Signed-off-by: Anoob Joseph <anoob.joseph@caviumnetworks.com>
>>>>> ...
>>>>>
>>>>>> --- a/app/test-crypto-perf/cperf_test_common.c
>>>>>> +++ b/app/test-crypto-perf/cperf_test_common.c
>>>>> ...
>>>>>
>>>>>> fill_multi_seg_mbuf(struct rte_mbuf *m, struct rte_mempool *mp,
>>>>>>             m->buf_iova = next_seg_phys_addr;
>>>>>>             next_seg_phys_addr += mbuf_hdr_size + segment_sz;
>>>>>>             m->buf_len = segment_sz;
>>>>>> -         m->data_len = segment_sz;
>>>>>> +         m->data_len = data_len;
>>>>>>
>>>>>> -         /* No headroom needed for the buffer */
>>>>>> -         m->data_off = 0;
>>>>>> +         /* Use headroom specified for the buffer */
>>>>>> +         m->data_off = headroom;
>>>>> Headroom is only applicable for the first segment/s.
>>>>> This is adding headroom in all the segments, which looks wrong.
>>>>>
>>>> I think "max_size" needs to be recalculated in
>>>> "cperf_alloc_common_memory", adding headroom and tailroom size, which
>>>> will potentially increase the number of segments required.
>>>> Then, headroom size needs to be checked in case it is bigger than
>>>> segment size, so data might need to start in the next segment.
>>>> Similar thing for tailroom.
>>> Actually, forget about this. I have been thinking about it, and it looks artificial
>> to do this.
>>> Generally, in a mbuf pool, headroom is the same for all mbufs/segments.
>> Do I need to revisit this patch? Or is this fine?
> I'd say it is ok then. I might rework the app in the future, to deal better with the pool creation
> (without needing to set the mbuf parameters manually).
Agreed.
>>> In any case, I have a concern though about this. Headroom size is got from a
>> compile time option:
>>> CONFIG_RTE_PKTMBUF_HEADROOM=128. PMDs generally use this value to
>> set
>>> "data_off", but they could use another different value.
>>> So what happens if min_mbuf_headroom is more than this value?
>>> since this is not configurable, this won't work.
>> Since this is a PMD specific issue, we can have an extra check in the driver to
>> make sure "CONFIG_RTE_PKTMBUF_HEADROOM">= min_mbuf_headroom for
>> the PMD. If this check isn't satisfied, the driver probe would fail.
>> Is this approach fine?
> Probably ok, although eventually, a check in the actual headroom, per operation, will be required.
>
>> If application chooses to ignore CONFIG_RTE_PKTMBUF_HEADROOM
>> altogether, then it will be a problem for most PMDs. With protocol offloads etc,
>> headroom would be used internally, right?
> I am not sure what can be done here. Headroom availability depends on the network driver,
> but then the application can prepend data and make the headroom smaller.
>>> Also, generally, headroom and tailroom are used for encapsulation, so I am
>> not sure if this is the best place.
>> Is your concern about whether there is enough space in headroom for
>> encapsulation & PMD's usage? Application can probe the individual values and
>> see if there is enough space, right? In our use case, the headroom requirement is
>> 24 bytes & tailroom requirement is 8 bytes.
> Right, although this will have to be done in data path, right?
> Headroom and tailroom can only be known once packets are received.
Ideally yes. But with spec change we can move this responsibility to the 
application, and could skip the check from PMD. Is that a reasonable 
approach?

With this change we explicitly say the PMD needs this much headroom & 
tailroom. So application can be expected to honor that.
>
>>> What about using the private size of the mbuf? That is actually
>>> configurable, even though that data is not necessarily contiguous to the mbuf
>> data.
>> That memory being non contiguous is the problem. We use the headroom to
>> specify the command so that one single buffer can be sent to the h/w for
>> processing. If there is a gap of 128 bytes (headroom which lies in between
>> private space & data), it will not work.
> Ok, I understand. Then I'd say this is the only way to do it.
>
>>> Sorry for the confusion and this last minute concern.
>>>
>>> Thanks,
>>> Pablo
>>>
>>>
>>>> Thanks,
>>>> Pablo
>>>>
>>>>

[dpdk-dev] [PATCH v1 3/3] test/crypto: skip validation of head/tailroom used by PMD

[Anoob Joseph]

Crypto PMDs would specify the head/tailroom it would use while
processing the crypto requests. This need to be considered while
verifying buffers processed by crypto PMDs.

Signed-off-by: Anoob Joseph <anoob.joseph@caviumnetworks.com>
---
v1:
* Added patch

 test/test/test_cryptodev_blockcipher.c | 60 ++++++++++++++++++++++++++++++----
 1 file changed, 54 insertions(+), 6 deletions(-)

diff --git a/test/test/test_cryptodev_blockcipher.c b/test/test/test_cryptodev_blockcipher.c
index 256a7da..c64b0c1 100644
--- a/test/test/test_cryptodev_blockcipher.c
+++ b/test/test/test_cryptodev_blockcipher.c
@@ -75,8 +75,9 @@ test_blockcipher_one_case(const struct blockcipher_test_case *t,
 
 	int nb_segs = 1;
 
+	rte_cryptodev_info_get(dev_id, &dev_info);
+
 	if (t->feature_mask & BLOCKCIPHER_TEST_FEATURE_SG) {
-		rte_cryptodev_info_get(dev_id, &dev_info);
 		if (!(dev_info.feature_flags &
 				RTE_CRYPTODEV_FF_MBUF_SCATTER_GATHER)) {
 			printf("Device doesn't support scatter-gather. "
@@ -438,11 +439,34 @@ test_blockcipher_one_case(const struct blockcipher_test_case *t,
 		uint8_t value;
 		uint32_t head_unchanged_len, changed_len = 0;
 		uint32_t i;
+		uint32_t hdroom_used = 0, tlroom_used = 0;
+		uint32_t hdroom = 0;
 
 		mbuf = sym_op->m_src;
+		/*
+		 * Crypto PMDs specify the headroom & tailroom it would use
+		 * when processing the crypto operation. PMD is free to modify
+		 * this space, and so the verification check should skip that
+		 * block.
+		 */
+		hdroom_used = dev_info.min_mbuf_headroom_req;
+		tlroom_used = dev_info.min_mbuf_tailroom_req;
+
+		/* Get headroom */
+		hdroom = rte_pktmbuf_headroom(mbuf);
+
 		head_unchanged_len = mbuf->buf_len;
 
 		for (i = 0; i < mbuf->buf_len; i++) {
+
+			/* Skip headroom used by PMD */
+			if (i == hdroom - hdroom_used)
+				i += hdroom_used;
+
+			/* Skip tailroom used by PMD */
+			if (i == (hdroom + mbuf->data_len))
+				i += tlroom_used;
+
 			value = *((uint8_t *)(mbuf->buf_addr)+i);
 			if (value != tmp_src_buf[i]) {
 				snprintf(test_msg, BLOCKCIPHER_TEST_MSG_LEN,
@@ -455,14 +479,13 @@ test_blockcipher_one_case(const struct blockcipher_test_case *t,
 
 		mbuf = sym_op->m_dst;
 		if (t->op_mask & BLOCKCIPHER_TEST_OP_AUTH) {
-			head_unchanged_len = rte_pktmbuf_headroom(mbuf) +
-						sym_op->auth.data.offset;
+			head_unchanged_len = hdroom + sym_op->auth.data.offset;
 			changed_len = sym_op->auth.data.length;
 			if (t->op_mask & BLOCKCIPHER_TEST_OP_AUTH_GEN)
 				changed_len += digest_len;
 		} else {
 			/* cipher-only */
-			head_unchanged_len = rte_pktmbuf_headroom(mbuf) +
+			head_unchanged_len = hdroom +
 					sym_op->cipher.data.offset;
 			changed_len = sym_op->cipher.data.length;
 		}
@@ -486,15 +509,30 @@ test_blockcipher_one_case(const struct blockcipher_test_case *t,
 		uint8_t value;
 		uint32_t head_unchanged_len = 0, changed_len = 0;
 		uint32_t i;
+		uint32_t hdroom_used = 0, tlroom_used = 0;
+		uint32_t hdroom = 0;
+
+		/*
+		 * Crypto PMDs specify the headroom & tailroom it would use
+		 * when processing the crypto operation. PMD is free to modify
+		 * this space, and so the verification check should skip that
+		 * block.
+		 */
+		hdroom_used = dev_info.min_mbuf_headroom_req;
+		tlroom_used = dev_info.min_mbuf_tailroom_req;
 
 		mbuf = sym_op->m_src;
+
+		/* Get headroom */
+		hdroom = rte_pktmbuf_headroom(mbuf);
+
 		if (t->op_mask & BLOCKCIPHER_TEST_OP_CIPHER) {
-			head_unchanged_len = rte_pktmbuf_headroom(mbuf) +
+			head_unchanged_len = hdroom +
 					sym_op->cipher.data.offset;
 			changed_len = sym_op->cipher.data.length;
 		} else {
 			/* auth-only */
-			head_unchanged_len = rte_pktmbuf_headroom(mbuf) +
+			head_unchanged_len = hdroom +
 					sym_op->auth.data.offset +
 					sym_op->auth.data.length;
 			changed_len = 0;
@@ -504,8 +542,18 @@ test_blockcipher_one_case(const struct blockcipher_test_case *t,
 			changed_len += digest_len;
 
 		for (i = 0; i < mbuf->buf_len; i++) {
+
+			/* Skip headroom used by PMD */
+			if (i == hdroom - hdroom_used)
+				i += hdroom_used;
+
 			if (i == head_unchanged_len)
 				i += changed_len;
+
+			/* Skip tailroom used by PMD */
+			if (i == (hdroom + mbuf->data_len))
+				i += tlroom_used;
+
 			value = *((uint8_t *)(mbuf->buf_addr)+i);
 			if (value != tmp_src_buf[i]) {
 				snprintf(test_msg, BLOCKCIPHER_TEST_MSG_LEN,
-- 
2.7.4

[dpdk-dev] [PATCH v2 0/4] add head/tailroom requirement for crypto PMDs

[Anoob Joseph]

This series adds the ability, for crypto PMDs, to communicate the
minimum head/tailroom requirement it may have, using the existing
cryptodev_info framework.

The availability and use of head/tailroom is an optimisation if the
hardware supports its use for crypto-op info. Devices that do not
support using the head/tailroom, can continue to operate without
any performance-drop.

Cavium's OcteonTX crypto hardware supports this feature and would use
headroom and tailroom for submitting crypto-ops to the hardware.

v2:
* Added corresponding change in scheduler PMD

v1:
* Removed deprecation notice and updated release notes
* Added corresponding change in test-cryptodev

Anoob Joseph (4):
  cryptodev: add min headroom and tailroom requirement
  app/crypto-perf: honour cryptodev's min headroom/tailroom
  test/crypto: skip validation of head/tailroom used by PMD
  crypto/scheduler: add minimum head/tailroom requirement

 app/test-crypto-perf/cperf_options.h         |  2 +
 app/test-crypto-perf/cperf_test_common.c     | 33 ++++++++++-----
 app/test-crypto-perf/main.c                  | 17 ++++++++
 doc/guides/rel_notes/release_18_08.rst       |  6 +++
 drivers/crypto/scheduler/scheduler_pmd_ops.c | 16 ++++++++
 lib/librte_cryptodev/rte_cryptodev.h         |  6 +++
 test/test/test_cryptodev_blockcipher.c       | 60 +++++++++++++++++++++++++---
 7 files changed, 123 insertions(+), 17 deletions(-)

-- 
2.7.4

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

[Anoob Joseph]

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
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.

Signed-off-by: Anoob Joseph <anoob.joseph@caviumnetworks.com>
---
v2:
* No change

v1:
* Removed deprecation notice
* Updated release note
* Renamed new fields to have 'mbuf' in the name
* Changed the type of new fields to uint16_t (instead of uint32_t)

 doc/guides/rel_notes/release_18_08.rst | 6 ++++++
 lib/librte_cryptodev/rte_cryptodev.h   | 6 ++++++
 2 files changed, 12 insertions(+)

diff --git a/doc/guides/rel_notes/release_18_08.rst b/doc/guides/rel_notes/release_18_08.rst
index 5bc23c5..fae0d26 100644
--- a/doc/guides/rel_notes/release_18_08.rst
+++ b/doc/guides/rel_notes/release_18_08.rst
@@ -70,6 +70,12 @@ ABI Changes
    Also, make sure to start the actual text at the margin.
    =========================================================
 
+* cryptodev: Additional fields in rte_cryptodev_info.
+
+  Two new fields of type ``uint16_t`` added in ``rte_cryptodev_info``
+  structure: ``min_mbuf_headroom_req`` and ``min_mbuf_tailroom_req``. These
+  parameters specify the recommended headroom and tailroom for mbufs to be
+  processed by the PMD.
 
 Removed Items
 -------------
diff --git a/lib/librte_cryptodev/rte_cryptodev.h b/lib/librte_cryptodev/rte_cryptodev.h
index 92ce6d4..4e5b5b4 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. */
 
+	uint16_t min_mbuf_headroom_req;
+	/**< Minimum mbuf headroom required by device */
+
+	uint16_t min_mbuf_tailroom_req;
+	/**< Minimum mbuf tailroom required by device */
+
 	struct {
 		unsigned max_nb_sessions;
 		/**< Maximum number of sessions supported by device. */
-- 
2.7.4

[dpdk-dev] [PATCH v2 2/4] app/crypto-perf: honour cryptodev's min headroom/tailroom

[Anoob Joseph]

Crypto dev would specify its headroom and tailroom requirement and the
application is expected to honour this while creating buffers.

Signed-off-by: Anoob Joseph <anoob.joseph@caviumnetworks.com>
---
v2:
* No change

v1:
* Accomodated the name change of new fields

 app/test-crypto-perf/cperf_options.h     |  2 ++
 app/test-crypto-perf/cperf_test_common.c | 33 +++++++++++++++++++++-----------
 app/test-crypto-perf/main.c              | 17 ++++++++++++++++
 3 files changed, 41 insertions(+), 11 deletions(-)

diff --git a/app/test-crypto-perf/cperf_options.h b/app/test-crypto-perf/cperf_options.h
index 350ad7e..f5bf03c 100644
--- a/app/test-crypto-perf/cperf_options.h
+++ b/app/test-crypto-perf/cperf_options.h
@@ -76,6 +76,8 @@ struct cperf_options {
 
 	uint32_t pool_sz;
 	uint32_t total_ops;
+	uint32_t headroom_sz;
+	uint32_t tailroom_sz;
 	uint32_t segment_sz;
 	uint32_t test_buffer_size;
 	uint32_t *imix_buffer_sizes;
diff --git a/app/test-crypto-perf/cperf_test_common.c b/app/test-crypto-perf/cperf_test_common.c
index 423782c..e803dc1 100644
--- a/app/test-crypto-perf/cperf_test_common.c
+++ b/app/test-crypto-perf/cperf_test_common.c
@@ -11,12 +11,15 @@ struct obj_params {
 	uint32_t src_buf_offset;
 	uint32_t dst_buf_offset;
 	uint16_t segment_sz;
+	uint16_t headroom_sz;
+	uint16_t data_len;
 	uint16_t segments_nb;
 };
 
 static void
 fill_single_seg_mbuf(struct rte_mbuf *m, struct rte_mempool *mp,
-		void *obj, uint32_t mbuf_offset, uint16_t segment_sz)
+		void *obj, uint32_t mbuf_offset, uint16_t segment_sz,
+		uint16_t headroom, uint16_t data_len)
 {
 	uint32_t mbuf_hdr_size = sizeof(struct rte_mbuf);
 
@@ -26,10 +29,10 @@ fill_single_seg_mbuf(struct rte_mbuf *m, struct rte_mempool *mp,
 	m->buf_iova = rte_mempool_virt2iova(obj) +
 		mbuf_offset + mbuf_hdr_size;
 	m->buf_len = segment_sz;
-	m->data_len = segment_sz;
+	m->data_len = data_len;
 
-	/* No headroom needed for the buffer */
-	m->data_off = 0;
+	/* Use headroom specified for the buffer */
+	m->data_off = headroom;
 
 	/* init some constant fields */
 	m->pool = mp;
@@ -42,7 +45,7 @@ fill_single_seg_mbuf(struct rte_mbuf *m, struct rte_mempool *mp,
 static void
 fill_multi_seg_mbuf(struct rte_mbuf *m, struct rte_mempool *mp,
 		void *obj, uint32_t mbuf_offset, uint16_t segment_sz,
-		uint16_t segments_nb)
+		uint16_t headroom, uint16_t data_len, uint16_t segments_nb)
 {
 	uint16_t mbuf_hdr_size = sizeof(struct rte_mbuf);
 	uint16_t remaining_segments = segments_nb;
@@ -57,10 +60,10 @@ fill_multi_seg_mbuf(struct rte_mbuf *m, struct rte_mempool *mp,
 		m->buf_iova = next_seg_phys_addr;
 		next_seg_phys_addr += mbuf_hdr_size + segment_sz;
 		m->buf_len = segment_sz;
-		m->data_len = segment_sz;
+		m->data_len = data_len;
 
-		/* No headroom needed for the buffer */
-		m->data_off = 0;
+		/* Use headroom specified for the buffer */
+		m->data_off = headroom;
 
 		/* init some constant fields */
 		m->pool = mp;
@@ -99,10 +102,12 @@ mempool_obj_init(struct rte_mempool *mp,
 	op->sym->m_src = m;
 	if (params->segments_nb == 1)
 		fill_single_seg_mbuf(m, mp, obj, params->src_buf_offset,
-				params->segment_sz);
+				params->segment_sz, params->headroom_sz,
+				params->data_len);
 	else
 		fill_multi_seg_mbuf(m, mp, obj, params->src_buf_offset,
-				params->segment_sz, params->segments_nb);
+				params->segment_sz, params->headroom_sz,
+				params->data_len, params->segments_nb);
 
 
 	/* Set destination buffer */
@@ -110,7 +115,8 @@ mempool_obj_init(struct rte_mempool *mp,
 		m = (struct rte_mbuf *) ((uint8_t *) obj +
 				params->dst_buf_offset);
 		fill_single_seg_mbuf(m, mp, obj, params->dst_buf_offset,
-				params->segment_sz);
+				params->segment_sz, params->headroom_sz,
+				params->data_len);
 		op->sym->m_dst = m;
 	} else
 		op->sym->m_dst = NULL;
@@ -172,6 +178,11 @@ cperf_alloc_common_memory(const struct cperf_options *options,
 
 	struct obj_params params = {
 		.segment_sz = options->segment_sz,
+		.headroom_sz = options->headroom_sz,
+		/* Data len = segment size - (headroom + tailroom) */
+		.data_len = options->segment_sz -
+			    options->headroom_sz -
+			    options->tailroom_sz,
 		.segments_nb = segments_nb,
 		.src_buf_offset = crypto_op_total_size_padded,
 		.dst_buf_offset = 0
diff --git a/app/test-crypto-perf/main.c b/app/test-crypto-perf/main.c
index 4ae1439..2c46525 100644
--- a/app/test-crypto-perf/main.c
+++ b/app/test-crypto-perf/main.c
@@ -149,6 +149,23 @@ cperf_initialize_cryptodev(struct cperf_options *opts, uint8_t *enabled_cdevs,
 			.nb_descriptors = opts->nb_descriptors
 		};
 
+		/**
+		 * Device info specifies the min headroom and tailroom
+		 * requirement for the crypto PMD. This need to be honoured
+		 * by the application, while creating mbuf.
+		 */
+		if (opts->headroom_sz < cdev_info.min_mbuf_headroom_req) {
+			/* Update headroom */
+			opts->headroom_sz = cdev_info.min_mbuf_headroom_req;
+		}
+		if (opts->tailroom_sz < cdev_info.min_mbuf_tailroom_req) {
+			/* Update tailroom */
+			opts->tailroom_sz = cdev_info.min_mbuf_tailroom_req;
+		}
+
+		/* Update segment size to include headroom & tailroom */
+		opts->segment_sz += (opts->headroom_sz + opts->tailroom_sz);
+
 		if (session_pool_socket[socket_id] == NULL) {
 			char mp_name[RTE_MEMPOOL_NAMESIZE];
 			struct rte_mempool *sess_mp;
-- 
2.7.4

[dpdk-dev] [PATCH v2 3/4] test/crypto: skip validation of head/tailroom used by PMD

[Anoob Joseph]

Crypto PMDs would specify the head/tailroom it would use while
processing the crypto requests. This need to be considered while
verifying buffers processed by crypto PMDs.

Signed-off-by: Anoob Joseph <anoob.joseph@caviumnetworks.com>
---
v2:
* No change

v1:
* Added patch

 test/test/test_cryptodev_blockcipher.c | 60 ++++++++++++++++++++++++++++++----
 1 file changed, 54 insertions(+), 6 deletions(-)

diff --git a/test/test/test_cryptodev_blockcipher.c b/test/test/test_cryptodev_blockcipher.c
index 256a7da..c64b0c1 100644
--- a/test/test/test_cryptodev_blockcipher.c
+++ b/test/test/test_cryptodev_blockcipher.c
@@ -75,8 +75,9 @@ test_blockcipher_one_case(const struct blockcipher_test_case *t,
 
 	int nb_segs = 1;
 
+	rte_cryptodev_info_get(dev_id, &dev_info);
+
 	if (t->feature_mask & BLOCKCIPHER_TEST_FEATURE_SG) {
-		rte_cryptodev_info_get(dev_id, &dev_info);
 		if (!(dev_info.feature_flags &
 				RTE_CRYPTODEV_FF_MBUF_SCATTER_GATHER)) {
 			printf("Device doesn't support scatter-gather. "
@@ -438,11 +439,34 @@ test_blockcipher_one_case(const struct blockcipher_test_case *t,
 		uint8_t value;
 		uint32_t head_unchanged_len, changed_len = 0;
 		uint32_t i;
+		uint32_t hdroom_used = 0, tlroom_used = 0;
+		uint32_t hdroom = 0;
 
 		mbuf = sym_op->m_src;
+		/*
+		 * Crypto PMDs specify the headroom & tailroom it would use
+		 * when processing the crypto operation. PMD is free to modify
+		 * this space, and so the verification check should skip that
+		 * block.
+		 */
+		hdroom_used = dev_info.min_mbuf_headroom_req;
+		tlroom_used = dev_info.min_mbuf_tailroom_req;
+
+		/* Get headroom */
+		hdroom = rte_pktmbuf_headroom(mbuf);
+
 		head_unchanged_len = mbuf->buf_len;
 
 		for (i = 0; i < mbuf->buf_len; i++) {
+
+			/* Skip headroom used by PMD */
+			if (i == hdroom - hdroom_used)
+				i += hdroom_used;
+
+			/* Skip tailroom used by PMD */
+			if (i == (hdroom + mbuf->data_len))
+				i += tlroom_used;
+
 			value = *((uint8_t *)(mbuf->buf_addr)+i);
 			if (value != tmp_src_buf[i]) {
 				snprintf(test_msg, BLOCKCIPHER_TEST_MSG_LEN,
@@ -455,14 +479,13 @@ test_blockcipher_one_case(const struct blockcipher_test_case *t,
 
 		mbuf = sym_op->m_dst;
 		if (t->op_mask & BLOCKCIPHER_TEST_OP_AUTH) {
-			head_unchanged_len = rte_pktmbuf_headroom(mbuf) +
-						sym_op->auth.data.offset;
+			head_unchanged_len = hdroom + sym_op->auth.data.offset;
 			changed_len = sym_op->auth.data.length;
 			if (t->op_mask & BLOCKCIPHER_TEST_OP_AUTH_GEN)
 				changed_len += digest_len;
 		} else {
 			/* cipher-only */
-			head_unchanged_len = rte_pktmbuf_headroom(mbuf) +
+			head_unchanged_len = hdroom +
 					sym_op->cipher.data.offset;
 			changed_len = sym_op->cipher.data.length;
 		}
@@ -486,15 +509,30 @@ test_blockcipher_one_case(const struct blockcipher_test_case *t,
 		uint8_t value;
 		uint32_t head_unchanged_len = 0, changed_len = 0;
 		uint32_t i;
+		uint32_t hdroom_used = 0, tlroom_used = 0;
+		uint32_t hdroom = 0;
+
+		/*
+		 * Crypto PMDs specify the headroom & tailroom it would use
+		 * when processing the crypto operation. PMD is free to modify
+		 * this space, and so the verification check should skip that
+		 * block.
+		 */
+		hdroom_used = dev_info.min_mbuf_headroom_req;
+		tlroom_used = dev_info.min_mbuf_tailroom_req;
 
 		mbuf = sym_op->m_src;
+
+		/* Get headroom */
+		hdroom = rte_pktmbuf_headroom(mbuf);
+
 		if (t->op_mask & BLOCKCIPHER_TEST_OP_CIPHER) {
-			head_unchanged_len = rte_pktmbuf_headroom(mbuf) +
+			head_unchanged_len = hdroom +
 					sym_op->cipher.data.offset;
 			changed_len = sym_op->cipher.data.length;
 		} else {
 			/* auth-only */
-			head_unchanged_len = rte_pktmbuf_headroom(mbuf) +
+			head_unchanged_len = hdroom +
 					sym_op->auth.data.offset +
 					sym_op->auth.data.length;
 			changed_len = 0;
@@ -504,8 +542,18 @@ test_blockcipher_one_case(const struct blockcipher_test_case *t,
 			changed_len += digest_len;
 
 		for (i = 0; i < mbuf->buf_len; i++) {
+
+			/* Skip headroom used by PMD */
+			if (i == hdroom - hdroom_used)
+				i += hdroom_used;
+
 			if (i == head_unchanged_len)
 				i += changed_len;
+
+			/* Skip tailroom used by PMD */
+			if (i == (hdroom + mbuf->data_len))
+				i += tlroom_used;
+
 			value = *((uint8_t *)(mbuf->buf_addr)+i);
 			if (value != tmp_src_buf[i]) {
 				snprintf(test_msg, BLOCKCIPHER_TEST_MSG_LEN,
-- 
2.7.4

[dpdk-dev] [PATCH v2 4/4] crypto/scheduler: add minimum head/tailroom requirement

[Anoob Joseph]

Minimum head/tailroom requirement for each PMD has to be considered
while populating the dev_info.

Signed-off-by: Anoob Joseph <anoob.joseph@caviumnetworks.com>
---
v2:
* Added this patch

 drivers/crypto/scheduler/scheduler_pmd_ops.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/crypto/scheduler/scheduler_pmd_ops.c b/drivers/crypto/scheduler/scheduler_pmd_ops.c
index 147dc51..984ea8c 100644
--- a/drivers/crypto/scheduler/scheduler_pmd_ops.c
+++ b/drivers/crypto/scheduler/scheduler_pmd_ops.c
@@ -323,6 +323,8 @@ scheduler_pmd_info_get(struct rte_cryptodev *dev,
 	struct scheduler_ctx *sched_ctx = dev->data->dev_private;
 	uint32_t max_nb_sessions = sched_ctx->nb_slaves ?
 			UINT32_MAX : RTE_CRYPTODEV_PMD_DEFAULT_MAX_NB_SESSIONS;
+	uint16_t headroom_sz = 0;
+	uint16_t tailroom_sz = 0;
 	uint32_t i;
 
 	if (!dev_info)
@@ -342,12 +344,26 @@ scheduler_pmd_info_get(struct rte_cryptodev *dev,
 				max_nb_sessions ?
 				slave_info.sym.max_nb_sessions :
 				max_nb_sessions;
+
+		/* Get the max headroom requirement among slave PMDs */
+		headroom_sz = slave_info.min_mbuf_headroom_req >
+				headroom_sz ?
+				slave_info.min_mbuf_headroom_req :
+				headroom_sz;
+
+		/* Get the max tailroom requirement among slave PMDs */
+		tailroom_sz = slave_info.min_mbuf_tailroom_req >
+				tailroom_sz ?
+				slave_info.min_mbuf_tailroom_req :
+				tailroom_sz;
 	}
 
 	dev_info->driver_id = dev->driver_id;
 	dev_info->feature_flags = dev->feature_flags;
 	dev_info->capabilities = sched_ctx->capabilities;
 	dev_info->max_nb_queue_pairs = sched_ctx->max_nb_queue_pairs;
+	dev_info->min_mbuf_headroom_req = headroom_sz;
+	dev_info->min_mbuf_tailroom_req = tailroom_sz;
 	dev_info->sym.max_nb_sessions = max_nb_sessions;
 }
 
-- 
2.7.4

Re: [dpdk-dev] [PATCH v2 0/4] add head/tailroom requirement for crypto PMDs

[De Lara Guarch, Pablo]

> -----Original Message-----
> From: Anoob Joseph [mailto:anoob.joseph@caviumnetworks.com]
> Sent: Tuesday, July 10, 2018 3:43 PM
> To: Doherty, Declan <declan.doherty@intel.com>; De Lara Guarch, Pablo
> <pablo.de.lara.guarch@intel.com>
> Cc: Anoob Joseph <anoob.joseph@caviumnetworks.com>; 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: [PATCH v2 0/4] add head/tailroom requirement for crypto PMDs
> 
> This series adds the ability, for crypto PMDs, to communicate the minimum
> head/tailroom requirement it may have, using the existing cryptodev_info
> framework.
> 
> The availability and use of head/tailroom is an optimisation if the hardware
> supports its use for crypto-op info. Devices that do not support using the
> head/tailroom, can continue to operate without any performance-drop.
> 
> Cavium's OcteonTX crypto hardware supports this feature and would use
> headroom and tailroom for submitting crypto-ops to the hardware.
> 

Series-acked-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>

Re: [dpdk-dev] [PATCH v2 0/4] add head/tailroom requirement for crypto PMDs

[De Lara Guarch, Pablo]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of De Lara Guarch, Pablo
> Sent: Tuesday, July 10, 2018 6:21 PM
> To: Anoob Joseph <anoob.joseph@caviumnetworks.com>; Doherty, Declan
> <declan.doherty@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: [dpdk-dev] [PATCH v2 0/4] add head/tailroom requirement for
> crypto PMDs
> 
> 
> 
> > -----Original Message-----
> > From: Anoob Joseph [mailto:anoob.joseph@caviumnetworks.com]
> > Sent: Tuesday, July 10, 2018 3:43 PM
> > To: Doherty, Declan <declan.doherty@intel.com>; De Lara Guarch, Pablo
> > <pablo.de.lara.guarch@intel.com>
> > Cc: Anoob Joseph <anoob.joseph@caviumnetworks.com>; 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: [PATCH v2 0/4] add head/tailroom requirement for crypto PMDs
> >
> > This series adds the ability, for crypto PMDs, to communicate the
> > minimum head/tailroom requirement it may have, using the existing
> > cryptodev_info framework.
> >
> > The availability and use of head/tailroom is an optimisation if the
> > hardware supports its use for crypto-op info. Devices that do not
> > support using the head/tailroom, can continue to operate without any
> performance-drop.
> >
> > Cavium's OcteonTX crypto hardware supports this feature and would use
> > headroom and tailroom for submitting crypto-ops to the hardware.
> >
> 
> Series-acked-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>

Series applied to dpdk-next-crypto.
Thanks,

Pablo