[dpdk-dev,v2] app/testpmd: add bitrate stats option
Checks
Commit Message
From: Remy Horton <remy.horton@intel.com>
Bit-rate collation should only be done by one core. This patch adds
an option to select which core performs the bit-rate calculation,
which is also disabled by default.
Fixes: 7e4441c8efb9 ("app/testpmd: add bitrate statistics calculation")
Signed-off-by: Remy Horton <remy.horton@intel.com>
Acked-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>
---
Changes in v2:
- Added parameter to documentation
app/test-pmd/parameters.c | 19 +++++++++++++++++-
app/test-pmd/testpmd.c | 36 +++++++++++++++++++++++++----------
app/test-pmd/testpmd.h | 5 +++++
doc/guides/testpmd_app_ug/run_app.rst | 4 ++++
4 files changed, 53 insertions(+), 11 deletions(-)
Comments
28/04/2017 13:00, Pablo de Lara:
> From: Remy Horton <remy.horton@intel.com>
>
> Bit-rate collation should only be done by one core. This patch adds
> an option to select which core performs the bit-rate calculation,
> which is also disabled by default.
>
> Fixes: 7e4441c8efb9 ("app/testpmd: add bitrate statistics calculation")
>
> Signed-off-by: Remy Horton <remy.horton@intel.com>
> Acked-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>
Applied, thanks
>From: Remy Horton <remy.horton@intel.com>
>
>Bit-rate collation should only be done by one core. This patch adds
>an option to select which core performs the bit-rate calculation,
>which is also disabled by default.
>
>Fixes: 7e4441c8efb9 ("app/testpmd: add bitrate statistics calculation")
>
>Signed-off-by: Remy Horton <remy.horton@intel.com>
>Acked-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>
>---
>
>Changes in v2:
>- Added parameter to documentation
>
> app/test-pmd/parameters.c | 19 +++++++++++++++++-
> app/test-pmd/testpmd.c | 36
>+++++++++++++++++++++++++----------
> app/test-pmd/testpmd.h | 5 +++++
> doc/guides/testpmd_app_ug/run_app.rst | 4 ++++
> 4 files changed, 53 insertions(+), 11 deletions(-)
>
>diff --git a/app/test-pmd/parameters.c b/app/test-pmd/parameters.c
>index 3f4d3a2..a0300eb 100644
>--- a/app/test-pmd/parameters.c
>+++ b/app/test-pmd/parameters.c
>@@ -201,7 +201,9 @@ usage(char* progname)
> printf(" --disable-link-check: disable check on link status when "
> "starting/stopping ports.\n");
> printf(" --no-lsc-interrupt: disable link status change interrupt.\n");
>- printf(" --no-rmv-interrupt: disable device removal interrupt.");
>+ printf(" --no-rmv-interrupt: disable device removal interrupt.\n");
>+ printf(" --bitrate-stats=N: set the logical core N to perform "
>+ "bit-rate calculation.\n");
> }
>
> #ifdef RTE_LIBRTE_CMDLINE
>@@ -536,6 +538,9 @@ launch_args_parse(int argc, char** argv)
> #ifdef RTE_LIBRTE_LATENCY_STATS
> { "latencystats", 1, 0, 0 },
> #endif
>+#ifdef RTE_LIBRTE_BITRATE
>+ { "bitrate-stats", 1, 0, 0 },
>+#endif
> { "disable-crc-strip", 0, 0, 0 },
> { "enable-lro", 0, 0, 0 },
> { "enable-rx-cksum", 0, 0, 0 },
>@@ -793,6 +798,18 @@ launch_args_parse(int argc, char** argv)
> " must be >= 0\n", n);
> }
> #endif
>+#ifdef RTE_LIBRTE_BITRATE
>+ if (!strcmp(lgopts[opt_idx].name, "bitrate-stats")) {
>+ n = atoi(optarg);
>+ if (n >= 0) {
>+ bitrate_lcore_id = (lcoreid_t) n;
>+ bitrate_enabled = 1;
>+ } else
>+ rte_exit(EXIT_FAILURE,
>+ "invalid lcore id %d for bitrate stats"
>+ " must be >= 0\n", n);
>+ }
>+#endif
> if (!strcmp(lgopts[opt_idx].name, "disable-crc-strip"))
> rx_mode.hw_strip_crc = 0;
> if (!strcmp(lgopts[opt_idx].name, "enable-lro"))
>diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
>index 3a57348..cfd5382 100644
>--- a/app/test-pmd/testpmd.c
>+++ b/app/test-pmd/testpmd.c
>@@ -355,8 +355,12 @@ uint16_t nb_rx_queue_stats_mappings = 0;
>
> unsigned max_socket = 0;
>
>+#ifdef RTE_LIBRTE_BITRATE
> /* Bitrate statistics */
> struct rte_stats_bitrates *bitrate_data;
>+lcoreid_t bitrate_lcore_id;
>+uint8_t bitrate_enabled;
>+#endif
>
> /* Forward function declarations */
> static void map_port_queue_stats_mapping_registers(uint8_t pi, struct
>rte_port *port);
>@@ -962,12 +966,18 @@ run_pkt_fwd_on_lcore(struct fwd_lcore *fc,
>packet_fwd_t pkt_fwd)
> for (sm_id = 0; sm_id < nb_fs; sm_id++)
> (*pkt_fwd)(fsm[sm_id]);
> #ifdef RTE_LIBRTE_BITRATE
>- tics_current = rte_rdtsc();
>- if (tics_current - tics_datum >= tics_per_1sec) {
>- /* Periodic bitrate calculation */
>- for (idx_port = 0; idx_port < cnt_ports; idx_port++)
>- rte_stats_bitrate_calc(bitrate_data, idx_port);
>- tics_datum = tics_current;
>+ if (bitrate_enabled != 0 &&
>+ bitrate_lcore_id == rte_lcore_id()) {
>+ tics_current = rte_rdtsc();
>+ if (tics_current - tics_datum >= tics_per_1sec) {
>+ /* Periodic bitrate calculation */
>+ for (idx_port = 0;
>+ idx_port < cnt_ports;
>+ idx_port++)
>+ rte_stats_bitrate_calc(bitrate_data,
>+ idx_port);
>+ tics_datum = tics_current;
>+ }
> }
> #endif
> #ifdef RTE_LIBRTE_LATENCY_STATS
>@@ -2238,6 +2248,9 @@ main(int argc, char** argv)
> rte_panic("Empty set of forwarding logical cores - check the "
> "core mask supplied in the command parameters\n");
>
>+ /* Bitrate stats disabled by default */
>+ bitrate_enabled = 0;
>+
> argc -= diag;
> argv += diag;
> if (argc > 1)
>@@ -2275,10 +2288,13 @@ main(int argc, char** argv)
>
> /* Setup bitrate stats */
> #ifdef RTE_LIBRTE_BITRATE
>- bitrate_data = rte_stats_bitrate_create();
>- if (bitrate_data == NULL)
>- rte_exit(EXIT_FAILURE, "Could not allocate bitrate data.\n");
>- rte_stats_bitrate_reg(bitrate_data);
>+ if (bitrate_enabled != 0) {
>+ bitrate_data = rte_stats_bitrate_create();
>+ if (bitrate_data == NULL)
>+ rte_exit(EXIT_FAILURE,
>+ "Could not allocate bitrate data.\n");
>+ rte_stats_bitrate_reg(bitrate_data);
>+ }
> #endif
>
>
>diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
>index a9ff07e..6443f7e 100644
>--- a/app/test-pmd/testpmd.h
>+++ b/app/test-pmd/testpmd.h
>@@ -380,6 +380,11 @@ extern uint8_t latencystats_enabled;
> extern lcoreid_t latencystats_lcore_id;
> #endif
>
>+#ifdef RTE_LIBRTE_BITRATE
>+extern lcoreid_t bitrate_lcore_id;
>+extern uint8_t bitrate_enabled;
>+#endif
>+
> extern struct rte_fdir_conf fdir_conf;
>
> /*
>diff --git a/doc/guides/testpmd_app_ug/run_app.rst
>b/doc/guides/testpmd_app_ug/run_app.rst
>index df5a0ee..b9be1e6 100644
>--- a/doc/guides/testpmd_app_ug/run_app.rst
>+++ b/doc/guides/testpmd_app_ug/run_app.rst
>@@ -465,3 +465,7 @@ The commandline options are:
> * ``--disable-link-check``
>
> Disable check on link status when starting/stopping ports.
>+
>+* ``--bitrate-stats=N``
>+
>+ Set the logical core N to perform bitrate calculation.
>--
>2.7.4
>
>
Hi Remy,
Have a small suggestion here.
Since testpmd uses new libraries of librte_latencystats and
librte_bitratestats it hurts packet processing performance.
Many users who use testpmd to do the initial performance benchmarks may
not be aware of such a feature is default enabled.
So can we disable this feature by default in the config?
· CONFIG_RTE_LIBRTE_BITRATE=n
· CONFIG_RTE_LIBRTE_LATENCY_STATS=n
Only those folks interested in latency/jitter measurements can recompile
with those configs enabled.
Thanks.
01/05/2017 22:07, Patil, Harish:
> Hi Remy,
> Have a small suggestion here.
> Since testpmd uses new libraries of librte_latencystats and
> librte_bitratestats it hurts packet processing performance.
> Many users who use testpmd to do the initial performance benchmarks may
> not be aware of such a feature is default enabled.
Yes, the default config of testpmd must give good performance.
> So can we disable this feature by default in the config?
> · CONFIG_RTE_LIBRTE_BITRATE=n
> · CONFIG_RTE_LIBRTE_LATENCY_STATS=n
> Only those folks interested in latency/jitter measurements can recompile
> with those configs enabled.
I disagree about compile-time options.
It should be a run-time option of testpmd.
Please Remy (or others),
disable the metrics in the default configuration of testpmd,
before the 17.05 release.
You have few days, it is urgent.
> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Monday, May 01, 2017 9:22 PM
> To: Patil, Harish; Horton, Remy
> Cc: dev@dpdk.org; De Lara Guarch, Pablo; Wu, Jingjing
> Subject: Re: [dpdk-dev] [PATCH v2] app/testpmd: add bitrate stats option
>
> 01/05/2017 22:07, Patil, Harish:
> > Hi Remy,
> > Have a small suggestion here.
> > Since testpmd uses new libraries of librte_latencystats and
> > librte_bitratestats it hurts packet processing performance.
> > Many users who use testpmd to do the initial performance benchmarks
> may
> > not be aware of such a feature is default enabled.
>
> Yes, the default config of testpmd must give good performance.
>
> > So can we disable this feature by default in the config?
> > * CONFIG_RTE_LIBRTE_BITRATE=n
> > * CONFIG_RTE_LIBRTE_LATENCY_STATS=n
> > Only those folks interested in latency/jitter measurements can recompile
> > with those configs enabled.
>
> I disagree about compile-time options.
> It should be a run-time option of testpmd.
>
> Please Remy (or others),
> disable the metrics in the default configuration of testpmd,
> before the 17.05 release.
> You have few days, it is urgent.
Bitrate stats are disabled by default, in testpmd.
I assume that the code that you want to avoid is:
for (sm_id = 0; sm_id < nb_fs; sm_id++)
(*pkt_fwd)(fsm[sm_id]);
#ifdef RTE_LIBRTE_BITRATE
if (bitrate_enabled != 0 &&
bitrate_lcore_id == rte_lcore_id()) {
tics_current = rte_rdtsc();
if (tics_current - tics_datum >= tics_per_1sec) {
Unless --bitrate-stats is used, bitrate_enabled = 0, so all this code won't be run.
I can send a patch to check the latencystats_enabled flag first, following the approach above, here:
#ifdef RTE_LIBRTE_LATENCY_STATS
if (latencystats_lcore_id == rte_lcore_id())
Thanks,
Pablo
@@ -201,7 +201,9 @@ usage(char* progname)
printf(" --disable-link-check: disable check on link status when "
"starting/stopping ports.\n");
printf(" --no-lsc-interrupt: disable link status change interrupt.\n");
- printf(" --no-rmv-interrupt: disable device removal interrupt.");
+ printf(" --no-rmv-interrupt: disable device removal interrupt.\n");
+ printf(" --bitrate-stats=N: set the logical core N to perform "
+ "bit-rate calculation.\n");
}
#ifdef RTE_LIBRTE_CMDLINE
@@ -536,6 +538,9 @@ launch_args_parse(int argc, char** argv)
#ifdef RTE_LIBRTE_LATENCY_STATS
{ "latencystats", 1, 0, 0 },
#endif
+#ifdef RTE_LIBRTE_BITRATE
+ { "bitrate-stats", 1, 0, 0 },
+#endif
{ "disable-crc-strip", 0, 0, 0 },
{ "enable-lro", 0, 0, 0 },
{ "enable-rx-cksum", 0, 0, 0 },
@@ -793,6 +798,18 @@ launch_args_parse(int argc, char** argv)
" must be >= 0\n", n);
}
#endif
+#ifdef RTE_LIBRTE_BITRATE
+ if (!strcmp(lgopts[opt_idx].name, "bitrate-stats")) {
+ n = atoi(optarg);
+ if (n >= 0) {
+ bitrate_lcore_id = (lcoreid_t) n;
+ bitrate_enabled = 1;
+ } else
+ rte_exit(EXIT_FAILURE,
+ "invalid lcore id %d for bitrate stats"
+ " must be >= 0\n", n);
+ }
+#endif
if (!strcmp(lgopts[opt_idx].name, "disable-crc-strip"))
rx_mode.hw_strip_crc = 0;
if (!strcmp(lgopts[opt_idx].name, "enable-lro"))
@@ -355,8 +355,12 @@ uint16_t nb_rx_queue_stats_mappings = 0;
unsigned max_socket = 0;
+#ifdef RTE_LIBRTE_BITRATE
/* Bitrate statistics */
struct rte_stats_bitrates *bitrate_data;
+lcoreid_t bitrate_lcore_id;
+uint8_t bitrate_enabled;
+#endif
/* Forward function declarations */
static void map_port_queue_stats_mapping_registers(uint8_t pi, struct rte_port *port);
@@ -962,12 +966,18 @@ run_pkt_fwd_on_lcore(struct fwd_lcore *fc, packet_fwd_t pkt_fwd)
for (sm_id = 0; sm_id < nb_fs; sm_id++)
(*pkt_fwd)(fsm[sm_id]);
#ifdef RTE_LIBRTE_BITRATE
- tics_current = rte_rdtsc();
- if (tics_current - tics_datum >= tics_per_1sec) {
- /* Periodic bitrate calculation */
- for (idx_port = 0; idx_port < cnt_ports; idx_port++)
- rte_stats_bitrate_calc(bitrate_data, idx_port);
- tics_datum = tics_current;
+ if (bitrate_enabled != 0 &&
+ bitrate_lcore_id == rte_lcore_id()) {
+ tics_current = rte_rdtsc();
+ if (tics_current - tics_datum >= tics_per_1sec) {
+ /* Periodic bitrate calculation */
+ for (idx_port = 0;
+ idx_port < cnt_ports;
+ idx_port++)
+ rte_stats_bitrate_calc(bitrate_data,
+ idx_port);
+ tics_datum = tics_current;
+ }
}
#endif
#ifdef RTE_LIBRTE_LATENCY_STATS
@@ -2238,6 +2248,9 @@ main(int argc, char** argv)
rte_panic("Empty set of forwarding logical cores - check the "
"core mask supplied in the command parameters\n");
+ /* Bitrate stats disabled by default */
+ bitrate_enabled = 0;
+
argc -= diag;
argv += diag;
if (argc > 1)
@@ -2275,10 +2288,13 @@ main(int argc, char** argv)
/* Setup bitrate stats */
#ifdef RTE_LIBRTE_BITRATE
- bitrate_data = rte_stats_bitrate_create();
- if (bitrate_data == NULL)
- rte_exit(EXIT_FAILURE, "Could not allocate bitrate data.\n");
- rte_stats_bitrate_reg(bitrate_data);
+ if (bitrate_enabled != 0) {
+ bitrate_data = rte_stats_bitrate_create();
+ if (bitrate_data == NULL)
+ rte_exit(EXIT_FAILURE,
+ "Could not allocate bitrate data.\n");
+ rte_stats_bitrate_reg(bitrate_data);
+ }
#endif
@@ -380,6 +380,11 @@ extern uint8_t latencystats_enabled;
extern lcoreid_t latencystats_lcore_id;
#endif
+#ifdef RTE_LIBRTE_BITRATE
+extern lcoreid_t bitrate_lcore_id;
+extern uint8_t bitrate_enabled;
+#endif
+
extern struct rte_fdir_conf fdir_conf;
/*
@@ -465,3 +465,7 @@ The commandline options are:
* ``--disable-link-check``
Disable check on link status when starting/stopping ports.
+
+* ``--bitrate-stats=N``
+
+ Set the logical core N to perform bitrate calculation.