| Message ID | 20200626220943.22526-5-honnappa.nagarahalli@arm.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2,1/5] app/testpmd: clock gettime call in throughput calculation | expand |
On 6/26/2020 11:09 PM, Honnappa Nagarahalli wrote: > The number of empty polls provides information about available > CPU head room in the presence of continuous polling. +1 to have it, it can be useful. > > 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> <...> > /* > * First compute the total number of packet bursts and the > * two highest numbers of bursts of the same number of packets. > */ > total_burst = 0; > - burst_stats[0] = burst_stats[1] = burst_stats[2] = 0; > - pktnb_stats[0] = pktnb_stats[1] = pktnb_stats[2] = 0; > + memset(&burst_stats, 0x0, sizeof(burst_stats)); > + memset(&pktnb_stats, 0x0, sizeof(pktnb_stats)); > + > for (nb_pkt = 0; nb_pkt < MAX_PKT_BURST; nb_pkt++) { > nb_burst = pbs->pkt_burst_spread[nb_pkt]; > - if (nb_burst == 0) > - continue; > total_burst += nb_burst; > - if (nb_burst > burst_stats[0]) { > - burst_stats[1] = burst_stats[0]; > - pktnb_stats[1] = pktnb_stats[0]; > + > + /* Show empty poll stats always */ > + if (nb_pkt == 0) { > burst_stats[0] = nb_burst; > pktnb_stats[0] = nb_pkt; just a minor issue but this assignment is not needed. > - } else if (nb_burst > burst_stats[1]) { > + continue; > + } > + > + if (nb_burst == 0) > + continue; again another minor issue, can have this check above 'nb_pkt == 0', to prevent unnecssary "burst_stats[0] = nb_burst;" assignment if "nb_burst == 0". <...> Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
Hi Ferruh, Thanks for the review comments <snip> > Subject: Re: [PATCH v2 5/5] app/testpmd: enable empty polls in burst stats > > On 6/26/2020 11:09 PM, Honnappa Nagarahalli wrote: > > The number of empty polls provides information about available CPU > > head room in the presence of continuous polling. > > +1 to have it, it can be useful. > > > > > 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> > > <...> > > > /* > > * First compute the total number of packet bursts and the > > * two highest numbers of bursts of the same number of packets. > > */ > > total_burst = 0; > > - burst_stats[0] = burst_stats[1] = burst_stats[2] = 0; > > - pktnb_stats[0] = pktnb_stats[1] = pktnb_stats[2] = 0; > > + memset(&burst_stats, 0x0, sizeof(burst_stats)); > > + memset(&pktnb_stats, 0x0, sizeof(pktnb_stats)); > > + > > for (nb_pkt = 0; nb_pkt < MAX_PKT_BURST; nb_pkt++) { > > nb_burst = pbs->pkt_burst_spread[nb_pkt]; > > - if (nb_burst == 0) > > - continue; > > total_burst += nb_burst; > > - if (nb_burst > burst_stats[0]) { > > - burst_stats[1] = burst_stats[0]; > > - pktnb_stats[1] = pktnb_stats[0]; > > + > > + /* Show empty poll stats always */ > > + if (nb_pkt == 0) { > > burst_stats[0] = nb_burst; > > pktnb_stats[0] = nb_pkt; > > just a minor issue but this assignment is not needed. The empty poll stats are being shown always (even if there were no empty polls). The check 'nb_pkts == 0' indicates that the burst stats are for empty polls. Hence this assignment is required. But, this can be moved out of the loop to provide clarity. I will do that. > > > - } else if (nb_burst > burst_stats[1]) { > > + continue; > > + } > > + > > + if (nb_burst == 0) > > + continue; > > again another minor issue, can have this check above 'nb_pkt == 0', to prevent > unnecssary "burst_stats[0] = nb_burst;" assignment if "nb_burst == 0". The empty polls are always shown, even if there were 0% empty polls. > > <...> > > Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
On 7/6/2020 8:11 PM, Honnappa Nagarahalli wrote: > Hi Ferruh, > Thanks for the review comments > > <snip> > >> Subject: Re: [PATCH v2 5/5] app/testpmd: enable empty polls in burst stats >> >> On 6/26/2020 11:09 PM, Honnappa Nagarahalli wrote: >>> The number of empty polls provides information about available CPU >>> head room in the presence of continuous polling. >> >> +1 to have it, it can be useful. >> >>> >>> 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> >> >> <...> >> >>> /* >>> * First compute the total number of packet bursts and the >>> * two highest numbers of bursts of the same number of packets. >>> */ >>> total_burst = 0; >>> - burst_stats[0] = burst_stats[1] = burst_stats[2] = 0; >>> - pktnb_stats[0] = pktnb_stats[1] = pktnb_stats[2] = 0; >>> + memset(&burst_stats, 0x0, sizeof(burst_stats)); >>> + memset(&pktnb_stats, 0x0, sizeof(pktnb_stats)); >>> + >>> for (nb_pkt = 0; nb_pkt < MAX_PKT_BURST; nb_pkt++) { >>> nb_burst = pbs->pkt_burst_spread[nb_pkt]; >>> - if (nb_burst == 0) >>> - continue; >>> total_burst += nb_burst; >>> - if (nb_burst > burst_stats[0]) { >>> - burst_stats[1] = burst_stats[0]; >>> - pktnb_stats[1] = pktnb_stats[0]; >>> + >>> + /* Show empty poll stats always */ >>> + if (nb_pkt == 0) { >>> burst_stats[0] = nb_burst; >>> pktnb_stats[0] = nb_pkt; >> >> just a minor issue but this assignment is not needed. > The empty poll stats are being shown always (even if there were no empty polls). The check 'nb_pkts == 0' indicates that the burst stats are for empty polls. Hence this assignment is required. But, this can be moved out of the loop to provide clarity. I will do that. It is not required because of 'memset' above, we know 'nb_pkt == 0' and we know 'pktnb_stats[0] == 0', so "pktnb_stats[0] = nb_pkt;" is redundant. > >> >>> - } else if (nb_burst > burst_stats[1]) { >>> + continue; >>> + } >>> + >>> + if (nb_burst == 0) >>> + continue; >> >> again another minor issue, can have this check above 'nb_pkt == 0', to prevent >> unnecssary "burst_stats[0] = nb_burst;" assignment if "nb_burst == 0". > The empty polls are always shown, even if there were 0% empty polls. I got that, again because of memset, "burst_stats[0] == 0", you can skip "burst_stats[0] = nb_burst;" in case "nb_burst == 0", that is why you can move it up. Anyway, both are trivial issues ... > >> >> <...> >> >> Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c index 862622379..0a96b24eb 100644 --- a/app/test-pmd/csumonly.c +++ b/app/test-pmd/csumonly.c @@ -802,11 +802,12 @@ pkt_burst_checksum_forward(struct fwd_stream *fs) /* receive a burst of packet */ nb_rx = rte_eth_rx_burst(fs->rx_port, fs->rx_queue, pkts_burst, nb_pkt_per_burst); - if (unlikely(nb_rx == 0)) - return; #ifdef RTE_TEST_PMD_RECORD_BURST_STATS fs->rx_burst_stats.pkt_burst_spread[nb_rx]++; #endif + if (unlikely(nb_rx == 0)) + return; + fs->rx_packets += nb_rx; rx_bad_ip_csum = 0; rx_bad_l4_csum = 0; diff --git a/app/test-pmd/icmpecho.c b/app/test-pmd/icmpecho.c index 65aece16c..78e3adf2c 100644 --- a/app/test-pmd/icmpecho.c +++ b/app/test-pmd/icmpecho.c @@ -308,12 +308,12 @@ reply_to_icmp_echo_rqsts(struct fwd_stream *fs) */ nb_rx = rte_eth_rx_burst(fs->rx_port, fs->rx_queue, pkts_burst, nb_pkt_per_burst); - if (unlikely(nb_rx == 0)) - return; - #ifdef RTE_TEST_PMD_RECORD_BURST_STATS fs->rx_burst_stats.pkt_burst_spread[nb_rx]++; #endif + if (unlikely(nb_rx == 0)) + return; + fs->rx_packets += nb_rx; nb_replies = 0; for (i = 0; i < nb_rx; i++) { diff --git a/app/test-pmd/iofwd.c b/app/test-pmd/iofwd.c index 9dce76efe..22b59bbda 100644 --- a/app/test-pmd/iofwd.c +++ b/app/test-pmd/iofwd.c @@ -66,13 +66,13 @@ pkt_burst_io_forward(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)) return; fs->rx_packets += nb_rx; -#ifdef RTE_TEST_PMD_RECORD_BURST_STATS - fs->rx_burst_stats.pkt_burst_spread[nb_rx]++; -#endif nb_tx = rte_eth_tx_burst(fs->tx_port, fs->tx_queue, pkts_burst, nb_rx); /* diff --git a/app/test-pmd/macfwd.c b/app/test-pmd/macfwd.c index d2ebb1105..0a89d94be 100644 --- a/app/test-pmd/macfwd.c +++ b/app/test-pmd/macfwd.c @@ -71,12 +71,12 @@ pkt_burst_mac_forward(struct fwd_stream *fs) */ nb_rx = rte_eth_rx_burst(fs->rx_port, fs->rx_queue, pkts_burst, nb_pkt_per_burst); - if (unlikely(nb_rx == 0)) - return; - #ifdef RTE_TEST_PMD_RECORD_BURST_STATS fs->rx_burst_stats.pkt_burst_spread[nb_rx]++; #endif + if (unlikely(nb_rx == 0)) + return; + fs->rx_packets += nb_rx; txp = &ports[fs->tx_port]; tx_offloads = txp->dev_conf.txmode.offloads; diff --git a/app/test-pmd/macswap.c b/app/test-pmd/macswap.c index 8428c26d8..fbe8cb39e 100644 --- a/app/test-pmd/macswap.c +++ b/app/test-pmd/macswap.c @@ -72,12 +72,12 @@ pkt_burst_mac_swap(struct fwd_stream *fs) */ nb_rx = rte_eth_rx_burst(fs->rx_port, fs->rx_queue, pkts_burst, nb_pkt_per_burst); - if (unlikely(nb_rx == 0)) - return; - #ifdef RTE_TEST_PMD_RECORD_BURST_STATS fs->rx_burst_stats.pkt_burst_spread[nb_rx]++; #endif + if (unlikely(nb_rx == 0)) + return; + fs->rx_packets += nb_rx; txp = &ports[fs->tx_port]; diff --git a/app/test-pmd/rxonly.c b/app/test-pmd/rxonly.c index 5c65fc42d..18d59a68d 100644 --- a/app/test-pmd/rxonly.c +++ b/app/test-pmd/rxonly.c @@ -63,12 +63,12 @@ pkt_burst_receive(struct fwd_stream *fs) */ nb_rx = rte_eth_rx_burst(fs->rx_port, fs->rx_queue, pkts_burst, nb_pkt_per_burst); - if (unlikely(nb_rx == 0)) - return; - #ifdef RTE_TEST_PMD_RECORD_BURST_STATS fs->rx_burst_stats.pkt_burst_spread[nb_rx]++; #endif + if (unlikely(nb_rx == 0)) + return; + fs->rx_packets += nb_rx; for (i = 0; i < nb_rx; i++) rte_pktmbuf_free(pkts_burst[i]); diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index 2e1493da2..9075143b7 100644 --- a/app/test-pmd/testpmd.c +++ b/app/test-pmd/testpmd.c @@ -1692,57 +1692,69 @@ init_fwd_streams(void) static void pkt_burst_stats_display(const char *rx_tx, struct pkt_burst_stats *pbs) { - uint64_t total_burst; + uint64_t total_burst, sburst; uint64_t nb_burst; - uint64_t burst_stats[3]; - uint16_t pktnb_stats[3]; + uint64_t burst_stats[4]; + uint16_t pktnb_stats[4]; uint16_t nb_pkt; - int burst_percent[3]; + int burst_percent[4], sburstp; + int i; /* * First compute the total number of packet bursts and the * two highest numbers of bursts of the same number of packets. */ total_burst = 0; - burst_stats[0] = burst_stats[1] = burst_stats[2] = 0; - pktnb_stats[0] = pktnb_stats[1] = pktnb_stats[2] = 0; + memset(&burst_stats, 0x0, sizeof(burst_stats)); + memset(&pktnb_stats, 0x0, sizeof(pktnb_stats)); + for (nb_pkt = 0; nb_pkt < MAX_PKT_BURST; nb_pkt++) { nb_burst = pbs->pkt_burst_spread[nb_pkt]; - if (nb_burst == 0) - continue; total_burst += nb_burst; - if (nb_burst > burst_stats[0]) { - burst_stats[1] = burst_stats[0]; - pktnb_stats[1] = pktnb_stats[0]; + + /* Show empty poll stats always */ + if (nb_pkt == 0) { burst_stats[0] = nb_burst; pktnb_stats[0] = nb_pkt; - } else if (nb_burst > burst_stats[1]) { + continue; + } + + if (nb_burst == 0) + continue; + + if (nb_burst > burst_stats[1]) { + burst_stats[2] = burst_stats[1]; + pktnb_stats[2] = pktnb_stats[1]; burst_stats[1] = nb_burst; pktnb_stats[1] = nb_pkt; + } else if (nb_burst > burst_stats[2]) { + burst_stats[2] = nb_burst; + pktnb_stats[2] = nb_pkt; } } if (total_burst == 0) return; - 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"); - return; - } - if (burst_stats[0] + burst_stats[1] == total_burst) { - printf(" + %d%% of %d pkts]\n", - 100 - burst_percent[0], pktnb_stats[1]); - return; - } - 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]); - return; + + printf(" %s-bursts : %"PRIu64" [", rx_tx, total_burst); + for (i = 0, sburst = 0, sburstp = 0; i < 4; i++) { + if (i == 3) { + printf("%d%% of other]\n", 100 - sburstp); + return; + } + + sburst += burst_stats[i]; + if (sburst == total_burst) { + printf("%d%% of %d pkts]\n", + 100 - sburstp, (int) pktnb_stats[i]); + return; + } + + burst_percent[i] = + (double)burst_stats[i] / total_burst * 100; + printf("%d%% of %d pkts + ", + burst_percent[i], (int) pktnb_stats[i]); + sburstp += burst_percent[i]; } - printf(" + %d%% of %d pkts + %d%% of others]\n", - burst_percent[1], (int) pktnb_stats[1], burst_percent[2]); } #endif /* RTE_TEST_PMD_RECORD_BURST_STATS */