Opened 2 years ago

Closed 2 years ago

#23538 closed defect (wontfix)

Allow KISTSchedRunInterval to be negative

Reported by: pastly Owned by: dgoulet
Priority: Medium Milestone: Tor: 0.3.2.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-sched, easy
Cc: dgoulet Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

We use negative values to indicate "don't use KIST."

Child Tickets

Change History (7)

comment:1 Changed 2 years ago by pastly

Component: - Select a componentCore Tor/Tor

comment:2 Changed 2 years ago by pastly

diff --git a/src/or/config.c b/src/or/config.c
index 4ee140381..70203a801 100644
--- a/src/or/config.c
+++ b/src/or/config.c
@@ -491,7 +491,7 @@ static config_var_t option_vars_[] = {
   OBSOLETE("SchedulerLowWaterMark__"),
   OBSOLETE("SchedulerHighWaterMark__"),
   OBSOLETE("SchedulerMaxFlushCells__"),
-  V(KISTSchedRunInterval,        MSEC_INTERVAL, "0 msec"),
+  V(KISTSchedRunInterval,        INT, "0 msec"),
   V(KISTSockBufSizeFactor,       DOUBLE,   "1.0"),
   V(Schedulers,                  CSV,      "KIST,KISTLite,Vanilla"),
   V(ShutdownWaitLength,          INTERVAL, "30 seconds"),
@@ -2974,7 +2974,7 @@ options_validate_scheduler(or_options_t *options, char **msg)

   /* Don't need to validate that the Interval is less than anything because
    * zero is valid and all negative values are valid. */
-  if (options->KISTSchedRunInterval > KIST_SCHED_RUN_INTERVAL_MAX) {
+  if ((int)options->KISTSchedRunInterval > KIST_SCHED_RUN_INTERVAL_MAX) {
     tor_asprintf(msg, "KISTSchedRunInterval must not be more than %d (ms)",
                  KIST_SCHED_RUN_INTERVAL_MAX);
     return -1;

This is all that is needed if to allow negative values. HOWEVER. Actually using one leads to

Sep 18 11:06:22.897 [notice] Scheduler type KIST has been disabled by the consensus.
Sep 18 11:06:22.897 [notice] Scheduler type KISTLite has been enabled.
Sep 18 11:06:22.897 [warn] tor_bug_occurred_(): Bug: src/or/scheduler_kist.c:495: kist_scheduler_init: Non-fatal assertion !((sched_run_interval <= 0)) failed. (Future instances of this warning will be silenced.) (on Tor 0.3.2.0-alpha-dev c7af923567bca5b0)
Sep 18 11:06:22.897 [warn] Bug: Non-fatal assertion !((sched_run_interval <= 0)) failed in kist_scheduler_init at src/or/scheduler_kist.c:495. Stack trace: (on Tor 0.3.2.0-alpha-dev c7af923567bca5b0)
Sep 18 11:06:22.897 [warn] Bug:     ./src/or/tor(log_backtrace+0x42) [0x55b43214dcc2] (on Tor 0.3.2.0-alpha-dev c7af923567bca5b0)
Sep 18 11:06:22.897 [warn] Bug:     ./src/or/tor(tor_bug_occurred_+0xb9) [0x55b432168df9] (on Tor 0.3.2.0-alpha-dev c7af923567bca5b0)
Sep 18 11:06:22.897 [warn] Bug:     ./src/or/tor(+0xb8a0f) [0x55b432084a0f] (on Tor 0.3.2.0-alpha-dev c7af923567bca5b0)
Sep 18 11:06:22.897 [warn] Bug:     ./src/or/tor(set_options+0x17d6) [0x55b4320c6166] (on Tor 0.3.2.0-alpha-dev c7af923567bca5b0)
Sep 18 11:06:22.897 [warn] Bug:     ./src/or/tor(options_init_from_string+0x34a) [0x55b4320c71ca] (on Tor 0.3.2.0-alpha-dev c7af923567bca5b0)
Sep 18 11:06:22.897 [warn] Bug:     ./src/or/tor(options_init_from_torrc+0x1be) [0x55b4320c754e] (on Tor 0.3.2.0-alpha-dev c7af923567bca5b0)
Sep 18 11:06:22.897 [warn] Bug:     ./src/or/tor(tor_init+0x303) [0x55b43201e283] (on Tor 0.3.2.0-alpha-dev c7af923567bca5b0)
Sep 18 11:06:22.897 [warn] Bug:     ./src/or/tor(tor_main+0x5f) [0x55b43201f6bf] (on Tor 0.3.2.0-alpha-dev c7af923567bca5b0)
Sep 18 11:06:22.897 [warn] Bug:     ./src/or/tor(main+0x19) [0x55b432019179] (on Tor 0.3.2.0-alpha-dev c7af923567bca5b0)
Sep 18 11:06:22.897 [warn] Bug:     /lib64/libc.so.6(__libc_start_main+0xf0) [0x7f3952fd4640] (on Tor 0.3.2.0-alpha-dev c7af923567bca5b0)
Sep 18 11:06:22.897 [warn] Bug:     ./src/or/tor(_start+0x29) [0x55b4320191c9] (on Tor 0.3.2.0-alpha-dev c7af923567bca5b0)
Sep 18 11:06:22.897 [warn] 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 10ms.
  1. It wasn't disabled by the consensus. It was disabled by the user.
  2. It would be nice if we removed kist schedulers from the Schedulers torrc option when the interval asks for kist to be disabled
  3. We need to not use kist

Maybe ... just maybe ... using a negative run interval is obsolete logic and the Schedulers option should be the way to disable KIST :) That might be simpler.

comment:3 Changed 2 years ago by pastly

Owner: set to dgoulet
Status: newassigned

comment:4 in reply to:  2 Changed 2 years ago by dgoulet

Replying to pastly:

Maybe ... just maybe ... using a negative run interval is obsolete logic and the Schedulers option should be the way to disable KIST :) That might be simpler.

Yes... I do think 0 should be "use consensus value" else use torrc. And not magically "disable KIST" with -1.

Then. for the scheduler type, it should be Schedulers that is used in the consensus but for that we need tor to be able to parse consensus param as resulting string values. We will also require some sort of value that tells tor to look in the consensus by default and fallback to defaults. Like: Schedulers auto would be the default value in tor and it basically does the above (consensus then default list).

Or we go with an explicit option that is KISTDisable 0|1 but then what to do with KISTLite ... ? I'm not a big fan of this...

comment:5 Changed 2 years ago by pastly

I think Schedulers should be the way to disable KIST. I don't think we should allow KISTSchedRunInterval to be negative anymore. The hard part is making consensus params that are strings.

I really dislike more flags like KISTDisable 0|1.

comment:6 in reply to:  5 Changed 2 years ago by pastly

Replying to pastly:

I think Schedulers should be the way to disable KIST. I don't think we should allow KISTSchedRunInterval to be negative anymore. The hard part is making consensus params that are strings.

Actually, let's not open that consensus param can of worms.

comment:7 Changed 2 years ago by dgoulet

Resolution: wontfix
Status: assignedclosed

Ok we are not going down that path anymore. This interval value should never be negative, consensus param or not. We'll figure out another option to disable KIST if we really want that.

Note: See TracTickets for help on using tickets.