I've got one ok for this. Can I have another one please?
On Mon, Nov 24, 2014 at 13:54 +0100, Mike Belopuhov wrote:
> On Sun, Nov 23, 2014 at 12:06 +0100, Martin Pieuchot wrote:
> > On 23/11/14(Sun) 02:10, 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?
> >
> > I'm basically ok, some remarks below.
> >
> >
> > What about putting the allocation inside if_attach_common() with the
> > hooks that are allocated for the same reason?
>
> Sure.
>
> > Maybe we should explain why we allocate them?
> >
>
> We all know why, don't think it will help anything.
>
> New version. OK?
>
>
> diff --git sys/net/if.c sys/net/if.c
> index 0632eec..679c8c8 100644
> --- sys/net/if.c
> +++ sys/net/if.c
> @@ -133,11 +133,10 @@ void if_attachdomain1(struct ifnet *);
> 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);
> int if_getgroupattribs(caddr_t);
> int if_setgroupattribs(caddr_t);
> @@ -169,16 +168,12 @@ int net_livelocked(void);
> * parameters.
> */
> 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);
> }
>
> static unsigned int if_index = 0;
> static unsigned int if_indexlim = 0;
> @@ -270,10 +265,13 @@ if_attachsetup(struct ifnet *ifp)
> if_attachdomain1(ifp);
> #if NPF > 0
> pfi_attach_ifnet(ifp);
> #endif
>
> + timeout_set(ifp->if_slowtimo, if_slowtimo, ifp);
> + if_slowtimo(ifp);
> +
> /* Announce the interface. */
> rt_ifannouncemsg(ifp, IFAN_ARRIVAL);
> }
>
> /*
> @@ -426,10 +424,13 @@ if_attach_common(struct ifnet *ifp)
> M_TEMP, M_WAITOK);
> TAILQ_INIT(ifp->if_linkstatehooks);
> ifp->if_detachhooks = malloc(sizeof(*ifp->if_detachhooks),
> M_TEMP, M_WAITOK);
> TAILQ_INIT(ifp->if_detachhooks);
> +
> + ifp->if_slowtimo = malloc(sizeof(*ifp->if_slowtimo), M_TEMP,
> + M_WAITOK|M_ZERO);
> }
>
> void
> if_start(struct ifnet *ifp)
> {
> @@ -481,19 +482,22 @@ if_detach(struct ifnet *ifp)
> struct domain *dp;
>
> 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
> * hooks are executed in the reverse order they were added, all
> * the hooks have to be added to the head!
> */
> dohooks(ifp->if_detachhooks, HOOK_REMOVE | HOOK_FREE);
>
> + /* Remove the watchdog timeout */
> + timeout_del(ifp->if_slowtimo);
> +
> #if NBRIDGE > 0
> /* Remove the interface from any bridge it is part of. */
> if (ifp->if_bridgeport)
> bridge_ifdetach(ifp);
> #endif
> @@ -577,10 +581,12 @@ do { \
>
> free(ifp->if_addrhooks, M_TEMP, 0);
> free(ifp->if_linkstatehooks, M_TEMP, 0);
> free(ifp->if_detachhooks, M_TEMP, 0);
>
> + free(ifp->if_slowtimo, M_TEMP, sizeof(*ifp->if_slowtimo));
> +
> for (dp = domains; dp; dp = dp->dom_next) {
> if (dp->dom_ifdetach && ifp->if_afdata[dp->dom_family])
> (*dp->dom_ifdetach)(ifp,
> ifp->if_afdata[dp->dom_family]);
> }
> @@ -1160,29 +1166,26 @@ if_link_state_change_task(void *arg, void *unused)
> }
> splx(s);
> }
>
> /*
> - * 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_slowtimo, hz / IFNET_SLOWHZ);
> }
> splx(s);
> - timeout_add(to, hz / IFNET_SLOWHZ);
> }
>
> /*
> * Map interface name to interface structure pointer.
> */
> @@ -1824,16 +1827,10 @@ int
> if_detached_ioctl(struct ifnet *ifp, u_long a, caddr_t b)
> {
> return ENODEV;
> }
>
> -void
> -if_detached_watchdog(struct ifnet *ifp)
> -{
> - /* nothing */
> -}
> -
> /*
> * Create interface group without members
> */
> struct ifg_group *
> if_creategroup(const char *groupname)
> diff --git sys/net/if_trunk.c sys/net/if_trunk.c
> index 90e7fe9..c4bc760 100644
> --- sys/net/if_trunk.c
> +++ sys/net/if_trunk.c
> @@ -342,15 +342,17 @@ trunk_port_create(struct trunk_softc *tr, struct ifnet
> *ifp)
>
> /* Change the interface type */
> 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_slowtimo);
> + tp->tp_watchdog = ifp->if_watchdog;
> + ifp->if_watchdog = trunk_port_watchdog;
> +
> tp->tp_if = ifp;
> tp->tp_trunk = tr;
>
> /* Save port link layer address */
> bcopy(((struct arpcom *)ifp)->ac_enaddr, tp->tp_lladdr, ETHER_ADDR_LEN);
> @@ -463,10 +465,13 @@ trunk_port_destroy(struct trunk_port *tp)
> free(tp, M_DEVBUF, 0);
>
> /* Update trunk capabilities */
> tr->tr_capabilities = trunk_capabilities(tr);
>
> + /* Reestablish watchdog timeout */
> + if_slowtimo(ifp);
> +
> return (0);
> }
>
> void
> trunk_port_watchdog(struct ifnet *ifp)
> diff --git sys/net/if_var.h sys/net/if_var.h
> index cd40d8b..19f4f56 100644
> --- sys/net/if_var.h
> +++ sys/net/if_var.h
> @@ -70,10 +70,11 @@
>
> struct mbuf;
> struct proc;
> struct rtentry;
> struct socket;
> +struct timeout;
> struct ether_header;
> struct arpcom;
> struct rt_addrinfo;
> struct ifnet;
> struct hfsc_if;
> @@ -145,10 +146,11 @@ struct ifnet { /* and the
> entries */
> u_int32_t if_hardmtu; /* maximum MTU device supports */
> u_int if_rdomain; /* routing instance */
> char if_description[IFDESCRSIZE]; /* interface description */
> u_short if_rtlabelid; /* next route label */
> u_int8_t if_priority;
> + struct timeout *if_slowtimo; /* watchdog timeout */
>
> /* procedure handles */
> /* output routine (enqueue) */
> int (*if_output)(struct ifnet *, struct mbuf *, struct sockaddr *,
> struct rtentry *);
