sched: Make KISTSchedRunInterval non negative

Fixes #23539.

Signed-off-by: David Goulet <dgoulet@torproject.org>

changes/bug23539

@@ -0,0 +1,4 @@
+  o Minor bugfixes (scheduler, KIST):
+    - Make the KISTSchedRunInterval option a non negative value. With this,
+      the way to disable KIST through the consensus is to set it to 0.
+      Fixes bug 23539; bugfix on 0.3.2.1-alpha.

src/or/or.h

@@ -4615,7 +4615,7 @@ typedef struct {
    * not use the KIST scheduler but use the old vanilla scheduler instead. If
    * zero, do what the consensus says and fall back to using KIST as if this is
    * set to "10 msec" if the consensus doesn't say anything. */
-  int64_t KISTSchedRunInterval;
+  uint32_t KISTSchedRunInterval;
 
   /** A multiplier for the KIST per-socket limit calculation. */
   double KISTSockBufSizeFactor;

src/or/scheduler.c

@@ -249,13 +249,8 @@ select_scheduler(void)
     case SCHEDULER_KIST:
       if (!scheduler_can_use_kist()) {
 #ifdef HAVE_KIST_SUPPORT
-        if (get_options()->KISTSchedRunInterval == -1) {
-          log_info(LD_SCHED, "Scheduler type KIST can not be used. It is "
-                             "disabled because KISTSchedRunInterval=-1");
-        } else {
         log_notice(LD_SCHED, "Scheduler type KIST has been disabled by "
-                               "the consensus.");
-        }
+                             "the consensus or no kernel support.");
 #else /* !(defined(HAVE_KIST_SUPPORT)) */
         log_info(LD_SCHED, "Scheduler type KIST not built in");
 #endif /* defined(HAVE_KIST_SUPPORT) */

src/or/scheduler.h

@@ -189,7 +189,7 @@ int scheduler_can_use_kist(void);
 void scheduler_kist_set_full_mode(void);
 void scheduler_kist_set_lite_mode(void);
 scheduler_t *get_kist_scheduler(void);
-int32_t kist_scheduler_run_interval(const networkstatus_t *ns);
+uint32_t kist_scheduler_run_interval(const networkstatus_t *ns);
 
 #ifdef TOR_UNIT_TESTS
 extern int32_t sched_run_interval;

src/or/scheduler_kist.c

@@ -208,8 +208,8 @@ update_socket_info_impl, (socket_table_ent_t *ent))
        * support. */
       log_notice(LD_SCHED, "Looks like our kernel doesn't have the support "
                            "for KIST anymore. We will fallback to the naive "
-                           "approach. Set KISTSchedRunInterval=-1 to disable "
-                           "KIST.");
+                           "approach. Remove KIST from the Schedulers list "
+                           "to disable.");
       kist_no_kernel_support = 1;
     }
     goto fallback;
@@ -218,8 +218,8 @@ update_socket_info_impl, (socket_table_ent_t *ent))
     if (errno == EINVAL) {
       log_notice(LD_SCHED, "Looks like our kernel doesn't have the support "
                            "for KIST anymore. We will fallback to the naive "
-                           "approach. Set KISTSchedRunInterval=-1 to disable "
-                           "KIST.");
+                           "approach. Remove KIST from the Schedulers list "
+                           "to disable.");
       /* Same reason as the above. */
       kist_no_kernel_support = 1;
     }
@@ -491,7 +491,7 @@ static void
 kist_scheduler_init(void)
 {
   kist_scheduler_on_new_options();
-  IF_BUG_ONCE(sched_run_interval <= 0) {
+  IF_BUG_ONCE(sched_run_interval == 0) {
     log_warn(LD_SCHED, "We are initing the KIST scheduler and noticed the "
              "KISTSchedRunInterval is telling us to not use KIST. That's "
              "weird! We'll continue using KIST, but at %dms.",
@@ -705,33 +705,34 @@ get_kist_scheduler(void)
   return &kist_scheduler;
 }
 
-/* Check the torrc for the configured KIST scheduler run interval.
- * - If torrc < 0, then return the negative torrc value (shouldn't even be
- *   using KIST)
+/* Check the torrc (and maybe consensus) for the configured KIST scheduler run
+ * interval.
  * - If torrc > 0, then return the positive torrc value (should use KIST, and
  *   should use the set value)
  * - If torrc == 0, then look in the consensus for what the value should be.
- *   - If == 0, then return -1 (don't use KIST)
+ *   - If == 0, then return 0 (don't use KIST)
  *   - If > 0, then return the positive consensus value
- *   - If consensus doesn't say anything, return 10 milliseconds
+ *   - If consensus doesn't say anything, return 10 milliseconds, default.
  */
-int32_t
+uint32_t
 kist_scheduler_run_interval(const networkstatus_t *ns)
 {
-  int32_t run_interval = (int32_t)get_options()->KISTSchedRunInterval;
+  uint32_t run_interval = get_options()->KISTSchedRunInterval;
+
   if (run_interval != 0) {
-    log_debug(LD_SCHED, "Found KISTSchedRunInterval in torrc. Using that.");
+    log_debug(LD_SCHED, "Found KISTSchedRunInterval=%" PRIu32 " in torrc. "
+                        "Using that.", run_interval);
     return run_interval;
   }
 
-  log_debug(LD_SCHED, "Turning to the consensus for KISTSchedRunInterval");
-  run_interval = networkstatus_get_param(ns, "KISTSchedRunInterval",
+  log_debug(LD_SCHED, "KISTSchedRunInterval=0, turning to the consensus.");
+
+  /* Will either be the consensus value or the default. Note that 0 can be
+   * returned which means the consensus wants us to NOT use KIST. */
+  return networkstatus_get_param(ns, "KISTSchedRunInterval",
                                  KIST_SCHED_RUN_INTERVAL_DEFAULT,
                                  KIST_SCHED_RUN_INTERVAL_MIN,
                                  KIST_SCHED_RUN_INTERVAL_MAX);
-  if (run_interval <= 0)
-    return -1;
-  return run_interval;
 }
 
 /* Set KISTLite mode that is KIST without kernel support. */
@@ -767,9 +768,9 @@ scheduler_can_use_kist(void)
 
   /* We do have the support, time to check if we can get the interval that the
    * consensus can be disabling. */
-  int64_t run_interval = kist_scheduler_run_interval(NULL);
+  uint32_t run_interval = kist_scheduler_run_interval(NULL);
   log_debug(LD_SCHED, "Determined KIST sched_run_interval should be "
-                      "%" PRId64 ". Can%s use KIST.",
+                      "%" PRIu32 ". Can%s use KIST.",
            run_interval, (run_interval > 0 ? "" : " not"));
   return run_interval > 0;
 }

src/test/test_scheduler.c

@@ -84,7 +84,7 @@ mock_vanilla_networkstatus_get_param(
   (void)max_val;
   // only support KISTSchedRunInterval right now
   tor_assert(strcmp(param_name, "KISTSchedRunInterval")==0);
-  return -1;
+  return 0;
 }
 
 static int32_t
@@ -628,7 +628,7 @@ test_scheduler_loop_vanilla(void *arg)
   MOCK(get_options, mock_get_options);
   clear_options();
   set_scheduler_options(SCHEDULER_VANILLA);
-  mocked_options.KISTSchedRunInterval = -1;
+  mocked_options.KISTSchedRunInterval = 0;
 
   /* Set up libevent and scheduler */
 
@@ -932,14 +932,6 @@ test_scheduler_can_use_kist(void *arg)
   int res_should, res_freq;
   MOCK(get_options, mock_get_options);
 
-  /* Test force disabling of KIST */
-  clear_options();
-  mocked_options.KISTSchedRunInterval = -1;
-  res_should = scheduler_can_use_kist();
-  res_freq = kist_scheduler_run_interval(NULL);
-  tt_int_op(res_should, ==, 0);
-  tt_int_op(res_freq, ==, -1);
-
   /* Test force enabling of KIST */
   clear_options();
   mocked_options.KISTSchedRunInterval = 1234;
@@ -985,7 +977,7 @@ test_scheduler_can_use_kist(void *arg)
   res_should = scheduler_can_use_kist();
   res_freq = kist_scheduler_run_interval(NULL);
   tt_int_op(res_should, ==, 0);
-  tt_int_op(res_freq, ==, -1);
+  tt_int_op(res_freq, ==, 0);
   UNMOCK(networkstatus_get_param);
 
  done: