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 2:01 AM, Taylor R Campbell
<campbell+netbsd-tech-kern%mumble.net@localhost> wrote:
>    Date: Thu, 13 Nov 2014 15:37:29 +0900
>    From: Ryota Ozaki <ozaki-r%netbsd.org@localhost>
>
>    On Thu, Nov 13, 2014 at 1:26 PM, Taylor R Campbell
>    <campbell+netbsd-tech-kern%mumble.net@localhost> wrote:
>    > - You call malloc(M_WAITOK) while the ifnet lock is held, in
>    >   if_alloc_sadl_locked, which is not allowed.
>
>    Oh, I didn't know that restriction. LOCKDEBUG didn't correct me...
>
> We should probably have an ASSERT_SLEEPABLE() in malloc for M_WAITOK,
> and likewise in kmem_(intr_)alloc for KM_SLEEP.
>
> I'd guess we don't have that right now mainly because there's too much
> code that tends to work in practice but would break immediately if we
> made that change.

Yes, there are so many mutex of such usages. In my working area, ifnet_lock
for ioctl and softnet_lock is big walls to prevent us.

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?
- If blocking with holding a mutex does matter, we also should
  avoid nested mutexes?

>
>    > - You call copyout in a pserialize read section, in ifconf, which is
>    >   not allowed because copyout may block.
>
>    Which one? I think I've fixed such usages this time.
>
> I must have misread.  But there are some uses of copyout while the
> ifnet mutex is held, which is not OK.

Got it. I thought calling copyout with holding a mutex is OK...

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.

>
>    > - I don't know what cpu_intr_p is working around but it's probably not
>    >   a good idea!
>
>    Yes :-| We have to fix that in the future, but it works as same as it is
>    until we get rid of all KERNE_LOCK.
>
> Can you explain what it is there for?
>
> 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.

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

>
>    > Generally, all that you are allowed to do in a pserialize read section
>    > is read a small piece of information, or grab a reference to a data
>    > structure which you are then going to use outside the read section.
>
>    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
replace ifunit with ifget on holding an ifp and call ifput when releasing
the ifp. ifget may return NULL when an ifp is absent or being destroyed.
So basic usage is like this:

  ifp = ifget(name);
  if (ifp == NULL)
    return ENXIO;
  // do something on ifp
  ifput(ifp);

It uses an adaptive lock so it cannot be used in hardware interrupt
(it's ok because ifunit isn't used in it).

On ifget, we don't need locking, however, we need locking on ifput
to call cv_wait safely. It's good to remove the locking somehow.

>
>    > I don't think it's going to be easy to scalably parallelize this code
>    > without restructuring it, unless as a stop-gap you use a heaver-weight
>    > reader-writer lock like the prwlock at
>    > <https://www.NetBSD.org/~riastradh/tmp/20140517/rwlock/prwlock.c>.
>    > (No idea how much overhead this might add.)
>
>    Could you elaborate how to use it?
>
> Rules are the same as those for rwlocks, except you are allowed to
> block while holding a reader or writer.
>
> (Caveat: I haven't tested that code at all.  It should go through some
> review if we want to actually use it in-tree.)
>
> #include <sys/prwlock.h>
>
> struct prwlock *ifnet_lock __read_mostly;
>
> init()
> {
>         ifnet_lock = prwlock_create("ifnet");
> }
>
> fini()
> {
>         prwlock_destroy(ifnet_lock);
> }
>
> readem()
> {
>         struct prw_reader *reader;
>
>         prwlock_reader_enter(ifnet_lock, &reader);
>         IFNET_FOREACH(ifp) { ... };
>         prwlock_reader_exit(ifnet_lock, reader);
> }
>
> changeit()
> {
>         struct prw_writer *writer;
>
>         prwlock_writer_enter(ifnet_lock, &writer);
>         IFNET_FOREACH(ifp) { frobnitz(ifp); }
>         prwlock_writer_exit(ifnet_lock, writer);
> }

Thanks. It looks what I want :) Hmm, testing the facility seems hard.

  ozaki-r