[v2] security: add statistics definitions and update API

  Update IPsec statistics struct definition, add per SA
statistics collection enable flag.

Signed-off-by: Radu Nicolau <radu.nicolau@intel.com>
---
v2: added second reserved field

 lib/librte_security/rte_security.h | 24 ++++++++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)
  

Hi Radu,

Please see inline.

Thanks,
Anoob

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Radu Nicolau
> Sent: Tuesday, September 3, 2019 3:12 PM
> To: dev@dpdk.org
> Cc: akhil.goyal@nxp.com; konstantin.ananyev@intel.com;
> bernard.iremonger@intel.com; declan.doherty@intel.com;
> stephen@networkplumber.org; Radu Nicolau <radu.nicolau@intel.com>
> Subject: [dpdk-dev] [PATCH v2] security: add statistics definitions and update
> API
> 
> Update IPsec statistics struct definition, add per SA statistics collection enable
> flag.
> 
> Signed-off-by: Radu Nicolau <radu.nicolau@intel.com>
> ---
> v2: added second reserved field
> 
>  lib/librte_security/rte_security.h | 24 ++++++++++++++++++++----
>  1 file changed, 20 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/librte_security/rte_security.h
> b/lib/librte_security/rte_security.h
> index 96806e3..21bbee2 100644
> --- a/lib/librte_security/rte_security.h
> +++ b/lib/librte_security/rte_security.h
> @@ -172,6 +172,14 @@ struct rte_security_ipsec_sa_options {
>  	 * * 0: Inner/outer header are not modified.
>  	 */
>  	uint32_t ecn : 1;
> +
> +	/**< Security statistics
> +	 *
> +	 * * 1: Enable per session security statistics collection for
> +	 *      this SA, if supported by the driver.
> +	 * * 0: Disable per session security statistics collection for this SA.
> +	 */

[Anoob] I believe you will have to add the above description after the item. Else the documentation generated could end up wrong. Description of all items of this structure is actually wrong.
https://doc.dpdk.org/api/structrte__security__ipsec__sa__options.html
 
> +	uint32_t stats : 1;
>  };
> 
>  /** IPSec security association direction */ @@ -482,8 +490,14 @@ struct
> rte_security_macsec_stats {  };
> 
>  struct rte_security_ipsec_stats {
> -	uint64_t reserved;
> -
> +	uint64_t ipackets;  /**< Successfully received IPsec packets. */
> +	uint64_t opackets;  /**< Successfully transmitted IPsec packets.*/
> +	uint64_t ibytes;    /**< Successfully received IPsec bytes. */
> +	uint64_t obytes;    /**< Successfully transmitted IPsec bytes. */
> +	uint64_t ierrors;   /**< IPsec packets receive/decrypt errors. */
> +	uint64_t oerrors;   /**< IPsec packets transmit/encrypt errors. */
> +	uint64_t reserved1; /**< Reserved for future use. */
> +	uint64_t reserved2; /**< Reserved for future use. */
>  };
> 
>  struct rte_security_pdcp_stats {
> @@ -507,10 +521,12 @@ struct rte_security_stats {
>   *
>   * @param	instance	security instance
>   * @param	sess		security session
> + * If security session is NULL then global (per security instance)
> + statistics
> + * will be retrieved, if supported

[Anoob] With NULL as security session, do we expect PMDs to return stats for all sessions or only for the ones 'stats' is enabled?
 
>   * @param	stats		statistics
>   * @return
> - *  - On success return 0
> - *  - On failure errno
> + *  - On success, return 0
> + *  - On failure, a negative value

[Anoob] PMDs which doesn't support this would return ENOTSUP, right? Do you think we should document that?
 
>   */
>  __rte_experimental
>  int
> --
> 2.7.4
  

Hi, replies inline:

On 9/3/2019 11:04 AM, Anoob Joseph wrote:
> Hi Radu,
>
> Please see inline.
>
> Thanks,
> Anoob
>
>> -----Original Message-----
>> From: dev <dev-bounces@dpdk.org> On Behalf Of Radu Nicolau
>> Sent: Tuesday, September 3, 2019 3:12 PM
>> To: dev@dpdk.org
>> Cc: akhil.goyal@nxp.com; konstantin.ananyev@intel.com;
>> bernard.iremonger@intel.com; declan.doherty@intel.com;
>> stephen@networkplumber.org; Radu Nicolau <radu.nicolau@intel.com>
>> Subject: [dpdk-dev] [PATCH v2] security: add statistics definitions and update
>> API
>>
>> Update IPsec statistics struct definition, add per SA statistics collection enable
>> flag.
>>
>> Signed-off-by: Radu Nicolau <radu.nicolau@intel.com>
>> ---
>> v2: added second reserved field
>>
>>   lib/librte_security/rte_security.h | 24 ++++++++++++++++++++----
>>   1 file changed, 20 insertions(+), 4 deletions(-)
>>
>> diff --git a/lib/librte_security/rte_security.h
>> b/lib/librte_security/rte_security.h
>> index 96806e3..21bbee2 100644
>> --- a/lib/librte_security/rte_security.h
>> +++ b/lib/librte_security/rte_security.h
>> @@ -172,6 +172,14 @@ struct rte_security_ipsec_sa_options {
>>   	 * * 0: Inner/outer header are not modified.
>>   	 */
>>   	uint32_t ecn : 1;
>> +
>> +	/**< Security statistics
>> +	 *
>> +	 * * 1: Enable per session security statistics collection for
>> +	 *      this SA, if supported by the driver.
>> +	 * * 0: Disable per session security statistics collection for this SA.
>> +	 */
> [Anoob] I believe you will have to add the above description after the item. Else the documentation generated could end up wrong. Description of all items of this structure is actually wrong.
> https://doc.dpdk.org/api/structrte__security__ipsec__sa__options.html
It is wrong indeed, I will fix it for the whole struct.
>   
>> +	uint32_t stats : 1;
>>   };
>>
>>   /** IPSec security association direction */ @@ -482,8 +490,14 @@ struct
>> rte_security_macsec_stats {  };
>>
>>   struct rte_security_ipsec_stats {
>> -	uint64_t reserved;
>> -
>> +	uint64_t ipackets;  /**< Successfully received IPsec packets. */
>> +	uint64_t opackets;  /**< Successfully transmitted IPsec packets.*/
>> +	uint64_t ibytes;    /**< Successfully received IPsec bytes. */
>> +	uint64_t obytes;    /**< Successfully transmitted IPsec bytes. */
>> +	uint64_t ierrors;   /**< IPsec packets receive/decrypt errors. */
>> +	uint64_t oerrors;   /**< IPsec packets transmit/encrypt errors. */
>> +	uint64_t reserved1; /**< Reserved for future use. */
>> +	uint64_t reserved2; /**< Reserved for future use. */
>>   };
>>
>>   struct rte_security_pdcp_stats {
>> @@ -507,10 +521,12 @@ struct rte_security_stats {
>>    *
>>    * @param	instance	security instance
>>    * @param	sess		security session
>> + * If security session is NULL then global (per security instance)
>> + statistics
>> + * will be retrieved, if supported
> [Anoob] With NULL as security session, do we expect PMDs to return stats for all sessions or only for the ones 'stats' is enabled?
We expect global stats, not a sum of per session stats, and independent 
of the per session configuration - that is, if there are 2 sessions and 
the per seesion stats are enabled for only one, this will still return 
the global total. I will make a note of this in the doc.
>   
>>    * @param	stats		statistics
>>    * @return
>> - *  - On success return 0
>> - *  - On failure errno
>> + *  - On success, return 0
>> + *  - On failure, a negative value
> [Anoob] PMDs which doesn't support this would return ENOTSUP, right? Do you think we should document that?
I don't think we need to explicitly document it, as the ENOTSUP is quite 
self explanatory.
>   
>>    */
>>   __rte_experimental
>>   int
>> --
>> 2.7.4
  

Hi Radu,

Please see inline.

Thanks,
Anoob

> -----Original Message-----
> From: Nicolau, Radu <radu.nicolau@intel.com>
> Sent: Tuesday, September 3, 2019 3:44 PM
> To: Anoob Joseph <anoobj@marvell.com>; dev@dpdk.org
> Cc: akhil.goyal@nxp.com; konstantin.ananyev@intel.com;
> bernard.iremonger@intel.com; declan.doherty@intel.com;
> stephen@networkplumber.org
> Subject: [EXT] Re: [dpdk-dev] [PATCH v2] security: add statistics definitions
> and update API
> 
> External Email
> 
> ----------------------------------------------------------------------
> Hi, replies inline:
> 
> On 9/3/2019 11:04 AM, Anoob Joseph wrote:
> > Hi Radu,
> >
> > Please see inline.
> >
> > Thanks,
> > Anoob
> >
> >> -----Original Message-----
> >> From: dev <dev-bounces@dpdk.org> On Behalf Of Radu Nicolau
> >> Sent: Tuesday, September 3, 2019 3:12 PM
> >> To: dev@dpdk.org
> >> Cc: akhil.goyal@nxp.com; konstantin.ananyev@intel.com;
> >> bernard.iremonger@intel.com; declan.doherty@intel.com;
> >> stephen@networkplumber.org; Radu Nicolau <radu.nicolau@intel.com>
> >> Subject: [dpdk-dev] [PATCH v2] security: add statistics definitions
> >> and update API
> >>
> >> Update IPsec statistics struct definition, add per SA statistics
> >> collection enable flag.
> >>
> >> Signed-off-by: Radu Nicolau <radu.nicolau@intel.com>
> >> ---
> >> v2: added second reserved field
> >>
> >>   lib/librte_security/rte_security.h | 24 ++++++++++++++++++++----
> >>   1 file changed, 20 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/lib/librte_security/rte_security.h
> >> b/lib/librte_security/rte_security.h
> >> index 96806e3..21bbee2 100644
> >> --- a/lib/librte_security/rte_security.h
> >> +++ b/lib/librte_security/rte_security.h
> >> @@ -172,6 +172,14 @@ struct rte_security_ipsec_sa_options {
> >>   	 * * 0: Inner/outer header are not modified.
> >>   	 */
> >>   	uint32_t ecn : 1;
> >> +
> >> +	/**< Security statistics
> >> +	 *
> >> +	 * * 1: Enable per session security statistics collection for
> >> +	 *      this SA, if supported by the driver.
> >> +	 * * 0: Disable per session security statistics collection for this SA.
> >> +	 */
> > [Anoob] I believe you will have to add the above description after the item.
> Else the documentation generated could end up wrong. Description of all
> items of this structure is actually wrong.
> > https://doc.dpdk.org/api/structrte__security__ipsec__sa__options.html
> It is wrong indeed, I will fix it for the whole struct.
> >
> >> +	uint32_t stats : 1;
> >>   };
> >>
> >>   /** IPSec security association direction */ @@ -482,8 +490,14 @@
> >> struct rte_security_macsec_stats {  };
> >>
> >>   struct rte_security_ipsec_stats {
> >> -	uint64_t reserved;
> >> -
> >> +	uint64_t ipackets;  /**< Successfully received IPsec packets. */
> >> +	uint64_t opackets;  /**< Successfully transmitted IPsec packets.*/
> >> +	uint64_t ibytes;    /**< Successfully received IPsec bytes. */
> >> +	uint64_t obytes;    /**< Successfully transmitted IPsec bytes. */
> >> +	uint64_t ierrors;   /**< IPsec packets receive/decrypt errors. */
> >> +	uint64_t oerrors;   /**< IPsec packets transmit/encrypt errors. */
> >> +	uint64_t reserved1; /**< Reserved for future use. */
> >> +	uint64_t reserved2; /**< Reserved for future use. */
> >>   };
> >>
> >>   struct rte_security_pdcp_stats {
> >> @@ -507,10 +521,12 @@ struct rte_security_stats {
> >>    *
> >>    * @param	instance	security instance
> >>    * @param	sess		security session
> >> + * If security session is NULL then global (per security instance)
> >> + statistics
> >> + * will be retrieved, if supported
> > [Anoob] With NULL as security session, do we expect PMDs to return stats
> for all sessions or only for the ones 'stats' is enabled?
> We expect global stats, not a sum of per session stats, and independent of
> the per session configuration - that is, if there are 2 sessions and the per
> seesion stats are enabled for only one, this will still return the global total. I
> will make a note of this in the doc.

[Anoob] That sounds good.
 
> >
> >>    * @param	stats		statistics
> >>    * @return
> >> - *  - On success return 0
> >> - *  - On failure errno
> >> + *  - On success, return 0
> >> + *  - On failure, a negative value
> > [Anoob] PMDs which doesn't support this would return ENOTSUP, right?
> Do you think we should document that?
> I don't think we need to explicitly document it, as the ENOTSUP is quite self
> explanatory.

[Anoob] Agreed.
 
> >
> >>    */
> >>   __rte_experimental
> >>   int
> >> --
> >> 2.7.4