diff mbox series

[v2,4/9] examples/ipsec-secgw: add stats interval argument

Message ID 20210915134522.1311843-5-radu.nicolau@intel.com (mailing list archive)
State Superseded
Delegated to: akhil goyal
Headers show
Series IPsec Sec GW new features | expand

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Nicolau, Radu Sept. 15, 2021, 1:45 p.m. UTC
Add -t for stats screen update interval, disabled by default.

Signed-off-by: Radu Nicolau <radu.nicolau@intel.com>
---
 doc/guides/sample_app_ug/ipsec_secgw.rst |  5 ++++
 examples/ipsec-secgw/ipsec-secgw.c       | 29 ++++++++++++++++--------
 examples/ipsec-secgw/ipsec-secgw.h       | 15 ------------
 3 files changed, 25 insertions(+), 24 deletions(-)

Comments

Hemant Agrawal Sept. 16, 2021, 9:13 a.m. UTC | #1
On 9/15/2021 7:15 PM, Radu Nicolau wrote:
> Add -t for stats screen update interval, disabled by default.
>
> Signed-off-by: Radu Nicolau <radu.nicolau@intel.com>
> ---
>   doc/guides/sample_app_ug/ipsec_secgw.rst |  5 ++++
>   examples/ipsec-secgw/ipsec-secgw.c       | 29 ++++++++++++++++--------
>   examples/ipsec-secgw/ipsec-secgw.h       | 15 ------------
>   3 files changed, 25 insertions(+), 24 deletions(-)
>
> diff --git a/doc/guides/sample_app_ug/ipsec_secgw.rst b/doc/guides/sample_app_ug/ipsec_secgw.rst
> index 20bc1e6bc4..0d55e74022 100644
> --- a/doc/guides/sample_app_ug/ipsec_secgw.rst
> +++ b/doc/guides/sample_app_ug/ipsec_secgw.rst
> @@ -127,6 +127,7 @@ The application has a number of command line options::
>                           -p PORTMASK -P -u PORTMASK -j FRAMESIZE
>                           -l -w REPLAY_WINDOW_SIZE -e -a
>                           -c SAD_CACHE_SIZE
> +                        -t STATISTICS_INTERVAL
>                           -s NUMBER_OF_MBUFS_IN_PACKET_POOL
>                           -f CONFIG_FILE_PATH
>                           --config (port,queue,lcore)[,(port,queue,lcore)]
> @@ -176,6 +177,10 @@ Where:
>       Zero value disables cache.
>       Default value: 128.
>   
> +*   ``-t``: specifies the statistics screen update interval. If set to zero or
> +    omitted statistics screen is disabled.
> +    Default value: 0.
> +
>   *   ``-s``: sets number of mbufs in packet pool, if not provided number of mbufs
>       will be calculated based on number of cores, eth ports and crypto queues.
>   
> diff --git a/examples/ipsec-secgw/ipsec-secgw.c b/examples/ipsec-secgw/ipsec-secgw.c
> index 265fff4bef..60b25be872 100644
> --- a/examples/ipsec-secgw/ipsec-secgw.c
> +++ b/examples/ipsec-secgw/ipsec-secgw.c
> @@ -181,6 +181,7 @@ static uint32_t frag_tbl_sz;
>   static uint32_t frame_buf_size = RTE_MBUF_DEFAULT_BUF_SIZE;
>   static uint32_t mtu_size = RTE_ETHER_MTU;
>   static uint64_t frag_ttl_ns = MAX_FRAG_TTL_NS;
> +static uint32_t stats_interval;
>   
>   /* application wide librte_ipsec/SA parameters */
>   struct app_sa_prm app_sa_prm = {
> @@ -292,7 +293,6 @@ adjust_ipv6_pktlen(struct rte_mbuf *m, const struct rte_ipv6_hdr *iph,
>   	}
>   }
>   
> -#if (STATS_INTERVAL > 0)
>   
>   /* Print out statistics on packet distribution */
>   static void
> @@ -352,9 +352,8 @@ print_stats_cb(__rte_unused void *param)
>   		   total_packets_dropped);
>   	printf("\n====================================================\n");
>   
> -	rte_eal_alarm_set(STATS_INTERVAL * US_PER_S, print_stats_cb, NULL);
> +	rte_eal_alarm_set(stats_interval * US_PER_S, print_stats_cb, NULL);
>   }
> -#endif /* STATS_INTERVAL */
>   
>   static inline void
>   prepare_one_packet(struct rte_mbuf *pkt, struct ipsec_traffic *t)
> @@ -1435,6 +1434,7 @@ print_usage(const char *prgname)
>   		" [-e]"
>   		" [-a]"
>   		" [-c]"
> +		" [-t STATS_INTERVAL]"
>   		" [-s NUMBER_OF_MBUFS_IN_PKT_POOL]"
>   		" -f CONFIG_FILE"
>   		" --config (port,queue,lcore)[,(port,queue,lcore)]"
> @@ -1459,6 +1459,8 @@ print_usage(const char *prgname)
>   		"  -a enables SA SQN atomic behaviour\n"
>   		"  -c specifies inbound SAD cache size,\n"
>   		"     zero value disables the cache (default value: 128)\n"
> +		"  -t specifies statistics screen update interval,\n"
> +		"     zero disables statistics screen (default value: 0)\n"
>   		"  -s number of mbufs in packet pool, if not specified number\n"
>   		"     of mbufs will be calculated based on number of cores,\n"
>   		"     ports and crypto queues\n"
> @@ -1666,7 +1668,7 @@ parse_args(int32_t argc, char **argv, struct eh_conf *eh_conf)
>   
>   	argvopt = argv;
>   
> -	while ((opt = getopt_long(argc, argvopt, "aelp:Pu:f:j:w:c:s:",
> +	while ((opt = getopt_long(argc, argvopt, "aelp:Pu:f:j:w:c:t:s:",
>   				lgopts, &option_index)) != EOF) {
>   
>   		switch (opt) {
> @@ -1747,6 +1749,15 @@ parse_args(int32_t argc, char **argv, struct eh_conf *eh_conf)
>   			}
>   			app_sa_prm.cache_sz = ret;
>   			break;
> +		case 't':
> +			ret = parse_decimal(optarg);
> +			if (ret < 0) {
> +				printf("Invalid interval value: %s\n", optarg);
> +				print_usage(prgname);
> +				return -1;
> +			}
> +			stats_interval = ret;
> +			break;
>   		case CMD_LINE_OPT_CONFIG_NUM:
>   			ret = parse_config(optarg);
>   			if (ret) {
> @@ -3350,11 +3361,11 @@ main(int32_t argc, char **argv)
>   
>   	check_all_ports_link_status(enabled_port_mask);
>   
> -#if (STATS_INTERVAL > 0)
> -	rte_eal_alarm_set(STATS_INTERVAL * US_PER_S, print_stats_cb, NULL);
> -#else
> -	RTE_LOG(INFO, IPSEC, "Stats display disabled\n");
> -#endif /* STATS_INTERVAL */
> +	if (stats_interval > 0)
> +		rte_eal_alarm_set(stats_interval * US_PER_S,
> +				print_stats_cb, NULL);
> +	else
> +		RTE_LOG(INFO, IPSEC, "Stats display disabled\n");
>   
>   	/* launch per-lcore init on every lcore */
>   	rte_eal_mp_remote_launch(ipsec_launch_one_lcore, eh_conf, CALL_MAIN);
> diff --git a/examples/ipsec-secgw/ipsec-secgw.h b/examples/ipsec-secgw/ipsec-secgw.h
> index f3082a1037..de9f382742 100644
> --- a/examples/ipsec-secgw/ipsec-secgw.h
> +++ b/examples/ipsec-secgw/ipsec-secgw.h
> @@ -6,9 +6,6 @@
>   
>   #include <stdbool.h>
>   
> -#ifndef STATS_INTERVAL
> -#define STATS_INTERVAL 0
> -#endif
>   
>   #define NB_SOCKETS 4
>   
> @@ -144,38 +141,26 @@ is_unprotected_port(uint16_t port_id)
>   static inline void
>   core_stats_update_rx(int n)
>   {
> -#if (STATS_INTERVAL > 0)
>   	int lcore_id = rte_lcore_id();
>   	core_statistics[lcore_id].rx += n;
>   	core_statistics[lcore_id].rx_call++;
>   	if (n == MAX_PKT_BURST)
>   		core_statistics[lcore_id].burst_rx += n;
> -#else
> -	RTE_SET_USED(n);
> -#endif /* STATS_INTERVAL */
>   }
>   
>   static inline void
>   core_stats_update_tx(int n)
>   {
> -#if (STATS_INTERVAL > 0)
>   	int lcore_id = rte_lcore_id();
>   	core_statistics[lcore_id].tx += n;
>   	core_statistics[lcore_id].tx_call++;
> -#else
> -	RTE_SET_USED(n);
> -#endif /* STATS_INTERVAL */
>   }
>   
>   static inline void
>   core_stats_update_drop(int n)
>   {
> -#if (STATS_INTERVAL > 0)
>   	int lcore_id = rte_lcore_id();
>   	core_statistics[lcore_id].dropped += n;
> -#else
> -	RTE_SET_USED(n);
> -#endif /* STATS_INTERVAL */
>   }
>   
>   /* helper routine to free bulk of packets */
Acked-by: Hemant Agrawal <hemant.agrawal@nxp.com>
Anoob Joseph Sept. 16, 2021, 9:30 a.m. UTC | #2
Hi Radu,

Making stats dynamic would have a perf cost. That was the reason it was introduced as a compile time option. Why do we need it changed?

Also, it looks like this patch is only disabling stats printing when timeout is 0. I don't see stats collection being conditional. Stats collection would also have perf impact, right?

Thanks,
Anoob

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Radu Nicolau
> Sent: Wednesday, September 15, 2021 7:15 PM
> To: Radu Nicolau <radu.nicolau@intel.com>; Akhil Goyal
> <gakhil@marvell.com>
> Cc: dev@dpdk.org; declan.doherty@intel.com;
> hemant.agrawal@oss.nxp.com
> Subject: [EXT] [dpdk-dev] [PATCH v2 4/9] examples/ipsec-secgw: add stats
> interval argument
> 
> External Email
> 
> ----------------------------------------------------------------------
> Add -t for stats screen update interval, disabled by default.
> 
> Signed-off-by: Radu Nicolau <radu.nicolau@intel.com>
> ---
>  doc/guides/sample_app_ug/ipsec_secgw.rst |  5 ++++
>  examples/ipsec-secgw/ipsec-secgw.c       | 29 ++++++++++++++++--------
>  examples/ipsec-secgw/ipsec-secgw.h       | 15 ------------
>  3 files changed, 25 insertions(+), 24 deletions(-)
> 
> diff --git a/doc/guides/sample_app_ug/ipsec_secgw.rst
> b/doc/guides/sample_app_ug/ipsec_secgw.rst
> index 20bc1e6bc4..0d55e74022 100644
> --- a/doc/guides/sample_app_ug/ipsec_secgw.rst
> +++ b/doc/guides/sample_app_ug/ipsec_secgw.rst
> @@ -127,6 +127,7 @@ The application has a number of command line
> options::
>                          -p PORTMASK -P -u PORTMASK -j FRAMESIZE
>                          -l -w REPLAY_WINDOW_SIZE -e -a
>                          -c SAD_CACHE_SIZE
> +                        -t STATISTICS_INTERVAL
>                          -s NUMBER_OF_MBUFS_IN_PACKET_POOL
>                          -f CONFIG_FILE_PATH
>                          --config (port,queue,lcore)[,(port,queue,lcore)]
> @@ -176,6 +177,10 @@ Where:
>      Zero value disables cache.
>      Default value: 128.
> 
> +*   ``-t``: specifies the statistics screen update interval. If set to zero or
> +    omitted statistics screen is disabled.
> +    Default value: 0.
> +
>  *   ``-s``: sets number of mbufs in packet pool, if not provided number of
> mbufs
>      will be calculated based on number of cores, eth ports and crypto queues.
> 
> diff --git a/examples/ipsec-secgw/ipsec-secgw.c b/examples/ipsec-
> secgw/ipsec-secgw.c
> index 265fff4bef..60b25be872 100644
> --- a/examples/ipsec-secgw/ipsec-secgw.c
> +++ b/examples/ipsec-secgw/ipsec-secgw.c
> @@ -181,6 +181,7 @@ static uint32_t frag_tbl_sz;  static uint32_t
> frame_buf_size = RTE_MBUF_DEFAULT_BUF_SIZE;  static uint32_t mtu_size
> = RTE_ETHER_MTU;  static uint64_t frag_ttl_ns = MAX_FRAG_TTL_NS;
> +static uint32_t stats_interval;
> 
>  /* application wide librte_ipsec/SA parameters */  struct app_sa_prm
> app_sa_prm = { @@ -292,7 +293,6 @@ adjust_ipv6_pktlen(struct rte_mbuf
> *m, const struct rte_ipv6_hdr *iph,
>  	}
>  }
> 
> -#if (STATS_INTERVAL > 0)
> 
>  /* Print out statistics on packet distribution */  static void @@ -352,9 +352,8
> @@ print_stats_cb(__rte_unused void *param)
>  		   total_packets_dropped);
> 
> 	printf("\n=============================================
> =======\n");
> 
> -	rte_eal_alarm_set(STATS_INTERVAL * US_PER_S, print_stats_cb,
> NULL);
> +	rte_eal_alarm_set(stats_interval * US_PER_S, print_stats_cb, NULL);
>  }
> -#endif /* STATS_INTERVAL */
> 
>  static inline void
>  prepare_one_packet(struct rte_mbuf *pkt, struct ipsec_traffic *t) @@ -
> 1435,6 +1434,7 @@ print_usage(const char *prgname)
>  		" [-e]"
>  		" [-a]"
>  		" [-c]"
> +		" [-t STATS_INTERVAL]"
>  		" [-s NUMBER_OF_MBUFS_IN_PKT_POOL]"
>  		" -f CONFIG_FILE"
>  		" --config (port,queue,lcore)[,(port,queue,lcore)]"
> @@ -1459,6 +1459,8 @@ print_usage(const char *prgname)
>  		"  -a enables SA SQN atomic behaviour\n"
>  		"  -c specifies inbound SAD cache size,\n"
>  		"     zero value disables the cache (default value: 128)\n"
> +		"  -t specifies statistics screen update interval,\n"
> +		"     zero disables statistics screen (default value: 0)\n"
>  		"  -s number of mbufs in packet pool, if not specified
> number\n"
>  		"     of mbufs will be calculated based on number of cores,\n"
>  		"     ports and crypto queues\n"
> @@ -1666,7 +1668,7 @@ parse_args(int32_t argc, char **argv, struct
> eh_conf *eh_conf)
> 
>  	argvopt = argv;
> 
> -	while ((opt = getopt_long(argc, argvopt, "aelp:Pu:f:j:w:c:s:",
> +	while ((opt = getopt_long(argc, argvopt, "aelp:Pu:f:j:w:c:t:s:",
>  				lgopts, &option_index)) != EOF) {
> 
>  		switch (opt) {
> @@ -1747,6 +1749,15 @@ parse_args(int32_t argc, char **argv, struct
> eh_conf *eh_conf)
>  			}
>  			app_sa_prm.cache_sz = ret;
>  			break;
> +		case 't':
> +			ret = parse_decimal(optarg);
> +			if (ret < 0) {
> +				printf("Invalid interval value: %s\n", optarg);
> +				print_usage(prgname);
> +				return -1;
> +			}
> +			stats_interval = ret;
> +			break;
>  		case CMD_LINE_OPT_CONFIG_NUM:
>  			ret = parse_config(optarg);
>  			if (ret) {
> @@ -3350,11 +3361,11 @@ main(int32_t argc, char **argv)
> 
>  	check_all_ports_link_status(enabled_port_mask);
> 
> -#if (STATS_INTERVAL > 0)
> -	rte_eal_alarm_set(STATS_INTERVAL * US_PER_S, print_stats_cb,
> NULL);
> -#else
> -	RTE_LOG(INFO, IPSEC, "Stats display disabled\n");
> -#endif /* STATS_INTERVAL */
> +	if (stats_interval > 0)
> +		rte_eal_alarm_set(stats_interval * US_PER_S,
> +				print_stats_cb, NULL);
> +	else
> +		RTE_LOG(INFO, IPSEC, "Stats display disabled\n");
> 
>  	/* launch per-lcore init on every lcore */
>  	rte_eal_mp_remote_launch(ipsec_launch_one_lcore, eh_conf,
> CALL_MAIN); diff --git a/examples/ipsec-secgw/ipsec-secgw.h
> b/examples/ipsec-secgw/ipsec-secgw.h
> index f3082a1037..de9f382742 100644
> --- a/examples/ipsec-secgw/ipsec-secgw.h
> +++ b/examples/ipsec-secgw/ipsec-secgw.h
> @@ -6,9 +6,6 @@
> 
>  #include <stdbool.h>
> 
> -#ifndef STATS_INTERVAL
> -#define STATS_INTERVAL 0
> -#endif
> 
>  #define NB_SOCKETS 4
> 
> @@ -144,38 +141,26 @@ is_unprotected_port(uint16_t port_id)  static inline
> void  core_stats_update_rx(int n)  { -#if (STATS_INTERVAL > 0)
>  	int lcore_id = rte_lcore_id();
>  	core_statistics[lcore_id].rx += n;
>  	core_statistics[lcore_id].rx_call++;
>  	if (n == MAX_PKT_BURST)
>  		core_statistics[lcore_id].burst_rx += n; -#else
> -	RTE_SET_USED(n);
> -#endif /* STATS_INTERVAL */
>  }
> 
>  static inline void
>  core_stats_update_tx(int n)
>  {
> -#if (STATS_INTERVAL > 0)
>  	int lcore_id = rte_lcore_id();
>  	core_statistics[lcore_id].tx += n;
>  	core_statistics[lcore_id].tx_call++;
> -#else
> -	RTE_SET_USED(n);
> -#endif /* STATS_INTERVAL */
>  }
> 
>  static inline void
>  core_stats_update_drop(int n)
>  {
> -#if (STATS_INTERVAL > 0)
>  	int lcore_id = rte_lcore_id();
>  	core_statistics[lcore_id].dropped += n; -#else
> -	RTE_SET_USED(n);
> -#endif /* STATS_INTERVAL */
>  }
> 
>  /* helper routine to free bulk of packets */
> --
> 2.25.1
Nicolau, Radu Sept. 16, 2021, 10:24 a.m. UTC | #3
Hi Anoob,

On 9/16/2021 10:30 AM, Anoob Joseph wrote:
> Hi Radu,
>
> Making stats dynamic would have a perf cost. That was the reason it was introduced as a compile time option. Why do we need it changed?
We changed it for two reasons, for a better UX and for the telemetry 
feature that is introduced with patch 3.
>
> Also, it looks like this patch is only disabling stats printing when timeout is 0. I don't see stats collection being conditional. Stats collection would also have perf impact, right?
I don't think there is a measurable performance impact because these are 
per burst calls.

Regards,
Radu
>
> Thanks,
> Anoob
>
>> -----Original Message-----
>> From: dev <dev-bounces@dpdk.org> On Behalf Of Radu Nicolau
>> Sent: Wednesday, September 15, 2021 7:15 PM
>> To: Radu Nicolau <radu.nicolau@intel.com>; Akhil Goyal
>> <gakhil@marvell.com>
>> Cc: dev@dpdk.org; declan.doherty@intel.com;
>> hemant.agrawal@oss.nxp.com
>> Subject: [EXT] [dpdk-dev] [PATCH v2 4/9] examples/ipsec-secgw: add stats
>> interval argument
>>
>> External Email
>>
>> ----------------------------------------------------------------------
>> Add -t for stats screen update interval, disabled by default.
>>
>> Signed-off-by: Radu Nicolau <radu.nicolau@intel.com>
>> ---
>>   doc/guides/sample_app_ug/ipsec_secgw.rst |  5 ++++
>>   examples/ipsec-secgw/ipsec-secgw.c       | 29 ++++++++++++++++--------
>>   examples/ipsec-secgw/ipsec-secgw.h       | 15 ------------
>>   3 files changed, 25 insertions(+), 24 deletions(-)
>>
>> diff --git a/doc/guides/sample_app_ug/ipsec_secgw.rst
>> b/doc/guides/sample_app_ug/ipsec_secgw.rst
>> index 20bc1e6bc4..0d55e74022 100644
>> --- a/doc/guides/sample_app_ug/ipsec_secgw.rst
>> +++ b/doc/guides/sample_app_ug/ipsec_secgw.rst
>> @@ -127,6 +127,7 @@ The application has a number of command line
>> options::
>>                           -p PORTMASK -P -u PORTMASK -j FRAMESIZE
>>                           -l -w REPLAY_WINDOW_SIZE -e -a
>>                           -c SAD_CACHE_SIZE
>> +                        -t STATISTICS_INTERVAL
>>                           -s NUMBER_OF_MBUFS_IN_PACKET_POOL
>>                           -f CONFIG_FILE_PATH
>>                           --config (port,queue,lcore)[,(port,queue,lcore)]
>> @@ -176,6 +177,10 @@ Where:
>>       Zero value disables cache.
>>       Default value: 128.
>>
>> +*   ``-t``: specifies the statistics screen update interval. If set to zero or
>> +    omitted statistics screen is disabled.
>> +    Default value: 0.
>> +
>>   *   ``-s``: sets number of mbufs in packet pool, if not provided number of
>> mbufs
>>       will be calculated based on number of cores, eth ports and crypto queues.
>>
>> diff --git a/examples/ipsec-secgw/ipsec-secgw.c b/examples/ipsec-
>> secgw/ipsec-secgw.c
>> index 265fff4bef..60b25be872 100644
>> --- a/examples/ipsec-secgw/ipsec-secgw.c
>> +++ b/examples/ipsec-secgw/ipsec-secgw.c
>> @@ -181,6 +181,7 @@ static uint32_t frag_tbl_sz;  static uint32_t
>> frame_buf_size = RTE_MBUF_DEFAULT_BUF_SIZE;  static uint32_t mtu_size
>> = RTE_ETHER_MTU;  static uint64_t frag_ttl_ns = MAX_FRAG_TTL_NS;
>> +static uint32_t stats_interval;
>>
>>   /* application wide librte_ipsec/SA parameters */  struct app_sa_prm
>> app_sa_prm = { @@ -292,7 +293,6 @@ adjust_ipv6_pktlen(struct rte_mbuf
>> *m, const struct rte_ipv6_hdr *iph,
>>   	}
>>   }
>>
>> -#if (STATS_INTERVAL > 0)
>>
>>   /* Print out statistics on packet distribution */  static void @@ -352,9 +352,8
>> @@ print_stats_cb(__rte_unused void *param)
>>   		   total_packets_dropped);
>>
>> 	printf("\n=============================================
>> =======\n");
>>
>> -	rte_eal_alarm_set(STATS_INTERVAL * US_PER_S, print_stats_cb,
>> NULL);
>> +	rte_eal_alarm_set(stats_interval * US_PER_S, print_stats_cb, NULL);
>>   }
>> -#endif /* STATS_INTERVAL */
>>
>>   static inline void
>>   prepare_one_packet(struct rte_mbuf *pkt, struct ipsec_traffic *t) @@ -
>> 1435,6 +1434,7 @@ print_usage(const char *prgname)
>>   		" [-e]"
>>   		" [-a]"
>>   		" [-c]"
>> +		" [-t STATS_INTERVAL]"
>>   		" [-s NUMBER_OF_MBUFS_IN_PKT_POOL]"
>>   		" -f CONFIG_FILE"
>>   		" --config (port,queue,lcore)[,(port,queue,lcore)]"
>> @@ -1459,6 +1459,8 @@ print_usage(const char *prgname)
>>   		"  -a enables SA SQN atomic behaviour\n"
>>   		"  -c specifies inbound SAD cache size,\n"
>>   		"     zero value disables the cache (default value: 128)\n"
>> +		"  -t specifies statistics screen update interval,\n"
>> +		"     zero disables statistics screen (default value: 0)\n"
>>   		"  -s number of mbufs in packet pool, if not specified
>> number\n"
>>   		"     of mbufs will be calculated based on number of cores,\n"
>>   		"     ports and crypto queues\n"
>> @@ -1666,7 +1668,7 @@ parse_args(int32_t argc, char **argv, struct
>> eh_conf *eh_conf)
>>
>>   	argvopt = argv;
>>
>> -	while ((opt = getopt_long(argc, argvopt, "aelp:Pu:f:j:w:c:s:",
>> +	while ((opt = getopt_long(argc, argvopt, "aelp:Pu:f:j:w:c:t:s:",
>>   				lgopts, &option_index)) != EOF) {
>>
>>   		switch (opt) {
>> @@ -1747,6 +1749,15 @@ parse_args(int32_t argc, char **argv, struct
>> eh_conf *eh_conf)
>>   			}
>>   			app_sa_prm.cache_sz = ret;
>>   			break;
>> +		case 't':
>> +			ret = parse_decimal(optarg);
>> +			if (ret < 0) {
>> +				printf("Invalid interval value: %s\n", optarg);
>> +				print_usage(prgname);
>> +				return -1;
>> +			}
>> +			stats_interval = ret;
>> +			break;
>>   		case CMD_LINE_OPT_CONFIG_NUM:
>>   			ret = parse_config(optarg);
>>   			if (ret) {
>> @@ -3350,11 +3361,11 @@ main(int32_t argc, char **argv)
>>
>>   	check_all_ports_link_status(enabled_port_mask);
>>
>> -#if (STATS_INTERVAL > 0)
>> -	rte_eal_alarm_set(STATS_INTERVAL * US_PER_S, print_stats_cb,
>> NULL);
>> -#else
>> -	RTE_LOG(INFO, IPSEC, "Stats display disabled\n");
>> -#endif /* STATS_INTERVAL */
>> +	if (stats_interval > 0)
>> +		rte_eal_alarm_set(stats_interval * US_PER_S,
>> +				print_stats_cb, NULL);
>> +	else
>> +		RTE_LOG(INFO, IPSEC, "Stats display disabled\n");
>>
>>   	/* launch per-lcore init on every lcore */
>>   	rte_eal_mp_remote_launch(ipsec_launch_one_lcore, eh_conf,
>> CALL_MAIN); diff --git a/examples/ipsec-secgw/ipsec-secgw.h
>> b/examples/ipsec-secgw/ipsec-secgw.h
>> index f3082a1037..de9f382742 100644
>> --- a/examples/ipsec-secgw/ipsec-secgw.h
>> +++ b/examples/ipsec-secgw/ipsec-secgw.h
>> @@ -6,9 +6,6 @@
>>
>>   #include <stdbool.h>
>>
>> -#ifndef STATS_INTERVAL
>> -#define STATS_INTERVAL 0
>> -#endif
>>
>>   #define NB_SOCKETS 4
>>
>> @@ -144,38 +141,26 @@ is_unprotected_port(uint16_t port_id)  static inline
>> void  core_stats_update_rx(int n)  { -#if (STATS_INTERVAL > 0)
>>   	int lcore_id = rte_lcore_id();
>>   	core_statistics[lcore_id].rx += n;
>>   	core_statistics[lcore_id].rx_call++;
>>   	if (n == MAX_PKT_BURST)
>>   		core_statistics[lcore_id].burst_rx += n; -#else
>> -	RTE_SET_USED(n);
>> -#endif /* STATS_INTERVAL */
>>   }
>>
>>   static inline void
>>   core_stats_update_tx(int n)
>>   {
>> -#if (STATS_INTERVAL > 0)
>>   	int lcore_id = rte_lcore_id();
>>   	core_statistics[lcore_id].tx += n;
>>   	core_statistics[lcore_id].tx_call++;
>> -#else
>> -	RTE_SET_USED(n);
>> -#endif /* STATS_INTERVAL */
>>   }
>>
>>   static inline void
>>   core_stats_update_drop(int n)
>>   {
>> -#if (STATS_INTERVAL > 0)
>>   	int lcore_id = rte_lcore_id();
>>   	core_statistics[lcore_id].dropped += n; -#else
>> -	RTE_SET_USED(n);
>> -#endif /* STATS_INTERVAL */
>>   }
>>
>>   /* helper routine to free bulk of packets */
>> --
>> 2.25.1
Anoob Joseph Sept. 17, 2021, 12:51 p.m. UTC | #4
Hi Radu,

Please see inline.

Thanks,
Anoob

> Subject: Re: [EXT] [dpdk-dev] [PATCH v2 4/9] examples/ipsec-secgw: add
> stats interval argument
> 
> Hi Anoob,
> 
> On 9/16/2021 10:30 AM, Anoob Joseph wrote:
> > Hi Radu,
> >
> > Making stats dynamic would have a perf cost. That was the reason it was
> introduced as a compile time option. Why do we need it changed?
> We changed it for two reasons, for a better UX and for the telemetry feature
> that is introduced with patch 3.
> >
> > Also, it looks like this patch is only disabling stats printing when timeout is 0.
> I don't see stats collection being conditional. Stats collection would also have
> perf impact, right?
> I don't think there is a measurable performance impact because these are
> per burst calls.

[Anoob] Agreed. If it was done intentionally, then I don’t see any problem.

Acked-by: Anoob Joseph <anoobj@marvell.com>
 
[snip]
diff mbox series

Patch

diff --git a/doc/guides/sample_app_ug/ipsec_secgw.rst b/doc/guides/sample_app_ug/ipsec_secgw.rst
index 20bc1e6bc4..0d55e74022 100644
--- a/doc/guides/sample_app_ug/ipsec_secgw.rst
+++ b/doc/guides/sample_app_ug/ipsec_secgw.rst
@@ -127,6 +127,7 @@  The application has a number of command line options::
                         -p PORTMASK -P -u PORTMASK -j FRAMESIZE
                         -l -w REPLAY_WINDOW_SIZE -e -a
                         -c SAD_CACHE_SIZE
+                        -t STATISTICS_INTERVAL
                         -s NUMBER_OF_MBUFS_IN_PACKET_POOL
                         -f CONFIG_FILE_PATH
                         --config (port,queue,lcore)[,(port,queue,lcore)]
@@ -176,6 +177,10 @@  Where:
     Zero value disables cache.
     Default value: 128.
 
+*   ``-t``: specifies the statistics screen update interval. If set to zero or
+    omitted statistics screen is disabled.
+    Default value: 0.
+
 *   ``-s``: sets number of mbufs in packet pool, if not provided number of mbufs
     will be calculated based on number of cores, eth ports and crypto queues.
 
diff --git a/examples/ipsec-secgw/ipsec-secgw.c b/examples/ipsec-secgw/ipsec-secgw.c
index 265fff4bef..60b25be872 100644
--- a/examples/ipsec-secgw/ipsec-secgw.c
+++ b/examples/ipsec-secgw/ipsec-secgw.c
@@ -181,6 +181,7 @@  static uint32_t frag_tbl_sz;
 static uint32_t frame_buf_size = RTE_MBUF_DEFAULT_BUF_SIZE;
 static uint32_t mtu_size = RTE_ETHER_MTU;
 static uint64_t frag_ttl_ns = MAX_FRAG_TTL_NS;
+static uint32_t stats_interval;
 
 /* application wide librte_ipsec/SA parameters */
 struct app_sa_prm app_sa_prm = {
@@ -292,7 +293,6 @@  adjust_ipv6_pktlen(struct rte_mbuf *m, const struct rte_ipv6_hdr *iph,
 	}
 }
 
-#if (STATS_INTERVAL > 0)
 
 /* Print out statistics on packet distribution */
 static void
@@ -352,9 +352,8 @@  print_stats_cb(__rte_unused void *param)
 		   total_packets_dropped);
 	printf("\n====================================================\n");
 
-	rte_eal_alarm_set(STATS_INTERVAL * US_PER_S, print_stats_cb, NULL);
+	rte_eal_alarm_set(stats_interval * US_PER_S, print_stats_cb, NULL);
 }
-#endif /* STATS_INTERVAL */
 
 static inline void
 prepare_one_packet(struct rte_mbuf *pkt, struct ipsec_traffic *t)
@@ -1435,6 +1434,7 @@  print_usage(const char *prgname)
 		" [-e]"
 		" [-a]"
 		" [-c]"
+		" [-t STATS_INTERVAL]"
 		" [-s NUMBER_OF_MBUFS_IN_PKT_POOL]"
 		" -f CONFIG_FILE"
 		" --config (port,queue,lcore)[,(port,queue,lcore)]"
@@ -1459,6 +1459,8 @@  print_usage(const char *prgname)
 		"  -a enables SA SQN atomic behaviour\n"
 		"  -c specifies inbound SAD cache size,\n"
 		"     zero value disables the cache (default value: 128)\n"
+		"  -t specifies statistics screen update interval,\n"
+		"     zero disables statistics screen (default value: 0)\n"
 		"  -s number of mbufs in packet pool, if not specified number\n"
 		"     of mbufs will be calculated based on number of cores,\n"
 		"     ports and crypto queues\n"
@@ -1666,7 +1668,7 @@  parse_args(int32_t argc, char **argv, struct eh_conf *eh_conf)
 
 	argvopt = argv;
 
-	while ((opt = getopt_long(argc, argvopt, "aelp:Pu:f:j:w:c:s:",
+	while ((opt = getopt_long(argc, argvopt, "aelp:Pu:f:j:w:c:t:s:",
 				lgopts, &option_index)) != EOF) {
 
 		switch (opt) {
@@ -1747,6 +1749,15 @@  parse_args(int32_t argc, char **argv, struct eh_conf *eh_conf)
 			}
 			app_sa_prm.cache_sz = ret;
 			break;
+		case 't':
+			ret = parse_decimal(optarg);
+			if (ret < 0) {
+				printf("Invalid interval value: %s\n", optarg);
+				print_usage(prgname);
+				return -1;
+			}
+			stats_interval = ret;
+			break;
 		case CMD_LINE_OPT_CONFIG_NUM:
 			ret = parse_config(optarg);
 			if (ret) {
@@ -3350,11 +3361,11 @@  main(int32_t argc, char **argv)
 
 	check_all_ports_link_status(enabled_port_mask);
 
-#if (STATS_INTERVAL > 0)
-	rte_eal_alarm_set(STATS_INTERVAL * US_PER_S, print_stats_cb, NULL);
-#else
-	RTE_LOG(INFO, IPSEC, "Stats display disabled\n");
-#endif /* STATS_INTERVAL */
+	if (stats_interval > 0)
+		rte_eal_alarm_set(stats_interval * US_PER_S,
+				print_stats_cb, NULL);
+	else
+		RTE_LOG(INFO, IPSEC, "Stats display disabled\n");
 
 	/* launch per-lcore init on every lcore */
 	rte_eal_mp_remote_launch(ipsec_launch_one_lcore, eh_conf, CALL_MAIN);
diff --git a/examples/ipsec-secgw/ipsec-secgw.h b/examples/ipsec-secgw/ipsec-secgw.h
index f3082a1037..de9f382742 100644
--- a/examples/ipsec-secgw/ipsec-secgw.h
+++ b/examples/ipsec-secgw/ipsec-secgw.h
@@ -6,9 +6,6 @@ 
 
 #include <stdbool.h>
 
-#ifndef STATS_INTERVAL
-#define STATS_INTERVAL 0
-#endif
 
 #define NB_SOCKETS 4
 
@@ -144,38 +141,26 @@  is_unprotected_port(uint16_t port_id)
 static inline void
 core_stats_update_rx(int n)
 {
-#if (STATS_INTERVAL > 0)
 	int lcore_id = rte_lcore_id();
 	core_statistics[lcore_id].rx += n;
 	core_statistics[lcore_id].rx_call++;
 	if (n == MAX_PKT_BURST)
 		core_statistics[lcore_id].burst_rx += n;
-#else
-	RTE_SET_USED(n);
-#endif /* STATS_INTERVAL */
 }
 
 static inline void
 core_stats_update_tx(int n)
 {
-#if (STATS_INTERVAL > 0)
 	int lcore_id = rte_lcore_id();
 	core_statistics[lcore_id].tx += n;
 	core_statistics[lcore_id].tx_call++;
-#else
-	RTE_SET_USED(n);
-#endif /* STATS_INTERVAL */
 }
 
 static inline void
 core_stats_update_drop(int n)
 {
-#if (STATS_INTERVAL > 0)
 	int lcore_id = rte_lcore_id();
 	core_statistics[lcore_id].dropped += n;
-#else
-	RTE_SET_USED(n);
-#endif /* STATS_INTERVAL */
 }
 
 /* helper routine to free bulk of packets */