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 3:26 PM, Masao Uebayashi <uebayasi%gmail.com@localhost> wrote:
> (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.

Hmm, if_flags.

http://mail-index.netbsd.org/tech-net/2009/01/27/msg000985.html

Do we have to care about kvm(3) users (i.e., netstat) as well as
the callout_t issue?

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

I don't understand how kvm(3) works though, if we don't change existing
members of struct ifnet, can old netstat(1) run on a new kernel that
adds a value hidden in _KERNEL to struct ifnet?

  ozaki-r

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