Re: MP-safe IPsec SPD

[Ryota Ozaki <ozaki-r%netbsd.org@localhost>]

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-kern%mumble.net@localhost> wrote:
>> 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.)