From adaf3e9b89f62d68ab631b8f672d9bff996689b9 Mon Sep 17 00:00:00 2001 From: David Goulet Date: Wed, 31 Jan 2018 13:46:31 -0500 Subject: [PATCH 1/2] sched: Avoid adding the same channel twice to the KIST pending list This is the quick fix that is keeping the channel in PENDING state so if we ever try to reschedule the same channel, it won't happened. Fixes #24700 Signed-off-by: David Goulet --- changes/bug24700 | 4 ++++ src/or/scheduler_kist.c | 1 - 2 files changed, 4 insertions(+), 1 deletion(-) create mode 100644 changes/bug24700 diff --git a/changes/bug24700 b/changes/bug24700 new file mode 100644 index 000000000..74dc581a0 --- /dev/null +++ b/changes/bug24700 @@ -0,0 +1,4 @@ + o Minor bugfixes (scheduler, KIST): + - Avoid adding the same channel twice in the KIST scheduler pending list + wasting CPU cycles at handling the same channel twice. Fixes bug 24700; + bugfix on 0.3.2.1-alpha. diff --git a/src/or/scheduler_kist.c b/src/or/scheduler_kist.c index f50f3aa9e..7b5d138be 100644 --- a/src/or/scheduler_kist.c +++ b/src/or/scheduler_kist.c @@ -693,7 +693,6 @@ kist_scheduler_run(void) * after the scheduling loop is over. They can hopefully be taken care of * in the next scheduling round. */ - chan->scheduler_state = SCHED_CHAN_WAITING_TO_WRITE; if (!to_readd) { to_readd = smartlist_new(); } From cb5654f300312a8f4c777378d68696667eff427b Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Thu, 1 Feb 2018 11:05:50 -0500 Subject: [PATCH 2/2] sched: Use the sched_heap_idx field to double-check our fix for 24700. Signed-off-by: David Goulet --- src/or/channel.c | 3 +++ src/or/scheduler.c | 20 ++++++++++++-------- src/or/scheduler_kist.c | 13 ++++++++++--- 3 files changed, 25 insertions(+), 11 deletions(-) diff --git a/src/or/channel.c b/src/or/channel.c index 0b5a7fde9..049a5290d 100644 --- a/src/or/channel.c +++ b/src/or/channel.c @@ -951,6 +951,9 @@ channel_init(channel_t *chan) /* Scheduler state is idle */ chan->scheduler_state = SCHED_CHAN_IDLE; + + /* Channel is not in the scheduler heap. */ + chan->sched_heap_idx = -1; } /** diff --git a/src/or/scheduler.c b/src/or/scheduler.c index cbf51447b..26f927fd8 100644 --- a/src/or/scheduler.c +++ b/src/or/scheduler.c @@ -538,10 +538,12 @@ scheduler_channel_has_waiting_cells,(channel_t *chan)) * channels_pending. */ chan->scheduler_state = SCHED_CHAN_PENDING; - smartlist_pqueue_add(channels_pending, - scheduler_compare_channels, - offsetof(channel_t, sched_heap_idx), - chan); + if (!SCHED_BUG(chan->sched_heap_idx != -1, chan)) { + smartlist_pqueue_add(channels_pending, + scheduler_compare_channels, + offsetof(channel_t, sched_heap_idx), + chan); + } log_debug(LD_SCHED, "Channel " U64_FORMAT " at %p went from waiting_for_cells " "to pending", @@ -665,10 +667,12 @@ scheduler_channel_wants_writes(channel_t *chan) */ log_debug(LD_SCHED, "chan=%" PRIu64 " became pending", chan->global_identifier); - smartlist_pqueue_add(channels_pending, - scheduler_compare_channels, - offsetof(channel_t, sched_heap_idx), - chan); + if (!SCHED_BUG(chan->sched_heap_idx != -1, chan)) { + smartlist_pqueue_add(channels_pending, + scheduler_compare_channels, + offsetof(channel_t, sched_heap_idx), + chan); + } chan->scheduler_state = SCHED_CHAN_PENDING; log_debug(LD_SCHED, "Channel " U64_FORMAT " at %p went from waiting_to_write " diff --git a/src/or/scheduler_kist.c b/src/or/scheduler_kist.c index 7b5d138be..d2878437c 100644 --- a/src/or/scheduler_kist.c +++ b/src/or/scheduler_kist.c @@ -704,8 +704,10 @@ kist_scheduler_run(void) /* Case 4: cells to send, and still open for writes */ chan->scheduler_state = SCHED_CHAN_PENDING; - smartlist_pqueue_add(cp, scheduler_compare_channels, - offsetof(channel_t, sched_heap_idx), chan); + if (!SCHED_BUG(chan->sched_heap_idx != -1, chan)) { + smartlist_pqueue_add(cp, scheduler_compare_channels, + offsetof(channel_t, sched_heap_idx), chan); + } } } /* End of main scheduling loop */ @@ -725,8 +727,13 @@ kist_scheduler_run(void) SMARTLIST_FOREACH_BEGIN(to_readd, channel_t *, readd_chan) { readd_chan->scheduler_state = SCHED_CHAN_PENDING; if (!smartlist_contains(cp, readd_chan)) { - smartlist_pqueue_add(cp, scheduler_compare_channels, + if (!SCHED_BUG(chan->sched_heap_idx != -1, chan)) { + /* XXXX Note that the check above is in theory redundant with + * the smartlist_contains check. But let's make sure we're + * not messing anything up, and leave them both for now. */ + smartlist_pqueue_add(cp, scheduler_compare_channels, offsetof(channel_t, sched_heap_idx), readd_chan); + } } } SMARTLIST_FOREACH_END(readd_chan); smartlist_free(to_readd);