On Mon, Nov 24, 2014 at 3:26 PM, Masao Uebayashi <uebay...@gmail.com> 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-k...@mumble.net> wrote: >> Date: Mon, 24 Nov 2014 11:23:28 +0900 >> From: Masao Uebayashi <uebay...@gmail.com> >> >> 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!
