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: Fri, 14 Nov 2014 13:59:10 +0900
   From: Ryota Ozaki <ozaki-r%netbsd.org@localhost>

   Related to mutex I have two questions:
   - If we really want to alloc an object with holding a mutex,
     what should we do? use pool(9)? or we should avoid such a situation?

Preallocate before you acquire the lock.  See the kmem(9) man page for
an example.

   - If blocking with holding a mutex does matter, we also should
     avoid nested mutexes?

No, nested locks are OK, as long as you only nest ascending IPLs.  The
intent is that each lock should be held for a limited -- preferably
small constant -- duration, independent of I/O.

Although acquiring an adaptive lock technically blocks, that's more a
power-conserving feature -- it doesn't have to do with I/O or resource
management like waiting on a condvar for a driver interrupt or like
waiting for memory allocation.

   Well, honestly said, I want them as they are at this point of this work
   because they aren't performance critical paths. Of course, we should
   tackle them in the future though.

The copyout path may not be performance-critical itself, but it will
block every other path that acquires the lock, and if the copyout path
makes threads in those other paths hang, it's annoying to diagnose why
those threads are hanging -- the mutex abstraction is not designed for
cases like that.

   > Wild guess: ifnet_mtx is an IPL_NONE mutex, so we can't take it in
   > interrupt context.

   Yes.

   > If we need to take it interrupt context, why not
   > make it an IPL_NET mutex?

   Because the mutex (ifnet_mtx) is used for blockable sections
   (this assumption is denied though...) and pserialize_perform
   cannot be used with a spin mutex IIUC.

I see.  Ugh.  OK, at the very least, you should write a big scary
comment among the locking rules for the ifnet list that explains these
constraints and lays out the roadmap for obviating them.

Otherwise (and even so), someone is likely to remove the kernel lock
without understanding the constraints and wind up with an interrupt
handler on cpu0 munging the ifnet list at the same time as a thread on
cpu1.

   Anyway we should get rid of calling m_reclaim in hardware interrupt
   as rmind said.

Sounds good to me!  Seems like the only actual user of it in-tree is
ieee1394if.

   >    Yes. I'm implementing a facility of the latter for ifunit:
   >    http://www.netbsd.org/~ozaki-r/ifget-ifput.diff
   >
   > Looks a little better.  Have you written down the locking scheme and
   > rules for usage?

   ifget/ifput are ifunit with reference counting. To use them, we need to
   [...]

Don't just tell me -- write the rules down in comments around the
relevant code!  (I will take a closer look later.)