On Sun, Nov 23, 2014 at 02:10:24AM +0100, Mike Belopuhov wrote:
> Hi,
>
> This removes the system wide if_slowtimo timeout and lets every
> interface with a valid if_watchdog method register it's own.
> The rational is to get rid of the ifnet loop in the softclock
> context to avoid further complications with concurrent access
> to the ifnet list. This might also save some cpu cycles in the
> runtime if number of interfaces is large while only a fraction
> all of them have a watchdog routine.
>
> This has originally come up on NetBSD mailing lists with some
> rather complicated locking strategy. Masao Uebayashi has later
> proposed a similar solution.
>
> The diff was looked at and corrected by mpi@. I've tested the
> trunk and a system build as well. OK?
>
> Index: sys/net/if.c
> ===================================================================
> RCS file: /home/cvs/src/sys/net/if.c,v
> retrieving revision 1.303
> diff -u -p -r1.303 if.c
> --- sys/net/if.c 3 Nov 2014 11:02:08 -0000 1.303
> +++ sys/net/if.c 22 Nov 2014 16:25:34 -0000
> @@ -135,7 +135,6 @@ void if_attach_common(struct ifnet *);
> void if_detach_queues(struct ifnet *, struct ifqueue *);
> void if_detached_start(struct ifnet *);
> int if_detached_ioctl(struct ifnet *, u_long, caddr_t);
> -void if_detached_watchdog(struct ifnet *);
>
> int if_getgroup(caddr_t, struct ifnet *);
> int if_getgroupmembers(caddr_t);
> @@ -171,12 +170,8 @@ int net_livelocked(void);
> void
> ifinit()
> {
> - static struct timeout if_slowtim;
> -
> - timeout_set(&if_slowtim, if_slowtimo, &if_slowtim);
> timeout_set(&net_tick_to, net_tick, &net_tick_to);
>
> - if_slowtimo(&if_slowtim);
> net_tick(&net_tick_to);
> }
>
> @@ -272,6 +267,11 @@ if_attachsetup(struct ifnet *ifp)
> pfi_attach_ifnet(ifp);
> #endif
>
> + ifp->if_wdtimo = malloc(sizeof(*ifp->if_wdtimo), M_TEMP,
> + M_WAITOK|M_ZERO);
> + timeout_set(ifp->if_wdtimo, if_slowtimo, ifp);
> + if_slowtimo(ifp);
> +
Please do not use M_TEMP for this (this is not a temporary buffer).
Maybe even remove the need to allocate the struct timeout (which requires
if_var.h to include sys/timeout.h and adds 24 / 40 bytes to ifnet even
when there is no watchdog used (like on vlan)).
In any case I think M_DEVBUF is the right type.
We could also rename if_slowtimo to something more concrete like
if_watchdog_timeout but that's just a bikeshed.
> /* Announce the interface. */
> rt_ifannouncemsg(ifp, IFAN_ARRIVAL);
> }
> @@ -483,7 +483,7 @@ if_detach(struct ifnet *ifp)
> ifp->if_flags &= ~IFF_OACTIVE;
> ifp->if_start = if_detached_start;
> ifp->if_ioctl = if_detached_ioctl;
> - ifp->if_watchdog = if_detached_watchdog;
> + ifp->if_watchdog = NULL;
>
> /*
> * Call detach hooks from head to tail. To make sure detach
> @@ -492,6 +492,10 @@ if_detach(struct ifnet *ifp)
> */
> dohooks(ifp->if_detachhooks, HOOK_REMOVE | HOOK_FREE);
>
> + /* Remove the watchdog timeout */
> + timeout_del(ifp->if_wdtimo);
> + free(ifp->if_wdtimo, M_TEMP, sizeof(*ifp->if_wdtimo));
> +
> #if NBRIDGE > 0
> /* Remove the interface from any bridge it is part of. */
> if (ifp->if_bridgeport)
> @@ -1162,25 +1166,22 @@ if_link_state_change_task(void *arg, voi
> }
>
> /*
> - * Handle interface watchdog timer routines. Called
> - * from softclock, we decrement timers (if set) and
> + * Handle interface watchdog timer routine. Called
> + * from softclock, we decrement timer (if set) and
> * call the appropriate interface routine on expiration.
> */
> void
> if_slowtimo(void *arg)
> {
> - struct timeout *to = (struct timeout *)arg;
> - struct ifnet *ifp;
> + struct ifnet *ifp = arg;
> int s = splnet();
>
> - TAILQ_FOREACH(ifp, &ifnet, if_list) {
> - if (ifp->if_timer == 0 || --ifp->if_timer)
> - continue;
> - if (ifp->if_watchdog)
> + if (ifp->if_watchdog) {
> + if (ifp->if_timer > 0 && --ifp->if_timer == 0)
> (*ifp->if_watchdog)(ifp);
> + timeout_add(ifp->if_wdtimo, hz / IFNET_SLOWHZ);
> }
> splx(s);
> - timeout_add(to, hz / IFNET_SLOWHZ);
> }
>
> /*
> @@ -1824,12 +1825,6 @@ int
> if_detached_ioctl(struct ifnet *ifp, u_long a, caddr_t b)
> {
> return ENODEV;
> -}
> -
> -void
> -if_detached_watchdog(struct ifnet *ifp)
> -{
> - /* nothing */
> }
>
> /*
> Index: sys/net/if_trunk.c
> ===================================================================
> RCS file: /home/cvs/src/sys/net/if_trunk.c,v
> retrieving revision 1.91
> diff -u -p -r1.91 if_trunk.c
> --- sys/net/if_trunk.c 18 Nov 2014 02:37:31 -0000 1.91
> +++ sys/net/if_trunk.c 22 Nov 2014 16:25:34 -0000
> @@ -344,11 +344,13 @@ trunk_port_create(struct trunk_softc *tr
> tp->tp_iftype = ifp->if_type;
> ifp->if_type = IFT_IEEE8023ADLAG;
> ifp->if_tp = (caddr_t)tp;
> - tp->tp_watchdog = ifp->if_watchdog;
> - ifp->if_watchdog = trunk_port_watchdog;
> tp->tp_ioctl = ifp->if_ioctl;
> ifp->if_ioctl = trunk_port_ioctl;
>
> + timeout_del(ifp->if_wdtimo);
> + tp->tp_watchdog = ifp->if_watchdog;
> + ifp->if_watchdog = trunk_port_watchdog;
> +
> tp->tp_if = ifp;
> tp->tp_trunk = tr;
>
> @@ -427,8 +429,8 @@ trunk_port_destroy(struct trunk_port *tp
>
> /* Restore interface */
> ifp->if_type = tp->tp_iftype;
> - ifp->if_watchdog = tp->tp_watchdog;
> ifp->if_ioctl = tp->tp_ioctl;
> + ifp->if_watchdog = tp->tp_watchdog;
> ifp->if_tp = NULL;
>
> hook_disestablish(ifp->if_linkstatehooks, tp->lh_cookie);
> @@ -464,6 +466,9 @@ trunk_port_destroy(struct trunk_port *tp
>
> /* Update trunk capabilities */
> tr->tr_capabilities = trunk_capabilities(tr);
> +
> + /* Reestablish watchdog timeout */
> + if_slowtimo(ifp);
>
> return (0);
> }
> Index: sys/net/if_var.h
> ===================================================================
> RCS file: /home/cvs/src/sys/net/if_var.h,v
> retrieving revision 1.12
> diff -u -p -r1.12 if_var.h
> --- sys/net/if_var.h 8 Jul 2014 04:02:14 -0000 1.12
> +++ sys/net/if_var.h 22 Nov 2014 16:25:34 -0000
> @@ -72,6 +72,7 @@ struct mbuf;
> struct proc;
> struct rtentry;
> struct socket;
> +struct timeout;
> struct ether_header;
> struct arpcom;
> struct rt_addrinfo;
> @@ -147,6 +148,7 @@ struct ifnet { /* and the
> entries */
> char if_description[IFDESCRSIZE]; /* interface description */
> u_short if_rtlabelid; /* next route label */
> u_int8_t if_priority;
> + struct timeout *if_wdtimo; /* watchdog timeout */
>
> /* procedure handles */
> /* output routine (enqueue) */
>
--
:wq Claudio
