| Message ID | 20210915134522.1311843-5-radu.nicolau@intel.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | akhil goyal |
| Headers | show |
| Series | IPsec Sec GW new features | expand |
| Context | Check | Description |
|---|---|---|
| ci/checkpatch | success | coding style OK |
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>
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
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
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 --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 */
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(-)