Re: struct ifnet and ifaddr handling [was: Re: Making global variables of if.c MPSAFE]

[Ryota Ozaki <ozaki-r%netbsd.org@localhost>]

On Mon, Nov 24, 2014 at 11:23 AM, Masao Uebayashi <uebayasi%gmail.com@localhost> wrote:
> http://marc.info/?t=141670552700001&r=1&w=2
>
> Following the ideas raised in that thread:

Thank you for the information.

>
> - Allocate callout_t dynamically.  struct ifnet only has a pointer to struct
>   callout, which will not be read by netstat(1) anyway.

Yes, it works (actually I'm doing it in refcount). I worried that
using malloc/free makes MP-ifying difficult and wanted another way
if there is.

>
> - Prefer the name "slowtimo" to "watchdog", because those callbacks do a
>   little more than what "watchdog" suggests.

Sure.

  ozaki-r

>
> Index: sys/net/if.c
> ===================================================================
> RCS file: /cvsroot/src/sys/net/if.c,v
> retrieving revision 1.293
> diff -p -u -r1.293 if.c
> --- sys/net/if.c        17 Nov 2014 13:58:53 -0000      1.293
> +++ sys/net/if.c        24 Nov 2014 01:49:14 -0000
> @@ -168,8 +168,6 @@ static kmutex_t                     if_clone_mtx;
>
>  static struct ifaddr **                ifnet_addrs = NULL;
>
> -static callout_t               if_slowtimo_ch;
> -
>  struct ifnet *lo0ifp;
>  int    ifqmaxlen = IFQ_MAXLEN;
>
> @@ -194,6 +192,7 @@ static void ifnet_lock_exit(struct ifnet
>  static void if_detach_queues(struct ifnet *, struct ifqueue *);
>  static void sysctl_sndq_setup(struct sysctllog **, const char *,
>      struct ifaltq *);
> +static void if_slowtimo(void *);
>
>  #if defined(INET) || defined(INET6)
>  static void sysctl_net_pktq_setup(struct sysctllog **, int);
> @@ -235,9 +234,6 @@ ifinit(void)
>         sysctl_net_pktq_setup(NULL, PF_INET6);
>  #endif
>
> -       callout_init(&if_slowtimo_ch, 0);
> -       if_slowtimo(NULL);
> -
>         if_listener = kauth_listen_scope(KAUTH_SCOPE_NETWORK,
>             if_listener_cb, NULL);
>
> @@ -337,7 +333,7 @@ if_nullstop(struct ifnet *ifp, int disab
>  }
>
>  void
> -if_nullwatchdog(struct ifnet *ifp)
> +if_nullslowtimo(struct ifnet *ifp)
>  {
>
>         /* Nothing. */
> @@ -637,6 +633,13 @@ if_attach(ifnet_t *ifp)
>
>         /* Announce the interface. */
>         rt_ifannouncemsg(ifp, IFAN_ARRIVAL);
> +
> +       if (ifp->if_slowtimo != NULL) {
> +               ifp->if_slowtimo_ch =
> +                   kmem_zalloc(sizeof(*ifp->if_slowtimo_ch), KM_SLEEP);
> +               callout_init(ifp->if_slowtimo_ch, 0);
> +               if_slowtimo(ifp);
> +       }
>  }
>
>  void
> @@ -687,7 +690,7 @@ if_deactivate(struct ifnet *ifp)
>         ifp->if_ioctl    = if_nullioctl;
>         ifp->if_init     = if_nullinit;
>         ifp->if_stop     = if_nullstop;
> -       ifp->if_watchdog = if_nullwatchdog;
> +       ifp->if_slowtimo = if_nullslowtimo;
>         ifp->if_drain    = if_nulldrain;
>
>         /* No more packets may be enqueued. */
> @@ -736,6 +739,12 @@ if_detach(struct ifnet *ifp)
>
>         s = splnet();
>
> +       if (ifp->if_slowtimo != NULL) {
> +               callout_halt(ifp->if_slowtimo_ch, NULL);
> +               callout_destroy(ifp->if_slowtimo_ch);
> +               kmem_free(ifp->if_slowtimo_ch, sizeof(*ifp->if_slowtimo_ch));
> +       }
> +
>         /*
>          * Do an if_down() to give protocols a chance to do something.
>          */
> @@ -1493,24 +1502,23 @@ if_up(struct ifnet *ifp)
>  }
>
>  /*
> - * Handle interface watchdog timer routines.  Called
> - * from softclock, we decrement timers (if set) and
> + * Handle interface slow timeout routine.  Called
> + * from softclock, we decrement timer (if set) and
>   * call the appropriate interface routine on expiration.
>   */
> -void
> +static void
>  if_slowtimo(void *arg)
>  {
> -       struct ifnet *ifp;
> +       struct ifnet *ifp = arg;
>         int s = splnet();
>
> -       IFNET_FOREACH(ifp) {
> -               if (ifp->if_timer == 0 || --ifp->if_timer)
> -                       continue;
> -               if (ifp->if_watchdog != NULL)
> -                       (*ifp->if_watchdog)(ifp);
> -       }
> +       KASSERT(ifp->if_slowtimo != NULL);
> +
> +       if (ifp->if_timer != 0 && --ifp->if_timer == 0)
> +               (*ifp->if_slowtimo)(ifp);
> +
>         splx(s);
> -       callout_reset(&if_slowtimo_ch, hz / IFNET_SLOWHZ, if_slowtimo, NULL);
> +       callout_reset(ifp->if_slowtimo_ch, hz / IFNET_SLOWHZ, if_slowtimo, ifp);
>  }
>
>  /*
> Index: sys/net/if.h
> ===================================================================
> RCS file: /cvsroot/src/sys/net/if.h,v
> retrieving revision 1.175
> diff -p -u -r1.175 if.h
> --- sys/net/if.h        9 Sep 2014 20:16:12 -0000       1.175
> +++ sys/net/if.h        24 Nov 2014 01:49:14 -0000
> @@ -244,6 +244,7 @@ TAILQ_HEAD(ifnet_head, ifnet);              /* the a
>
>  struct bridge_softc;
>  struct bridge_iflist;
> +struct callout;
>
>  typedef struct ifnet {
>         void    *if_softc;              /* lower-level data for this if */
> @@ -253,7 +254,7 @@ typedef struct ifnet {
>         int     if_pcount;              /* number of promiscuous listeners */
>         struct bpf_if *if_bpf;          /* packet filter structure */
>         u_short if_index;               /* numeric abbreviation for this if */
> -       short   if_timer;               /* time 'til if_watchdog called */
> +       short   if_timer;               /* time 'til if_slowtimo called */
>         short   if_flags;               /* up/down, broadcast, etc. */
>         short   if__pad1;               /* be nice to m68k ports */
>         struct  if_data if_data;        /* statistics and other data about if */
> @@ -274,8 +275,9 @@ typedef struct ifnet {
>                     (struct ifnet *);
>         void    (*if_stop)              /* stop routine */
>                     (struct ifnet *, int);
> -       void    (*if_watchdog)          /* timer routine */
> +       void    (*if_slowtimo)          /* timer routine */
>                     (struct ifnet *);
> +#define        if_watchdog     if_slowtimo
>         void    (*if_drain)             /* routine to release resources */
>                     (struct ifnet *);
>         struct ifaltq if_snd;           /* output queue (includes altq) */
> @@ -341,6 +343,7 @@ typedef struct ifnet {
>             const struct sockaddr *);
>         int (*if_setflags)(struct ifnet *, const short);
>         struct ifnet_lock *if_ioctl_lock;
> +       struct callout *if_slowtimo_ch;
>  } ifnet_t;
>
>  #define        if_mtu          if_data.ifi_mtu
> @@ -867,7 +870,6 @@ void        if_purgeaddrs(struct ifnet *, int,
>  void   if_detach(struct ifnet *);
>  void   if_down(struct ifnet *);
>  void   if_link_state_change(struct ifnet *, int);
> -void   if_slowtimo(void *);
>  void   if_up(struct ifnet *);
>  int    ifconf(u_long, void *);
>  void   ifinit(void);
> @@ -926,7 +928,8 @@ void        if_nullstart(struct ifnet *);
>  int    if_nullioctl(struct ifnet *, u_long, void *);
>  int    if_nullinit(struct ifnet *);
>  void   if_nullstop(struct ifnet *, int);
> -void   if_nullwatchdog(struct ifnet *);
> +void   if_nullslowtimo(struct ifnet *);
> +#define        if_nullwatchdog if_nullslowtimo
>  void   if_nulldrain(struct ifnet *);
>  #else
>  struct if_nameindex {