patches for DPDK stable branches
 help / color / Atom feed
* [dpdk-stable] [PATCH 1/5] app/testpmd: clock gettime call in throughput calculation
@ 2020-06-17 14:43 Honnappa Nagarahalli
  2020-06-17 14:43 ` [dpdk-stable] [PATCH 2/5] app/testpmd: enable burst stats for flowgen mode rx path Honnappa Nagarahalli
                   ` (7 more replies)
  0 siblings, 8 replies; 32+ messages in thread
From: Honnappa Nagarahalli @ 2020-06-17 14:43 UTC (permalink / raw)
  To: dev, honnappa.nagarahalli, alialnu, orgerlitz, wenzhuo.lu,
	beilei.xing, bernard.iremonger
  Cc: hemant.agrawal, jerinj, viacheslavo, thomas, ruifeng.wang,
	phil.yang, nd, zhihong.wang, stable

The throughput calculation requires a counter that measures
passing of time. The PMU cycle counter does not do that. This
results in incorrect throughput numbers when
RTE_ARM_EAL_RDTSC_USE_PMU is enabled. Use clock_gettime
system call to calculate the time passed since last call.

Bugzilla ID: 450
Fixes: 0e106980301d ("app/testpmd: show throughput in port stats")
Cc: zhihong.wang@intel.com
Cc: stable@dpdk.org

Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
Reviewed-by: Phil Yang <phil.yang@arm.com>
Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
---
 app/test-pmd/config.c | 44 +++++++++++++++++++++++++++++--------------
 1 file changed, 30 insertions(+), 14 deletions(-)

diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index 016bcb09c..91fbf99f8 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -54,6 +54,14 @@
 
 #define ETHDEV_FWVERS_LEN 32
 
+#ifdef CLOCK_MONOTONIC_RAW /* Defined in glibc bits/time.h */
+#define CLOCK_TYPE_ID CLOCK_MONOTONIC_RAW
+#else
+#define CLOCK_TYPE_ID CLOCK_MONOTONIC
+#endif
+
+#define NS_PER_SEC 1E9
+
 static char *flowtype_to_str(uint16_t flow_type);
 
 static const struct {
@@ -136,9 +144,10 @@ nic_stats_display(portid_t port_id)
 	static uint64_t prev_pkts_tx[RTE_MAX_ETHPORTS];
 	static uint64_t prev_bytes_rx[RTE_MAX_ETHPORTS];
 	static uint64_t prev_bytes_tx[RTE_MAX_ETHPORTS];
-	static uint64_t prev_cycles[RTE_MAX_ETHPORTS];
+	static uint64_t prev_ns[RTE_MAX_ETHPORTS];
+	struct timespec cur_time;
 	uint64_t diff_pkts_rx, diff_pkts_tx, diff_bytes_rx, diff_bytes_tx,
-								diff_cycles;
+								diff_ns;
 	uint64_t mpps_rx, mpps_tx, mbps_rx, mbps_tx;
 	struct rte_eth_stats stats;
 	struct rte_port *port = &ports[port_id];
@@ -195,10 +204,17 @@ nic_stats_display(portid_t port_id)
 		}
 	}
 
-	diff_cycles = prev_cycles[port_id];
-	prev_cycles[port_id] = rte_rdtsc();
-	if (diff_cycles > 0)
-		diff_cycles = prev_cycles[port_id] - diff_cycles;
+	diff_ns = 0;
+	if (clock_gettime(CLOCK_TYPE_ID, &cur_time) == 0) {
+		uint64_t ns;
+
+		ns = cur_time.tv_sec * NS_PER_SEC;
+		ns += cur_time.tv_nsec;
+
+		if (prev_ns[port_id] != 0)
+			diff_ns = ns - prev_ns[port_id];
+		prev_ns[port_id] = ns;
+	}
 
 	diff_pkts_rx = (stats.ipackets > prev_pkts_rx[port_id]) ?
 		(stats.ipackets - prev_pkts_rx[port_id]) : 0;
@@ -206,10 +222,10 @@ nic_stats_display(portid_t port_id)
 		(stats.opackets - prev_pkts_tx[port_id]) : 0;
 	prev_pkts_rx[port_id] = stats.ipackets;
 	prev_pkts_tx[port_id] = stats.opackets;
-	mpps_rx = diff_cycles > 0 ?
-		diff_pkts_rx * rte_get_tsc_hz() / diff_cycles : 0;
-	mpps_tx = diff_cycles > 0 ?
-		diff_pkts_tx * rte_get_tsc_hz() / diff_cycles : 0;
+	mpps_rx = diff_ns > 0 ?
+		(double)diff_pkts_rx / diff_ns * NS_PER_SEC : 0;
+	mpps_tx = diff_ns > 0 ?
+		(double)diff_pkts_tx / diff_ns * NS_PER_SEC : 0;
 
 	diff_bytes_rx = (stats.ibytes > prev_bytes_rx[port_id]) ?
 		(stats.ibytes - prev_bytes_rx[port_id]) : 0;
@@ -217,10 +233,10 @@ nic_stats_display(portid_t port_id)
 		(stats.obytes - prev_bytes_tx[port_id]) : 0;
 	prev_bytes_rx[port_id] = stats.ibytes;
 	prev_bytes_tx[port_id] = stats.obytes;
-	mbps_rx = diff_cycles > 0 ?
-		diff_bytes_rx * rte_get_tsc_hz() / diff_cycles : 0;
-	mbps_tx = diff_cycles > 0 ?
-		diff_bytes_tx * rte_get_tsc_hz() / diff_cycles : 0;
+	mbps_rx = diff_ns > 0 ?
+		(double)diff_bytes_rx / diff_ns * NS_PER_SEC : 0;
+	mbps_tx = diff_ns > 0 ?
+		(double)diff_bytes_tx / diff_ns * NS_PER_SEC : 0;
 
 	printf("\n  Throughput (since last show)\n");
 	printf("  Rx-pps: %12"PRIu64"          Rx-bps: %12"PRIu64"\n  Tx-pps: %12"
-- 
2.17.1


^ permalink raw reply	[flat|nested] 32+ messages in thread

* [dpdk-stable] [PATCH 2/5] app/testpmd: enable burst stats for flowgen mode rx path
  2020-06-17 14:43 [dpdk-stable] [PATCH 1/5] app/testpmd: clock gettime call in throughput calculation Honnappa Nagarahalli
@ 2020-06-17 14:43 ` Honnappa Nagarahalli
  2020-06-18 14:57   ` Phil Yang
  2020-06-17 14:43 ` [dpdk-stable] [PATCH 3/5] app/testpmd: enable burst stats for noisy vnf mode Honnappa Nagarahalli
                   ` (6 subsequent siblings)
  7 siblings, 1 reply; 32+ messages in thread
From: Honnappa Nagarahalli @ 2020-06-17 14:43 UTC (permalink / raw)
  To: dev, honnappa.nagarahalli, alialnu, orgerlitz, wenzhuo.lu,
	beilei.xing, bernard.iremonger
  Cc: hemant.agrawal, jerinj, viacheslavo, thomas, ruifeng.wang,
	phil.yang, nd, stable

Enable missing burst stats for rx path for flowgen mode.

Fixes: e9e23a617eb8 ("app/testpmd: add flowgen forwarding engine")
Cc: stable@dpdk.org

Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
Reviewed-by: Phil Yang <phil.yang@arm.com>
Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
---
 app/test-pmd/flowgen.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/app/test-pmd/flowgen.c b/app/test-pmd/flowgen.c
index 4bd351e67..011887c61 100644
--- a/app/test-pmd/flowgen.c
+++ b/app/test-pmd/flowgen.c
@@ -111,6 +111,10 @@ pkt_burst_flow_gen(struct fwd_stream *fs)
 	/* Receive a burst of packets and discard them. */
 	nb_rx = rte_eth_rx_burst(fs->rx_port, fs->rx_queue, pkts_burst,
 				 nb_pkt_per_burst);
+#ifdef RTE_TEST_PMD_RECORD_BURST_STATS
+	fs->rx_burst_stats.pkt_burst_spread[nb_rx]++;
+#endif
+
 	fs->rx_packets += nb_rx;
 
 	for (i = 0; i < nb_rx; i++)
-- 
2.17.1


^ permalink raw reply	[flat|nested] 32+ messages in thread

* [dpdk-stable] [PATCH 3/5] app/testpmd: enable burst stats for noisy vnf mode
  2020-06-17 14:43 [dpdk-stable] [PATCH 1/5] app/testpmd: clock gettime call in throughput calculation Honnappa Nagarahalli
  2020-06-17 14:43 ` [dpdk-stable] [PATCH 2/5] app/testpmd: enable burst stats for flowgen mode rx path Honnappa Nagarahalli
@ 2020-06-17 14:43 ` Honnappa Nagarahalli
  2020-06-18  7:16   ` Jens Freimann
  2020-06-17 14:43 ` [dpdk-stable] [PATCH 4/5] app/testpmd: fix burst percentage calculation Honnappa Nagarahalli
                   ` (5 subsequent siblings)
  7 siblings, 1 reply; 32+ messages in thread
From: Honnappa Nagarahalli @ 2020-06-17 14:43 UTC (permalink / raw)
  To: dev, honnappa.nagarahalli, alialnu, orgerlitz, wenzhuo.lu,
	beilei.xing, bernard.iremonger
  Cc: hemant.agrawal, jerinj, viacheslavo, thomas, ruifeng.wang,
	phil.yang, nd, stable, jfreimann

From: Phil Yang <phil.yang@arm.com>

Add burst stats for noisy vnf mode.

Fixes: 3c156061b938 ("app/testpmd: add noisy neighbour forwarding mode")
Cc: stable@dpdk.org
Cc: jfreimann@redhat.com

Signed-off-by: Phil Yang <phil.yang@arm.com>
Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
---
 app/test-pmd/noisy_vnf.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/app/test-pmd/noisy_vnf.c b/app/test-pmd/noisy_vnf.c
index 58c4ee925..24f286da6 100644
--- a/app/test-pmd/noisy_vnf.c
+++ b/app/test-pmd/noisy_vnf.c
@@ -154,6 +154,9 @@ pkt_burst_noisy_vnf(struct fwd_stream *fs)
 
 	nb_rx = rte_eth_rx_burst(fs->rx_port, fs->rx_queue,
 			pkts_burst, nb_pkt_per_burst);
+#ifdef RTE_TEST_PMD_RECORD_BURST_STATS
+	fs->rx_burst_stats.pkt_burst_spread[nb_rx]++;
+#endif
 	if (unlikely(nb_rx == 0))
 		goto flush;
 	fs->rx_packets += nb_rx;
@@ -164,6 +167,9 @@ pkt_burst_noisy_vnf(struct fwd_stream *fs)
 				pkts_burst, nb_rx);
 		if (unlikely(nb_tx < nb_rx) && fs->retry_enabled)
 			nb_tx += do_retry(nb_rx, nb_tx, pkts_burst, fs);
+#ifdef RTE_TEST_PMD_RECORD_BURST_STATS
+		fs->tx_burst_stats.pkt_burst_spread[nb_tx]++;
+#endif
 		fs->tx_packets += nb_tx;
 		fs->fwd_dropped += drop_pkts(pkts_burst, nb_rx, nb_tx);
 		return;
@@ -187,6 +193,9 @@ pkt_burst_noisy_vnf(struct fwd_stream *fs)
 					nb_deqd);
 			if (unlikely(nb_tx < nb_rx) && fs->retry_enabled)
 				nb_tx += do_retry(nb_rx, nb_tx, tmp_pkts, fs);
+#ifdef RTE_TEST_PMD_RECORD_BURST_STATS
+			fs->tx_burst_stats.pkt_burst_spread[nb_tx]++;
+#endif
 			fs->fwd_dropped += drop_pkts(tmp_pkts, nb_deqd, nb_tx);
 		}
 	}
@@ -211,6 +220,9 @@ pkt_burst_noisy_vnf(struct fwd_stream *fs)
 					 tmp_pkts, nb_deqd);
 		if (unlikely(sent < nb_deqd) && fs->retry_enabled)
 			nb_tx += do_retry(nb_rx, nb_tx, tmp_pkts, fs);
+#ifdef RTE_TEST_PMD_RECORD_BURST_STATS
+		fs->tx_burst_stats.pkt_burst_spread[nb_tx]++;
+#endif
 		fs->fwd_dropped += drop_pkts(tmp_pkts, nb_deqd, sent);
 		ncf->prev_time = rte_get_timer_cycles();
 	}
-- 
2.17.1


^ permalink raw reply	[flat|nested] 32+ messages in thread

* [dpdk-stable] [PATCH 4/5] app/testpmd: fix burst percentage calculation
  2020-06-17 14:43 [dpdk-stable] [PATCH 1/5] app/testpmd: clock gettime call in throughput calculation Honnappa Nagarahalli
  2020-06-17 14:43 ` [dpdk-stable] [PATCH 2/5] app/testpmd: enable burst stats for flowgen mode rx path Honnappa Nagarahalli
  2020-06-17 14:43 ` [dpdk-stable] [PATCH 3/5] app/testpmd: enable burst stats for noisy vnf mode Honnappa Nagarahalli
@ 2020-06-17 14:43 ` Honnappa Nagarahalli
  2020-06-18 15:01   ` Phil Yang
  2020-06-23 13:33   ` Ali Alnubani
  2020-06-17 15:16 ` [dpdk-stable] [dpdk-dev] [PATCH 1/5] app/testpmd: clock gettime call in throughput calculation Jerin Jacob
                   ` (4 subsequent siblings)
  7 siblings, 2 replies; 32+ messages in thread
From: Honnappa Nagarahalli @ 2020-06-17 14:43 UTC (permalink / raw)
  To: dev, honnappa.nagarahalli, alialnu, orgerlitz, wenzhuo.lu,
	beilei.xing, bernard.iremonger
  Cc: hemant.agrawal, jerinj, viacheslavo, thomas, ruifeng.wang,
	phil.yang, nd, stable

The burst % calculation can over flow due to multiplication.
Fix the multiplication and increase the size of variables to
64b.

Fixes: af75078fece3 ("first public release")
Cc: stable@dpdk.org

Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
Reviewed-by: Phil Yang <phil.yang@arm.com>
Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
---
 app/test-pmd/testpmd.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index 4989d22ca..2e1493da2 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -1692,9 +1692,9 @@ init_fwd_streams(void)
 static void
 pkt_burst_stats_display(const char *rx_tx, struct pkt_burst_stats *pbs)
 {
-	unsigned int total_burst;
-	unsigned int nb_burst;
-	unsigned int burst_stats[3];
+	uint64_t total_burst;
+	uint64_t nb_burst;
+	uint64_t burst_stats[3];
 	uint16_t pktnb_stats[3];
 	uint16_t nb_pkt;
 	int burst_percent[3];
@@ -1723,8 +1723,8 @@ pkt_burst_stats_display(const char *rx_tx, struct pkt_burst_stats *pbs)
 	}
 	if (total_burst == 0)
 		return;
-	burst_percent[0] = (burst_stats[0] * 100) / total_burst;
-	printf("  %s-bursts : %u [%d%% of %d pkts", rx_tx, total_burst,
+	burst_percent[0] = (double)burst_stats[0] / total_burst * 100;
+	printf("  %s-bursts : %"PRIu64" [%d%% of %d pkts", rx_tx, total_burst,
 	       burst_percent[0], (int) pktnb_stats[0]);
 	if (burst_stats[0] == total_burst) {
 		printf("]\n");
@@ -1735,7 +1735,7 @@ pkt_burst_stats_display(const char *rx_tx, struct pkt_burst_stats *pbs)
 		       100 - burst_percent[0], pktnb_stats[1]);
 		return;
 	}
-	burst_percent[1] = (burst_stats[1] * 100) / total_burst;
+	burst_percent[1] = (double)burst_stats[1] / total_burst * 100;
 	burst_percent[2] = 100 - (burst_percent[0] + burst_percent[1]);
 	if ((burst_percent[1] == 0) || (burst_percent[2] == 0)) {
 		printf(" + %d%% of others]\n", 100 - burst_percent[0]);
-- 
2.17.1


^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [dpdk-stable] [dpdk-dev] [PATCH 1/5] app/testpmd: clock gettime call in throughput calculation
  2020-06-17 14:43 [dpdk-stable] [PATCH 1/5] app/testpmd: clock gettime call in throughput calculation Honnappa Nagarahalli
                   ` (2 preceding siblings ...)
  2020-06-17 14:43 ` [dpdk-stable] [PATCH 4/5] app/testpmd: fix burst percentage calculation Honnappa Nagarahalli
@ 2020-06-17 15:16 ` Jerin Jacob
  2020-06-18  4:03   ` Honnappa Nagarahalli
  2020-06-18 15:08 ` [dpdk-stable] " Phil Yang
                   ` (3 subsequent siblings)
  7 siblings, 1 reply; 32+ messages in thread
From: Jerin Jacob @ 2020-06-17 15:16 UTC (permalink / raw)
  To: Honnappa Nagarahalli
  Cc: dpdk-dev, Ali Alnubani, orgerlitz, Wenzhuo Lu, Beilei Xing,
	Bernard Iremonger, Hemant Agrawal, Jerin Jacob, Slava Ovsiienko,
	Thomas Monjalon, Ruifeng Wang (Arm Technology China),
	Phil Yang, nd, Zhihong Wang, dpdk stable

On Wed, Jun 17, 2020 at 8:13 PM Honnappa Nagarahalli
<honnappa.nagarahalli@arm.com> wrote:
>
> The throughput calculation requires a counter that measures
> passing of time. The PMU cycle counter does not do that. This


It is not clear from git commit on why PMU cycle counter does not do that?
On dpdk bootup, we are figuring out the Hz value based on PMU counter cycles.
What is the missing piece here?

IMO, PMU counter should have less latency and more granularity than
clock_getime.

> results in incorrect throughput numbers when
> RTE_ARM_EAL_RDTSC_USE_PMU is enabled. Use clock_gettime
> system call to calculate the time passed since last call.
>
> Bugzilla ID: 450
> Fixes: 0e106980301d ("app/testpmd: show throughput in port stats")
> Cc: zhihong.wang@intel.com
> Cc: stable@dpdk.org
>
> Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> Reviewed-by: Phil Yang <phil.yang@arm.com>
> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> ---
>  app/test-pmd/config.c | 44 +++++++++++++++++++++++++++++--------------
>  1 file changed, 30 insertions(+), 14 deletions(-)
>
> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
> index 016bcb09c..91fbf99f8 100644
> --- a/app/test-pmd/config.c
> +++ b/app/test-pmd/config.c
> @@ -54,6 +54,14 @@
>
>  #define ETHDEV_FWVERS_LEN 32
>
> +#ifdef CLOCK_MONOTONIC_RAW /* Defined in glibc bits/time.h */
> +#define CLOCK_TYPE_ID CLOCK_MONOTONIC_RAW
> +#else
> +#define CLOCK_TYPE_ID CLOCK_MONOTONIC
> +#endif
> +
> +#define NS_PER_SEC 1E9
> +
>  static char *flowtype_to_str(uint16_t flow_type);
>
>  static const struct {
> @@ -136,9 +144,10 @@ nic_stats_display(portid_t port_id)
>         static uint64_t prev_pkts_tx[RTE_MAX_ETHPORTS];
>         static uint64_t prev_bytes_rx[RTE_MAX_ETHPORTS];
>         static uint64_t prev_bytes_tx[RTE_MAX_ETHPORTS];
> -       static uint64_t prev_cycles[RTE_MAX_ETHPORTS];
> +       static uint64_t prev_ns[RTE_MAX_ETHPORTS];
> +       struct timespec cur_time;
>         uint64_t diff_pkts_rx, diff_pkts_tx, diff_bytes_rx, diff_bytes_tx,
> -                                                               diff_cycles;
> +                                                               diff_ns;
>         uint64_t mpps_rx, mpps_tx, mbps_rx, mbps_tx;
>         struct rte_eth_stats stats;
>         struct rte_port *port = &ports[port_id];
> @@ -195,10 +204,17 @@ nic_stats_display(portid_t port_id)
>                 }
>         }
>
> -       diff_cycles = prev_cycles[port_id];
> -       prev_cycles[port_id] = rte_rdtsc();
> -       if (diff_cycles > 0)
> -               diff_cycles = prev_cycles[port_id] - diff_cycles;
> +       diff_ns = 0;
> +       if (clock_gettime(CLOCK_TYPE_ID, &cur_time) == 0) {
> +               uint64_t ns;
> +
> +               ns = cur_time.tv_sec * NS_PER_SEC;
> +               ns += cur_time.tv_nsec;
> +
> +               if (prev_ns[port_id] != 0)
> +                       diff_ns = ns - prev_ns[port_id];
> +               prev_ns[port_id] = ns;
> +       }
>
>         diff_pkts_rx = (stats.ipackets > prev_pkts_rx[port_id]) ?
>                 (stats.ipackets - prev_pkts_rx[port_id]) : 0;
> @@ -206,10 +222,10 @@ nic_stats_display(portid_t port_id)
>                 (stats.opackets - prev_pkts_tx[port_id]) : 0;
>         prev_pkts_rx[port_id] = stats.ipackets;
>         prev_pkts_tx[port_id] = stats.opackets;
> -       mpps_rx = diff_cycles > 0 ?
> -               diff_pkts_rx * rte_get_tsc_hz() / diff_cycles : 0;
> -       mpps_tx = diff_cycles > 0 ?
> -               diff_pkts_tx * rte_get_tsc_hz() / diff_cycles : 0;
> +       mpps_rx = diff_ns > 0 ?
> +               (double)diff_pkts_rx / diff_ns * NS_PER_SEC : 0;
> +       mpps_tx = diff_ns > 0 ?
> +               (double)diff_pkts_tx / diff_ns * NS_PER_SEC : 0;
>
>         diff_bytes_rx = (stats.ibytes > prev_bytes_rx[port_id]) ?
>                 (stats.ibytes - prev_bytes_rx[port_id]) : 0;
> @@ -217,10 +233,10 @@ nic_stats_display(portid_t port_id)
>                 (stats.obytes - prev_bytes_tx[port_id]) : 0;
>         prev_bytes_rx[port_id] = stats.ibytes;
>         prev_bytes_tx[port_id] = stats.obytes;
> -       mbps_rx = diff_cycles > 0 ?
> -               diff_bytes_rx * rte_get_tsc_hz() / diff_cycles : 0;
> -       mbps_tx = diff_cycles > 0 ?
> -               diff_bytes_tx * rte_get_tsc_hz() / diff_cycles : 0;
> +       mbps_rx = diff_ns > 0 ?
> +               (double)diff_bytes_rx / diff_ns * NS_PER_SEC : 0;
> +       mbps_tx = diff_ns > 0 ?
> +               (double)diff_bytes_tx / diff_ns * NS_PER_SEC : 0;
>
>         printf("\n  Throughput (since last show)\n");
>         printf("  Rx-pps: %12"PRIu64"          Rx-bps: %12"PRIu64"\n  Tx-pps: %12"
> --
> 2.17.1
>

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [dpdk-stable] [dpdk-dev] [PATCH 1/5] app/testpmd: clock gettime call in throughput calculation
  2020-06-17 15:16 ` [dpdk-stable] [dpdk-dev] [PATCH 1/5] app/testpmd: clock gettime call in throughput calculation Jerin Jacob
@ 2020-06-18  4:03   ` Honnappa Nagarahalli
  2020-06-18 10:16     ` Jerin Jacob
  0 siblings, 1 reply; 32+ messages in thread
From: Honnappa Nagarahalli @ 2020-06-18  4:03 UTC (permalink / raw)
  To: Jerin Jacob
  Cc: dpdk-dev, Ali Alnubani, orgerlitz, Wenzhuo Lu, Beilei Xing,
	Bernard Iremonger, hemant.agrawal, jerinj, Slava Ovsiienko,
	thomas, Ruifeng Wang, Phil Yang, nd, Zhihong Wang, dpdk stable,
	Honnappa Nagarahalli, nd

Thanks Jerin for the feedback

> -----Original Message-----
> From: Jerin Jacob <jerinjacobk@gmail.com>
> Sent: Wednesday, June 17, 2020 10:16 AM
> To: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>
> Cc: dpdk-dev <dev@dpdk.org>; Ali Alnubani <alialnu@mellanox.com>;
> orgerlitz@mellanox.com; Wenzhuo Lu <wenzhuo.lu@intel.com>; Beilei Xing
> <beilei.xing@intel.com>; Bernard Iremonger <bernard.iremonger@intel.com>;
> hemant.agrawal@nxp.com; jerinj@marvell.com; Slava Ovsiienko
> <viacheslavo@mellanox.com>; thomas@monjalon.net; Ruifeng Wang
> <Ruifeng.Wang@arm.com>; Phil Yang <Phil.Yang@arm.com>; nd
> <nd@arm.com>; Zhihong Wang <zhihong.wang@intel.com>; dpdk stable
> <stable@dpdk.org>
> Subject: Re: [dpdk-dev] [PATCH 1/5] app/testpmd: clock gettime call in
> throughput calculation
> 
> On Wed, Jun 17, 2020 at 8:13 PM Honnappa Nagarahalli
> <honnappa.nagarahalli@arm.com> wrote:
> >
> > The throughput calculation requires a counter that measures passing of
> > time. The PMU cycle counter does not do that. This
> 
> 
> It is not clear from git commit on why PMU cycle counter does not do that?
> On dpdk bootup, we are figuring out the Hz value based on PMU counter
> cycles.
> What is the missing piece here?
As I understand Linux kernel saves the PMU state and restores it every time a thread is scheduled out and in. So, when the thread is scheduled out the PMU cycles are not counted towards that thread. The thread that prints the statistics issues good amount of system calls and I am guessing it is getting scheduled out. So, it is reporting very low cycle count.
 
> 
> IMO, PMU counter should have less latency and more granularity than
> clock_getime.
In general, agree. In this particular calculation the granularity has not mattered much (for ex: numbers are fine with 50Mhz generic counter and 2.5Ghz CPU). The latency also does not matter as it is getting amortized over a large number of packets. So, I do not see it affecting the reported PPS/BPS numbers.

> 
> > results in incorrect throughput numbers when
> RTE_ARM_EAL_RDTSC_USE_PMU
> > is enabled. Use clock_gettime system call to calculate the time passed
> > since last call.
> >
> > Bugzilla ID: 450
> > Fixes: 0e106980301d ("app/testpmd: show throughput in port stats")
> > Cc: zhihong.wang@intel.com
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> > Reviewed-by: Phil Yang <phil.yang@arm.com>
> > Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> > ---
> >  app/test-pmd/config.c | 44
> > +++++++++++++++++++++++++++++--------------
> >  1 file changed, 30 insertions(+), 14 deletions(-)
> >
> > diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c index
> > 016bcb09c..91fbf99f8 100644
> > --- a/app/test-pmd/config.c
> > +++ b/app/test-pmd/config.c
> > @@ -54,6 +54,14 @@
> >
> >  #define ETHDEV_FWVERS_LEN 32
> >
> > +#ifdef CLOCK_MONOTONIC_RAW /* Defined in glibc bits/time.h */ #define
> > +CLOCK_TYPE_ID CLOCK_MONOTONIC_RAW #else #define CLOCK_TYPE_ID
> > +CLOCK_MONOTONIC #endif
> > +
> > +#define NS_PER_SEC 1E9
> > +
> >  static char *flowtype_to_str(uint16_t flow_type);
> >
> >  static const struct {
> > @@ -136,9 +144,10 @@ nic_stats_display(portid_t port_id)
> >         static uint64_t prev_pkts_tx[RTE_MAX_ETHPORTS];
> >         static uint64_t prev_bytes_rx[RTE_MAX_ETHPORTS];
> >         static uint64_t prev_bytes_tx[RTE_MAX_ETHPORTS];
> > -       static uint64_t prev_cycles[RTE_MAX_ETHPORTS];
> > +       static uint64_t prev_ns[RTE_MAX_ETHPORTS];
> > +       struct timespec cur_time;
> >         uint64_t diff_pkts_rx, diff_pkts_tx, diff_bytes_rx, diff_bytes_tx,
> > -                                                               diff_cycles;
> > +
> > + diff_ns;
> >         uint64_t mpps_rx, mpps_tx, mbps_rx, mbps_tx;
> >         struct rte_eth_stats stats;
> >         struct rte_port *port = &ports[port_id]; @@ -195,10 +204,17 @@
> > nic_stats_display(portid_t port_id)
> >                 }
> >         }
> >
> > -       diff_cycles = prev_cycles[port_id];
> > -       prev_cycles[port_id] = rte_rdtsc();
> > -       if (diff_cycles > 0)
> > -               diff_cycles = prev_cycles[port_id] - diff_cycles;
> > +       diff_ns = 0;
> > +       if (clock_gettime(CLOCK_TYPE_ID, &cur_time) == 0) {
> > +               uint64_t ns;
> > +
> > +               ns = cur_time.tv_sec * NS_PER_SEC;
> > +               ns += cur_time.tv_nsec;
> > +
> > +               if (prev_ns[port_id] != 0)
> > +                       diff_ns = ns - prev_ns[port_id];
> > +               prev_ns[port_id] = ns;
> > +       }
> >
> >         diff_pkts_rx = (stats.ipackets > prev_pkts_rx[port_id]) ?
> >                 (stats.ipackets - prev_pkts_rx[port_id]) : 0; @@
> > -206,10 +222,10 @@ nic_stats_display(portid_t port_id)
> >                 (stats.opackets - prev_pkts_tx[port_id]) : 0;
> >         prev_pkts_rx[port_id] = stats.ipackets;
> >         prev_pkts_tx[port_id] = stats.opackets;
> > -       mpps_rx = diff_cycles > 0 ?
> > -               diff_pkts_rx * rte_get_tsc_hz() / diff_cycles : 0;
> > -       mpps_tx = diff_cycles > 0 ?
> > -               diff_pkts_tx * rte_get_tsc_hz() / diff_cycles : 0;
> > +       mpps_rx = diff_ns > 0 ?
> > +               (double)diff_pkts_rx / diff_ns * NS_PER_SEC : 0;
> > +       mpps_tx = diff_ns > 0 ?
> > +               (double)diff_pkts_tx / diff_ns * NS_PER_SEC : 0;
> >
> >         diff_bytes_rx = (stats.ibytes > prev_bytes_rx[port_id]) ?
> >                 (stats.ibytes - prev_bytes_rx[port_id]) : 0; @@
> > -217,10 +233,10 @@ nic_stats_display(portid_t port_id)
> >                 (stats.obytes - prev_bytes_tx[port_id]) : 0;
> >         prev_bytes_rx[port_id] = stats.ibytes;
> >         prev_bytes_tx[port_id] = stats.obytes;
> > -       mbps_rx = diff_cycles > 0 ?
> > -               diff_bytes_rx * rte_get_tsc_hz() / diff_cycles : 0;
> > -       mbps_tx = diff_cycles > 0 ?
> > -               diff_bytes_tx * rte_get_tsc_hz() / diff_cycles : 0;
> > +       mbps_rx = diff_ns > 0 ?
> > +               (double)diff_bytes_rx / diff_ns * NS_PER_SEC : 0;
> > +       mbps_tx = diff_ns > 0 ?
> > +               (double)diff_bytes_tx / diff_ns * NS_PER_SEC : 0;
> >
> >         printf("\n  Throughput (since last show)\n");
> >         printf("  Rx-pps: %12"PRIu64"          Rx-bps: %12"PRIu64"\n  Tx-
> pps: %12"
> > --
> > 2.17.1
> >

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [dpdk-stable] [PATCH 3/5] app/testpmd: enable burst stats for noisy vnf mode
  2020-06-17 14:43 ` [dpdk-stable] [PATCH 3/5] app/testpmd: enable burst stats for noisy vnf mode Honnappa Nagarahalli
@ 2020-06-18  7:16   ` Jens Freimann
  0 siblings, 0 replies; 32+ messages in thread
From: Jens Freimann @ 2020-06-18  7:16 UTC (permalink / raw)
  To: Honnappa Nagarahalli
  Cc: dev, alialnu, orgerlitz, wenzhuo.lu, beilei.xing,
	bernard.iremonger, hemant.agrawal, jerinj, viacheslavo, thomas,
	ruifeng.wang, phil.yang, nd, stable

On Wed, Jun 17, 2020 at 09:43:05AM -0500, Honnappa Nagarahalli wrote:
>From: Phil Yang <phil.yang@arm.com>
>
>Add burst stats for noisy vnf mode.
>
>Fixes: 3c156061b938 ("app/testpmd: add noisy neighbour forwarding mode")
>Cc: stable@dpdk.org
>Cc: jfreimann@redhat.com
>
>Signed-off-by: Phil Yang <phil.yang@arm.com>
>Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
>Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
>---
> app/test-pmd/noisy_vnf.c | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>

Reviewed-by: Jens Freimann <jfreimann@redhat.com>

regards,
Jens 


^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [dpdk-stable] [dpdk-dev] [PATCH 1/5] app/testpmd: clock gettime call in throughput calculation
  2020-06-18  4:03   ` Honnappa Nagarahalli
@ 2020-06-18 10:16     ` Jerin Jacob
  0 siblings, 0 replies; 32+ messages in thread
From: Jerin Jacob @ 2020-06-18 10:16 UTC (permalink / raw)
  To: Honnappa Nagarahalli
  Cc: dpdk-dev, Ali Alnubani, orgerlitz, Wenzhuo Lu, Beilei Xing,
	Bernard Iremonger, hemant.agrawal, jerinj, Slava Ovsiienko,
	thomas, Ruifeng Wang, Phil Yang, nd, Zhihong Wang, dpdk stable

On Thu, Jun 18, 2020 at 9:33 AM Honnappa Nagarahalli
<Honnappa.Nagarahalli@arm.com> wrote:
>
> Thanks Jerin for the feedback
>
> > -----Original Message-----
> > From: Jerin Jacob <jerinjacobk@gmail.com>
> > Sent: Wednesday, June 17, 2020 10:16 AM
> > To: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>
> > Cc: dpdk-dev <dev@dpdk.org>; Ali Alnubani <alialnu@mellanox.com>;
> > orgerlitz@mellanox.com; Wenzhuo Lu <wenzhuo.lu@intel.com>; Beilei Xing
> > <beilei.xing@intel.com>; Bernard Iremonger <bernard.iremonger@intel.com>;
> > hemant.agrawal@nxp.com; jerinj@marvell.com; Slava Ovsiienko
> > <viacheslavo@mellanox.com>; thomas@monjalon.net; Ruifeng Wang
> > <Ruifeng.Wang@arm.com>; Phil Yang <Phil.Yang@arm.com>; nd
> > <nd@arm.com>; Zhihong Wang <zhihong.wang@intel.com>; dpdk stable
> > <stable@dpdk.org>
> > Subject: Re: [dpdk-dev] [PATCH 1/5] app/testpmd: clock gettime call in
> > throughput calculation
> >
> > On Wed, Jun 17, 2020 at 8:13 PM Honnappa Nagarahalli
> > <honnappa.nagarahalli@arm.com> wrote:
> > >
> > > The throughput calculation requires a counter that measures passing of
> > > time. The PMU cycle counter does not do that. This
> >
> >
> > It is not clear from git commit on why PMU cycle counter does not do that?
> > On dpdk bootup, we are figuring out the Hz value based on PMU counter
> > cycles.
> > What is the missing piece here?
> As I understand Linux kernel saves the PMU state and restores it every time a thread is scheduled out and in. So, when the thread is scheduled out the PMU cycles are not counted towards that thread. The thread that prints the statistics issues good amount of system calls and I am guessing it is getting scheduled out. So, it is reporting very low cycle count.

OK. Probably add this info in git commit.

> >
> > IMO, PMU counter should have less latency and more granularity than
> > clock_getime.
> In general, agree. In this particular calculation the granularity has not mattered much (for ex: numbers are fine with 50Mhz generic counter and 2.5Ghz CPU). The latency also does not matter as it is getting amortized over a large number of packets. So, I do not see it affecting the reported PPS/BPS numbers.

Reasonable to use clock_gettime for the control thread.

>
> >
> > > results in incorrect throughput numbers when
> > RTE_ARM_EAL_RDTSC_USE_PMU
> > > is enabled. Use clock_gettime system call to calculate the time passed
> > > since last call.
> > >
> > > Bugzilla ID: 450
> > > Fixes: 0e106980301d ("app/testpmd: show throughput in port stats")
> > > Cc: zhihong.wang@intel.com
> > > Cc: stable@dpdk.org
> > >
> > > Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> > > Reviewed-by: Phil Yang <phil.yang@arm.com>
> > > Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> > > ---
> > >  app/test-pmd/config.c | 44
> > > +++++++++++++++++++++++++++++--------------
> > >  1 file changed, 30 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c index
> > > 016bcb09c..91fbf99f8 100644
> > > --- a/app/test-pmd/config.c
> > > +++ b/app/test-pmd/config.c
> > > @@ -54,6 +54,14 @@
> > >
> > >  #define ETHDEV_FWVERS_LEN 32
> > >
> > > +#ifdef CLOCK_MONOTONIC_RAW /* Defined in glibc bits/time.h */ #define
> > > +CLOCK_TYPE_ID CLOCK_MONOTONIC_RAW #else #define CLOCK_TYPE_ID
> > > +CLOCK_MONOTONIC #endif
> > > +
> > > +#define NS_PER_SEC 1E9
> > > +
> > >  static char *flowtype_to_str(uint16_t flow_type);
> > >
> > >  static const struct {
> > > @@ -136,9 +144,10 @@ nic_stats_display(portid_t port_id)
> > >         static uint64_t prev_pkts_tx[RTE_MAX_ETHPORTS];
> > >         static uint64_t prev_bytes_rx[RTE_MAX_ETHPORTS];
> > >         static uint64_t prev_bytes_tx[RTE_MAX_ETHPORTS];
> > > -       static uint64_t prev_cycles[RTE_MAX_ETHPORTS];
> > > +       static uint64_t prev_ns[RTE_MAX_ETHPORTS];
> > > +       struct timespec cur_time;
> > >         uint64_t diff_pkts_rx, diff_pkts_tx, diff_bytes_rx, diff_bytes_tx,
> > > -                                                               diff_cycles;
> > > +
> > > + diff_ns;
> > >         uint64_t mpps_rx, mpps_tx, mbps_rx, mbps_tx;
> > >         struct rte_eth_stats stats;
> > >         struct rte_port *port = &ports[port_id]; @@ -195,10 +204,17 @@
> > > nic_stats_display(portid_t port_id)
> > >                 }
> > >         }
> > >
> > > -       diff_cycles = prev_cycles[port_id];
> > > -       prev_cycles[port_id] = rte_rdtsc();
> > > -       if (diff_cycles > 0)
> > > -               diff_cycles = prev_cycles[port_id] - diff_cycles;
> > > +       diff_ns = 0;
> > > +       if (clock_gettime(CLOCK_TYPE_ID, &cur_time) == 0) {
> > > +               uint64_t ns;
> > > +
> > > +               ns = cur_time.tv_sec * NS_PER_SEC;
> > > +               ns += cur_time.tv_nsec;
> > > +
> > > +               if (prev_ns[port_id] != 0)
> > > +                       diff_ns = ns - prev_ns[port_id];
> > > +               prev_ns[port_id] = ns;
> > > +       }
> > >
> > >         diff_pkts_rx = (stats.ipackets > prev_pkts_rx[port_id]) ?
> > >                 (stats.ipackets - prev_pkts_rx[port_id]) : 0; @@
> > > -206,10 +222,10 @@ nic_stats_display(portid_t port_id)
> > >                 (stats.opackets - prev_pkts_tx[port_id]) : 0;
> > >         prev_pkts_rx[port_id] = stats.ipackets;
> > >         prev_pkts_tx[port_id] = stats.opackets;
> > > -       mpps_rx = diff_cycles > 0 ?
> > > -               diff_pkts_rx * rte_get_tsc_hz() / diff_cycles : 0;
> > > -       mpps_tx = diff_cycles > 0 ?
> > > -               diff_pkts_tx * rte_get_tsc_hz() / diff_cycles : 0;
> > > +       mpps_rx = diff_ns > 0 ?
> > > +               (double)diff_pkts_rx / diff_ns * NS_PER_SEC : 0;
> > > +       mpps_tx = diff_ns > 0 ?
> > > +               (double)diff_pkts_tx / diff_ns * NS_PER_SEC : 0;
> > >
> > >         diff_bytes_rx = (stats.ibytes > prev_bytes_rx[port_id]) ?
> > >                 (stats.ibytes - prev_bytes_rx[port_id]) : 0; @@
> > > -217,10 +233,10 @@ nic_stats_display(portid_t port_id)
> > >                 (stats.obytes - prev_bytes_tx[port_id]) : 0;
> > >         prev_bytes_rx[port_id] = stats.ibytes;
> > >         prev_bytes_tx[port_id] = stats.obytes;
> > > -       mbps_rx = diff_cycles > 0 ?
> > > -               diff_bytes_rx * rte_get_tsc_hz() / diff_cycles : 0;
> > > -       mbps_tx = diff_cycles > 0 ?
> > > -               diff_bytes_tx * rte_get_tsc_hz() / diff_cycles : 0;
> > > +       mbps_rx = diff_ns > 0 ?
> > > +               (double)diff_bytes_rx / diff_ns * NS_PER_SEC : 0;
> > > +       mbps_tx = diff_ns > 0 ?
> > > +               (double)diff_bytes_tx / diff_ns * NS_PER_SEC : 0;
> > >
> > >         printf("\n  Throughput (since last show)\n");
> > >         printf("  Rx-pps: %12"PRIu64"          Rx-bps: %12"PRIu64"\n  Tx-
> > pps: %12"
> > > --
> > > 2.17.1
> > >

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [dpdk-stable] [PATCH 2/5] app/testpmd: enable burst stats for flowgen mode rx path
  2020-06-17 14:43 ` [dpdk-stable] [PATCH 2/5] app/testpmd: enable burst stats for flowgen mode rx path Honnappa Nagarahalli
@ 2020-06-18 14:57   ` Phil Yang
  0 siblings, 0 replies; 32+ messages in thread
From: Phil Yang @ 2020-06-18 14:57 UTC (permalink / raw)
  To: Honnappa Nagarahalli, dev, Honnappa Nagarahalli, alialnu,
	orgerlitz, wenzhuo.lu, beilei.xing, bernard.iremonger
  Cc: hemant.agrawal, jerinj, viacheslavo, thomas, Ruifeng Wang, nd,
	stable, nd

> -----Original Message-----
> From: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> Sent: Wednesday, June 17, 2020 10:43 PM
> To: dev@dpdk.org; Honnappa Nagarahalli
> <Honnappa.Nagarahalli@arm.com>; alialnu@mellanox.com;
> orgerlitz@mellanox.com; wenzhuo.lu@intel.com; beilei.xing@intel.com;
> bernard.iremonger@intel.com
> Cc: hemant.agrawal@nxp.com; jerinj@marvell.com;
> viacheslavo@mellanox.com; thomas@monjalon.net; Ruifeng Wang
> <Ruifeng.Wang@arm.com>; Phil Yang <Phil.Yang@arm.com>; nd
> <nd@arm.com>; stable@dpdk.org
> Subject: [PATCH 2/5] app/testpmd: enable burst stats for flowgen mode rx
> path
> 
> Enable missing burst stats for rx path for flowgen mode.
> 
> Fixes: e9e23a617eb8 ("app/testpmd: add flowgen forwarding engine")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> Reviewed-by: Phil Yang <phil.yang@arm.com>
> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> ---
Tested-by: Phil Yang <phil.yang@arm.com>

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [dpdk-stable] [PATCH 4/5] app/testpmd: fix burst percentage calculation
  2020-06-17 14:43 ` [dpdk-stable] [PATCH 4/5] app/testpmd: fix burst percentage calculation Honnappa Nagarahalli
@ 2020-06-18 15:01   ` Phil Yang
  2020-06-23 13:33   ` Ali Alnubani
  1 sibling, 0 replies; 32+ messages in thread
From: Phil Yang @ 2020-06-18 15:01 UTC (permalink / raw)
  To: Honnappa Nagarahalli, dev, Honnappa Nagarahalli, alialnu,
	orgerlitz, wenzhuo.lu, beilei.xing, bernard.iremonger
  Cc: hemant.agrawal, jerinj, viacheslavo, thomas, Ruifeng Wang, nd,
	stable, nd

> -----Original Message-----
> From: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> Sent: Wednesday, June 17, 2020 10:43 PM
> To: dev@dpdk.org; Honnappa Nagarahalli
> <Honnappa.Nagarahalli@arm.com>; alialnu@mellanox.com;
> orgerlitz@mellanox.com; wenzhuo.lu@intel.com; beilei.xing@intel.com;
> bernard.iremonger@intel.com
> Cc: hemant.agrawal@nxp.com; jerinj@marvell.com;
> viacheslavo@mellanox.com; thomas@monjalon.net; Ruifeng Wang
> <Ruifeng.Wang@arm.com>; Phil Yang <Phil.Yang@arm.com>; nd
> <nd@arm.com>; stable@dpdk.org
> Subject: [PATCH 4/5] app/testpmd: fix burst percentage calculation
> 
> The burst % calculation can over flow due to multiplication.
> Fix the multiplication and increase the size of variables to
> 64b.
> 
> Fixes: af75078fece3 ("first public release")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> Reviewed-by: Phil Yang <phil.yang@arm.com>
> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> ---
Tested-by: Phil Yang <phil.yang@arm.com>

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [dpdk-stable] [PATCH 1/5] app/testpmd: clock gettime call in throughput calculation
  2020-06-17 14:43 [dpdk-stable] [PATCH 1/5] app/testpmd: clock gettime call in throughput calculation Honnappa Nagarahalli
                   ` (3 preceding siblings ...)
  2020-06-17 15:16 ` [dpdk-stable] [dpdk-dev] [PATCH 1/5] app/testpmd: clock gettime call in throughput calculation Jerin Jacob
@ 2020-06-18 15:08 ` " Phil Yang
  2020-06-23 13:33 ` Ali Alnubani
                   ` (2 subsequent siblings)
  7 siblings, 0 replies; 32+ messages in thread
From: Phil Yang @ 2020-06-18 15:08 UTC (permalink / raw)
  To: Honnappa Nagarahalli, dev, Honnappa Nagarahalli, alialnu,
	orgerlitz, wenzhuo.lu, beilei.xing, bernard.iremonger
  Cc: hemant.agrawal, jerinj, viacheslavo, thomas, Ruifeng Wang, nd,
	zhihong.wang, stable, nd

> -----Original Message-----
> From: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> Sent: Wednesday, June 17, 2020 10:43 PM
> To: dev@dpdk.org; Honnappa Nagarahalli
> <Honnappa.Nagarahalli@arm.com>; alialnu@mellanox.com;
> orgerlitz@mellanox.com; wenzhuo.lu@intel.com; beilei.xing@intel.com;
> bernard.iremonger@intel.com
> Cc: hemant.agrawal@nxp.com; jerinj@marvell.com;
> viacheslavo@mellanox.com; thomas@monjalon.net; Ruifeng Wang
> <Ruifeng.Wang@arm.com>; Phil Yang <Phil.Yang@arm.com>; nd
> <nd@arm.com>; zhihong.wang@intel.com; stable@dpdk.org
> Subject: [PATCH 1/5] app/testpmd: clock gettime call in throughput
> calculation
> 
> The throughput calculation requires a counter that measures
> passing of time. The PMU cycle counter does not do that. This
> results in incorrect throughput numbers when
> RTE_ARM_EAL_RDTSC_USE_PMU is enabled. Use clock_gettime
> system call to calculate the time passed since last call.
> 
> Bugzilla ID: 450
> Fixes: 0e106980301d ("app/testpmd: show throughput in port stats")
> Cc: zhihong.wang@intel.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> Reviewed-by: Phil Yang <phil.yang@arm.com>
> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> ---

Tested-by: Phil Yang <phil.yang@arm.com> 

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [dpdk-stable] [PATCH 1/5] app/testpmd: clock gettime call in throughput calculation
  2020-06-17 14:43 [dpdk-stable] [PATCH 1/5] app/testpmd: clock gettime call in throughput calculation Honnappa Nagarahalli
                   ` (4 preceding siblings ...)
  2020-06-18 15:08 ` [dpdk-stable] " Phil Yang
@ 2020-06-23 13:33 ` Ali Alnubani
  2020-06-26 22:09 ` [dpdk-stable] [PATCH v2 " Honnappa Nagarahalli
  2020-07-06 23:32 ` [dpdk-stable] [PATCH v3 1/3] " Honnappa Nagarahalli
  7 siblings, 0 replies; 32+ messages in thread
From: Ali Alnubani @ 2020-06-23 13:33 UTC (permalink / raw)
  To: Honnappa Nagarahalli, dev, orgerlitz, wenzhuo.lu, beilei.xing,
	bernard.iremonger
  Cc: hemant.agrawal, jerinj, Slava Ovsiienko, Thomas Monjalon,
	ruifeng.wang, phil.yang, nd, zhihong.wang, stable

> -----Original Message-----
> From: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> Sent: Wednesday, June 17, 2020 5:43 PM
> To: dev@dpdk.org; honnappa.nagarahalli@arm.com; Ali Alnubani
> <alialnu@mellanox.com>; orgerlitz@mellanox.com; wenzhuo.lu@intel.com;
> beilei.xing@intel.com; bernard.iremonger@intel.com
> Cc: hemant.agrawal@nxp.com; jerinj@marvell.com; Slava Ovsiienko
> <viacheslavo@mellanox.com>; Thomas Monjalon <thomas@monjalon.net>;
> ruifeng.wang@arm.com; phil.yang@arm.com; nd@arm.com;
> zhihong.wang@intel.com; stable@dpdk.org
> Subject: [PATCH 1/5] app/testpmd: clock gettime call in throughput
> calculation
> 
> The throughput calculation requires a counter that measures passing of time.
> The PMU cycle counter does not do that. This results in incorrect throughput
> numbers when RTE_ARM_EAL_RDTSC_USE_PMU is enabled. Use
> clock_gettime system call to calculate the time passed since last call.
> 
> Bugzilla ID: 450
> Fixes: 0e106980301d ("app/testpmd: show throughput in port stats")
> Cc: zhihong.wang@intel.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> Reviewed-by: Phil Yang <phil.yang@arm.com>
> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>

Tested-by: Ali Alnubani <alialnu@mellanox.com>

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [dpdk-stable] [PATCH 4/5] app/testpmd: fix burst percentage calculation
  2020-06-17 14:43 ` [dpdk-stable] [PATCH 4/5] app/testpmd: fix burst percentage calculation Honnappa Nagarahalli
  2020-06-18 15:01   ` Phil Yang
@ 2020-06-23 13:33   ` Ali Alnubani
  1 sibling, 0 replies; 32+ messages in thread
From: Ali Alnubani @ 2020-06-23 13:33 UTC (permalink / raw)
  To: Honnappa Nagarahalli, dev, orgerlitz, wenzhuo.lu, beilei.xing,
	bernard.iremonger
  Cc: hemant.agrawal, jerinj, Slava Ovsiienko, Thomas Monjalon,
	ruifeng.wang, phil.yang, nd, stable

> -----Original Message-----
> From: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> Sent: Wednesday, June 17, 2020 5:43 PM
> To: dev@dpdk.org; honnappa.nagarahalli@arm.com; Ali Alnubani
> <alialnu@mellanox.com>; orgerlitz@mellanox.com; wenzhuo.lu@intel.com;
> beilei.xing@intel.com; bernard.iremonger@intel.com
> Cc: hemant.agrawal@nxp.com; jerinj@marvell.com; Slava Ovsiienko
> <viacheslavo@mellanox.com>; Thomas Monjalon <thomas@monjalon.net>;
> ruifeng.wang@arm.com; phil.yang@arm.com; nd@arm.com;
> stable@dpdk.org
> Subject: [PATCH 4/5] app/testpmd: fix burst percentage calculation
> 
> The burst % calculation can over flow due to multiplication.
> Fix the multiplication and increase the size of variables to 64b.
> 
> Fixes: af75078fece3 ("first public release")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> Reviewed-by: Phil Yang <phil.yang@arm.com>
> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>

Tested-by: Ali Alnubani <alialnu@mellanox.com>

^ permalink raw reply	[flat|nested] 32+ messages in thread

* [dpdk-stable] [PATCH v2 1/5] app/testpmd: clock gettime call in throughput calculation
  2020-06-17 14:43 [dpdk-stable] [PATCH 1/5] app/testpmd: clock gettime call in throughput calculation Honnappa Nagarahalli
                   ` (5 preceding siblings ...)
  2020-06-23 13:33 ` Ali Alnubani
@ 2020-06-26 22:09 ` " Honnappa Nagarahalli
  2020-06-26 22:09   ` [dpdk-stable] [PATCH v2 2/5] app/testpmd: enable burst stats for flowgen mode rx path Honnappa Nagarahalli
                     ` (3 more replies)
  2020-07-06 23:32 ` [dpdk-stable] [PATCH v3 1/3] " Honnappa Nagarahalli
  7 siblings, 4 replies; 32+ messages in thread
From: Honnappa Nagarahalli @ 2020-06-26 22:09 UTC (permalink / raw)
  To: dev, honnappa.nagarahalli, alialnu, orgerlitz, wenzhuo.lu,
	beilei.xing, bernard.iremonger, ferruh.yigit
  Cc: hemant.agrawal, jerinj, viacheslavo, thomas, ruifeng.wang,
	phil.yang, nd, zhihong.wang, stable

The throughput calculation requires a counter that measures
passing of time. However, the kernel saves and restores the PMU
state when a thread is unscheduled and scheduled. This ensures
that the PMU cycles are not counted towards a thread that is
not scheduled. Hence, when RTE_ARM_EAL_RDTSC_USE_PMU is enabled,
the PMU cycles do not represent the passing of time.
This results in incorrect calculation of throughput numbers.
Use clock_gettime system call to calculate the time passed since
last call.

Bugzilla ID: 450
Fixes: 0e106980301d ("app/testpmd: show throughput in port stats")
Cc: zhihong.wang@intel.com
Cc: stable@dpdk.org

Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
Reviewed-by: Phil Yang <phil.yang@arm.com>
Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
Tested-by: Phil Yang <phil.yang@arm.com>
Tested-by: Ali Alnubani <alialnu@mellanox.com>
---
v2: Updated commit log in 1/5 (Jerin)

 app/test-pmd/config.c | 44 +++++++++++++++++++++++++++++--------------
 1 file changed, 30 insertions(+), 14 deletions(-)

diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index 016bcb09c..91fbf99f8 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -54,6 +54,14 @@
 
 #define ETHDEV_FWVERS_LEN 32
 
+#ifdef CLOCK_MONOTONIC_RAW /* Defined in glibc bits/time.h */
+#define CLOCK_TYPE_ID CLOCK_MONOTONIC_RAW
+#else
+#define CLOCK_TYPE_ID CLOCK_MONOTONIC
+#endif
+
+#define NS_PER_SEC 1E9
+
 static char *flowtype_to_str(uint16_t flow_type);
 
 static const struct {
@@ -136,9 +144,10 @@ nic_stats_display(portid_t port_id)
 	static uint64_t prev_pkts_tx[RTE_MAX_ETHPORTS];
 	static uint64_t prev_bytes_rx[RTE_MAX_ETHPORTS];
 	static uint64_t prev_bytes_tx[RTE_MAX_ETHPORTS];
-	static uint64_t prev_cycles[RTE_MAX_ETHPORTS];
+	static uint64_t prev_ns[RTE_MAX_ETHPORTS];
+	struct timespec cur_time;
 	uint64_t diff_pkts_rx, diff_pkts_tx, diff_bytes_rx, diff_bytes_tx,
-								diff_cycles;
+								diff_ns;
 	uint64_t mpps_rx, mpps_tx, mbps_rx, mbps_tx;
 	struct rte_eth_stats stats;
 	struct rte_port *port = &ports[port_id];
@@ -195,10 +204,17 @@ nic_stats_display(portid_t port_id)
 		}
 	}
 
-	diff_cycles = prev_cycles[port_id];
-	prev_cycles[port_id] = rte_rdtsc();
-	if (diff_cycles > 0)
-		diff_cycles = prev_cycles[port_id] - diff_cycles;
+	diff_ns = 0;
+	if (clock_gettime(CLOCK_TYPE_ID, &cur_time) == 0) {
+		uint64_t ns;
+
+		ns = cur_time.tv_sec * NS_PER_SEC;
+		ns += cur_time.tv_nsec;
+
+		if (prev_ns[port_id] != 0)
+			diff_ns = ns - prev_ns[port_id];
+		prev_ns[port_id] = ns;
+	}
 
 	diff_pkts_rx = (stats.ipackets > prev_pkts_rx[port_id]) ?
 		(stats.ipackets - prev_pkts_rx[port_id]) : 0;
@@ -206,10 +222,10 @@ nic_stats_display(portid_t port_id)
 		(stats.opackets - prev_pkts_tx[port_id]) : 0;
 	prev_pkts_rx[port_id] = stats.ipackets;
 	prev_pkts_tx[port_id] = stats.opackets;
-	mpps_rx = diff_cycles > 0 ?
-		diff_pkts_rx * rte_get_tsc_hz() / diff_cycles : 0;
-	mpps_tx = diff_cycles > 0 ?
-		diff_pkts_tx * rte_get_tsc_hz() / diff_cycles : 0;
+	mpps_rx = diff_ns > 0 ?
+		(double)diff_pkts_rx / diff_ns * NS_PER_SEC : 0;
+	mpps_tx = diff_ns > 0 ?
+		(double)diff_pkts_tx / diff_ns * NS_PER_SEC : 0;
 
 	diff_bytes_rx = (stats.ibytes > prev_bytes_rx[port_id]) ?
 		(stats.ibytes - prev_bytes_rx[port_id]) : 0;
@@ -217,10 +233,10 @@ nic_stats_display(portid_t port_id)
 		(stats.obytes - prev_bytes_tx[port_id]) : 0;
 	prev_bytes_rx[port_id] = stats.ibytes;
 	prev_bytes_tx[port_id] = stats.obytes;
-	mbps_rx = diff_cycles > 0 ?
-		diff_bytes_rx * rte_get_tsc_hz() / diff_cycles : 0;
-	mbps_tx = diff_cycles > 0 ?
-		diff_bytes_tx * rte_get_tsc_hz() / diff_cycles : 0;
+	mbps_rx = diff_ns > 0 ?
+		(double)diff_bytes_rx / diff_ns * NS_PER_SEC : 0;
+	mbps_tx = diff_ns > 0 ?
+		(double)diff_bytes_tx / diff_ns * NS_PER_SEC : 0;
 
 	printf("\n  Throughput (since last show)\n");
 	printf("  Rx-pps: %12"PRIu64"          Rx-bps: %12"PRIu64"\n  Tx-pps: %12"
-- 
2.17.1


^ permalink raw reply	[flat|nested] 32+ messages in thread

* [dpdk-stable] [PATCH v2 2/5] app/testpmd: enable burst stats for flowgen mode rx path
  2020-06-26 22:09 ` [dpdk-stable] [PATCH v2 " Honnappa Nagarahalli
@ 2020-06-26 22:09   ` Honnappa Nagarahalli
  2020-07-06 16:02     ` Ferruh Yigit
  2020-06-26 22:09   ` [dpdk-stable] [PATCH v2 3/5] app/testpmd: enable burst stats for noisy vnf mode Honnappa Nagarahalli
                     ` (2 subsequent siblings)
  3 siblings, 1 reply; 32+ messages in thread
From: Honnappa Nagarahalli @ 2020-06-26 22:09 UTC (permalink / raw)
  To: dev, honnappa.nagarahalli, alialnu, orgerlitz, wenzhuo.lu,
	beilei.xing, bernard.iremonger, ferruh.yigit
  Cc: hemant.agrawal, jerinj, viacheslavo, thomas, ruifeng.wang,
	phil.yang, nd, stable

Enable missing burst stats for rx path for flowgen mode.

Fixes: e9e23a617eb8 ("app/testpmd: add flowgen forwarding engine")
Cc: stable@dpdk.org

Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
Reviewed-by: Phil Yang <phil.yang@arm.com>
Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
Tested-by: Phil Yang <phil.yang@arm.com>
---
 app/test-pmd/flowgen.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/app/test-pmd/flowgen.c b/app/test-pmd/flowgen.c
index 4bd351e67..011887c61 100644
--- a/app/test-pmd/flowgen.c
+++ b/app/test-pmd/flowgen.c
@@ -111,6 +111,10 @@ pkt_burst_flow_gen(struct fwd_stream *fs)
 	/* Receive a burst of packets and discard them. */
 	nb_rx = rte_eth_rx_burst(fs->rx_port, fs->rx_queue, pkts_burst,
 				 nb_pkt_per_burst);
+#ifdef RTE_TEST_PMD_RECORD_BURST_STATS
+	fs->rx_burst_stats.pkt_burst_spread[nb_rx]++;
+#endif
+
 	fs->rx_packets += nb_rx;
 
 	for (i = 0; i < nb_rx; i++)
-- 
2.17.1


^ permalink raw reply	[flat|nested] 32+ messages in thread

* [dpdk-stable] [PATCH v2 3/5] app/testpmd: enable burst stats for noisy vnf mode
  2020-06-26 22:09 ` [dpdk-stable] [PATCH v2 " Honnappa Nagarahalli
  2020-06-26 22:09   ` [dpdk-stable] [PATCH v2 2/5] app/testpmd: enable burst stats for flowgen mode rx path Honnappa Nagarahalli
@ 2020-06-26 22:09   ` Honnappa Nagarahalli
  2020-07-06 16:08     ` Ferruh Yigit
  2020-06-26 22:09   ` [dpdk-stable] [PATCH v2 4/5] app/testpmd: fix burst percentage calculation Honnappa Nagarahalli
  2020-07-06 15:36   ` [dpdk-stable] [PATCH v2 1/5] app/testpmd: clock gettime call in throughput calculation Ferruh Yigit
  3 siblings, 1 reply; 32+ messages in thread
From: Honnappa Nagarahalli @ 2020-06-26 22:09 UTC (permalink / raw)
  To: dev, honnappa.nagarahalli, alialnu, orgerlitz, wenzhuo.lu,
	beilei.xing, bernard.iremonger, ferruh.yigit
  Cc: hemant.agrawal, jerinj, viacheslavo, thomas, ruifeng.wang,
	phil.yang, nd, stable, jfreimann

From: Phil Yang <phil.yang@arm.com>

Add burst stats for noisy vnf mode.

Fixes: 3c156061b938 ("app/testpmd: add noisy neighbour forwarding mode")
Cc: stable@dpdk.org
Cc: jfreimann@redhat.com

Signed-off-by: Phil Yang <phil.yang@arm.com>
Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
Reviewed-by: Jens Freimann <jfreimann@redhat.com>
---
 app/test-pmd/noisy_vnf.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/app/test-pmd/noisy_vnf.c b/app/test-pmd/noisy_vnf.c
index 58c4ee925..24f286da6 100644
--- a/app/test-pmd/noisy_vnf.c
+++ b/app/test-pmd/noisy_vnf.c
@@ -154,6 +154,9 @@ pkt_burst_noisy_vnf(struct fwd_stream *fs)
 
 	nb_rx = rte_eth_rx_burst(fs->rx_port, fs->rx_queue,
 			pkts_burst, nb_pkt_per_burst);
+#ifdef RTE_TEST_PMD_RECORD_BURST_STATS
+	fs->rx_burst_stats.pkt_burst_spread[nb_rx]++;
+#endif
 	if (unlikely(nb_rx == 0))
 		goto flush;
 	fs->rx_packets += nb_rx;
@@ -164,6 +167,9 @@ pkt_burst_noisy_vnf(struct fwd_stream *fs)
 				pkts_burst, nb_rx);
 		if (unlikely(nb_tx < nb_rx) && fs->retry_enabled)
 			nb_tx += do_retry(nb_rx, nb_tx, pkts_burst, fs);
+#ifdef RTE_TEST_PMD_RECORD_BURST_STATS
+		fs->tx_burst_stats.pkt_burst_spread[nb_tx]++;
+#endif
 		fs->tx_packets += nb_tx;
 		fs->fwd_dropped += drop_pkts(pkts_burst, nb_rx, nb_tx);
 		return;
@@ -187,6 +193,9 @@ pkt_burst_noisy_vnf(struct fwd_stream *fs)
 					nb_deqd);
 			if (unlikely(nb_tx < nb_rx) && fs->retry_enabled)
 				nb_tx += do_retry(nb_rx, nb_tx, tmp_pkts, fs);
+#ifdef RTE_TEST_PMD_RECORD_BURST_STATS
+			fs->tx_burst_stats.pkt_burst_spread[nb_tx]++;
+#endif
 			fs->fwd_dropped += drop_pkts(tmp_pkts, nb_deqd, nb_tx);
 		}
 	}
@@ -211,6 +220,9 @@ pkt_burst_noisy_vnf(struct fwd_stream *fs)
 					 tmp_pkts, nb_deqd);
 		if (unlikely(sent < nb_deqd) && fs->retry_enabled)
 			nb_tx += do_retry(nb_rx, nb_tx, tmp_pkts, fs);
+#ifdef RTE_TEST_PMD_RECORD_BURST_STATS
+		fs->tx_burst_stats.pkt_burst_spread[nb_tx]++;
+#endif
 		fs->fwd_dropped += drop_pkts(tmp_pkts, nb_deqd, sent);
 		ncf->prev_time = rte_get_timer_cycles();
 	}
-- 
2.17.1


^ permalink raw reply	[flat|nested] 32+ messages in thread

* [dpdk-stable] [PATCH v2 4/5] app/testpmd: fix burst percentage calculation
  2020-06-26 22:09 ` [dpdk-stable] [PATCH v2 " Honnappa Nagarahalli
  2020-06-26 22:09   ` [dpdk-stable] [PATCH v2 2/5] app/testpmd: enable burst stats for flowgen mode rx path Honnappa Nagarahalli
  2020-06-26 22:09   ` [dpdk-stable] [PATCH v2 3/5] app/testpmd: enable burst stats for noisy vnf mode Honnappa Nagarahalli
@ 2020-06-26 22:09   ` Honnappa Nagarahalli
  2020-07-06 16:10     ` Ferruh Yigit
  2020-07-06 15:36   ` [dpdk-stable] [PATCH v2 1/5] app/testpmd: clock gettime call in throughput calculation Ferruh Yigit
  3 siblings, 1 reply; 32+ messages in thread
From: Honnappa Nagarahalli @ 2020-06-26 22:09 UTC (permalink / raw)
  To: dev, honnappa.nagarahalli, alialnu, orgerlitz, wenzhuo.lu,
	beilei.xing, bernard.iremonger, ferruh.yigit
  Cc: hemant.agrawal, jerinj, viacheslavo, thomas, ruifeng.wang,
	phil.yang, nd, stable

The burst % calculation can over flow due to multiplication.
Fix the multiplication and increase the size of variables to
64b.

Fixes: af75078fece3 ("first public release")
Cc: stable@dpdk.org

Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
Reviewed-by: Phil Yang <phil.yang@arm.com>
Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
Tested-by: Phil Yang <phil.yang@arm.com>
Tested-by: Ali Alnubani <alialnu@mellanox.com>
---
 app/test-pmd/testpmd.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index 4989d22ca..2e1493da2 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -1692,9 +1692,9 @@ init_fwd_streams(void)
 static void
 pkt_burst_stats_display(const char *rx_tx, struct pkt_burst_stats *pbs)
 {
-	unsigned int total_burst;
-	unsigned int nb_burst;
-	unsigned int burst_stats[3];
+	uint64_t total_burst;
+	uint64_t nb_burst;
+	uint64_t burst_stats[3];
 	uint16_t pktnb_stats[3];
 	uint16_t nb_pkt;
 	int burst_percent[3];
@@ -1723,8 +1723,8 @@ pkt_burst_stats_display(const char *rx_tx, struct pkt_burst_stats *pbs)
 	}
 	if (total_burst == 0)
 		return;
-	burst_percent[0] = (burst_stats[0] * 100) / total_burst;
-	printf("  %s-bursts : %u [%d%% of %d pkts", rx_tx, total_burst,
+	burst_percent[0] = (double)burst_stats[0] / total_burst * 100;
+	printf("  %s-bursts : %"PRIu64" [%d%% of %d pkts", rx_tx, total_burst,
 	       burst_percent[0], (int) pktnb_stats[0]);
 	if (burst_stats[0] == total_burst) {
 		printf("]\n");
@@ -1735,7 +1735,7 @@ pkt_burst_stats_display(const char *rx_tx, struct pkt_burst_stats *pbs)
 		       100 - burst_percent[0], pktnb_stats[1]);
 		return;
 	}
-	burst_percent[1] = (burst_stats[1] * 100) / total_burst;
+	burst_percent[1] = (double)burst_stats[1] / total_burst * 100;
 	burst_percent[2] = 100 - (burst_percent[0] + burst_percent[1]);
 	if ((burst_percent[1] == 0) || (burst_percent[2] == 0)) {
 		printf(" + %d%% of others]\n", 100 - burst_percent[0]);
-- 
2.17.1


^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [dpdk-stable] [PATCH v2 1/5] app/testpmd: clock gettime call in throughput calculation
  2020-06-26 22:09 ` [dpdk-stable] [PATCH v2 " Honnappa Nagarahalli
                     ` (2 preceding siblings ...)
  2020-06-26 22:09   ` [dpdk-stable] [PATCH v2 4/5] app/testpmd: fix burst percentage calculation Honnappa Nagarahalli
@ 2020-07-06 15:36   ` Ferruh Yigit
  2020-07-06 16:53     ` Honnappa Nagarahalli
  3 siblings, 1 reply; 32+ messages in thread
From: Ferruh Yigit @ 2020-07-06 15:36 UTC (permalink / raw)
  To: Honnappa Nagarahalli, dev, alialnu, orgerlitz, wenzhuo.lu,
	beilei.xing, bernard.iremonger
  Cc: hemant.agrawal, jerinj, viacheslavo, thomas, ruifeng.wang,
	phil.yang, nd, zhihong.wang, stable

On 6/26/2020 11:09 PM, Honnappa Nagarahalli wrote:
> The throughput calculation requires a counter that measures
> passing of time. However, the kernel saves and restores the PMU
> state when a thread is unscheduled and scheduled. This ensures
> that the PMU cycles are not counted towards a thread that is
> not scheduled. Hence, when RTE_ARM_EAL_RDTSC_USE_PMU is enabled,
> the PMU cycles do not represent the passing of time.

Does this mean 'rte_rdtsc()' is broken for Arm?
Wouldn't it cause more serious issue than testpmd throughput stats?

> This results in incorrect calculation of throughput numbers.
> Use clock_gettime system call to calculate the time passed since
> last call.
> 
> Bugzilla ID: 450
> Fixes: 0e106980301d ("app/testpmd: show throughput in port stats")
> Cc: zhihong.wang@intel.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> Reviewed-by: Phil Yang <phil.yang@arm.com>
> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> Tested-by: Phil Yang <phil.yang@arm.com>
> Tested-by: Ali Alnubani <alialnu@mellanox.com>

<...>

> @@ -195,10 +204,17 @@ nic_stats_display(portid_t port_id)
>  		}
>  	}
>  
> -	diff_cycles = prev_cycles[port_id];
> -	prev_cycles[port_id] = rte_rdtsc();
> -	if (diff_cycles > 0)
> -		diff_cycles = prev_cycles[port_id] - diff_cycles;
> +	diff_ns = 0;
> +	if (clock_gettime(CLOCK_TYPE_ID, &cur_time) == 0) {

I guess 'rte_rdtsc()' is faster, but since this is not in the data path, I think
it is OK.

<...>

> @@ -217,10 +233,10 @@ nic_stats_display(portid_t port_id)
>  		(stats.obytes - prev_bytes_tx[port_id]) : 0;
>  	prev_bytes_rx[port_id] = stats.ibytes;
>  	prev_bytes_tx[port_id] = stats.obytes;
> -	mbps_rx = diff_cycles > 0 ?
> -		diff_bytes_rx * rte_get_tsc_hz() / diff_cycles : 0;
> -	mbps_tx = diff_cycles > 0 ?
> -		diff_bytes_tx * rte_get_tsc_hz() / diff_cycles : 0;
> +	mbps_rx = diff_ns > 0 ?
> +		(double)diff_bytes_rx / diff_ns * NS_PER_SEC : 0;
> +	mbps_tx = diff_ns > 0 ?
> +		(double)diff_bytes_tx / diff_ns * NS_PER_SEC : 0;

The calculation also fixes other issue in the stats.
With previous method, if the sampling between two stats is a little long,
"diff_pkts_rx * rte_get_tsc_hz()" can overflow and produce wrong 'bps'.


Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [dpdk-stable] [PATCH v2 2/5] app/testpmd: enable burst stats for flowgen mode rx path
  2020-06-26 22:09   ` [dpdk-stable] [PATCH v2 2/5] app/testpmd: enable burst stats for flowgen mode rx path Honnappa Nagarahalli
@ 2020-07-06 16:02     ` Ferruh Yigit
  2020-07-06 17:06       ` Honnappa Nagarahalli
  0 siblings, 1 reply; 32+ messages in thread
From: Ferruh Yigit @ 2020-07-06 16:02 UTC (permalink / raw)
  To: Honnappa Nagarahalli, dev, alialnu, orgerlitz, wenzhuo.lu,
	beilei.xing, bernard.iremonger
  Cc: hemant.agrawal, jerinj, viacheslavo, thomas, ruifeng.wang,
	phil.yang, nd, stable

On 6/26/2020 11:09 PM, Honnappa Nagarahalli wrote:
> Enable missing burst stats for rx path for flowgen mode.
> 
> Fixes: e9e23a617eb8 ("app/testpmd: add flowgen forwarding engine")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> Reviewed-by: Phil Yang <phil.yang@arm.com>
> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> Tested-by: Phil Yang <phil.yang@arm.com>
> ---
>  app/test-pmd/flowgen.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/app/test-pmd/flowgen.c b/app/test-pmd/flowgen.c
> index 4bd351e67..011887c61 100644
> --- a/app/test-pmd/flowgen.c
> +++ b/app/test-pmd/flowgen.c
> @@ -111,6 +111,10 @@ pkt_burst_flow_gen(struct fwd_stream *fs)
>  	/* Receive a burst of packets and discard them. */
>  	nb_rx = rte_eth_rx_burst(fs->rx_port, fs->rx_queue, pkts_burst,
>  				 nb_pkt_per_burst);
> +#ifdef RTE_TEST_PMD_RECORD_BURST_STATS
> +	fs->rx_burst_stats.pkt_burst_spread[nb_rx]++;
> +#endif
> +

Flow generator is for Tx, what is the point of getting stats for the Rx?

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [dpdk-stable] [PATCH v2 3/5] app/testpmd: enable burst stats for noisy vnf mode
  2020-06-26 22:09   ` [dpdk-stable] [PATCH v2 3/5] app/testpmd: enable burst stats for noisy vnf mode Honnappa Nagarahalli
@ 2020-07-06 16:08     ` Ferruh Yigit
  2020-07-06 16:59       ` Honnappa Nagarahalli
  0 siblings, 1 reply; 32+ messages in thread
From: Ferruh Yigit @ 2020-07-06 16:08 UTC (permalink / raw)
  To: Honnappa Nagarahalli, dev, alialnu, orgerlitz, wenzhuo.lu,
	beilei.xing, bernard.iremonger
  Cc: hemant.agrawal, jerinj, viacheslavo, thomas, ruifeng.wang,
	phil.yang, nd, stable, jfreimann

On 6/26/2020 11:09 PM, Honnappa Nagarahalli wrote:
> From: Phil Yang <phil.yang@arm.com>
> 
> Add burst stats for noisy vnf mode.
> 
> Fixes: 3c156061b938 ("app/testpmd: add noisy neighbour forwarding mode")
> Cc: stable@dpdk.org
> Cc: jfreimann@redhat.com
> 
> Signed-off-by: Phil Yang <phil.yang@arm.com>
> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> Reviewed-by: Jens Freimann <jfreimann@redhat.com>
> ---
>  app/test-pmd/noisy_vnf.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/app/test-pmd/noisy_vnf.c b/app/test-pmd/noisy_vnf.c
> index 58c4ee925..24f286da6 100644
> --- a/app/test-pmd/noisy_vnf.c
> +++ b/app/test-pmd/noisy_vnf.c
> @@ -154,6 +154,9 @@ pkt_burst_noisy_vnf(struct fwd_stream *fs)
>  
>  	nb_rx = rte_eth_rx_burst(fs->rx_port, fs->rx_queue,
>  			pkts_burst, nb_pkt_per_burst);
> +#ifdef RTE_TEST_PMD_RECORD_BURST_STATS
> +	fs->rx_burst_stats.pkt_burst_spread[nb_rx]++;
> +#endif
>  	if (unlikely(nb_rx == 0))
>  		goto flush;
>  	fs->rx_packets += nb_rx;
> @@ -164,6 +167,9 @@ pkt_burst_noisy_vnf(struct fwd_stream *fs)
>  				pkts_burst, nb_rx);
>  		if (unlikely(nb_tx < nb_rx) && fs->retry_enabled)
>  			nb_tx += do_retry(nb_rx, nb_tx, pkts_burst, fs);
> +#ifdef RTE_TEST_PMD_RECORD_BURST_STATS
> +		fs->tx_burst_stats.pkt_burst_spread[nb_tx]++;
> +#endif
>  		fs->tx_packets += nb_tx;
>  		fs->fwd_dropped += drop_pkts(pkts_burst, nb_rx, nb_tx);
>  		return;

No objection to add the burst stats to more forwarding engines, but this config
does not exist for the meson and make is going away.

We need to figure out how to support this option with meson before spreading it out.


^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [dpdk-stable] [PATCH v2 4/5] app/testpmd: fix burst percentage calculation
  2020-06-26 22:09   ` [dpdk-stable] [PATCH v2 4/5] app/testpmd: fix burst percentage calculation Honnappa Nagarahalli
@ 2020-07-06 16:10     ` Ferruh Yigit
  0 siblings, 0 replies; 32+ messages in thread
From: Ferruh Yigit @ 2020-07-06 16:10 UTC (permalink / raw)
  To: Honnappa Nagarahalli, dev, alialnu, orgerlitz, wenzhuo.lu,
	beilei.xing, bernard.iremonger
  Cc: hemant.agrawal, jerinj, viacheslavo, thomas, ruifeng.wang,
	phil.yang, nd, stable

On 6/26/2020 11:09 PM, Honnappa Nagarahalli wrote:
> The burst % calculation can over flow due to multiplication.
> Fix the multiplication and increase the size of variables to
> 64b.
> 
> Fixes: af75078fece3 ("first public release")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> Reviewed-by: Phil Yang <phil.yang@arm.com>
> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> Tested-by: Phil Yang <phil.yang@arm.com>
> Tested-by: Ali Alnubani <alialnu@mellanox.com>

Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>


^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [dpdk-stable] [PATCH v2 1/5] app/testpmd: clock gettime call in throughput calculation
  2020-07-06 15:36   ` [dpdk-stable] [PATCH v2 1/5] app/testpmd: clock gettime call in throughput calculation Ferruh Yigit
@ 2020-07-06 16:53     ` Honnappa Nagarahalli
  2020-07-06 17:19       ` Ferruh Yigit
  0 siblings, 1 reply; 32+ messages in thread
From: Honnappa Nagarahalli @ 2020-07-06 16:53 UTC (permalink / raw)
  To: Ferruh Yigit, dev, alialnu, orgerlitz, wenzhuo.lu, beilei.xing,
	bernard.iremonger
  Cc: hemant.agrawal, jerinj, viacheslavo, thomas, Ruifeng Wang,
	Phil Yang, nd, zhihong.wang, stable, Honnappa Nagarahalli, nd

Hi Ferruh,
	Thanks for your comments.

<snip>

> Subject: Re: [PATCH v2 1/5] app/testpmd: clock gettime call in throughput
> calculation
> 
> On 6/26/2020 11:09 PM, Honnappa Nagarahalli wrote:
> > The throughput calculation requires a counter that measures passing of
> > time. However, the kernel saves and restores the PMU state when a
> > thread is unscheduled and scheduled. This ensures that the PMU cycles
> > are not counted towards a thread that is not scheduled. Hence, when
> > RTE_ARM_EAL_RDTSC_USE_PMU is enabled, the PMU cycles do not
> represent
> > the passing of time.
> 
> Does this mean 'rte_rdtsc()' is broken for Arm?
It depends on the kernel I think. IMO, for isolated CPUs it should be fine. It is currently getting fixed through a kernel patch. Once the kernel patch is up streamed, we will make the required changes in DPDK.

> Wouldn't it cause more serious issue than testpmd throughput stats?
Within DPDK, it does not seem to be used for anything critical (I have fixed one).

> 
> > This results in incorrect calculation of throughput numbers.
> > Use clock_gettime system call to calculate the time passed since last
> > call.
> >
> > Bugzilla ID: 450
> > Fixes: 0e106980301d ("app/testpmd: show throughput in port stats")
> > Cc: zhihong.wang@intel.com
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> > Reviewed-by: Phil Yang <phil.yang@arm.com>
> > Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> > Tested-by: Phil Yang <phil.yang@arm.com>
> > Tested-by: Ali Alnubani <alialnu@mellanox.com>
> 
> <...>
> 
> > @@ -195,10 +204,17 @@ nic_stats_display(portid_t port_id)
> >  		}
> >  	}
> >
> > -	diff_cycles = prev_cycles[port_id];
> > -	prev_cycles[port_id] = rte_rdtsc();
> > -	if (diff_cycles > 0)
> > -		diff_cycles = prev_cycles[port_id] - diff_cycles;
> > +	diff_ns = 0;
> > +	if (clock_gettime(CLOCK_TYPE_ID, &cur_time) == 0) {
> 
> I guess 'rte_rdtsc()' is faster, but since this is not in the data path, I think it is
> OK.
> 
> <...>
> 
> > @@ -217,10 +233,10 @@ nic_stats_display(portid_t port_id)
> >  		(stats.obytes - prev_bytes_tx[port_id]) : 0;
> >  	prev_bytes_rx[port_id] = stats.ibytes;
> >  	prev_bytes_tx[port_id] = stats.obytes;
> > -	mbps_rx = diff_cycles > 0 ?
> > -		diff_bytes_rx * rte_get_tsc_hz() / diff_cycles : 0;
> > -	mbps_tx = diff_cycles > 0 ?
> > -		diff_bytes_tx * rte_get_tsc_hz() / diff_cycles : 0;
> > +	mbps_rx = diff_ns > 0 ?
> > +		(double)diff_bytes_rx / diff_ns * NS_PER_SEC : 0;
> > +	mbps_tx = diff_ns > 0 ?
> > +		(double)diff_bytes_tx / diff_ns * NS_PER_SEC : 0;
> 
> The calculation also fixes other issue in the stats.
> With previous method, if the sampling between two stats is a little long,
> "diff_pkts_rx * rte_get_tsc_hz()" can overflow and produce wrong 'bps'.
> 
> 
> Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [dpdk-stable] [PATCH v2 3/5] app/testpmd: enable burst stats for noisy vnf mode
  2020-07-06 16:08     ` Ferruh Yigit
@ 2020-07-06 16:59       ` Honnappa Nagarahalli
  2020-07-06 17:11         ` Ferruh Yigit
  0 siblings, 1 reply; 32+ messages in thread
From: Honnappa Nagarahalli @ 2020-07-06 16:59 UTC (permalink / raw)
  To: Ferruh Yigit, dev, alialnu, orgerlitz, wenzhuo.lu, beilei.xing,
	bernard.iremonger
  Cc: hemant.agrawal, jerinj, viacheslavo, thomas, Ruifeng Wang,
	Phil Yang, nd, stable, jfreimann, Honnappa Nagarahalli, nd

<snip>

> Subject: Re: [PATCH v2 3/5] app/testpmd: enable burst stats for noisy vnf
> mode
> 
> On 6/26/2020 11:09 PM, Honnappa Nagarahalli wrote:
> > From: Phil Yang <phil.yang@arm.com>
> >
> > Add burst stats for noisy vnf mode.
> >
> > Fixes: 3c156061b938 ("app/testpmd: add noisy neighbour forwarding
> > mode")
> > Cc: stable@dpdk.org
> > Cc: jfreimann@redhat.com
> >
> > Signed-off-by: Phil Yang <phil.yang@arm.com>
> > Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> > Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> > Reviewed-by: Jens Freimann <jfreimann@redhat.com>
> > ---
> >  app/test-pmd/noisy_vnf.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> >
> > diff --git a/app/test-pmd/noisy_vnf.c b/app/test-pmd/noisy_vnf.c index
> > 58c4ee925..24f286da6 100644
> > --- a/app/test-pmd/noisy_vnf.c
> > +++ b/app/test-pmd/noisy_vnf.c
> > @@ -154,6 +154,9 @@ pkt_burst_noisy_vnf(struct fwd_stream *fs)
> >
> >  	nb_rx = rte_eth_rx_burst(fs->rx_port, fs->rx_queue,
> >  			pkts_burst, nb_pkt_per_burst);
> > +#ifdef RTE_TEST_PMD_RECORD_BURST_STATS
> > +	fs->rx_burst_stats.pkt_burst_spread[nb_rx]++;
> > +#endif
> >  	if (unlikely(nb_rx == 0))
> >  		goto flush;
> >  	fs->rx_packets += nb_rx;
> > @@ -164,6 +167,9 @@ pkt_burst_noisy_vnf(struct fwd_stream *fs)
> >  				pkts_burst, nb_rx);
> >  		if (unlikely(nb_tx < nb_rx) && fs->retry_enabled)
> >  			nb_tx += do_retry(nb_rx, nb_tx, pkts_burst, fs);
> > +#ifdef RTE_TEST_PMD_RECORD_BURST_STATS
> > +		fs->tx_burst_stats.pkt_burst_spread[nb_tx]++;
> > +#endif
> >  		fs->tx_packets += nb_tx;
> >  		fs->fwd_dropped += drop_pkts(pkts_burst, nb_rx, nb_tx);
> >  		return;
> 
> No objection to add the burst stats to more forwarding engines, but this
> config does not exist for the meson and make is going away.
> 
> We need to figure out how to support this option with meson before
> spreading it out.
Dharmik is working on making this a run time flag that can be enabled or disabled from command line (as was suggested in other emails). That would remove the requirement on meson.

This compile time flag existed before and this patch fixes a bug. IMO, we should separate the issue of how to enable this flag using meson from this bug fix.

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [dpdk-stable] [PATCH v2 2/5] app/testpmd: enable burst stats for flowgen mode rx path
  2020-07-06 16:02     ` Ferruh Yigit
@ 2020-07-06 17:06       ` Honnappa Nagarahalli
  2020-07-06 17:17         ` Ferruh Yigit
  0 siblings, 1 reply; 32+ messages in thread
From: Honnappa Nagarahalli @ 2020-07-06 17:06 UTC (permalink / raw)
  To: Ferruh Yigit, dev, alialnu, orgerlitz, wenzhuo.lu, beilei.xing,
	bernard.iremonger
  Cc: hemant.agrawal, jerinj, viacheslavo, thomas, Ruifeng Wang,
	Phil Yang, nd, stable, Honnappa Nagarahalli, nd

<snip>

> Subject: Re: [PATCH v2 2/5] app/testpmd: enable burst stats for flowgen mode
> rx path
> 
> On 6/26/2020 11:09 PM, Honnappa Nagarahalli wrote:
> > Enable missing burst stats for rx path for flowgen mode.
> >
> > Fixes: e9e23a617eb8 ("app/testpmd: add flowgen forwarding engine")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> > Reviewed-by: Phil Yang <phil.yang@arm.com>
> > Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> > Tested-by: Phil Yang <phil.yang@arm.com>
> > ---
> >  app/test-pmd/flowgen.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/app/test-pmd/flowgen.c b/app/test-pmd/flowgen.c index
> > 4bd351e67..011887c61 100644
> > --- a/app/test-pmd/flowgen.c
> > +++ b/app/test-pmd/flowgen.c
> > @@ -111,6 +111,10 @@ pkt_burst_flow_gen(struct fwd_stream *fs)
> >  	/* Receive a burst of packets and discard them. */
> >  	nb_rx = rte_eth_rx_burst(fs->rx_port, fs->rx_queue, pkts_burst,
> >  				 nb_pkt_per_burst);
> > +#ifdef RTE_TEST_PMD_RECORD_BURST_STATS
> > +	fs->rx_burst_stats.pkt_burst_spread[nb_rx]++;
> > +#endif
> > +
> 
> Flow generator is for Tx, what is the point of getting stats for the Rx?
Is there any reason for this mode to receive the packets?
The reason I ask is, the core cycles include the RX call as well. So, we have cycles/packet that includes the RX call and burst stats that does not show the RX burst stats.
Do we need to remove the call to rte_eth_rx_burst API?

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [dpdk-stable] [PATCH v2 3/5] app/testpmd: enable burst stats for noisy vnf mode
  2020-07-06 16:59       ` Honnappa Nagarahalli
@ 2020-07-06 17:11         ` Ferruh Yigit
  2020-07-06 17:41           ` Honnappa Nagarahalli
  0 siblings, 1 reply; 32+ messages in thread
From: Ferruh Yigit @ 2020-07-06 17:11 UTC (permalink / raw)
  To: Honnappa Nagarahalli, dev, alialnu, orgerlitz, wenzhuo.lu,
	beilei.xing, bernard.iremonger
  Cc: hemant.agrawal, jerinj, viacheslavo, thomas, Ruifeng Wang,
	Phil Yang, nd, stable, jfreimann

On 7/6/2020 5:59 PM, Honnappa Nagarahalli wrote:
> <snip>
> 
>> Subject: Re: [PATCH v2 3/5] app/testpmd: enable burst stats for noisy vnf
>> mode
>>
>> On 6/26/2020 11:09 PM, Honnappa Nagarahalli wrote:
>>> From: Phil Yang <phil.yang@arm.com>
>>>
>>> Add burst stats for noisy vnf mode.
>>>
>>> Fixes: 3c156061b938 ("app/testpmd: add noisy neighbour forwarding
>>> mode")
>>> Cc: stable@dpdk.org
>>> Cc: jfreimann@redhat.com
>>>
>>> Signed-off-by: Phil Yang <phil.yang@arm.com>
>>> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
>>> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
>>> Reviewed-by: Jens Freimann <jfreimann@redhat.com>
>>> ---
>>>  app/test-pmd/noisy_vnf.c | 12 ++++++++++++
>>>  1 file changed, 12 insertions(+)
>>>
>>> diff --git a/app/test-pmd/noisy_vnf.c b/app/test-pmd/noisy_vnf.c index
>>> 58c4ee925..24f286da6 100644
>>> --- a/app/test-pmd/noisy_vnf.c
>>> +++ b/app/test-pmd/noisy_vnf.c
>>> @@ -154,6 +154,9 @@ pkt_burst_noisy_vnf(struct fwd_stream *fs)
>>>
>>>  	nb_rx = rte_eth_rx_burst(fs->rx_port, fs->rx_queue,
>>>  			pkts_burst, nb_pkt_per_burst);
>>> +#ifdef RTE_TEST_PMD_RECORD_BURST_STATS
>>> +	fs->rx_burst_stats.pkt_burst_spread[nb_rx]++;
>>> +#endif
>>>  	if (unlikely(nb_rx == 0))
>>>  		goto flush;
>>>  	fs->rx_packets += nb_rx;
>>> @@ -164,6 +167,9 @@ pkt_burst_noisy_vnf(struct fwd_stream *fs)
>>>  				pkts_burst, nb_rx);
>>>  		if (unlikely(nb_tx < nb_rx) && fs->retry_enabled)
>>>  			nb_tx += do_retry(nb_rx, nb_tx, pkts_burst, fs);
>>> +#ifdef RTE_TEST_PMD_RECORD_BURST_STATS
>>> +		fs->tx_burst_stats.pkt_burst_spread[nb_tx]++;
>>> +#endif
>>>  		fs->tx_packets += nb_tx;
>>>  		fs->fwd_dropped += drop_pkts(pkts_burst, nb_rx, nb_tx);
>>>  		return;
>>
>> No objection to add the burst stats to more forwarding engines, but this
>> config does not exist for the meson and make is going away.
>>
>> We need to figure out how to support this option with meson before
>> spreading it out.
> Dharmik is working on making this a run time flag that can be enabled or disabled from command line (as was suggested in other emails). That would remove the requirement on meson.

Cool, thanks.
Based on that work, shouldn't we need to update these lines again, instead why
not do the update after Dharmik's patch (or in that patch) based on what the new
way is?

> 
> This compile time flag existed before and this patch fixes a bug. IMO, we should separate the issue of how to enable this flag using meson from this bug fix.
> 

I know this flag exists before, but what is the bug this patch fixes? I thought
this patch enables burst stat for "noisy vnf" forwarding engine.

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [dpdk-stable] [PATCH v2 2/5] app/testpmd: enable burst stats for flowgen mode rx path
  2020-07-06 17:06       ` Honnappa Nagarahalli
@ 2020-07-06 17:17         ` Ferruh Yigit
  2020-07-06 18:46           ` Honnappa Nagarahalli
  0 siblings, 1 reply; 32+ messages in thread
From: Ferruh Yigit @ 2020-07-06 17:17 UTC (permalink / raw)
  To: Honnappa Nagarahalli, dev, alialnu, orgerlitz, wenzhuo.lu,
	beilei.xing, bernard.iremonger
  Cc: hemant.agrawal, jerinj, viacheslavo, thomas, Ruifeng Wang,
	Phil Yang, nd, stable

On 7/6/2020 6:06 PM, Honnappa Nagarahalli wrote:
> <snip>
> 
>> Subject: Re: [PATCH v2 2/5] app/testpmd: enable burst stats for flowgen mode
>> rx path
>>
>> On 6/26/2020 11:09 PM, Honnappa Nagarahalli wrote:
>>> Enable missing burst stats for rx path for flowgen mode.
>>>
>>> Fixes: e9e23a617eb8 ("app/testpmd: add flowgen forwarding engine")
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
>>> Reviewed-by: Phil Yang <phil.yang@arm.com>
>>> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
>>> Tested-by: Phil Yang <phil.yang@arm.com>
>>> ---
>>>  app/test-pmd/flowgen.c | 4 ++++
>>>  1 file changed, 4 insertions(+)
>>>
>>> diff --git a/app/test-pmd/flowgen.c b/app/test-pmd/flowgen.c index
>>> 4bd351e67..011887c61 100644
>>> --- a/app/test-pmd/flowgen.c
>>> +++ b/app/test-pmd/flowgen.c
>>> @@ -111,6 +111,10 @@ pkt_burst_flow_gen(struct fwd_stream *fs)
>>>  	/* Receive a burst of packets and discard them. */
>>>  	nb_rx = rte_eth_rx_burst(fs->rx_port, fs->rx_queue, pkts_burst,
>>>  				 nb_pkt_per_burst);
>>> +#ifdef RTE_TEST_PMD_RECORD_BURST_STATS
>>> +	fs->rx_burst_stats.pkt_burst_spread[nb_rx]++;
>>> +#endif
>>> +
>>
>> Flow generator is for Tx, what is the point of getting stats for the Rx?
> Is there any reason for this mode to receive the packets?
> The reason I ask is, the core cycles include the RX call as well. So, we have cycles/packet that includes the RX call and burst stats that does not show the RX burst stats.
> Do we need to remove the call to rte_eth_rx_burst API?
> 

The implementation receives packets and frees them immediately.
I assume the Rx is implemented in case port receives packets, like if other end
forwards the generated packets back to same port.
Yes it consumes cycles for Rx but received packets are ignored, I am not sure if
received packets have any meaning in flowgen to get additional burst stats.

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [dpdk-stable] [PATCH v2 1/5] app/testpmd: clock gettime call in throughput calculation
  2020-07-06 16:53     ` Honnappa Nagarahalli
@ 2020-07-06 17:19       ` Ferruh Yigit
  0 siblings, 0 replies; 32+ messages in thread
From: Ferruh Yigit @ 2020-07-06 17:19 UTC (permalink / raw)
  To: Honnappa Nagarahalli, dev, alialnu, orgerlitz, wenzhuo.lu,
	beilei.xing, bernard.iremonger
  Cc: hemant.agrawal, jerinj, viacheslavo, thomas, Ruifeng Wang,
	Phil Yang, nd, zhihong.wang, stable

On 7/6/2020 5:53 PM, Honnappa Nagarahalli wrote:
> Hi Ferruh,
> 	Thanks for your comments.
> 
> <snip>
> 
>> Subject: Re: [PATCH v2 1/5] app/testpmd: clock gettime call in throughput
>> calculation
>>
>> On 6/26/2020 11:09 PM, Honnappa Nagarahalli wrote:
>>> The throughput calculation requires a counter that measures passing of
>>> time. However, the kernel saves and restores the PMU state when a
>>> thread is unscheduled and scheduled. This ensures that the PMU cycles
>>> are not counted towards a thread that is not scheduled. Hence, when
>>> RTE_ARM_EAL_RDTSC_USE_PMU is enabled, the PMU cycles do not
>> represent
>>> the passing of time.
>>
>> Does this mean 'rte_rdtsc()' is broken for Arm?
> It depends on the kernel I think. IMO, for isolated CPUs it should be fine. It is currently getting fixed through a kernel patch. Once the kernel patch is up streamed, we will make the required changes in DPDK.
> 
>> Wouldn't it cause more serious issue than testpmd throughput stats?
> Within DPDK, it does not seem to be used for anything critical (I have fixed one).

OK, thanks for clarification.

> 
>>
>>> This results in incorrect calculation of throughput numbers.
>>> Use clock_gettime system call to calculate the time passed since last
>>> call.
>>>
>>> Bugzilla ID: 450
>>> Fixes: 0e106980301d ("app/testpmd: show throughput in port stats")
>>> Cc: zhihong.wang@intel.com
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
>>> Reviewed-by: Phil Yang <phil.yang@arm.com>
>>> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
>>> Tested-by: Phil Yang <phil.yang@arm.com>
>>> Tested-by: Ali Alnubani <alialnu@mellanox.com>
>>
>> <...>
>>
>>> @@ -195,10 +204,17 @@ nic_stats_display(portid_t port_id)
>>>  		}
>>>  	}
>>>
>>> -	diff_cycles = prev_cycles[port_id];
>>> -	prev_cycles[port_id] = rte_rdtsc();
>>> -	if (diff_cycles > 0)
>>> -		diff_cycles = prev_cycles[port_id] - diff_cycles;
>>> +	diff_ns = 0;
>>> +	if (clock_gettime(CLOCK_TYPE_ID, &cur_time) == 0) {
>>
>> I guess 'rte_rdtsc()' is faster, but since this is not in the data path, I think it is
>> OK.
>>
>> <...>
>>
>>> @@ -217,10 +233,10 @@ nic_stats_display(portid_t port_id)
>>>  		(stats.obytes - prev_bytes_tx[port_id]) : 0;
>>>  	prev_bytes_rx[port_id] = stats.ibytes;
>>>  	prev_bytes_tx[port_id] = stats.obytes;
>>> -	mbps_rx = diff_cycles > 0 ?
>>> -		diff_bytes_rx * rte_get_tsc_hz() / diff_cycles : 0;
>>> -	mbps_tx = diff_cycles > 0 ?
>>> -		diff_bytes_tx * rte_get_tsc_hz() / diff_cycles : 0;
>>> +	mbps_rx = diff_ns > 0 ?
>>> +		(double)diff_bytes_rx / diff_ns * NS_PER_SEC : 0;
>>> +	mbps_tx = diff_ns > 0 ?
>>> +		(double)diff_bytes_tx / diff_ns * NS_PER_SEC : 0;
>>
>> The calculation also fixes other issue in the stats.
>> With previous method, if the sampling between two stats is a little long,
>> "diff_pkts_rx * rte_get_tsc_hz()" can overflow and produce wrong 'bps'.
>>
>>
>> Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>


^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [dpdk-stable] [PATCH v2 3/5] app/testpmd: enable burst stats for noisy vnf mode
  2020-07-06 17:11         ` Ferruh Yigit
@ 2020-07-06 17:41           ` Honnappa Nagarahalli
  0 siblings, 0 replies; 32+ messages in thread
From: Honnappa Nagarahalli @ 2020-07-06 17:41 UTC (permalink / raw)
  To: Ferruh Yigit, dev, alialnu, orgerlitz, wenzhuo.lu, beilei.xing,
	bernard.iremonger
  Cc: hemant.agrawal, jerinj, viacheslavo, thomas, Ruifeng Wang,
	Phil Yang, nd, stable, jfreimann, Honnappa Nagarahalli, nd

<snip>

> >
> >> Subject: Re: [PATCH v2 3/5] app/testpmd: enable burst stats for noisy
> >> vnf mode
> >>
> >> On 6/26/2020 11:09 PM, Honnappa Nagarahalli wrote:
> >>> From: Phil Yang <phil.yang@arm.com>
> >>>
> >>> Add burst stats for noisy vnf mode.
> >>>
> >>> Fixes: 3c156061b938 ("app/testpmd: add noisy neighbour forwarding
> >>> mode")
> >>> Cc: stable@dpdk.org
> >>> Cc: jfreimann@redhat.com
> >>>
> >>> Signed-off-by: Phil Yang <phil.yang@arm.com>
> >>> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> >>> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> >>> Reviewed-by: Jens Freimann <jfreimann@redhat.com>
> >>> ---
> >>>  app/test-pmd/noisy_vnf.c | 12 ++++++++++++
> >>>  1 file changed, 12 insertions(+)
> >>>
> >>> diff --git a/app/test-pmd/noisy_vnf.c b/app/test-pmd/noisy_vnf.c
> >>> index
> >>> 58c4ee925..24f286da6 100644
> >>> --- a/app/test-pmd/noisy_vnf.c
> >>> +++ b/app/test-pmd/noisy_vnf.c
> >>> @@ -154,6 +154,9 @@ pkt_burst_noisy_vnf(struct fwd_stream *fs)
> >>>
> >>>  	nb_rx = rte_eth_rx_burst(fs->rx_port, fs->rx_queue,
> >>>  			pkts_burst, nb_pkt_per_burst);
> >>> +#ifdef RTE_TEST_PMD_RECORD_BURST_STATS
> >>> +	fs->rx_burst_stats.pkt_burst_spread[nb_rx]++;
> >>> +#endif
> >>>  	if (unlikely(nb_rx == 0))
> >>>  		goto flush;
> >>>  	fs->rx_packets += nb_rx;
> >>> @@ -164,6 +167,9 @@ pkt_burst_noisy_vnf(struct fwd_stream *fs)
> >>>  				pkts_burst, nb_rx);
> >>>  		if (unlikely(nb_tx < nb_rx) && fs->retry_enabled)
> >>>  			nb_tx += do_retry(nb_rx, nb_tx, pkts_burst, fs);
> >>> +#ifdef RTE_TEST_PMD_RECORD_BURST_STATS
> >>> +		fs->tx_burst_stats.pkt_burst_spread[nb_tx]++;
> >>> +#endif
> >>>  		fs->tx_packets += nb_tx;
> >>>  		fs->fwd_dropped += drop_pkts(pkts_burst, nb_rx, nb_tx);
> >>>  		return;
> >>
> >> No objection to add the burst stats to more forwarding engines, but
> >> this config does not exist for the meson and make is going away.
> >>
> >> We need to figure out how to support this option with meson before
> >> spreading it out.
> > Dharmik is working on making this a run time flag that can be enabled or
> disabled from command line (as was suggested in other emails). That would
> remove the requirement on meson.
> 
> Cool, thanks.
> Based on that work, shouldn't we need to update these lines again, instead
> why not do the update after Dharmik's patch (or in that patch) based on what
> the new way is?
Yes, agree, these lines need to be updated. I am fine to take this route as well.

> 
> >
> > This compile time flag existed before and this patch fixes a bug. IMO, we
> should separate the issue of how to enable this flag using meson from this
> bug fix.
> >
> 
> I know this flag exists before, but what is the bug this patch fixes? I thought
> this patch enables burst stat for "noisy vnf" forwarding engine.
If one enables 'RTE_TEST_PMD_RECORD_BURST_STATS' expecting the burst stats for 'noisy vnf' engine, it is not reported today.

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [dpdk-stable] [PATCH v2 2/5] app/testpmd: enable burst stats for flowgen mode rx path
  2020-07-06 17:17         ` Ferruh Yigit
@ 2020-07-06 18:46           ` Honnappa Nagarahalli
  0 siblings, 0 replies; 32+ messages in thread
From: Honnappa Nagarahalli @ 2020-07-06 18:46 UTC (permalink / raw)
  To: Ferruh Yigit, dev, alialnu, orgerlitz, wenzhuo.lu, beilei.xing,
	bernard.iremonger
  Cc: hemant.agrawal, jerinj, viacheslavo, thomas, Ruifeng Wang,
	Phil Yang, nd, stable, Honnappa Nagarahalli, nd

<snip>

> >
> >> Subject: Re: [PATCH v2 2/5] app/testpmd: enable burst stats for
> >> flowgen mode rx path
> >>
> >> On 6/26/2020 11:09 PM, Honnappa Nagarahalli wrote:
> >>> Enable missing burst stats for rx path for flowgen mode.
> >>>
> >>> Fixes: e9e23a617eb8 ("app/testpmd: add flowgen forwarding engine")
> >>> Cc: stable@dpdk.org
> >>>
> >>> Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> >>> Reviewed-by: Phil Yang <phil.yang@arm.com>
> >>> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> >>> Tested-by: Phil Yang <phil.yang@arm.com>
> >>> ---
> >>>  app/test-pmd/flowgen.c | 4 ++++
> >>>  1 file changed, 4 insertions(+)
> >>>
> >>> diff --git a/app/test-pmd/flowgen.c b/app/test-pmd/flowgen.c index
> >>> 4bd351e67..011887c61 100644
> >>> --- a/app/test-pmd/flowgen.c
> >>> +++ b/app/test-pmd/flowgen.c
> >>> @@ -111,6 +111,10 @@ pkt_burst_flow_gen(struct fwd_stream *fs)
> >>>  	/* Receive a burst of packets and discard them. */
> >>>  	nb_rx = rte_eth_rx_burst(fs->rx_port, fs->rx_queue, pkts_burst,
> >>>  				 nb_pkt_per_burst);
> >>> +#ifdef RTE_TEST_PMD_RECORD_BURST_STATS
> >>> +	fs->rx_burst_stats.pkt_burst_spread[nb_rx]++;
> >>> +#endif
> >>> +
> >>
> >> Flow generator is for Tx, what is the point of getting stats for the Rx?
> > Is there any reason for this mode to receive the packets?
> > The reason I ask is, the core cycles include the RX call as well. So, we have
> cycles/packet that includes the RX call and burst stats that does not show the
> RX burst stats.
> > Do we need to remove the call to rte_eth_rx_burst API?
> >
> 
> The implementation receives packets and frees them immediately.
> I assume the Rx is implemented in case port receives packets, like if other end
> forwards the generated packets back to same port.
I tried to find more info, there is not a lot. The original commit message says 'it is similar to 'txonly' engine, but will generate multiple L4 flows'.
Your assumption seems reasonable to me. In that case, I think it makes sense to include 'core cycles' only for TX. Otherwise, there is not enough information to debug any issues.

> Yes it consumes cycles for Rx but received packets are ignored, I am not sure if
> received packets have any meaning in flowgen to get additional burst stats.
Please see above, I think we need to adjust how the 'core cycles' are counted in this case.

^ permalink raw reply	[flat|nested] 32+ messages in thread

* [dpdk-stable] [PATCH v3 1/3] app/testpmd: clock gettime call in throughput calculation
  2020-06-17 14:43 [dpdk-stable] [PATCH 1/5] app/testpmd: clock gettime call in throughput calculation Honnappa Nagarahalli
                   ` (6 preceding siblings ...)
  2020-06-26 22:09 ` [dpdk-stable] [PATCH v2 " Honnappa Nagarahalli
@ 2020-07-06 23:32 ` " Honnappa Nagarahalli
  2020-07-06 23:32   ` [dpdk-stable] [PATCH v3 2/3] app/testpmd: fix burst percentage calculation Honnappa Nagarahalli
  2020-07-07 17:47   ` [dpdk-stable] [PATCH v3 1/3] app/testpmd: clock gettime call in throughput calculation Ferruh Yigit
  7 siblings, 2 replies; 32+ messages in thread
From: Honnappa Nagarahalli @ 2020-07-06 23:32 UTC (permalink / raw)
  To: dev, honnappa.nagarahalli, alialnu, orgerlitz, wenzhuo.lu,
	beilei.xing, bernard.iremonger, ferruh.yigit
  Cc: hemant.agrawal, jerinj, viacheslavo, thomas, ruifeng.wang,
	phil.yang, nd, zhihong.wang, stable

The throughput calculation requires a counter that measures
passing of time. However, the kernel saves and restores the PMU
state when a thread is unscheduled and scheduled. This ensures
that the PMU cycles are not counted towards a thread that is
not scheduled. Hence, when RTE_ARM_EAL_RDTSC_USE_PMU is enabled,
the PMU cycles do not represent the passing of time.
This results in incorrect calculation of throughput numbers.
Use clock_gettime system call to calculate the time passed since
last call.

Bugzilla ID: 450
Fixes: 0e106980301d ("app/testpmd: show throughput in port stats")
Cc: zhihong.wang@intel.com
Cc: stable@dpdk.org

Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
Reviewed-by: Phil Yang <phil.yang@arm.com>
Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
Tested-by: Phil Yang <phil.yang@arm.com>
Tested-by: Ali Alnubani <alialnu@mellanox.com>
---
v3:
1) Dropped 'noisy vnf' patch, will add with future Dharmik's patch
2) Dropped flow gen patch, send out a separate one if there is
   consensus on the mailing list
3) Changed the logic to display burst stats to make it easier
   to understand (Ferruh)

v2: Updated commit log in 1/5 (Jerin)

 app/test-pmd/config.c | 44 +++++++++++++++++++++++++++++--------------
 1 file changed, 30 insertions(+), 14 deletions(-)

diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index 016bcb09c..91fbf99f8 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -54,6 +54,14 @@
 
 #define ETHDEV_FWVERS_LEN 32
 
+#ifdef CLOCK_MONOTONIC_RAW /* Defined in glibc bits/time.h */
+#define CLOCK_TYPE_ID CLOCK_MONOTONIC_RAW
+#else
+#define CLOCK_TYPE_ID CLOCK_MONOTONIC
+#endif
+
+#define NS_PER_SEC 1E9
+
 static char *flowtype_to_str(uint16_t flow_type);
 
 static const struct {
@@ -136,9 +144,10 @@ nic_stats_display(portid_t port_id)
 	static uint64_t prev_pkts_tx[RTE_MAX_ETHPORTS];
 	static uint64_t prev_bytes_rx[RTE_MAX_ETHPORTS];
 	static uint64_t prev_bytes_tx[RTE_MAX_ETHPORTS];
-	static uint64_t prev_cycles[RTE_MAX_ETHPORTS];
+	static uint64_t prev_ns[RTE_MAX_ETHPORTS];
+	struct timespec cur_time;
 	uint64_t diff_pkts_rx, diff_pkts_tx, diff_bytes_rx, diff_bytes_tx,
-								diff_cycles;
+								diff_ns;
 	uint64_t mpps_rx, mpps_tx, mbps_rx, mbps_tx;
 	struct rte_eth_stats stats;
 	struct rte_port *port = &ports[port_id];
@@ -195,10 +204,17 @@ nic_stats_display(portid_t port_id)
 		}
 	}
 
-	diff_cycles = prev_cycles[port_id];
-	prev_cycles[port_id] = rte_rdtsc();
-	if (diff_cycles > 0)
-		diff_cycles = prev_cycles[port_id] - diff_cycles;
+	diff_ns = 0;
+	if (clock_gettime(CLOCK_TYPE_ID, &cur_time) == 0) {
+		uint64_t ns;
+
+		ns = cur_time.tv_sec * NS_PER_SEC;
+		ns += cur_time.tv_nsec;
+
+		if (prev_ns[port_id] != 0)
+			diff_ns = ns - prev_ns[port_id];
+		prev_ns[port_id] = ns;
+	}
 
 	diff_pkts_rx = (stats.ipackets > prev_pkts_rx[port_id]) ?
 		(stats.ipackets - prev_pkts_rx[port_id]) : 0;
@@ -206,10 +222,10 @@ nic_stats_display(portid_t port_id)
 		(stats.opackets - prev_pkts_tx[port_id]) : 0;
 	prev_pkts_rx[port_id] = stats.ipackets;
 	prev_pkts_tx[port_id] = stats.opackets;
-	mpps_rx = diff_cycles > 0 ?
-		diff_pkts_rx * rte_get_tsc_hz() / diff_cycles : 0;
-	mpps_tx = diff_cycles > 0 ?
-		diff_pkts_tx * rte_get_tsc_hz() / diff_cycles : 0;
+	mpps_rx = diff_ns > 0 ?
+		(double)diff_pkts_rx / diff_ns * NS_PER_SEC : 0;
+	mpps_tx = diff_ns > 0 ?
+		(double)diff_pkts_tx / diff_ns * NS_PER_SEC : 0;
 
 	diff_bytes_rx = (stats.ibytes > prev_bytes_rx[port_id]) ?
 		(stats.ibytes - prev_bytes_rx[port_id]) : 0;
@@ -217,10 +233,10 @@ nic_stats_display(portid_t port_id)
 		(stats.obytes - prev_bytes_tx[port_id]) : 0;
 	prev_bytes_rx[port_id] = stats.ibytes;
 	prev_bytes_tx[port_id] = stats.obytes;
-	mbps_rx = diff_cycles > 0 ?
-		diff_bytes_rx * rte_get_tsc_hz() / diff_cycles : 0;
-	mbps_tx = diff_cycles > 0 ?
-		diff_bytes_tx * rte_get_tsc_hz() / diff_cycles : 0;
+	mbps_rx = diff_ns > 0 ?
+		(double)diff_bytes_rx / diff_ns * NS_PER_SEC : 0;
+	mbps_tx = diff_ns > 0 ?
+		(double)diff_bytes_tx / diff_ns * NS_PER_SEC : 0;
 
 	printf("\n  Throughput (since last show)\n");
 	printf("  Rx-pps: %12"PRIu64"          Rx-bps: %12"PRIu64"\n  Tx-pps: %12"
-- 
2.17.1


^ permalink raw reply	[flat|nested] 32+ messages in thread

* [dpdk-stable] [PATCH v3 2/3] app/testpmd: fix burst percentage calculation
  2020-07-06 23:32 ` [dpdk-stable] [PATCH v3 1/3] " Honnappa Nagarahalli
@ 2020-07-06 23:32   ` Honnappa Nagarahalli
  2020-07-07 17:47   ` [dpdk-stable] [PATCH v3 1/3] app/testpmd: clock gettime call in throughput calculation Ferruh Yigit
  1 sibling, 0 replies; 32+ messages in thread
From: Honnappa Nagarahalli @ 2020-07-06 23:32 UTC (permalink / raw)
  To: dev, honnappa.nagarahalli, alialnu, orgerlitz, wenzhuo.lu,
	beilei.xing, bernard.iremonger, ferruh.yigit
  Cc: hemant.agrawal, jerinj, viacheslavo, thomas, ruifeng.wang,
	phil.yang, nd, stable

The burst % calculation can over flow due to multiplication.
Fix the multiplication and increase the size of variables to
64b.

Fixes: af75078fece3 ("first public release")
Cc: stable@dpdk.org

Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
Reviewed-by: Phil Yang <phil.yang@arm.com>
Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
Tested-by: Phil Yang <phil.yang@arm.com>
Tested-by: Ali Alnubani <alialnu@mellanox.com>
---
 app/test-pmd/testpmd.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index 4989d22ca..2e1493da2 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -1692,9 +1692,9 @@ init_fwd_streams(void)
 static void
 pkt_burst_stats_display(const char *rx_tx, struct pkt_burst_stats *pbs)
 {
-	unsigned int total_burst;
-	unsigned int nb_burst;
-	unsigned int burst_stats[3];
+	uint64_t total_burst;
+	uint64_t nb_burst;
+	uint64_t burst_stats[3];
 	uint16_t pktnb_stats[3];
 	uint16_t nb_pkt;
 	int burst_percent[3];
@@ -1723,8 +1723,8 @@ pkt_burst_stats_display(const char *rx_tx, struct pkt_burst_stats *pbs)
 	}
 	if (total_burst == 0)
 		return;
-	burst_percent[0] = (burst_stats[0] * 100) / total_burst;
-	printf("  %s-bursts : %u [%d%% of %d pkts", rx_tx, total_burst,
+	burst_percent[0] = (double)burst_stats[0] / total_burst * 100;
+	printf("  %s-bursts : %"PRIu64" [%d%% of %d pkts", rx_tx, total_burst,
 	       burst_percent[0], (int) pktnb_stats[0]);
 	if (burst_stats[0] == total_burst) {
 		printf("]\n");
@@ -1735,7 +1735,7 @@ pkt_burst_stats_display(const char *rx_tx, struct pkt_burst_stats *pbs)
 		       100 - burst_percent[0], pktnb_stats[1]);
 		return;
 	}
-	burst_percent[1] = (burst_stats[1] * 100) / total_burst;
+	burst_percent[1] = (double)burst_stats[1] / total_burst * 100;
 	burst_percent[2] = 100 - (burst_percent[0] + burst_percent[1]);
 	if ((burst_percent[1] == 0) || (burst_percent[2] == 0)) {
 		printf(" + %d%% of others]\n", 100 - burst_percent[0]);
-- 
2.17.1


^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [dpdk-stable] [PATCH v3 1/3] app/testpmd: clock gettime call in throughput calculation
  2020-07-06 23:32 ` [dpdk-stable] [PATCH v3 1/3] " Honnappa Nagarahalli
  2020-07-06 23:32   ` [dpdk-stable] [PATCH v3 2/3] app/testpmd: fix burst percentage calculation Honnappa Nagarahalli
@ 2020-07-07 17:47   ` Ferruh Yigit
  1 sibling, 0 replies; 32+ messages in thread
From: Ferruh Yigit @ 2020-07-07 17:47 UTC (permalink / raw)
  To: Honnappa Nagarahalli, dev, alialnu, orgerlitz, wenzhuo.lu,
	beilei.xing, bernard.iremonger
  Cc: hemant.agrawal, jerinj, viacheslavo, thomas, ruifeng.wang,
	phil.yang, nd, zhihong.wang, stable

On 7/7/2020 12:32 AM, Honnappa Nagarahalli wrote:
> The throughput calculation requires a counter that measures
> passing of time. However, the kernel saves and restores the PMU
> state when a thread is unscheduled and scheduled. This ensures
> that the PMU cycles are not counted towards a thread that is
> not scheduled. Hence, when RTE_ARM_EAL_RDTSC_USE_PMU is enabled,
> the PMU cycles do not represent the passing of time.
> This results in incorrect calculation of throughput numbers.
> Use clock_gettime system call to calculate the time passed since
> last call.
> 
> Bugzilla ID: 450
> Fixes: 0e106980301d ("app/testpmd: show throughput in port stats")
> Cc: zhihong.wang@intel.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> Reviewed-by: Phil Yang <phil.yang@arm.com>
> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
> Tested-by: Phil Yang <phil.yang@arm.com>
> Tested-by: Ali Alnubani <alialnu@mellanox.com>

Series applied to dpdk-next-net/master, thanks.

^ permalink raw reply	[flat|nested] 32+ messages in thread

end of thread, back to index

Thread overview: 32+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-17 14:43 [dpdk-stable] [PATCH 1/5] app/testpmd: clock gettime call in throughput calculation Honnappa Nagarahalli
2020-06-17 14:43 ` [dpdk-stable] [PATCH 2/5] app/testpmd: enable burst stats for flowgen mode rx path Honnappa Nagarahalli
2020-06-18 14:57   ` Phil Yang
2020-06-17 14:43 ` [dpdk-stable] [PATCH 3/5] app/testpmd: enable burst stats for noisy vnf mode Honnappa Nagarahalli
2020-06-18  7:16   ` Jens Freimann
2020-06-17 14:43 ` [dpdk-stable] [PATCH 4/5] app/testpmd: fix burst percentage calculation Honnappa Nagarahalli
2020-06-18 15:01   ` Phil Yang
2020-06-23 13:33   ` Ali Alnubani
2020-06-17 15:16 ` [dpdk-stable] [dpdk-dev] [PATCH 1/5] app/testpmd: clock gettime call in throughput calculation Jerin Jacob
2020-06-18  4:03   ` Honnappa Nagarahalli
2020-06-18 10:16     ` Jerin Jacob
2020-06-18 15:08 ` [dpdk-stable] " Phil Yang
2020-06-23 13:33 ` Ali Alnubani
2020-06-26 22:09 ` [dpdk-stable] [PATCH v2 " Honnappa Nagarahalli
2020-06-26 22:09   ` [dpdk-stable] [PATCH v2 2/5] app/testpmd: enable burst stats for flowgen mode rx path Honnappa Nagarahalli
2020-07-06 16:02     ` Ferruh Yigit
2020-07-06 17:06       ` Honnappa Nagarahalli
2020-07-06 17:17         ` Ferruh Yigit
2020-07-06 18:46           ` Honnappa Nagarahalli
2020-06-26 22:09   ` [dpdk-stable] [PATCH v2 3/5] app/testpmd: enable burst stats for noisy vnf mode Honnappa Nagarahalli
2020-07-06 16:08     ` Ferruh Yigit
2020-07-06 16:59       ` Honnappa Nagarahalli
2020-07-06 17:11         ` Ferruh Yigit
2020-07-06 17:41           ` Honnappa Nagarahalli
2020-06-26 22:09   ` [dpdk-stable] [PATCH v2 4/5] app/testpmd: fix burst percentage calculation Honnappa Nagarahalli
2020-07-06 16:10     ` Ferruh Yigit
2020-07-06 15:36   ` [dpdk-stable] [PATCH v2 1/5] app/testpmd: clock gettime call in throughput calculation Ferruh Yigit
2020-07-06 16:53     ` Honnappa Nagarahalli
2020-07-06 17:19       ` Ferruh Yigit
2020-07-06 23:32 ` [dpdk-stable] [PATCH v3 1/3] " Honnappa Nagarahalli
2020-07-06 23:32   ` [dpdk-stable] [PATCH v3 2/3] app/testpmd: fix burst percentage calculation Honnappa Nagarahalli
2020-07-07 17:47   ` [dpdk-stable] [PATCH v3 1/3] app/testpmd: clock gettime call in throughput calculation Ferruh Yigit

patches for DPDK stable branches

Archives are clonable:
	git clone --mirror http://inbox.dpdk.org/stable/0 stable/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 stable stable/ http://inbox.dpdk.org/stable \
		stable@dpdk.org
	public-inbox-index stable


Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.stable


AGPL code for this site: git clone https://public-inbox.org/ public-inbox