Merge remote-tracking branch 'pastly/bug23552_032_03'

changes/bug23552

@@ -0,0 +1,3 @@
+  o Minor bugfixes (scheduler):
+    - Only notice log the selected scheduler when we switch scheduler types.
+      Fixes bug 23552; bugfix on 0.3.2.1-alpha.

src/or/scheduler.c

@@ -174,6 +174,25 @@ STATIC struct event *run_sched_ev = NULL;
  * Functions that can only be accessed from this file.
  *****************************************************************************/
 
+/** Return a human readable string for the given scheduler type. */
+static const char *
+get_scheduler_type_string(scheduler_types_t type)
+{
+  switch(type) {
+  case SCHEDULER_VANILLA:
+    return "Vanilla";
+  case SCHEDULER_KIST:
+    return "KIST";
+  case SCHEDULER_KIST_LITE:
+    return "KISTLite";
+  case SCHEDULER_NONE:
+    /* fallthrough */
+  default:
+    tor_assert_unreached();
+    return "(N/A)";
+  }
+}
+
 /**
  * Scheduler event callback; this should get triggered once per event loop
  * if any scheduling work was created during the event loop.
@@ -204,65 +223,6 @@ scheduler_evt_callback(evutil_socket_t fd, short events, void *arg)
   the_scheduler->schedule();
 }
 
-/*****************************************************************************
- * Scheduling system private function definitions
- *
- * Functions that can only be accessed from scheduler*.c
- *****************************************************************************/
-
-/** Return the pending channel list. */
-smartlist_t *
-get_channels_pending(void)
-{
-  return channels_pending;
-}
-
-/** Comparison function to use when sorting pending channels. */
-MOCK_IMPL(int,
-scheduler_compare_channels, (const void *c1_v, const void *c2_v))
-{
-  const channel_t *c1 = NULL, *c2 = NULL;
-  /* These are a workaround for -Wbad-function-cast throwing a fit */
-  const circuitmux_policy_t *p1, *p2;
-  uintptr_t p1_i, p2_i;
-
-  tor_assert(c1_v);
-  tor_assert(c2_v);
-
-  c1 = (const channel_t *)(c1_v);
-  c2 = (const channel_t *)(c2_v);
-
-  if (c1 != c2) {
-    if (circuitmux_get_policy(c1->cmux) ==
-        circuitmux_get_policy(c2->cmux)) {
-      /* Same cmux policy, so use the mux comparison */
-      return circuitmux_compare_muxes(c1->cmux, c2->cmux);
-    } else {
-      /*
-       * Different policies; not important to get this edge case perfect
-       * because the current code never actually gives different channels
-       * different cmux policies anyway.  Just use this arbitrary but
-       * definite choice.
-       */
-      p1 = circuitmux_get_policy(c1->cmux);
-      p2 = circuitmux_get_policy(c2->cmux);
-      p1_i = (uintptr_t)p1;
-      p2_i = (uintptr_t)p2;
-
-      return (p1_i < p2_i) ? -1 : 1;
-    }
-  } else {
-    /* c1 == c2, so always equal */
-    return 0;
-  }
-}
-
-/*****************************************************************************
- * Scheduling system global functions
- *
- * Functions that can be accessed from anywhere in Tor.
- *****************************************************************************/
-
 /** Using the global options, select the scheduler we should be using. */
 static void
 select_scheduler(void)
@@ -312,6 +272,8 @@ select_scheduler(void)
       new_scheduler = get_kist_scheduler();
       scheduler_kist_set_lite_mode();
       goto end;
+    case SCHEDULER_NONE:
+      /* fallthrough */
     default:
       /* Our option validation should have caught this. */
       tor_assert_unreached();
@@ -333,8 +295,6 @@ select_scheduler(void)
 
   /* Set the chosen scheduler. */
   the_scheduler = new_scheduler;
-  log_notice(LD_CONFIG, "Scheduler type %s has been enabled.",
-             chosen_sched_type);
 }
 
 /**
@@ -346,11 +306,21 @@ static void
 set_scheduler(void)
 {
   const scheduler_t *old_scheduler = the_scheduler;
+  scheduler_types_t old_scheduler_type = SCHEDULER_NONE;
+
+  /* We keep track of the type in order to log only if the type switched. We
+   * can't just use the scheduler pointers because KIST and KISTLite share the
+   * same object. */
+  if (the_scheduler) {
+    old_scheduler_type = the_scheduler->type;
+  }
 
   /* From the options, select the scheduler type to set. */
   select_scheduler();
   tor_assert(the_scheduler);
 
+  /* We look at the pointer difference in case the old sched and new sched
+   * share the same scheduler object, as is the case with KIST and KISTLite. */
   if (old_scheduler != the_scheduler) {
     /* Allow the old scheduler to clean up, if needed. */
     if (old_scheduler && old_scheduler->free_all) {
@@ -362,8 +332,74 @@ set_scheduler(void)
       the_scheduler->init();
     }
   }
+
+  /* Finally we notice log if we switched schedulers. We use the type in case
+   * two schedulers share a scheduler object. */
+  if (old_scheduler_type != the_scheduler->type) {
+    log_notice(LD_CONFIG, "Scheduler type %s has been enabled.",
+               get_scheduler_type_string(the_scheduler->type));
+  }
+}
+
+/*****************************************************************************
+ * Scheduling system private function definitions
+ *
+ * Functions that can only be accessed from scheduler*.c
+ *****************************************************************************/
+
+/** Return the pending channel list. */
+smartlist_t *
+get_channels_pending(void)
+{
+  return channels_pending;
 }
 
+/** Comparison function to use when sorting pending channels. */
+MOCK_IMPL(int,
+scheduler_compare_channels, (const void *c1_v, const void *c2_v))
+{
+  const channel_t *c1 = NULL, *c2 = NULL;
+  /* These are a workaround for -Wbad-function-cast throwing a fit */
+  const circuitmux_policy_t *p1, *p2;
+  uintptr_t p1_i, p2_i;
+
+  tor_assert(c1_v);
+  tor_assert(c2_v);
+
+  c1 = (const channel_t *)(c1_v);
+  c2 = (const channel_t *)(c2_v);
+
+  if (c1 != c2) {
+    if (circuitmux_get_policy(c1->cmux) ==
+        circuitmux_get_policy(c2->cmux)) {
+      /* Same cmux policy, so use the mux comparison */
+      return circuitmux_compare_muxes(c1->cmux, c2->cmux);
+    } else {
+      /*
+       * Different policies; not important to get this edge case perfect
+       * because the current code never actually gives different channels
+       * different cmux policies anyway.  Just use this arbitrary but
+       * definite choice.
+       */
+      p1 = circuitmux_get_policy(c1->cmux);
+      p2 = circuitmux_get_policy(c2->cmux);
+      p1_i = (uintptr_t)p1;
+      p2_i = (uintptr_t)p2;
+
+      return (p1_i < p2_i) ? -1 : 1;
+    }
+  } else {
+    /* c1 == c2, so always equal */
+    return 0;
+  }
+}
+
+/*****************************************************************************
+ * Scheduling system global functions
+ *
+ * Functions that can be accessed from anywhere in Tor.
+ *****************************************************************************/
+
 /**
  * This is how the scheduling system is notified of Tor's configuration
  * changing. For example: a SIGHUP was issued.

src/or/scheduler.h

@@ -13,7 +13,17 @@
 #include "channel.h"
 #include "testsupport.h"
 
-/*
+/** Scheduler type, we build an ordered list with those values from the
+ * parsed strings in Schedulers. The reason to do such a thing is so we can
+ * quickly and without parsing strings select the scheduler at anytime. */
+typedef enum {
+  SCHEDULER_NONE =     -1,
+  SCHEDULER_VANILLA =   1,
+  SCHEDULER_KIST =      2,
+  SCHEDULER_KIST_LITE = 3,
+} scheduler_types_t;
+
+/**
  * A scheduler implementation is a collection of function pointers. If you
  * would like to add a new scheduler called foo, create scheduler_foo.c,
  * implement at least the mandatory ones, and implement get_foo_scheduler()
@@ -31,6 +41,10 @@
  * is shutting down), then set that function pointer to NULL.
  */
 typedef struct scheduler_s {
+  /* Scheduler type. This is used for logging when the scheduler is switched
+   * during runtime. */
+  scheduler_types_t type;
+
   /* (Optional) To be called when we want to prepare a scheduler for use.
    * Perhaps Tor just started and we are the lucky chosen scheduler, or
    * perhaps Tor is switching to this scheduler. No matter the case, this is
@@ -82,15 +96,6 @@ typedef struct scheduler_s {
   void (*on_new_options)(void);
 } scheduler_t;
 
-/** Scheduler type, we build an ordered list with those values from the
- * parsed strings in Schedulers. The reason to do such a thing is so we can
- * quickly and without parsing strings select the scheduler at anytime. */
-typedef enum {
-  SCHEDULER_VANILLA =   1,
-  SCHEDULER_KIST =      2,
-  SCHEDULER_KIST_LITE = 3,
-} scheduler_types_t;
-
 /*****************************************************************************
  * Globally visible scheduler variables/values
  *

src/or/scheduler_kist.c

@@ -687,6 +687,7 @@ kist_scheduler_run(void)
 
 /* Stores the kist scheduler function pointers. */
 static scheduler_t kist_scheduler = {
+  .type = SCHEDULER_KIST,
   .free_all = kist_free_all,
   .on_channel_free = kist_on_channel_free,
   .init = kist_scheduler_init,
@@ -738,6 +739,7 @@ void
 scheduler_kist_set_lite_mode(void)
 {
   kist_lite_mode = 1;
+  kist_scheduler.type = SCHEDULER_KIST_LITE;
   log_info(LD_SCHED,
            "Setting KIST scheduler without kernel support (KISTLite mode)");
 }
@@ -747,6 +749,7 @@ void
 scheduler_kist_set_full_mode(void)
 {
   kist_lite_mode = 0;
+  kist_scheduler.type = SCHEDULER_KIST;
   log_info(LD_SCHED,
            "Setting KIST scheduler with kernel support (KIST mode)");
 }

src/or/scheduler_vanilla.c

@@ -179,6 +179,7 @@ vanilla_scheduler_run(void)
 
 /* Stores the vanilla scheduler function pointers. */
 static scheduler_t vanilla_scheduler = {
+  .type = SCHEDULER_VANILLA,
   .free_all = NULL,
   .on_channel_free = NULL,
   .init = NULL,