Opened 7 months ago

Closed 6 months ago

#24531 closed defect (fixed)

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

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

Description

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.

Child Tickets

Change History (15)

comment:1 Changed 7 months ago by pastly

Keywords: easy added

comment:2 Changed 7 months ago by aruna1234

Could you give more information on this one?

comment:3 in reply to:  2 Changed 7 months ago by pastly

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)

comment:4 Changed 6 months ago by aruna1234

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

comment:5 Changed 6 months ago by pastly

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.

comment:6 Changed 6 months ago by cypherpunks

Status: newneeds_review

comment:7 Changed 6 months ago by pastly

Status: needs_reviewneeds_revision

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);

comment:8 Changed 6 months ago by aruna1234

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

comment:9 Changed 6 months ago by pastly

Status: needs_revisionneeds_review

comment:10 Changed 6 months ago by pastly

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.

comment:11 Changed 6 months ago by nickm

Keywords: review-group-28 added

comment:12 Changed 6 months ago by nickm

Resolution: fixed
Reviewer: nickm
Status: needs_reviewclosed

This looks okay to me. Merging it to master.

Note: See TracTickets for help on using tickets.