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

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

   > - 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.  If we need to take it interrupt context, why not
make it an IPL_NET mutex?

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

   > 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);
}