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

[Anoob Joseph <anoobj@marvell.com>]

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