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

[Anoob Joseph <anoobj@marvell.com>]

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