On 19/09/16(Mon) 13:47, David Gwynne wrote:
> [...]
> id rather not grow the timeout (or task for that matter) apis just
> yet. theyre nice and straightforward to read and understand so far.
So better introduce a new API, right?
> 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.
Is it really a specific problem? We already encounter this for the
linkstate and the watchdog.
I'm not convinced by this approach. I don't understand why:
- adding a task in every data structure is worth it
- introducing a new if_nettmo_* make things simpler
So there's something which isn't explain in this email.
And I'll bet that in the upcoming years we're going to stop using soft
interrupts. Meaning that timeout handlers will always have a stack
available. If/when this happens, it will be easier to do:
s/timeout_set_proc/timeout_set/
> 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)
>
