BUG/MINOR: time: frequency counters are not totally accurate

When a frontend is rate-limited to 1000 connections per second, the
effective rate measured from the client is 999/s, and connections
experience an average response time of 99.5 ms with a standard
deviation of 2 ms.

The reason for this inaccuracy is that when computing frequency
counters, we use one part of the previous value proportional to the
number of milliseconds remaining in the current second. But even the
last millisecond still uses a part of the past value, which is wrong :
since we have a 1ms resolution, the last millisecond must be dedicated
only to filling the current second.

So we slightly adjust the algorithm to use 999/1000 of the past value
during the first millisecond, and 0/1000 of the past value during the
last millisecond.  We also slightly improve the computation by computing
the remaining time instead of the current time in tv_update_date(), so
that we don't have to negate the value in each frequency counter.

Now with the fix, the connection rate measured by both the client and
haproxy is a steady 1000/s, the average response time measured is 99.2ms
and more importantly, the standard deviation has been divided by 3 to
0.6 millisecond.

This fix should also be backported to 1.4 which has the same issue.

diff --git a/include/common/time.h b/include/common/time.h
index 588180d20a5aef2ff3518c6ec77239959bba558b..5d86ad53ed04199927c1166f2db5433075d890f2 100644 (file)
--- a/include/common/time.h
+++ b/include/common/time.h
@@ -50,6 +50,7 @@
 #define SETNOW(a)              (*a=now)
 
 extern unsigned int   curr_sec_ms;      /* millisecond of current second (0..999) */
+extern unsigned int   ms_left_scaled;   /* milliseconds left for current second (0..2^32-1) */
 extern unsigned int   curr_sec_ms_scaled;  /* millisecond of current second (0..2^32-1) */
 extern unsigned int   now_ms;           /* internal date in milliseconds (may wrap) */
 extern unsigned int   samp_time;        /* total elapsed time over current sample */

diff --git a/src/freq_ctr.c b/src/freq_ctr.c
index 28f59369cfd62849ab83463e8e42ce33ce2d113d..6dec970024973d486d7b3e708e094fe263cc920b 100644 (file)
--- a/src/freq_ctr.c
+++ b/src/freq_ctr.c
@@ -47,7 +47,7 @@ unsigned int read_freq_ctr(struct freq_ctr *ctr)
        if (past <= 1 && !curr)
                return past; /* very low rate, avoid flapping */
 
-       return curr + mul32hi(past, ~curr_sec_ms_scaled);
+       return curr + mul32hi(past, ms_left_scaled);
 }
 
 /* returns the number of remaining events that can occur on this freq counter
@@ -59,7 +59,6 @@ unsigned int freq_ctr_remain(struct freq_ctr *ctr, unsigned int freq, unsigned i
        unsigned int curr, past;
        unsigned int age;
 
-       past = 0;
        curr = 0;               
        age = now.tv_sec - ctr->curr_sec;
 
@@ -69,7 +68,7 @@ unsigned int freq_ctr_remain(struct freq_ctr *ctr, unsigned int freq, unsigned i
                        curr = past;
                        past = ctr->prev_ctr;
                }
-               curr += mul32hi(past, ~curr_sec_ms_scaled);
+               curr += mul32hi(past, ms_left_scaled);
        }
        curr += pend;
 
@@ -99,7 +98,7 @@ unsigned int next_event_delay(struct freq_ctr *ctr, unsigned int freq, unsigned
                        curr = past;
                        past = ctr->prev_ctr;
                }
-               curr += mul32hi(past, ~curr_sec_ms_scaled);
+               curr += mul32hi(past, ms_left_scaled);
        }
        curr += pend;
 

diff --git a/src/time.c b/src/time.c
index 4149316d191e80a69d504db2ea4f8c653f368bb0..61187628f8f61a12dfbd8b40e3460394da20c7dd 100644 (file)
--- a/src/time.c
+++ b/src/time.c
@@ -16,8 +16,8 @@
 #include <common/standard.h>
 #include <common/time.h>
 
-unsigned int   curr_sec_ms;      /* millisecond of current second (0..999) */
-unsigned int   curr_sec_ms_scaled;  /* millisecond of current second (0..2^32-1) */
+unsigned int   curr_sec_ms;     /* millisecond of current second (0..999) */
+unsigned int   ms_left_scaled;  /* milliseconds left for current second (0..2^32-1) */
 unsigned int   now_ms;          /* internal date in milliseconds (may wrap) */
 unsigned int   samp_time;       /* total elapsed time over current sample */
 unsigned int   idle_time;       /* total idle time over current sample */
@@ -203,7 +203,17 @@ REGPRM2 void tv_update_date(int max_wait, int interrupted)
  to_ms:
        now = adjusted;
        curr_sec_ms = now.tv_usec / 1000;            /* ms of current second */
-       curr_sec_ms_scaled = curr_sec_ms * 4294971;  /* ms * 2^32 / 1000 */
+
+       /* For frequency counters, we'll need to know the ratio of the previous
+        * value to add to current value depending on the current millisecond.
+        * The principle is that during the first millisecond, we use 999/1000
+        * of the past value and that during the last millisecond we use 0/1000
+        * of the past value. In summary, we only use the past value during the
+        * first 999 ms of a second, and the last ms is used to complete the
+        * current measure. The value is scaled to (2^32-1) so that a simple
+        * multiply followed by a shift gives us the final value.
+        */
+       ms_left_scaled = (999U - curr_sec_ms) * 4294967U;
        now_ms = now.tv_sec * 1000 + curr_sec_ms;
        return;
 }