On Thu, 15 Sep 2016 16:29:45 +0200
Martin Pieuchot <m...@openbsd.org> wrote:
> 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?
Reads OK; I like the simple renaming.
The "softclock" thread name will be confusing, the timeouts are indeed
driven by the softclock interrupt, but the tasks have nothing to do
with softclock. Maybe "timeothread" ?
Will this new thread stay, or is it only to ease the transition to MP
networking ?
>
> Index: net/if_pflow.c
> ===================================================================
> RCS file: /cvs/src/sys/net/if_pflow.c,v
> retrieving revision 1.61
> diff -u -p -r1.61 if_pflow.c
> --- net/if_pflow.c 29 Apr 2016 08:55:03 -0000 1.61
> +++ net/if_pflow.c 15 Sep 2016 14:19:10 -0000
> @@ -548,15 +548,16 @@ pflow_init_timeouts(struct pflow_softc *
> if (timeout_initialized(&sc->sc_tmo_tmpl))
> timeout_del(&sc->sc_tmo_tmpl);
> if (!timeout_initialized(&sc->sc_tmo))
> - timeout_set(&sc->sc_tmo, pflow_timeout, sc);
> + timeout_set_proc(&sc->sc_tmo, pflow_timeout,
> sc); break;
> case PFLOW_PROTO_10:
> if (!timeout_initialized(&sc->sc_tmo_tmpl))
> - timeout_set(&sc->sc_tmo_tmpl,
> pflow_timeout_tmpl, sc);
> + timeout_set_proc(&sc->sc_tmo_tmpl,
> pflow_timeout_tmpl,
> + sc);
> if (!timeout_initialized(&sc->sc_tmo))
> - timeout_set(&sc->sc_tmo, pflow_timeout, sc);
> + timeout_set_proc(&sc->sc_tmo, pflow_timeout,
> sc); if (!timeout_initialized(&sc->sc_tmo6))
> - timeout_set(&sc->sc_tmo6, pflow_timeout6,
> sc);
> + timeout_set_proc(&sc->sc_tmo6,
> pflow_timeout6, sc);
> timeout_add_sec(&sc->sc_tmo_tmpl,
> PFLOW_TMPL_TIMEOUT); break;
> Index: net/if_pfsync.c
> ===================================================================
> RCS file: /cvs/src/sys/net/if_pfsync.c,v
> retrieving revision 1.231
> diff -u -p -r1.231 if_pfsync.c
> --- net/if_pfsync.c 15 Sep 2016 02:00:18 -0000 1.231
> +++ net/if_pfsync.c 15 Sep 2016 14:19:10 -0000
> @@ -328,9 +328,9 @@ pfsync_clone_create(struct if_clone *ifc
> IFQ_SET_MAXLEN(&ifp->if_snd, IFQ_MAXLEN);
> ifp->if_hdrlen = sizeof(struct pfsync_header);
> ifp->if_mtu = ETHERMTU;
> - timeout_set(&sc->sc_tmo, pfsync_timeout, sc);
> - timeout_set(&sc->sc_bulk_tmo, pfsync_bulk_update, sc);
> - timeout_set(&sc->sc_bulkfail_tmo, pfsync_bulk_fail, sc);
> + timeout_set_proc(&sc->sc_tmo, pfsync_timeout, sc);
> + timeout_set_proc(&sc->sc_bulk_tmo, pfsync_bulk_update, sc);
> + timeout_set_proc(&sc->sc_bulkfail_tmo, pfsync_bulk_fail, sc);
>
> if_attach(ifp);
> if_alloc_sadl(ifp);
> @@ -1723,7 +1723,7 @@ pfsync_defer(struct pf_state *st, struct
> sc->sc_deferred++;
> TAILQ_INSERT_TAIL(&sc->sc_deferrals, pd, pd_entry);
>
> - timeout_set(&pd->pd_tmo, pfsync_defer_tmo, pd);
> + timeout_set_proc(&pd->pd_tmo, pfsync_defer_tmo, pd);
> timeout_add_msec(&pd->pd_tmo, 20);
>
> schednetisr(NETISR_PFSYNC);
> Index: netinet/ip_carp.c
> ===================================================================
> RCS file: /cvs/src/sys/netinet/ip_carp.c,v
> retrieving revision 1.293
> diff -u -p -r1.293 ip_carp.c
> --- netinet/ip_carp.c 25 Jul 2016 16:44:04 -0000 1.293
> +++ netinet/ip_carp.c 15 Sep 2016 14:19:11 -0000
> @@ -831,9 +831,9 @@ carp_new_vhost(struct carp_softc *sc, in
> vhe->vhid = vhid;
> vhe->advskew = advskew;
> vhe->state = INIT;
> - timeout_set(&vhe->ad_tmo, carp_send_ad, vhe);
> - timeout_set(&vhe->md_tmo, carp_master_down, vhe);
> - timeout_set(&vhe->md6_tmo, carp_master_down, vhe);
> + timeout_set_proc(&vhe->ad_tmo, carp_send_ad, vhe);
> + timeout_set_proc(&vhe->md_tmo, carp_master_down, vhe);
> + timeout_set_proc(&vhe->md6_tmo, carp_master_down, vhe);
>
> KERNEL_ASSERT_LOCKED(); /* touching carp_vhosts */
>
> Index: netinet/tcp_timer.h
> ===================================================================
> RCS file: /cvs/src/sys/netinet/tcp_timer.h,v
> retrieving revision 1.13
> diff -u -p -r1.13 tcp_timer.h
> --- netinet/tcp_timer.h 6 Jul 2011 23:44:20 -0000 1.13
> +++ netinet/tcp_timer.h 15 Sep 2016 14:19:11 -0000
> @@ -116,7 +116,7 @@ const char *tcptimers[] =
> * Init, arm, disarm, and test TCP timers.
> */
> #define TCP_TIMER_INIT(tp,
> timer) \
> - timeout_set(&(tp)->t_timer[(timer)],
> tcp_timer_funcs[(timer)], tp)
> + timeout_set_proc(&(tp)->t_timer[(timer)],
> tcp_timer_funcs[(timer)], tp)
> #define TCP_TIMER_ARM(tp, timer,
> nticks) \
> timeout_add(&(tp)->t_timer[(timer)], (nticks) * (hz / PR_SLOWHZ))
> Index: netinet/tcp_var.h
> ===================================================================
> RCS file: /cvs/src/sys/netinet/tcp_var.h,v retrieving revision 1.115
> diff -u -p -r1.115 tcp_var.h
> --- netinet/tcp_var.h 20 Jul 2016 19:57:53 -0000 1.115
> +++ netinet/tcp_var.h 15 Sep 2016 14:19:11 -0000
> @@ -217,7 +217,7 @@ extern int tcp_delack_ticks;
> void tcp_delack(void *);
>
> #define
> TCP_INIT_DELACK(tp) \
> - timeout_set(&(tp)->t_delack_to, tcp_delack, tp)
> + timeout_set_proc(&(tp)->t_delack_to, tcp_delack, tp)
>
> #define
> TCP_RESTART_DELACK(tp)
> \ timeout_add(&(tp)->t_delack_to, tcp_delack_ticks) Index:
> kern/init_main.c
> ===================================================================
> RCS file: /cvs/src/sys/kern/init_main.c,v retrieving revision 1.257
> diff -u -p -r1.257 init_main.c
> --- kern/init_main.c 4 Sep 2016 09:22:29 -0000 1.257
> +++ kern/init_main.c 15 Sep 2016 14:19:10 -0000
> @@ -143,6 +143,7 @@ void prof_init(void);
> void init_exec(void);
> void kqueue_init(void);
> void taskq_init(void);
> +void timeout_proc_init(void);
> void pool_gc_pages(void *);
>
> extern char sigcode[], esigcode[], sigcoderet[];
> @@ -334,6 +335,9 @@ main(void *framep)
> sleep_queue_init();
> sched_init_cpu(curcpu());
> p->p_cpu->ci_randseed = (arc4random() & 0x7fffffff) + 1;
> +
> + /* Initialize timeouts in process context. */
> + timeout_proc_init();
>
> /* Initialize task queues */
> taskq_init();
> 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);
> +
> + 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 *);
>
