RE: [PATCH v3 1/5] latencystats: replace use of VLA

["Morten Brørup" <mb@smartsharesystems.com>]

> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Wednesday, 17 April 2024 19.07
> 
> The temporary array latencystats is not needed if the algorithm
> is converted into one pass.
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>  lib/latencystats/rte_latencystats.c | 31 +++++++++++++++--------------
>  1 file changed, 16 insertions(+), 15 deletions(-)
> 
> diff --git a/lib/latencystats/rte_latencystats.c
> b/lib/latencystats/rte_latencystats.c
> index 4ea9b0d75b..9b345bfb33 100644
> --- a/lib/latencystats/rte_latencystats.c
> +++ b/lib/latencystats/rte_latencystats.c
> @@ -157,9 +157,9 @@ calc_latency(uint16_t pid __rte_unused,
>  		uint16_t nb_pkts,
>  		void *_ __rte_unused)
>  {
> -	unsigned int i, cnt = 0;
> +	unsigned int i;
>  	uint64_t now;
> -	float latency[nb_pkts];
> +	float latency;
>  	static float prev_latency;
>  	/*
>  	 * Alpha represents degree of weighting decrease in EWMA,
> @@ -169,13 +169,14 @@ calc_latency(uint16_t pid __rte_unused,
>  	const float alpha = 0.2;
> 
>  	now = rte_rdtsc();
> -	for (i = 0; i < nb_pkts; i++) {
> -		if (pkts[i]->ol_flags & timestamp_dynflag)
> -			latency[cnt++] = now - *timestamp_dynfield(pkts[i]);
> -	}
> 
>  	rte_spinlock_lock(&glob_stats->lock);
> -	for (i = 0; i < cnt; i++) {
> +	for (i = 0; i < nb_pkts; i++) {
> +		if (!(pkts[i]->ol_flags & timestamp_dynflag))
> +			continue;
> +
> +		latency = now - *timestamp_dynfield(pkts[i]);
> +
>  		/*
>  		 * The jitter is calculated as statistical mean of
> interpacket
>  		 * delay variation. The "jitter estimate" is computed by
> taking
> @@ -187,22 +188,22 @@ calc_latency(uint16_t pid __rte_unused,
>  		 * Reference: Calculated as per RFC 5481, sec 4.1,
>  		 * RFC 3393 sec 4.5, RFC 1889 sec.
>  		 */
> -		glob_stats->jitter +=  (fabsf(prev_latency - latency[i])
> +		glob_stats->jitter +=  (fabsf(prev_latency - latency)
>  					- glob_stats->jitter)/16;
>  		if (glob_stats->min_latency == 0)

You could add unlikely() to this comparison. It should only happen once in a lifetime.

> -			glob_stats->min_latency = latency[i];
> -		else if (latency[i] < glob_stats->min_latency)
> -			glob_stats->min_latency = latency[i];
> -		else if (latency[i] > glob_stats->max_latency)
> -			glob_stats->max_latency = latency[i];
> +			glob_stats->min_latency = latency;

In theory, glob_stats->max_latency should also be initialized with the first sample value here.

> +		else if (latency < glob_stats->min_latency)
> +			glob_stats->min_latency = latency;
> +		else if (latency > glob_stats->max_latency)
> +			glob_stats->max_latency = latency;

The min/max comparisons are also unlikely.

>  		/*
>  		 * The average latency is measured using exponential moving
>  		 * average, i.e. using EWMA
>  		 * https://en.wikipedia.org/wiki/Moving_average
>  		 */
>  		glob_stats->avg_latency +=
> -			alpha * (latency[i] - glob_stats->avg_latency);
> -		prev_latency = latency[i];
> +			alpha * (latency - glob_stats->avg_latency);
> +		prev_latency = latency;
>  	}
>  	rte_spinlock_unlock(&glob_stats->lock);
> 
> --
> 2.43.0

With or without suggested changes,
Acked-by: Morten Brørup <mb@smartsharesystems.com>