[v2,5/5] app/testpmd: enable empty polls in burst stats
Checks
Commit Message
The number of empty polls provides information about available
CPU head room in the presence of continuous polling.
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/csumonly.c | 5 +--
app/test-pmd/icmpecho.c | 6 ++--
app/test-pmd/iofwd.c | 6 ++--
app/test-pmd/macfwd.c | 6 ++--
app/test-pmd/macswap.c | 6 ++--
app/test-pmd/rxonly.c | 6 ++--
app/test-pmd/testpmd.c | 74 ++++++++++++++++++++++++-----------------
7 files changed, 61 insertions(+), 48 deletions(-)
Comments
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>
@@ -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;
@@ -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++) {
@@ -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);
/*
@@ -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;
@@ -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];
@@ -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]);
@@ -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 */