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

["Nicolau, Radu" <radu.nicolau@intel.com>]

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