Hello,
Damien Zammit, le mar. 09 déc. 2025 05:20:08 +0000, a ecrit:
> the nextsoftcheck global solves the race condition we used to have
> with softclock and reset_timeout i think
I don't really see why?
> diff --git a/kern/mach_clock.c b/kern/mach_clock.c
> index d7858a79..38e3114e 100644
> --- a/kern/mach_clock.c
> +++ b/kern/mach_clock.c
> -/*
> - * There is a nasty race between softclock and reset_timeout.
> - * For example, scheduling code looks at timer_set and calls
> - * reset_timeout, thinking the timer is set. However, softclock
> - * has already removed the timer but hasn't called thread_timeout
> - * yet.
> - *
> - * Interim solution: We initialize timers after pulling
> - * them out of the queue, so a race with reset_timeout won't
> - * hurt. The timeout functions (eg, thread_timeout,
> - * thread_depress_timeout) check timer_set/depress_priority
> - * to see if the timer has been cancelled and if so do nothing.
> - *
> - * This still isn't correct. For example, softclock pulls a
> - * timer off the queue,
You code is still pulling the timer off the queue, then unlocking the
queue, and the scheduling code can tinker with that timer, before
t_fcn() is called and calls thread_timeout.
> - * then thread_go resets timer_set (but
> - * reset_timeout does nothing), then thread_set_timeout puts the
> - * timer back on the queue and sets timer_set,
> - * then thread_timeout finally runs and clears timer_set,
The above being said, the comment above doesn't seem true any more:
thread_timeout does not change timer_set, it just checks it's unset,
which is indeed not true if thread_set_timeout came in between and has
set the timer again.
It seems your code still has the issue, so better keep the comment and
update it.
> - * then
> - * thread_set_timeout tries to put the timer on the queue again
> - * and corrupts it.
> - */
> -
> void softclock(void)
> {
> /*
> * Handle timeouts.
> */
> + timeout_t t;
> + queue_head_t *spoke;
> + int steps; /* number of reinterrupts of hardclock during this softclock
> */
? I thought it was the number of items in the list that were checked for
nothing (because they are for later). I.e. we don't want to keep
interrupts masked for too long while going through a long list of
timeouts.
> + unsigned long curticks;
> spl_t s;
> - timer_elt_t telt;
> - void (*fcn)( void * param );
> - void *param;
> -
> - while (TRUE) {
> - s = simple_lock_irq(&timer_lock);
> - telt = (timer_elt_t) queue_first(&timer_head.chain);
> - if (telt->ticks > elapsed_ticks) {
> - simple_unlock_irq(s, &timer_lock);
> - break;
> - }
> - fcn = telt->fcn;
> - param = telt->param;
> -
> - remqueue(&timer_head.chain, (queue_entry_t)telt);
> - telt->set = TELT_UNSET;
> - simple_unlock_irq(s, &timer_lock);
>
> - assert(fcn != 0);
> - (*fcn)(param);
> + steps = 0;
> + s = simple_lock_irq(&twheel_lock);
> + while (softticks != elapsed_ticks) {
> + ++softticks;
> + curticks = softticks;
> + spoke = TIMEOUT_WHEEL_QUEUE(curticks);
> + t = (timeout_t)queue_next(spoke);
> + /* Iterate over all elements within spoke checking that we did
> not wrap back to a spoke */
> + while (! ((t >= (timeout_t)&timeoutwheel[0])
> + && (t < (timeout_t)&timeoutwheel[TIMEOUT_WHEEL_SIZE])) )
> {
> + if (t->t_time != curticks) {
> + t = (timeout_t)queue_next(&t->chain);
> +
> + if (++steps >= MAX_SOFTCLOCK_STEPS) {
I still don't see why MAX_SOFTCLOCK_STEPS is expressed in terms of
TIMEOUT_WHEEL_SIZE. AIUI this is meant to just avoid spending too much
time without interrupts enabled while going through a long list of
timeouts. But that time only depends on the number of elements, it is
independent from the indexing performed in the wheel, so I don't see
the relation.
> + nextsoftcheck = t;
> + /* Give hardclock() a chance, so that
> we don't delay the clock interrupts */
> + simple_unlock_irq(s, &twheel_lock);
> + s = simple_lock_irq(&twheel_lock);
> + t = nextsoftcheck;
> + steps = 0;
> + }
> + } else {
> + void (*t_fcn)(void *);
> + void *t_arg;
> +
> + nextsoftcheck =
> (timeout_t)queue_next(&t->chain);
> + queue_remove(spoke, t, timeout_t, chain);
> + t->chain.prev = t->chain.next = NULL;
> + t_fcn = t->fcn;
> + t_arg = t->param;
> + if (t->set & TIMEOUT_ALLOC)
> + t->set = TIMEOUT_ALLOC;
> + else
> + t->set &= ~TIMEOUT_PENDING;
> + simple_unlock_irq(s, &twheel_lock);
> + assert(t_fcn != 0);
> + t_fcn(t_arg); /* call function at elapsed */
> + s = simple_lock_irq(&twheel_lock);
> + steps = 0;
> + t = nextsoftcheck;
> + }
> + }
> }
> + nextsoftcheck = NULL;
> + simple_unlock_irq(s, &twheel_lock);
> }
>
> /*
> * Set timeout.
> *
> * Parameters:
> - * telt timer element. Function and param are already set.
> - * interval time-out interval, in hz.
> + * t preallocated timeout. Function and param are already
> set.
> + * interval time-out interval, in ticks.
> */
> void set_timeout(
> - timer_elt_t telt, /* already loaded */
> + timeout_t t, /* already loaded */
> unsigned int interval)
> {
> - spl_t s;
> - timer_elt_t next;
> + spl_t s;
> +
> + if (t->set & (TIMEOUT_ACTIVE | TIMEOUT_PENDING))
> + reset_timeout(t);
Again, does that really happen? From the existing code, it seems that
set_timeout does assume that it's not called on a timer that is already
set. So better assert that rather than work around it.
Samuel
