Date: Thu, 13 Nov 2014 15:37:29 +0900 From: Ryota Ozaki <ozak...@netbsd.org>
On Thu, Nov 13, 2014 at 1:26 PM, Taylor R Campbell <campbell+netbsd-tech-k...@mumble.net> 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); }