Hi, Thank you for your reviewing! I think I handled all your suggestions. Let me know if I missed or misunderstood something.
BTW can we have an API to assert that we are in a pserialize critical section? Sometimes I want to write something like: KASSERT(pserialize_in_read_critical_section()); rather than writing the constraint in a comment. Thanks, ozaki-r On Fri, Aug 4, 2017 at 12:02 AM, Taylor R Campbell <campbell+netbsd-tech-k...@mumble.net> wrote: >> Date: Fri, 28 Jul 2017 16:47:15 +0900 >> From: Ryota Ozaki <ozak...@netbsd.org> >> >> This is a patch to make IPsec SPD MP-safe: >> http://www.netbsd.org/~ozaki-r/mpsafe-ipsec-spd.diff >> >> Please check the locking notes in the patch to know >> how I MP-ify SPD. >> >> This is the first time for me to use localcount(9) >> so please comment/suggest me on it. > > Sorry, haven't had time for a detailed review! Some brief comments: > > General approach looks reasonable. Haven't had time to digest it all, > but nothing leaps out at me as wrong about your use of localcount. > > Glad to see the detailed locking notes. That will be helpful for > proper review. > - One typo: `pstree' should be `sptree'. (Aside: Why `tree', if list?) > - Can you say how key_mtx fits into it too? > - Can you add to that comment that you must KEY_SP_REF during psz > read, and KEY_SP_UNREF after use? (Rather than saying `incrementing > reference count', just to make it easier to cross-reference comment > with API.) > - Can you put a comment over key_sp_ref saying what the caller's > responsibilities are? (Must be in psz read section, must release > with key_sp_unref outside psz read section later.) > > /* > * Protect regtree, acqtree and items stored in the lists. > */ > static kmutex_t key_mtx __cacheline_aligned; > +static pserialize_t key_psz; > +static kmutex_t key_sp_mtx __cacheline_aligned; > +static kcondvar_t key_sp_cv __cacheline_aligned; > > Consider gathering these into a cacheline-aligned struct along with > the lists they cover? And/or split the two locks up with a comment > saying what each one is for? > > +void > +key_sp_unref(struct secpolicy *sp, const char* where, int tag) > +{ > + > + KASSERT(mutex_ownable(&key_sp_mtx)); > > I suggest using KDASSERT for mutex_ownable, so that it happens only > when you've enabled DEBUG *and* LOCKDEBUG -- it is a very expensive > check, moreso than LOCKDEBUG normally is, since it acquires and > releases a lock, and it is normally used in a path where we expect > *not* to acquire and release a lock in a fast path. > > @@ -279,7 +295,6 @@ ipsec_fillpcbcache(struct inpcbpolicy *pcbsp, struct > mbuf *m, > } > pcbsp->sp_cache[dir].cachesp = sp; > if (pcbsp->sp_cache[dir].cachesp) { > - KEY_SP_REF(pcbsp->sp_cache[dir].cachesp); > KEYDEBUG_PRINTF(KEYDEBUG_IPSEC_STAMP, > "DP cause refcnt++:%d SP:%p\n", > key_sp_refcnt(pcbsp->sp_cache[dir].cachesp), > > It looks like this debug message doesn't make sense any more? (Aside > from the stub definition of key_sp_refcnt.)