On Fri, Sep 16, 2016 at 04:58:39PM +0200, Mark Kettenis wrote:
> > 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:
id rather not grow the timeout (or task for that matter) apis just
yet. theyre nice and straightforward to read and understand so far.
for this specific problem can we do a specific fix for it? the diff
below is equiv to the timeout_set_proc change, but implements it
by using explicit tasks that are called by the timeouts from a
common trampoline in the network stack.
Index: net/if.c
===================================================================
RCS file: /cvs/src/sys/net/if.c,v
retrieving revision 1.448
diff -u -p -r1.448 if.c
--- net/if.c 13 Sep 2016 08:15:01 -0000 1.448
+++ net/if.c 19 Sep 2016 01:51:37 -0000
@@ -155,6 +155,8 @@ void if_watchdog_task(void *);
void if_input_process(void *);
void if_netisr(void *);
+void if_nettmo_add(void *);
+
#ifdef DDB
void ifa_print_all(void);
#endif
@@ -875,6 +877,21 @@ if_netisr(void *unused)
splx(s);
KERNEL_UNLOCK();
+}
+
+void
+if_nettmo_add(void *t)
+{
+ /* the task is added to systq so it inherits the KERNEL_LOCK */
+ task_add(systq, t);
+}
+
+void
+if_nettmo_set(struct timeout *tmo, struct task *task,
+ void (*fn)(void *), void *arg)
+{
+ task_set(task, fn, arg);
+ timeout_set(tmo, if_nettmo_add, task);
}
void
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 19 Sep 2016 01:51:37 -0000
@@ -36,6 +36,7 @@
#include <net/if_types.h>
#include <net/bpf.h>
#include <net/route.h>
+#include <net/netisr.h>
#include <netinet/in.h>
#include <netinet/if_ether.h>
#include <netinet/tcp.h>
@@ -547,16 +548,24 @@ pflow_init_timeouts(struct pflow_softc *
timeout_del(&sc->sc_tmo6);
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);
+ if (!timeout_initialized(&sc->sc_tmo)) {
+ if_nettmo_set(&sc->sc_tmo, &sc->sc_task,
+ 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);
- if (!timeout_initialized(&sc->sc_tmo))
- timeout_set(&sc->sc_tmo, pflow_timeout, sc);
- if (!timeout_initialized(&sc->sc_tmo6))
- timeout_set(&sc->sc_tmo6, pflow_timeout6, sc);
+ if (!timeout_initialized(&sc->sc_tmo_tmpl)) {
+ if_nettmo_set(&sc->sc_tmo_tmpl, &sc->sc_task_tmpl,
+ pflow_timeout_tmpl, sc);
+ }
+ if (!timeout_initialized(&sc->sc_tmo)) {
+ if_nettmo_set(&sc->sc_tmo, &sc->sc_task,
+ pflow_timeout, sc);
+ }
+ if (!timeout_initialized(&sc->sc_tmo6)) {
+ if_nettmo_set(&sc->sc_tmo6, &sc->sc_task6,
+ pflow_timeout6, sc);
+ }
timeout_add_sec(&sc->sc_tmo_tmpl, PFLOW_TMPL_TIMEOUT);
break;
Index: net/if_pflow.h
===================================================================
RCS file: /cvs/src/sys/net/if_pflow.h,v
retrieving revision 1.14
diff -u -p -r1.14 if_pflow.h
--- net/if_pflow.h 3 Oct 2015 10:44:23 -0000 1.14
+++ net/if_pflow.h 19 Sep 2016 01:51:37 -0000
@@ -182,8 +182,11 @@ struct pflow_softc {
u_int64_t sc_gcounter;
u_int32_t sc_sequence;
struct timeout sc_tmo;
- struct timeout sc_tmo6;
+ struct task sc_task;
struct timeout sc_tmo_tmpl;
+ struct task sc_task_tmpl;
+ struct timeout sc_tmo6;
+ struct task sc_task6;
struct socket *so;
struct mbuf *send_nam;
struct sockaddr *sc_flowsrc;
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 19 Sep 2016 01:51:37 -0000
@@ -50,6 +50,7 @@
#include <sys/socket.h>
#include <sys/ioctl.h>
#include <sys/timeout.h>
+#include <sys/task.h>
#include <sys/kernel.h>
#include <sys/sysctl.h>
#include <sys/pool.h>
@@ -186,6 +187,7 @@ struct pfsync_deferral {
struct pf_state *pd_st;
struct mbuf *pd_m;
struct timeout pd_tmo;
+ struct task pd_task;
};
TAILQ_HEAD(pfsync_deferrals, pfsync_deferral);
@@ -225,17 +227,20 @@ struct pfsync_softc {
u_int32_t sc_ureq_sent;
int sc_bulk_tries;
struct timeout sc_bulkfail_tmo;
+ struct task sc_bulkfail_task;
u_int32_t sc_ureq_received;
struct pf_state *sc_bulk_next;
struct pf_state *sc_bulk_last;
struct timeout sc_bulk_tmo;
+ struct task sc_bulk_task;
TAILQ_HEAD(, tdb) sc_tdb_q;
void *sc_lhcookie;
struct timeout sc_tmo;
+ struct task sc_task;
};
struct pfsync_softc *pfsyncif = NULL;
@@ -328,9 +333,11 @@ 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);
+ if_nettmo_set(&sc->sc_tmo, &sc->sc_task, pfsync_timeout, sc);
+ if_nettmo_set(&sc->sc_bulk_tmo, &sc->sc_bulk_task,
+ pfsync_bulk_update, sc);
+ if_nettmo_set(&sc->sc_bulkfail_tmo, &sc->sc_bulkfail_task,
+ pfsync_bulk_fail, sc);
if_attach(ifp);
if_alloc_sadl(ifp);
@@ -1723,7 +1730,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);
+ if_nettmo_set(&pd->pd_tmo, &pd->pd_task, pfsync_defer_tmo, pd);
timeout_add_msec(&pd->pd_tmo, 20);
schednetisr(NETISR_PFSYNC);
Index: net/netisr.h
===================================================================
RCS file: /cvs/src/sys/net/netisr.h,v
retrieving revision 1.47
diff -u -p -r1.47 netisr.h
--- net/netisr.h 1 Sep 2016 10:06:33 -0000 1.47
+++ net/netisr.h 19 Sep 2016 01:51:37 -0000
@@ -85,6 +85,11 @@ do {
\
task_add(softnettq, &if_input_task_locked); \
} while (/* CONSTCOND */0)
+struct timeout;
+
+void if_nettmo_set(struct timeout *, struct task *,
+ void (*)(void *), void *);
+
#endif /* _KERNEL */
#endif /*_LOCORE */
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 19 Sep 2016 01:51:37 -0000
@@ -42,6 +42,7 @@
#include <sys/socket.h>
#include <sys/socketvar.h>
#include <sys/timeout.h>
+#include <sys/task.h>
#include <sys/ioctl.h>
#include <sys/errno.h>
#include <sys/device.h>
@@ -108,8 +109,11 @@ struct carp_vhost_entry {
int advskew;
enum { INIT = 0, BACKUP, MASTER } state;
struct timeout ad_tmo; /* advertisement timeout */
+ struct task ad_task; /* advertisement timeout */
struct timeout md_tmo; /* master down timeout */
+ struct task md_task; /* master down timeout */
struct timeout md6_tmo; /* master down timeout */
+ struct task md6_task; /* master down timeout */
u_int64_t vhe_replay_cookie;
@@ -831,9 +835,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);
+ if_nettmo_set(&vhe->ad_tmo, &vhe->ad_task, carp_send_ad, vhe);
+ if_nettmo_set(&vhe->md_tmo, &vhe->md_task, carp_master_down, vhe);
+ if_nettmo_set(&vhe->md6_tmo, &vhe->md6_task, carp_master_down, vhe);
KERNEL_ASSERT_LOCKED(); /* touching carp_vhosts */
Index: netinet/tcp_subr.c
===================================================================
RCS file: /cvs/src/sys/netinet/tcp_subr.c,v
retrieving revision 1.155
diff -u -p -r1.155 tcp_subr.c
--- netinet/tcp_subr.c 15 Sep 2016 02:00:18 -0000 1.155
+++ netinet/tcp_subr.c 19 Sep 2016 01:51:37 -0000
@@ -79,6 +79,7 @@
#include <sys/pool.h>
#include <net/route.h>
+#include <net/netisr.h>
#include <netinet/in.h>
#include <netinet/ip.h>
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 19 Sep 2016 01:51:37 -0000
@@ -116,7 +116,8 @@ 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)
+ if_nettmo_set(&(tp)->t_timer[(timer)], &(tp)->t_task[(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 19 Sep 2016 01:51:37 -0000
@@ -36,6 +36,7 @@
#define _NETINET_TCP_VAR_H_
#include <sys/timeout.h>
+#include <sys/task.h>
/*
* Kernel variables for tcp.
@@ -70,6 +71,7 @@ struct tcpqent {
struct tcpcb {
struct tcpqehead t_segq; /* sequencing queue */
struct timeout t_timer[TCPT_NTIMERS]; /* tcp timers */
+ struct task t_task[TCPT_NTIMERS];
short t_state; /* state of this connection */
short t_rxtshift; /* log(2) of rexmt exp. backoff */
short t_rxtcur; /* current retransmit value */
@@ -104,6 +106,7 @@ struct tcpcb {
struct mbuf *t_template; /* skeletal packet for transmit */
struct inpcb *t_inpcb; /* back pointer to internet pcb */
struct timeout t_delack_to; /* delayed ACK callback */
+ struct task t_delack_task;
/*
* The following fields are used as in the protocol specification.
* See RFC793, Dec. 1981, page 21.
@@ -217,7 +220,8 @@ extern int tcp_delack_ticks;
void tcp_delack(void *);
#define TCP_INIT_DELACK(tp) \
- timeout_set(&(tp)->t_delack_to, tcp_delack, tp)
+ if_nettmo_set(&(tp)->t_delack_to, &(tp)->t_delack_task, \
+ tcp_delack, tp)
#define TCP_RESTART_DELACK(tp) \
timeout_add(&(tp)->t_delack_to, tcp_delack_ticks)
