Re: [dpdk-dev] [PATCH v2 3/3] app/testpmd: qualify profiling statistics on burst size

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 3/19/2020 1:50 PM, Viacheslav Ovsiienko wrote:
> The execution time of rx/tx burst routine depends on the burst size.
> It would be meaningful to research this dependency, the patch
> provides an extra profiling data per rx/tx burst size.
> 
> Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>

<...>

> @@ -1568,6 +1568,23 @@ struct extmem_param {
>  	}
>  	printf(" + %d%% of %d pkts + %d%% of others]\n",
>  	       burst_percent[1], (int) pktnb_stats[1], burst_percent[2]);
> +#ifdef RTE_TEST_PMD_RECORD_CORE_CYCLES
> +	if (!(fwdprof_flags & (nrx_tx ? RECORD_CORE_CYCLES_TX
> +				      : RECORD_CORE_CYCLES_RX)))
> +		return;
> +	for (nb_pkt = 0; nb_pkt < MAX_PKT_BURST; nb_pkt++) {
> +		nb_burst = nrx_tx ? pbs->pkt_retry_spread[nb_pkt]
> +				  : pbs->pkt_burst_spread[nb_pkt];
> +		if (nb_burst == 0)
> +			continue;

Why "nb_burst == 0" excluded, wouldn't be interesting to see number of calls
with 0 Rx/Tx and cycles spent there?

> +		printf("  CPU cycles/%u packet burst=%u (total cycles="
> +		       "%"PRIu64" / total %s bursts=%u)\n",
> +		       (unsigned int)nb_pkt,
> +		       (unsigned int)(pbs->pkt_ticks_spread[nb_pkt] / nb_burst),
> +		       pbs->pkt_ticks_spread[nb_pkt],
> +		       nrx_tx ? "TX" : "RX", nb_burst);
> +	}
> +#endif
>  }
>  #endif /* RTE_TEST_PMD_RECORD_BURST_STATS */
>  
> @@ -1601,8 +1618,8 @@ struct extmem_param {
>  	}
>  
>  #ifdef RTE_TEST_PMD_RECORD_BURST_STATS
> -	pkt_burst_stats_display("RX", &fs->rx_burst_stats);
> -	pkt_burst_stats_display("TX", &fs->tx_burst_stats);
> +	pkt_burst_stats_display(false, &fs->rx_burst_stats);
> +	pkt_burst_stats_display(true, &fs->tx_burst_stats);

Instead of this true/false, I would be OK to some duplication and have explicit
Rx/Tx, I believe it would be more clear that way, but no strong opinion on it.

<...>

> +++ b/app/test-pmd/testpmd.h
> @@ -89,6 +89,10 @@ enum {
>   */
>  struct pkt_burst_stats {
>  	unsigned int pkt_burst_spread[MAX_PKT_BURST];
> +#ifdef RTE_TEST_PMD_RECORD_CORE_CYCLES
> +	unsigned int pkt_retry_spread[MAX_PKT_BURST];

Isn't this keep the value of all Tx burst count, why named as 'retry'?