svn commit: r262806 - head/sys/net
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"
Modified:
head/sys/net/route.c
head/sys/net/route.h
Modified: head/sys/net/route.c
==
--- head/sys/net/route.cWed Mar 5 20:01:04 2014(r262805)
+++ head/sys/net/route.cWed 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.hWed Mar 5 20:01:04 2014(r262805)
+++ head/sys/net/route.hWed Mar 5 21:16:46 2014(r262806)
@@ -309,6 +309,10 @@ struct rt_addrinfo {
#defineRT_UNLOCK(_rt) mtx_unlock(&(_rt)->rt_mtx)
#defineRT_LOCK_DESTROY(_rt)mtx_destroy(&(_rt)->rt_mtx)
#defineRT_LOCK_ASSERT(_rt) mtx_assert(&(_rt)->rt_mtx, MA_OWNED)
+#defineRT_UNLOCK_COND(_rt) do {\
+ if (mtx_owned(&(_rt)->rt_mtx)) \
+ mtx_unlock(&(_rt)->rt_mtx); \
+} while (0)
#defineRT_ADDREF(_rt) do {\
RT_LOCK_ASSERT(_rt);\
___
svn-src-head@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r262806 - head/sys/net
... 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 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"
>
> Modified:
> head/sys/net/route.c
> head/sys/net/route.h
>
> Modified: head/sys/net/route.c
> ==
> --- head/sys/net/route.cWed Mar 5 20:01:04 2014(r262805)
> +++ head/sys/net/route.cWed 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.hWed Mar 5 20:01:04 2014(r262805)
> +++ head/sys/net/route.hWed Mar 5 21:16:46 2014(r262806)
> @@ -309,6 +309,10 @@ struct rt_addrinfo {
> #defineRT_UNLOCK(_rt) mtx_unlock(&(_rt)->rt_mtx)
> #defineRT_LOCK_DESTROY(_rt)mtx_destroy(&(_rt)->rt_mtx)
> #defineRT_LOCK_ASSERT(_rt) mtx_assert(&(_rt)->rt_mtx, MA_OWNED)
> +#defineRT_UNLOCK_COND(_rt) do {\
> + if (mtx_owned(&(_rt)->rt_mtx)) \
> + mtx_unlock(&(_rt)->rt_mtx); \
> +} while (0)
>
> #defineRT_ADDREF(_rt) do {\
> RT_LOCK_ASSERT(_rt);\
>
___
svn-src-head@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r262806 - head/sys/net
On Wed, Mar 05, 2014 at 03:29:55PM -0800, Adrian Chadd wrote: A> ... why's the code returning locked mutexes to UMA? A> A> Why not fix the places that are doing this and doing a lock assertion A> in the destructor? What's this buy us? A> A> I'm very wary of design patterns like this that do conditional A> unlocking in free/destructor routines; it allows for the caller to not A> necessarily get all the lock/unlock stuff to line up in the main code A> and it can make debugging more of a pain. I get your point, and even share it. I discussed that with melifaro@, he prefers destructors being smart. Note that mutex(9) by design allows to destroy a locked mutex. And route(4) relied on this semantics. Now I converted route(4) to lazy mutex deallocation. Of course, if mutex(9) didn't allow such trick, I wouldn't implement the code I did. But since code of route(4) was not considered broken before, I decided to keep status quo. -- Totus tuus, Glebius. ___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
