From 7a0e90a468dadf3501df3bee5e8aa449f224e63f Mon Sep 17 00:00:00 2001
From: Sylvain Joyeux <sylvain.joyeux@13robotics.com>
Date: Tue, 15 Jan 2019 09:58:18 -0200
Subject: [PATCH] fix handling of negative priorities

This match dynamically modifies the timer thread period to account
for negative priorities. Until now, the VM would not switch away
from threads with negative priorities at the expected period,
since the timer thread would be always waiting for a static
TIME_QUANTUM_USEC of 100ms.

To test this, I wrote a script that simply samples time and priority
in two threads and displays the expected and actual periods. Before
this patch, the result would be:

1 ran for 93.61 ms, expected 50.00 ms (prio=-1)
0 ran for 99.33 ms, expected 100.00 ms (prio=0)
1 ran for 100.08 ms, expected 50.00 ms (prio=-1)
0 ran for 100.10 ms, expected 100.00 ms (prio=0)
1 ran for 100.06 ms, expected 50.00 ms (prio=-1)
0 ran for 100.15 ms, expected 100.00 ms (prio=0)
1 ran for 99.98 ms, expected 50.00 ms (prio=-1)
0 ran for 100.09 ms, expected 100.00 ms (prio=0)
1 ran for 143.08 ms, expected 50.00 ms (prio=-1)
0 ran for 57.19 ms, expected 100.00 ms (prio=0)
1 ran for 99.95 ms, expected 25.00 ms (prio=-2)
0 ran for 136.81 ms, expected 50.00 ms (prio=-1)
1 ran for 63.32 ms, expected 25.00 ms (prio=-2)
0 ran for 100.10 ms, expected 50.00 ms (prio=-1)
1 ran for 100.04 ms, expected 25.00 ms (prio=-2)
0 ran for 100.09 ms, expected 50.00 ms (prio=-1)
1 ran for 100.49 ms, expected 25.00 ms (prio=-2)
0 ran for 130.23 ms, expected 50.00 ms (prio=-1)
1 ran for 69.48 ms, expected 25.00 ms (prio=-2)
0 ran for 100.09 ms, expected 50.00 ms (prio=-1)
1 ran for 176.32 ms, expected 25.00 ms (prio=-2)
0 ran for 23.80 ms, expected 50.00 ms (prio=-1)
1 ran for 100.04 ms, expected 12.00 ms (prio=-3)
0 ran for 203.00 ms, expected 200.00 ms (prio=1)
1 ran for 193.71 ms, expected 12.00 ms (prio=-3)
0 ran for 103.58 ms, expected 200.00 ms (prio=1)
1 ran for 34.62 ms, expected 12.00 ms (prio=-3)
0 ran for 106.44 ms, expected 200.00 ms (prio=1)
1 ran for 99.02 ms, expected 12.00 ms (prio=-3)
0 ran for 200.10 ms, expected 200.00 ms (prio=1)

After the patch:

1 ran for 50.13 ms, expected 50.00 ms (prio=-1)
0 ran for 100.26 ms, expected 100.00 ms (prio=0)
1 ran for 50.01 ms, expected 50.00 ms (prio=-1)
0 ran for 100.19 ms, expected 100.00 ms (prio=0)
1 ran for 59.21 ms, expected 50.00 ms (prio=-1)
0 ran for 90.90 ms, expected 100.00 ms (prio=0)
1 ran for 65.29 ms, expected 50.00 ms (prio=-1)
0 ran for 84.13 ms, expected 100.00 ms (prio=0)
1 ran for 25.91 ms, expected 50.00 ms (prio=-1)
0 ran for 100.21 ms, expected 100.00 ms (prio=0)
1 ran for 50.04 ms, expected 50.00 ms (prio=-1)
0 ran for 100.20 ms, expected 100.00 ms (prio=0)
1 ran for 50.22 ms, expected 50.00 ms (prio=-1)
0 ran for 100.14 ms, expected 50.00 ms (prio=-1)
1 ran for 25.08 ms, expected 25.00 ms (prio=-2)
0 ran for 50.19 ms, expected 50.00 ms (prio=-1)
1 ran for 25.08 ms, expected 25.00 ms (prio=-2)
0 ran for 50.20 ms, expected 50.00 ms (prio=-1)
1 ran for 25.10 ms, expected 25.00 ms (prio=-2)
0 ran for 107.87 ms, expected 50.00 ms (prio=-1)
1 ran for 17.47 ms, expected 25.00 ms (prio=-2)
0 ran for 50.10 ms, expected 50.00 ms (prio=-1)
1 ran for 25.03 ms, expected 25.00 ms (prio=-2)
0 ran for 50.18 ms, expected 50.00 ms (prio=-1)
1 ran for 25.15 ms, expected 25.00 ms (prio=-2)
0 ran for 28.81 ms, expected 50.00 ms (prio=-1)
1 ran for 17.38 ms, expected 25.00 ms (prio=-2)
0 ran for 49.98 ms, expected 50.00 ms (prio=-1)
1 ran for 24.96 ms, expected 25.00 ms (prio=-2)
0 ran for 50.18 ms, expected 50.00 ms (prio=-1)
1 ran for 25.10 ms, expected 25.00 ms (prio=-2)
0 ran for 50.20 ms, expected 50.00 ms (prio=-1)
1 ran for 32.38 ms, expected 25.00 ms (prio=-2)
0 ran for 42.85 ms, expected 50.00 ms (prio=-1)
1 ran for 25.09 ms, expected 25.00 ms (prio=-2)
0 ran for 100.35 ms, expected 50.00 ms (prio=-1)
1 ran for 25.19 ms, expected 25.00 ms (prio=-2)
0 ran for 207.48 ms, expected 200.00 ms (prio=1)
1 ran for 11.33 ms, expected 12.00 ms (prio=-3)
0 ran for 201.17 ms, expected 200.00 ms (prio=1)
1 ran for 12.58 ms, expected 12.00 ms (prio=-3)
0 ran for 326.26 ms, expected 200.00 ms (prio=1)
1 ran for 1.43 ms, expected 12.00 ms (prio=-3)
0 ran for 226.93 ms, expected 200.00 ms (prio=1)
1 ran for 12.60 ms, expected 12.00 ms (prio=-3)
---
 thread.c         | 27 ++++++++++++++++-----------
 thread_pthread.c | 20 +++++++++++++++++---
 thread_win32.c   |  6 +++++-
 3 files changed, 38 insertions(+), 15 deletions(-)

diff --git a/thread.c b/thread.c
index 58e7651446..97003348ec 100644
--- a/thread.c
+++ b/thread.c
@@ -80,7 +80,7 @@
 #ifndef USE_NATIVE_THREAD_PRIORITY
 #define USE_NATIVE_THREAD_PRIORITY 0
 #define RUBY_THREAD_PRIORITY_MAX 3
-#define RUBY_THREAD_PRIORITY_MIN -3
+#define RUBY_THREAD_PRIORITY_MIN -5
 #endif
 
 #ifndef THREAD_DEBUG
@@ -341,6 +341,8 @@ ubf_sigwait(void *ignore)
     rb_thread_wakeup_timer_thread(0);
 }
 
+static int thread_time_quantum_usec_from_priority(int priority);
+
 #if   defined(_WIN32)
 #include "thread_win32.c"
 
@@ -364,6 +366,15 @@ ubf_sigwait(void *ignore)
 #error "unsupported thread type"
 #endif
 
+static int thread_time_quantum_usec_from_priority(int priority) {
+    if (priority > 0) {
+        return TIME_QUANTUM_USEC_BASE << priority;
+    }
+    else {
+        return TIME_QUANTUM_USEC_BASE >> -priority;
+    }
+}
+
 /*
  * TODO: somebody with win32 knowledge should be able to get rid of
  * timer-thread by busy-waiting on signals.  And it should be possible
@@ -2216,12 +2227,7 @@ rb_threadptr_execute_interrupts(rb_thread_t *th, int blocking_timing)
 	}
 
 	if (timer_interrupt) {
-	    uint32_t limits_us = TIME_QUANTUM_USEC;
-
-	    if (th->priority > 0)
-		limits_us <<= th->priority;
-	    else
-		limits_us >>= -th->priority;
+	    uint32_t limits_us = thread_time_quantum_usec_from_priority(th->priority);
 
 	    if (th->status == THREAD_RUNNABLE)
 		th->running_time_us += TIME_QUANTUM_USEC;
@@ -3600,6 +3606,7 @@ rb_thread_priority_set(VALUE thread, VALUE prio)
     else if (priority < RUBY_THREAD_PRIORITY_MIN) {
 	priority = RUBY_THREAD_PRIORITY_MIN;
     }
+    native_update_quantum(priority);
     target_th->priority = (int8_t)priority;
 #endif
     return INT2NUM(target_th->priority);
@@ -3888,12 +3895,10 @@ static const rb_hrtime_t *
 sigwait_timeout(rb_thread_t *th, int sigwait_fd, const rb_hrtime_t *orig,
                 int *drained_p)
 {
-    static const rb_hrtime_t quantum = TIME_QUANTUM_USEC * 1000;
-
     if (sigwait_fd >= 0 && (!ubf_threads_empty() || BUSY_WAIT_SIGNALS)) {
         *drained_p = check_signals_nogvl(th, sigwait_fd);
-        if (!orig || *orig > quantum)
-            return &quantum;
+        if (!orig || *orig > TIME_QUANTUM_NSEC)
+            return &TIME_QUANTUM_NSEC;
     }
 
     return orig;
diff --git a/thread_pthread.c b/thread_pthread.c
index e074a854f5..eda8cd95ac 100644
--- a/thread_pthread.c
+++ b/thread_pthread.c
@@ -175,12 +175,26 @@ static const void *const condattr_monotonic = NULL;
 /* 100ms.  10ms is too small for user level thread scheduling
  * on recent Linux (tested on 2.6.35)
  */
-#define TIME_QUANTUM_MSEC (100)
-#define TIME_QUANTUM_USEC (TIME_QUANTUM_MSEC * 1000)
-#define TIME_QUANTUM_NSEC (TIME_QUANTUM_USEC * 1000)
+#define TIME_QUANTUM_USEC_BASE (100 * 1000)
+static rb_hrtime_t TIME_QUANTUM_MSEC = TIME_QUANTUM_USEC_BASE / 1000;
+static rb_hrtime_t TIME_QUANTUM_USEC = TIME_QUANTUM_USEC_BASE;
+static rb_hrtime_t TIME_QUANTUM_NSEC = TIME_QUANTUM_USEC_BASE * 1000;
 
 static rb_hrtime_t native_cond_timeout(rb_nativethread_cond_t *, rb_hrtime_t);
 
+static void
+native_update_quantum(int priority)
+{
+    rb_hrtime_t new_quantum_usec;
+    new_quantum_usec = thread_time_quantum_usec_from_priority(priority);
+    if (TIME_QUANTUM_USEC > new_quantum_usec)
+    {
+        TIME_QUANTUM_USEC = new_quantum_usec;
+        TIME_QUANTUM_MSEC = new_quantum_usec / 1000;
+        TIME_QUANTUM_NSEC = new_quantum_usec * 1000;
+    }
+}
+
 /*
  * Designate the next gvl.timer thread, favor the last thread in
  * the waitq since it will be in waitq longest
diff --git a/thread_win32.c b/thread_win32.c
index d4db9e3824..89a38986c3 100644
--- a/thread_win32.c
+++ b/thread_win32.c
@@ -13,9 +13,13 @@
 
 #include <process.h>
 
-#define TIME_QUANTUM_USEC (10 * 1000)
+#define TIME_QUANTUM_USEC_BASE (100 * 1000)
+#define TIME_QUANTUM_USEC TIME_QUANTUM_USEC
+static const rb_hrtime_t TIME_QUANTUM_NSEC = &TIME_QUANTUM_USEC * 1000;
 #define RB_CONDATTR_CLOCK_MONOTONIC 1 /* no effect */
 
+void native_update_quantum(int priority) { }
+
 #undef Sleep
 
 #define native_thread_yield() Sleep(0)
-- 
2.17.1

