From owner-p4-projects@FreeBSD.ORG Mon Sep 17 22:28:31 2007 Return-Path: Delivered-To: p4-projects@freebsd.org Received: by hub.freebsd.org (Postfix, from userid 32767) id 25FE916A41B; Mon, 17 Sep 2007 22:28:31 +0000 (UTC) Delivered-To: perforce@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id B822416A41A for ; Mon, 17 Sep 2007 22:28:30 +0000 (UTC) (envelope-from zec@icir.org) Received: from xaqua.tel.fer.hr (xaqua.tel.fer.hr [161.53.19.25]) by mx1.freebsd.org (Postfix) with ESMTP id 6139813C474 for ; Mon, 17 Sep 2007 22:28:30 +0000 (UTC) (envelope-from zec@icir.org) Received: by xaqua.tel.fer.hr (Postfix, from userid 20006) id 194A39B645; Tue, 18 Sep 2007 00:28:29 +0200 (CEST) X-Spam-Checker-Version: SpamAssassin 3.1.7 (2006-10-05) on xaqua.tel.fer.hr X-Spam-Level: X-Spam-Status: No, score=-4.3 required=5.0 tests=ALL_TRUSTED,AWL,BAYES_00 autolearn=ham version=3.1.7 Received: from localhost (imunes.tel.fer.hr [161.53.19.8]) by xaqua.tel.fer.hr (Postfix) with ESMTP id A8BBA9B644; Tue, 18 Sep 2007 00:28:27 +0200 (CEST) From: Marko Zec To: "Kip Macy" Date: Tue, 18 Sep 2007 00:28:23 +0200 User-Agent: KMail/1.9.7 References: <200708212351.l7LNpi6Q006480@repoman.freebsd.org> <46EED397.3040700@elischer.org> In-Reply-To: MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-Id: <200709180028.24481.zec@icir.org> Cc: Perforce Change Reviews , Julian Elischer Subject: Re: PERFORCE change 125520 for review X-BeenThere: p4-projects@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: p4 projects tree changes List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 17 Sep 2007 22:28:31 -0000 On Monday 17 September 2007 21:43:50 Kip Macy wrote: > On 9/17/07, Julian Elischer wrote: > > Marko Zec wrote: > > > On Monday 17 September 2007 20:03:59 Julian Elischer wrote: > > >> Marko Zec wrote: > > >>> http://perforce.freebsd.org/chv.cgi?CH=125520 > > >>> > > >>> Change 125520 by zec@zec_tpx32 on 2007/08/21 23:51:39 > > >>> > > >>> Given that ng_pipe nodes can be connected into arbitrary > > >>> topologies, and therefore it is possible for ngp_rcvdata() > > >>> to be recursively called from a single thread, it is > > >>> necessary to explicitly allow for the ng_pipe_giant mutex > > >>> to be recursively acquireable. > > >> > > >> OR use a different locking scheme. > > > > > > That's right, but I'm just wondering is there anything > > > fundamentally wrong with lock recursing (both in general as well > > > as in this particular case)? > > > > we are trying as a general rule trying to keep lock recursion to an > > absolute minimum. It can make debugging other things very hard. and > > can introduce bugs that are hard to find.. > > > > Generally a bad idea. If you don't know you are recursing, how can > > you avoid the problems you don't know about? (sounds silly but..) > > Just to add my 0.02$ from recent experiences ... > Lock recursion creates the following problems: > - lock is often held much longer than it really needs to be > - it is no longer possible from code inspection to determine > where the lock is or is not held > - it makes it near impossible to mix shared and exclusive access > to a lock, acquiring an exclusive lock that you already hold shared > results in a deadlock and vice versa > - it makes lock ordering more complicated to infer and work with > > So ... from code reading experience recently recursive locks are > typically needed for code where the control flow pre-dates any > thoughts of locking. ...which precisely might often be the case with netgraph, where it is impossible to know beforehand how the interconnection topology would look like, nor can it be always determined upfront which paths through the topology (i.e. translating to a call graph) would or would not be taken. Anyhow, point taken. Though given that ng_pipe is not really a critical part of our infrastructure, but more like a toy I'm occassionaly playing with, I'll take some time before more thoroughly thinking of and implementing an alternative locking scheme. Marko > -Kip > > > > Marko > > > > > >> i.e. reference counts or something. > > >> > > >>> Affected files ... > > >>> > > >>> .. //depot/projects/vimage/src/sys/netgraph/ng_pipe.c#2 edit > > >>> > > >>> Differences ... > > >>> > > >>> ==== //depot/projects/vimage/src/sys/netgraph/ng_pipe.c#2 > > >>> (text+ko) ==== > > >>> > > >>> @@ -1028,7 +1028,7 @@ > > >>> error = EEXIST; > > >>> else { > > >>> mtx_init(&ng_pipe_giant, "ng_pipe_giant", > > >>> NULL, - MTX_DEF); > > >>> + MTX_DEF | MTX_RECURSE); > > >>> LIST_INIT(&node_head); > > >>> LIST_INIT(&hook_head); > > >>> ds_handle = timeout((timeout_t *) > > >>> &pipe_scheduler,