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"