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

[Taylor R Campbell <campbell+netbsd-tech-kern%mumble.net@localhost>]

   Date: Mon, 24 Nov 2014 11:23:28 +0900
   From: Masao Uebayashi <uebayasi%gmail.com@localhost>

   http://marc.info/?t=141670552700001&r=1&w=2

   Following the ideas raised in that thread:

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

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

Can you (or ozaki-r@, whose earlier patch I missed until now) explain
specifically what this accomplishes?  I have two guesses about the
primary goal of this change: either

(a) to obviate the need to run if_watchdog/if_slowtimo callbacks
inside IFNET_FOREACH, or

(b) to obviate the need to run IFNET_FOREACH in a callout,

with a minor added bonus that each interface's watchdog/slowtimo can
run in parallel once we flip the CALLOUT_MPSAFE switch on them.

In case (a), what might an interface do in an if_watchdog/if_slowtimo
callback that is safe in a callout but not safe inside a pserialized
reader?  Is it simply that it's sort-of-kind-of OK for a callout to
block a little, but absolutely not OK for a pserialized reader to
block and thus switch?

In case (b), if we're going to use pserialized reads for IFNET_FOREACH
anyway, why would it matter that we don't run IFNET_FOREACH at a
callout?  Callouts are at IPL_SOFTCLOCK, and so are pserialized
readers.

If it's just the `minor added bonus' and general disentanglement of
the network stack, that sounds good to me too -- I'm just curious to
see a statement of what the purpose is.

Also, it seems the reason you're allocating the struct callout
dynamically is to allow userland to read struct ifnet without having
to see the guts of the callout.  But does that matter?  netstat(1) is
one of the last programs that uses kvm(3) to report kernel statistics,
and it seems to me we ought to publish the information via sysctl
instead.

   -	callout_reset(&if_slowtimo_ch, hz / IFNET_SLOWHZ, if_slowtimo, NULL);
   +	callout_reset(ifp->if_slowtimo_ch, hz / IFNET_SLOWHZ, if_slowtimo, ifp);

Prefer using callout_setfunc up front and using callout_schedule in
if_slowtimo, rather than callout_reset.  Makes it easier to be sure
what the callout is doing.