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

[Ryota Ozaki <ozaki-r%netbsd.org@localhost>]

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

Understood. Thank you for the explanations.

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

Okay. I'll try to remove copyout from the critical sections.

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

Sorry for that. I'll describe about that.

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

Really? If so, we can get rid of it easier than I expected.
I'm looking at it!

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

I'll describe it too.

  ozaki-r