Hi Radu,
Please see inline.
Thanks,
Anoob
> -----Original Message-----
> From: Nicolau, Radu <radu.nico...@intel.com>
> Sent: Tuesday, September 3, 2019 3:44 PM
> To: Anoob Joseph <ano...@marvell.com>; dev@dpdk.org
> Cc: akhil.go...@nxp.com; konstantin.anan...@intel.com;
> bernard.iremon...@intel.com; declan.dohe...@intel.com;
> step...@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-boun...@dpdk.org> On Behalf Of Radu Nicolau
> >> Sent: Tuesday, September 3, 2019 3:12 PM
> >> To: dev@dpdk.org
> >> Cc: akhil.go...@nxp.com; konstantin.anan...@intel.com;
> >> bernard.iremon...@intel.com; declan.dohe...@intel.com;
> >> step...@networkplumber.org; Radu Nicolau <radu.nico...@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.nico...@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
