[dpdk-dev] [PATCH] security: add statistics definitions and update API

[dpdk-dev] [PATCH] security: add statistics definitions and update API

[Radu Nicolau]

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

Signed-off-by: Radu Nicolau <radu.nicolau@intel.com>
---
 lib/librte_security/rte_security.h | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/lib/librte_security/rte_security.h b/lib/librte_security/rte_security.h
index 96806e3..8a2ff34 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.
+	 */
+	uint32_t stats : 1;
 };
 
 /** IPSec security association direction */
@@ -482,6 +490,12 @@ struct rte_security_macsec_stats {
 };
 
 struct rte_security_ipsec_stats {
+	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 reserved;
 
 };
@@ -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
  * @param	stats		statistics
  * @return
- *  - On success return 0
- *  - On failure errno
+ *  - On success, return 0
+ *  - On failure, a negative value
  */
 __rte_experimental
 int
-- 
2.7.4

Re: [dpdk-dev] [PATCH] security: add statistics definitions and update API

[Stephen Hemminger]

On Fri, 30 Aug 2019 10:19:20 +0100
Radu Nicolau <radu.nicolau@intel.com> wrote:

>  
>  struct rte_security_ipsec_stats {
> +	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 reserved;
>  
>  };

Why not a second reserved field, then it will be 8*8 64 bytes long
which is cache aligned. And you will have a pair for future counters.

Re: [dpdk-dev] [PATCH] security: add statistics definitions and update API

[Nicolau, Radu]

On 8/30/2019 3:33 PM, Stephen Hemminger wrote:
> On Fri, 30 Aug 2019 10:19:20 +0100
> Radu Nicolau <radu.nicolau@intel.com> wrote:
>
>>   
>>   struct rte_security_ipsec_stats {
>> +	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 reserved;
>>   
>>   };
> Why not a second reserved field, then it will be 8*8 64 bytes long
> which is cache aligned. And you will have a pair for future counters.
Yes, I will add a second reserved field.

[dpdk-dev] [PATCH v2] security: add statistics definitions and update API

[Radu Nicolau]

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.
+	 */
+	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
  * @param	stats		statistics
  * @return
- *  - On success return 0
- *  - On failure errno
+ *  - On success, return 0
+ *  - On failure, a negative value
  */
 __rte_experimental
 int
-- 
2.7.4

Re: [dpdk-dev] [PATCH v2] security: add statistics definitions and update API

[Anoob Joseph]

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

Re: [dpdk-dev] [PATCH v2] security: add statistics definitions and update API

[Nicolau, Radu]

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

Re: [dpdk-dev] [EXT] Re: [PATCH v2] security: add statistics definitions and update API

[Anoob Joseph]

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

[dpdk-dev] [PATCH v3] security: add statistics definitions and update API

[Radu Nicolau]

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
v3: update doc

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

diff --git a/lib/librte_security/rte_security.h b/lib/librte_security/rte_security.h
index d56907d..2d064f4 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.
+	 */
+	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,13 @@ 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. Global statistics collection is not
+ * dependent on the per session statistics configuration.
  * @param	stats		statistics
  * @return
- *  - On success return 0
- *  - On failure errno
+ *  - On success, return 0
+ *  - On failure, a negative value
  */
 __rte_experimental
 int
-- 
2.7.4

Re: [dpdk-dev] [PATCH v3] security: add statistics definitions and update API

[Akhil Goyal]

> 
> 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
> v3: update doc
> 
Acked-by: Akhil Goyal <akhil.goyal@nxp.com>

Re: [dpdk-dev] [EXT] [PATCH v3] security: add statistics definitions and update API

[Anoob Joseph]

> -----Original Message-----
> From: Radu Nicolau <radu.nicolau@intel.com>
> Sent: Tuesday, September 3, 2019 6:36 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; Anoob Joseph <anoobj@marvell.com>;
> Radu Nicolau <radu.nicolau@intel.com>
> Subject: [EXT] [PATCH v3] security: add statistics definitions and update API
> 
> External Email
> 
> ----------------------------------------------------------------------
> 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
> v3: update doc

Acked-by: Anoob Joseph <anoobj@marvell.com>

Re: [dpdk-dev] [PATCH v3] security: add statistics definitions and update API

[Akhil Goyal]

> >
> > 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
> > v3: update doc
> >
> Acked-by: Akhil Goyal <akhil.goyal@nxp.com>
Applied to dpdk-next-crypto

Thanks.