> Date: Mon, 19 Sep 2016 13:47:25 +1000
> From: David Gwynne <da...@gwynne.id.au>
> 
> 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.

But this is much more involved tha mpi@'s change.

So unless somebody shoots a hole into his approach, I'd prefer the
timeout_set_proc() approach.

> 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)
> 

Reply via email to