Re: MP-safe IPsec SPD

[Taylor R Campbell <campbell+netbsd-tech-kern%mumble.net@localhost>]

> Date: Fri, 28 Jul 2017 16:47:15 +0900
> From: Ryota Ozaki <ozaki-r%netbsd.org@localhost>
> 
> 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.)