> Date: Thu, 15 Sep 2016 16:29:45 +0200
> From: Martin Pieuchot <m...@openbsd.org>
>
> After discussing with a few people about a new "timed task" API I came
> to the conclusion that mixing timeouts and tasks will result in:
>
> - always including a 'struct timeout' in a 'struct task', or the other
> the way around
> or
>
> - introducing a new data structure, hence API.
>
> Since I'd like to keep the change as small as possible when converting
> existing timeout_set(9), neither option seem a good fit. So I decided
> to add a new kernel thread, curiously named "softclock", that will
> offer his stack to the poor timeout handlers that need one.
>
> With this approach, converting a timeout is just a matter of doing:
>
> s/timeout_set/timeout_set_proc/
>
>
> Diff below includes the conversions I need for the "netlock". I'm
> waiting for feedbacks and a better name to document the new function.
>
> Comments?
I like how minimal this is. Would like to see a few more people that
are familliar with the timeout code chime in, but it looks mostly
correct to me as well. One question though:
> Index: kern/kern_timeout.c
> ===================================================================
> RCS file: /cvs/src/sys/kern/kern_timeout.c,v
> retrieving revision 1.48
> diff -u -p -r1.48 kern_timeout.c
> --- kern/kern_timeout.c 6 Jul 2016 15:53:01 -0000 1.48
> +++ kern/kern_timeout.c 15 Sep 2016 14:19:10 -0000
> @@ -27,7 +27,7 @@
>
> #include <sys/param.h>
> #include <sys/systm.h>
> -#include <sys/lock.h>
> +#include <sys/kthread.h>
> #include <sys/timeout.h>
> #include <sys/mutex.h>
> #include <sys/kernel.h>
> @@ -54,6 +54,7 @@
>
> struct circq timeout_wheel[BUCKETS]; /* Queues of timeouts */
> struct circq timeout_todo; /* Worklist */
> +struct circq timeout_proc; /* Due timeouts needing proc. context */
>
> #define MASKWHEEL(wheel, time) (((time) >> ((wheel)*WHEELBITS)) & WHEELMASK)
>
> @@ -127,6 +128,9 @@ struct mutex timeout_mutex = MUTEX_INITI
>
> #define CIRCQ_EMPTY(elem) (CIRCQ_FIRST(elem) == (elem))
>
> +void softclock_thread(void *);
> +void softclock_create_thread(void *);
> +
> /*
> * Some of the "math" in here is a bit tricky.
> *
> @@ -147,11 +151,18 @@ timeout_startup(void)
> int b;
>
> CIRCQ_INIT(&timeout_todo);
> + CIRCQ_INIT(&timeout_proc);
> for (b = 0; b < nitems(timeout_wheel); b++)
> CIRCQ_INIT(&timeout_wheel[b]);
> }
>
> void
> +timeout_proc_init(void)
> +{
> + kthread_create_deferred(softclock_create_thread, curcpu());
> +}
> +
> +void
> timeout_set(struct timeout *new, void (*fn)(void *), void *arg)
> {
> new->to_func = fn;
> @@ -159,6 +170,12 @@ timeout_set(struct timeout *new, void (*
> new->to_flags = TIMEOUT_INITIALIZED;
> }
>
> +void
> +timeout_set_proc(struct timeout *new, void (*fn)(void *), void *arg)
> +{
> + timeout_set(new, fn, arg);
> + new->to_flags |= TIMEOUT_NEEDPROCCTX;
> +}
>
> int
> timeout_add(struct timeout *new, int to_ticks)
> @@ -334,38 +351,84 @@ timeout_hardclock_update(void)
> }
>
> void
> +timeout_run(struct timeout *to)
> +{
> + void (*fn)(void *);
> + void *arg;
> +
> + MUTEX_ASSERT_LOCKED(&timeout_mutex);
> +
> + to->to_flags &= ~TIMEOUT_ONQUEUE;
> + to->to_flags |= TIMEOUT_TRIGGERED;
> +
> + fn = to->to_func;
> + arg = to->to_arg;
> +
> + mtx_leave(&timeout_mutex);
> + fn(arg);
> + mtx_enter(&timeout_mutex);
> +}
> +
> +void
> softclock(void *arg)
> {
> int delta;
> struct circq *bucket;
> struct timeout *to;
> - void (*fn)(void *);
>
> mtx_enter(&timeout_mutex);
> while (!CIRCQ_EMPTY(&timeout_todo)) {
> to = timeout_from_circq(CIRCQ_FIRST(&timeout_todo));
> CIRCQ_REMOVE(&to->to_list);
>
> - /* If due run it, otherwise insert it into the right bucket. */
> + /*
> + * If due run it or defer execution to the thread,
> + * otherwise insert it into the right bucket.
> + */
> delta = to->to_time - ticks;
> if (delta > 0) {
> bucket = &BUCKET(delta, to->to_time);
> CIRCQ_INSERT(&to->to_list, bucket);
> + } else if (to->to_flags & TIMEOUT_NEEDPROCCTX) {
> + CIRCQ_INSERT(&to->to_list, &timeout_proc);
> + wakeup(&timeout_proc);
> } else {
> #ifdef DEBUG
> if (delta < 0)
> printf("timeout delayed %d\n", delta);
> #endif
> - to->to_flags &= ~TIMEOUT_ONQUEUE;
> - to->to_flags |= TIMEOUT_TRIGGERED;
> + timeout_run(to);
> + }
> + }
> + mtx_leave(&timeout_mutex);
> +}
>
> - fn = to->to_func;
> - arg = to->to_arg;
> +void
> +softclock_create_thread(void *xci)
> +{
> + if (kthread_create(softclock_thread, xci, NULL, "softclock"))
> + panic("fork softclock");
> +}
>
> - mtx_leave(&timeout_mutex);
> - fn(arg);
> - mtx_enter(&timeout_mutex);
> - }
> +void
> +softclock_thread(void *xci)
> +{
> + struct cpu_info *ci = xci;
> + struct timeout *to;
> +
> + KERNEL_ASSERT_LOCKED();
> +
> + /* Be conservative for the moment. */
> + sched_peg_curproc(ci);
The goal here is to pin the thread to the primary CPU. I think that's
the right thing to do for now since that is how normal timeouts work.
But code shouldn't rely on their timeouts being run on the primary
CPU.
Wonder if passing curproc() from timeout_proc_init() can be simplified
to explicitly using the primary CPU here. We currently don't a have a
convenient way to reference the primary CPU. So you'd have to iterate
over all CPUs and pick the one that has the appropriate flag set.
Perhaps we should just introduce a global variable for this?
> +
> + mtx_enter(&timeout_mutex);
> + for (;;) {
> + while (CIRCQ_EMPTY(&timeout_proc))
> + msleep(&timeout_proc, &timeout_mutex, PSWP, "bored", 0);
> +
> + to = timeout_from_circq(CIRCQ_FIRST(&timeout_proc));
> + CIRCQ_REMOVE(&to->to_list);
> + timeout_run(to);
> }
> mtx_leave(&timeout_mutex);
> }
> Index: sys/timeout.h
> ===================================================================
> RCS file: /cvs/src/sys/sys/timeout.h,v
> retrieving revision 1.25
> diff -u -p -r1.25 timeout.h
> --- sys/timeout.h 22 Dec 2014 04:43:38 -0000 1.25
> +++ sys/timeout.h 15 Sep 2016 14:19:11 -0000
> @@ -67,6 +67,7 @@ struct timeout {
> /*
> * flags in the to_flags field.
> */
> +#define TIMEOUT_NEEDPROCCTX 1 /* timeout needs a process context */
> #define TIMEOUT_ONQUEUE 2 /* timeout is on the todo queue
> */
> #define TIMEOUT_INITIALIZED 4 /* timeout is initialized */
> #define TIMEOUT_TRIGGERED 8 /* timeout is running or ran */
> @@ -88,6 +89,7 @@ struct timeout {
> struct bintime;
>
> void timeout_set(struct timeout *, void (*)(void *), void *);
> +void timeout_set_proc(struct timeout *, void (*)(void *), void *);
> int timeout_add(struct timeout *, int);
> int timeout_add_tv(struct timeout *, const struct timeval *);
> int timeout_add_ts(struct timeout *, const struct timespec *);
>
>
