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.
> 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);
What about putting the allocation inside if_attach_common() with the
hooks that are allocated for the same reason? Maybe we should explain
why we allocate them?
> + timeout_set(ifp->if_wdtimo, if_slowtimo, ifp);
> + if_slowtimo(ifp);
> +
> /* 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));
Same thing for the free?
> 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 */
I prefer if_slowtimo like you suggested :)
