[v4,3/6] latencystats: do not use floating point

Message ID 20240419172926.55447-4-stephen@networkplumber.org (mailing list archive)
State Superseded
Delegated to: Thomas Monjalon
Headers
Series latencystats: cleanup |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Stephen Hemminger April 19, 2024, 5:28 p.m. UTC
  The cycle counts do not need to be stored as floating point.
Instead keep track of latency in cycles, and convert to
nanoseconds when read.

Change Exponential Weighted Moving Average weight from .2 to .25
to avoid use of floating point for that.

The average latency took too long to "warm up".
Do what RFC 6298 suggests and initialize on first sample.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
Acked-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
---
 lib/latencystats/rte_latencystats.c | 88 +++++++++++++++--------------
 1 file changed, 45 insertions(+), 43 deletions(-)
  

Comments

Morten Brørup April 19, 2024, 6:49 p.m. UTC | #1
> +		if (unlikely(first_sample)) {
> +			first_sample = false;
> +
>  			glob_stats->min_latency = latency;
> -		else if (latency > glob_stats->max_latency)
>  			glob_stats->max_latency = latency;
> -		/*
> -		 * 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 - glob_stats->avg_latency);
> +			glob_stats->avg_latency = latency;
> +			glob_stats->jitter = latency / 2;

Setting jitter at first sample as latency / 2 is wrong.
Jitter should remain zero at first sample.

> +		} else {
> +			/*
> +			 * The jitter is calculated as statistical mean of
> interpacket
> +			 * delay variation. The "jitter estimate" is computed
> by taking
> +			 * the absolute values of the ipdv sequence and
> applying an
> +			 * exponential filter with parameter 1/16 to generate
> the
> +			 * estimate. i.e J=J+(|D(i-1,i)|-J)/16. Where J is
> jitter,
> +			 * D(i-1,i) is difference in latency of two
> consecutive packets
> +			 * i-1 and i.
> +			 * Reference: Calculated as per RFC 5481, sec 4.1,
> +			 * RFC 3393 sec 4.5, RFC 1889 sec.
> +			 */
> +			glob_stats->jitter += ((prev_latency - latency)
> +					       - glob_stats->jitter) / 16;

With jitter remaining zero at first sample,
Acked-by: Morten Brørup <mb@smartsharesystems.com>
  
Stephen Hemminger April 19, 2024, 10:45 p.m. UTC | #2
On Fri, 19 Apr 2024 20:49:56 +0200
Morten Brørup <mb@smartsharesystems.com> wrote:

> > -		/*
> > -		 * 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 - glob_stats->avg_latency);
> > +			glob_stats->avg_latency = latency;
> > +			glob_stats->jitter = latency / 2;  
> 
> Setting jitter at first sample as latency / 2 is wrong.
> Jitter should remain zero at first sample.

Chose that because it is what the TCP RFC does.
RFC 6298
	
   (2.2) When the first RTT measurement R is made, the host MUST set

            SRTT <- R
            RTTVAR <- R/2
            RTO <- SRTT + max (G, K*RTTVAR)

The problem is that the smoothing constant in this code is quite small.
Also, the TCP RFC has, not sure if matters.

   (2.3) When a subsequent RTT measurement R' is made, a host MUST set

            RTTVAR <- (1 - beta) * RTTVAR + beta * |SRTT - R'|
            SRTT <- (1 - alpha) * SRTT + alpha * R'

         The value of SRTT used in the update to RTTVAR is its value
         before updating SRTT itself using the second assignment.  That
         is, updating RTTVAR and SRTT MUST be computed in the above
         order.

         The above SHOULD be computed using alpha=1/8 and beta=1/4 (as
         suggested in [JK88]).

         After the computation, a host MUST update
         RTO <- SRTT + max (G, K*RTTVAR)
  
Morten Brørup April 20, 2024, 7:31 a.m. UTC | #3
> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Saturday, 20 April 2024 00.45
> 
> On Fri, 19 Apr 2024 20:49:56 +0200
> Morten Brørup <mb@smartsharesystems.com> wrote:
> 
> > > -		/*
> > > -		 * 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 - glob_stats->avg_latency);
> > > +			glob_stats->avg_latency = latency;
> > > +			glob_stats->jitter = latency / 2;
> >
> > Setting jitter at first sample as latency / 2 is wrong.
> > Jitter should remain zero at first sample.
> 
> Chose that because it is what the TCP RFC does.

I suppose it depends on what the measured jitter is being used for.

<rant>
1/2 is very far from the predominant Jitter/RTT relationship I usually see on internet connections.
Jitter is mostly in the ~2 ms order of magnitude, while RTT may be anything between 10 and 600 ms.
Although some links may experience much higher jitter under load, the RTT usually also increases to 100's of milliseconds under these conditions.
Some people are on a crusade to combat this "latency under load" bufferbloat phenomenon. The path towards achieving this begins with measuring and educating.
</rant>

Your RFC reference is a very strong argument.
If the measured jitter is used as a measure of uncertainty in the measured RTT, it makes sense starting high.
I'll accept it.

Acked-by: Morten Brørup <mb@smartsharesystems.com>

> RFC 6298
> 
>    (2.2) When the first RTT measurement R is made, the host MUST set
> 
>             SRTT <- R
>             RTTVAR <- R/2
>             RTO <- SRTT + max (G, K*RTTVAR)
> 
> The problem is that the smoothing constant in this code is quite small.
> Also, the TCP RFC has, not sure if matters.
> 
>    (2.3) When a subsequent RTT measurement R' is made, a host MUST set
> 
>             RTTVAR <- (1 - beta) * RTTVAR + beta * |SRTT - R'|
>             SRTT <- (1 - alpha) * SRTT + alpha * R'
> 
>          The value of SRTT used in the update to RTTVAR is its value
>          before updating SRTT itself using the second assignment.  That
>          is, updating RTTVAR and SRTT MUST be computed in the above
>          order.
> 
>          The above SHOULD be computed using alpha=1/8 and beta=1/4 (as
>          suggested in [JK88]).
> 
>          After the computation, a host MUST update
>          RTO <- SRTT + max (G, K*RTTVAR)
  

Patch

diff --git a/lib/latencystats/rte_latencystats.c b/lib/latencystats/rte_latencystats.c
index 55a099c818..6ef8e344bf 100644
--- a/lib/latencystats/rte_latencystats.c
+++ b/lib/latencystats/rte_latencystats.c
@@ -4,6 +4,7 @@ 
 
 #include <math.h>
 
+#include <rte_common.h>
 #include <rte_string_fns.h>
 #include <rte_mbuf_dyn.h>
 #include <rte_log.h>
@@ -42,10 +43,10 @@  static uint64_t timer_tsc;
 static uint64_t prev_tsc;
 
 struct rte_latency_stats {
-	float min_latency; /**< Minimum latency in nano seconds */
-	float avg_latency; /**< Average latency in nano seconds */
-	float max_latency; /**< Maximum latency in nano seconds */
-	float jitter; /** Latency variation */
+	uint64_t min_latency; /**< Minimum latency */
+	uint64_t avg_latency; /**< Average latency */
+	uint64_t max_latency; /**< Maximum latency */
+	uint64_t jitter; /** Latency variation */
 	rte_spinlock_t lock; /** Latency calculation lock */
 };
 
@@ -77,13 +78,12 @@  int32_t
 rte_latencystats_update(void)
 {
 	unsigned int i;
-	float *stats_ptr = NULL;
 	uint64_t values[NUM_LATENCY_STATS] = {0};
 	int ret;
 
 	for (i = 0; i < NUM_LATENCY_STATS; i++) {
-		stats_ptr = RTE_PTR_ADD(glob_stats,
-				lat_stats_strings[i].offset);
+		const uint64_t *stats_ptr
+			= RTE_PTR_ADD(glob_stats, lat_stats_strings[i].offset);
 		values[i] = floor(*stats_ptr / cycles_per_ns);
 	}
 
@@ -100,11 +100,10 @@  static void
 rte_latencystats_fill_values(struct rte_metric_value *values)
 {
 	unsigned int i;
-	float *stats_ptr = NULL;
 
 	for (i = 0; i < NUM_LATENCY_STATS; i++) {
-		stats_ptr = RTE_PTR_ADD(glob_stats,
-				lat_stats_strings[i].offset);
+		const uint64_t *stats_ptr
+			= RTE_PTR_ADD(glob_stats, lat_stats_strings[i].offset);
 		values[i].key = i;
 		values[i].value = floor(*stats_ptr / cycles_per_ns);
 	}
@@ -151,15 +150,9 @@  calc_latency(uint16_t pid __rte_unused,
 		void *_ __rte_unused)
 {
 	unsigned int i;
-	uint64_t now;
-	float latency;
-	static float prev_latency;
-	/*
-	 * Alpha represents degree of weighting decrease in EWMA,
-	 * a constant smoothing factor between 0 and 1. The value
-	 * is used below for measuring average latency.
-	 */
-	const float alpha = 0.2;
+	uint64_t now, latency;
+	static uint64_t prev_latency;
+	static bool first_sample = true;
 
 	now = rte_rdtsc();
 
@@ -170,32 +163,41 @@  calc_latency(uint16_t pid __rte_unused,
 
 		latency = now - *timestamp_dynfield(pkts[i]);
 
-		/*
-		 * The jitter is calculated as statistical mean of interpacket
-		 * delay variation. The "jitter estimate" is computed by taking
-		 * the absolute values of the ipdv sequence and applying an
-		 * exponential filter with parameter 1/16 to generate the
-		 * estimate. i.e J=J+(|D(i-1,i)|-J)/16. Where J is jitter,
-		 * D(i-1,i) is difference in latency of two consecutive packets
-		 * i-1 and i.
-		 * Reference: Calculated as per RFC 5481, sec 4.1,
-		 * RFC 3393 sec 4.5, RFC 1889 sec.
-		 */
-		glob_stats->jitter +=  (fabsf(prev_latency - latency)
-					- glob_stats->jitter)/16;
-		if (glob_stats->min_latency == 0)
-			glob_stats->min_latency = latency;
-		else if (latency < glob_stats->min_latency)
+		if (unlikely(first_sample)) {
+			first_sample = false;
+
 			glob_stats->min_latency = latency;
-		else if (latency > glob_stats->max_latency)
 			glob_stats->max_latency = latency;
-		/*
-		 * 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 - glob_stats->avg_latency);
+			glob_stats->avg_latency = latency;
+			glob_stats->jitter = latency / 2;
+		} else {
+			/*
+			 * The jitter is calculated as statistical mean of interpacket
+			 * delay variation. The "jitter estimate" is computed by taking
+			 * the absolute values of the ipdv sequence and applying an
+			 * exponential filter with parameter 1/16 to generate the
+			 * estimate. i.e J=J+(|D(i-1,i)|-J)/16. Where J is jitter,
+			 * D(i-1,i) is difference in latency of two consecutive packets
+			 * i-1 and i.
+			 * Reference: Calculated as per RFC 5481, sec 4.1,
+			 * RFC 3393 sec 4.5, RFC 1889 sec.
+			 */
+			glob_stats->jitter += ((prev_latency - latency)
+					       - glob_stats->jitter) / 16;
+			if (latency < glob_stats->min_latency)
+				glob_stats->min_latency = latency;
+			if (latency > glob_stats->max_latency)
+				glob_stats->max_latency = latency;
+			/*
+			 * The average latency is measured using exponential moving
+			 * average, i.e. using EWMA
+			 * https://en.wikipedia.org/wiki/Moving_average
+			 *
+			 * Alpha is .25
+			 */
+			glob_stats->avg_latency += (latency - glob_stats->avg_latency) / 4;
+		}
+
 		prev_latency = latency;
 	}
 	rte_spinlock_unlock(&glob_stats->lock);