sched: Add function to change scheduler state and always use it

I'm thinking about hypothetical future debugging efforts here.

I think it would be nice to have

channel_set_scheduler_state(chan, new_state) {
  log_changed_from_state_to_state()
  chan->scheduler_state = new_state
}

And then change every instance of chan->scheduler_state = [...] to channel_set_scheduler_state([...]). Future hypothetical debugging sessions can be guaranteed to have information regarding what state each channel at all times.

Extra points if it logs some of the stuff in scheduler_bug_occurred too.

changed milestone to %Tor: 0.3.3.x-final in legacy/trac

added component::core tor/tor in Legacy / Trac easy in Legacy / Trac milestone::Tor: 0.3.3.x-final in Legacy / Trac priority::medium in Legacy / Trac resolution::fixed in Legacy / Trac review-group-28 in Legacy / Trac reviewer::nickm in Legacy / Trac severity::normal in Legacy / Trac status::closed in Legacy / Trac tor-sched in Legacy / Trac type::defect in Legacy / Trac labels

Trac:
Keywords: N/A deleted, easy added

Could you give more information on this one?

Trac:
Username: aruna1234

Replying to aruna1234:

Could you give more information on this one?

In many places in the scheduling code we do (IDLE just used as an example)

chan->scheduler_state = SCHED_CHAN_IDLE;

I'm proposing we replace all these assignments to a channel's scheduler_state with a function that does it for us and logs the old and new state. It would most likely need to be defined in the "Private scheduler functions" section of scheduler.h in order to be reachable from all the scheduler source files.

In slightly better pseudocode, I propose the function looks like

scheduler_set_channel_state(chan, new_state)
{
  log_debug(LD_SCHED, "chan %d changed from scheduler state %d to %d",
      chan->global_id, chan->scheduler_state, new_state);
  chan->scheduler_state = new_state;
}

Ideally we would log "from state IDLE to WAITING_FOR_CELLS", which would call for a helper function if it doesn't already exist. And the int type on the channel's global ID needs to be correct, etc. etc.

The bonus points are for also logging the channel information that's logged in scheduler_bug_occurred like # of cmux cells and connection outbuf len.

(Note: bonus points aren't real, they just make dgoulet/pastly happy)

so, first we need to write that function, and then replace all the assignment statements in scheduler.c with the function call?

Trac:
Username: aruna1234

Write the function. Then replace every instance of

chan->scheduler_state = SCHED_CHAN_IDLE;

with

scheduler_set_channel_state(chan, SCHED_CHAN_IDLE);

Of course, do this for all states and not just SCHED_CHAN_IDLE.

Trac:
Username: aruna1234

0001-Bug-24531-Add-function-to-change-scheduler-state-and.patch

Trac:
Username: aruna1234

Trac:
Status: new to needs_review

The patch as provided needs revision.

• == are not assignments.
• function definitions need types for their arguments (unlike pseudocode)
• scheduler_set_channel is not a good name for the function
• "Function to log and change all the old and new states" does not describe what the function does

And this is just plain wrong

@@ -707,7 +712,7 @@ scheduler_bug_occurred(const channel_t *chan)
                  " Num cells on cmux: %d. Connection outbuf len: %lu.",
                  chan->global_identifier,
                  channel_state_to_string(chan->state),
-                 chan->scheduler_state, circuitmux_num_cells(chan->cmux),
+                 scheduler_set_channel(chan,new_state), circuitmux_num_cells(chan->cmux),
                  (unsigned long)outbuf_len);

Trac:
Status: needs_review to needs_revision

Trac:
Username: aruna1234

0003-Bug-24531-Function-to-change-the-channel-scheduler-s.patch

Trac:
Username: aruna1234

Please igoner the above patch. The following is the correct one.

Trac:
Username: aruna1234

Trac:
Username: aruna1234

0002-Bug-24531-Function-to-change-channel-scheduler-state.patch

Trac:
Username: aruna1234

Trac:
Status: needs_revision to needs_review

I've made significant changes to the above patches and pushed them to ticket24531_033_01. I think they should be reviewed by someone other than me.

Trac:
Keywords: N/A deleted, review-group-28 added

This looks okay to me. Merging it to master.

Trac:
Status: needs_review to closed
Resolution: N/A to fixed
Reviewer: N/A to nickm

closed

moved from legacy/trac#24531 (moved)

added Bug First Contribution labels and removed 1 deleted label

removed 1 deleted label