svn commit: r262806 - head/sys/net

[Adrian Chadd]

... why's the code returning locked mutexes to UMA?

Why not fix the places that are doing this and doing a lock assertion
in the destructor? What's this buy us?

I'm very wary of design patterns like this that do conditional
unlocking in free/destructor routines; it allows for the caller to not
necessarily get all the lock/unlock stuff to line up in the main code
and it can make debugging more of a pain.

Thanks,



-a


On 5 March 2014 13:16, Gleb Smirnoff <glebius at freebsd.org> wrote:
> Author: glebius
> Date: Wed Mar  5 21:16:46 2014
> New Revision: 262806
> URL: http://svnweb.freebsd.org/changeset/base/262806
>
> Log:
>   The route code used to mtx_destroy() a locked mutex before rtentry free. Now,
>   after r262763 it started to return locked mutexes to UMA. To fix that,
>   conditionally unlock the mutex in the destructor.
>
>   Tested by:    "Sergey V. Dyatko" <sergey.dyatko at gmail.com>
>
> Modified:
>   head/sys/net/route.c
>   head/sys/net/route.h
>
> Modified: head/sys/net/route.c
> ==============================================================================
> --- head/sys/net/route.c        Wed Mar  5 20:01:04 2014        (r262805)
> +++ head/sys/net/route.c        Wed Mar  5 21:16:46 2014        (r262806)
> @@ -237,6 +237,14 @@ rtentry_ctor(void *mem, int size, void *
>  }
>
>  static void
> +rtentry_dtor(void *mem, int size, void *arg)
> +{
> +       struct rtentry *rt = mem;
> +
> +       RT_UNLOCK_COND(rt);
> +}
> +
> +static void
>  vnet_route_init(const void *unused __unused)
>  {
>         struct domain *dom;
> @@ -248,7 +256,7 @@ vnet_route_init(const void *unused __unu
>             sizeof(struct radix_node_head *), M_RTABLE, M_WAITOK|M_ZERO);
>
>         V_rtzone = uma_zcreate("rtentry", sizeof(struct rtentry),
> -           rtentry_ctor, NULL,
> +           rtentry_ctor, rtentry_dtor,
>             rtentry_zinit, rtentry_zfini, UMA_ALIGN_PTR, 0);
>         for (dom = domains; dom; dom = dom->dom_next) {
>                 if (dom->dom_rtattach == NULL)
>
> Modified: head/sys/net/route.h
> ==============================================================================
> --- head/sys/net/route.h        Wed Mar  5 20:01:04 2014        (r262805)
> +++ head/sys/net/route.h        Wed Mar  5 21:16:46 2014        (r262806)
> @@ -309,6 +309,10 @@ struct rt_addrinfo {
>  #define        RT_UNLOCK(_rt)          mtx_unlock(&(_rt)->rt_mtx)
>  #define        RT_LOCK_DESTROY(_rt)    mtx_destroy(&(_rt)->rt_mtx)
>  #define        RT_LOCK_ASSERT(_rt)     mtx_assert(&(_rt)->rt_mtx, MA_OWNED)
> +#define        RT_UNLOCK_COND(_rt)     do {                            \
> +       if (mtx_owned(&(_rt)->rt_mtx))                          \
> +               mtx_unlock(&(_rt)->rt_mtx);                     \
> +} while (0)
>
>  #define        RT_ADDREF(_rt)  do {                                    \
>         RT_LOCK_ASSERT(_rt);                                    \
>