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

[Masao Uebayashi <uebayasi%gmail.com@localhost>]

(I'm trying, but I can't follow up all mails soon, because I need more
than x2 energy & time to write English than you.)

On Mon, Nov 24, 2014 at 12:12 PM, Taylor R Campbell
<campbell+netbsd-tech-kern%mumble.net@localhost> wrote:
>    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

I was thinking of this first.

> (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.

Good point.  There must be a way for drivers to declare if
CALLOUT_MPSAFE or not.  Need to extend if_flags.

> 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?

I have believed that pserialize(9) reader-side is a critical section.
pserialize(9) relies on scheduler to notify that readers have passed
throught those pserialize(9) protected code paths, by calling
pserialize_switchpoint() from mi_switch().  This obviously implies
that threads can't sleep from within those pserialize(9) protected
code paths.  Otherwise that notification has a different meaning.

I think pserialize_read_{enter,exit}() should explicitly call
KPREEMPT_{DISABLE,ENABLE}(), as is done in percpu_{getref,putref}().

> 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.

I think it's fine to enter pserialize(9)'ed code in callouts.

> 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.

Yes, netstat(1) uses sysctl(3) to fetch statistics numbers.  But
netstat(1) needs to know struct type information (by building if.c) to
read kernel cores via kvm(3).  Maybe making kvm(3) to understand DWARF
type information helps?

>    -    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.

Applied locally.  Thanks!