From owner-p4-projects@FreeBSD.ORG Mon Sep 17 19:43:52 2007 Return-Path: Delivered-To: p4-projects@freebsd.org Received: by hub.freebsd.org (Postfix, from userid 32767) id D2D5316A420; Mon, 17 Sep 2007 19:43:51 +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 1845416A419 for ; Mon, 17 Sep 2007 19:43:51 +0000 (UTC) (envelope-from kip.macy@gmail.com) Received: from rv-out-0910.google.com (rv-out-0910.google.com [209.85.198.189]) by mx1.freebsd.org (Postfix) with ESMTP id 033F613C474 for ; Mon, 17 Sep 2007 19:43:50 +0000 (UTC) (envelope-from kip.macy@gmail.com) Received: by rv-out-0910.google.com with SMTP id l15so1295547rvb for ; Mon, 17 Sep 2007 12:43:50 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=beta; h=domainkey-signature:received:received:message-id:date:from:to:subject:cc:in-reply-to:mime-version:content-type:content-transfer-encoding:content-disposition:references; bh=a6W2rb3Tg+FfncW/p4ix31yNFU1JKBYwodvzb8wqpgE=; b=KG0MWaKKFin0BWau5lZYAlPIlSMOyyyPXR+IH0ewAPTc2OvIWyq7GAwGVd3bfFZ2V8TTuVte8GGoXUScdeGsSbq0ZrpiO0DYCzqOXrUC2i05pQsnmJPKDza4/Nv3nOiJ+KkCFPDUV4ANFOb5hpCqaJjXpJMZD2k+XealGO5aF1M= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=beta; h=received:message-id:date:from:to:subject:cc:in-reply-to:mime-version:content-type:content-transfer-encoding:content-disposition:references; b=H4YocwQfM8Q7isWock78r5wEmtFnC0xELYq/YTOG9BZ8ld2zQ8VWxNKvJ52tqxp4qjLN16jgMlBZdYbd77yo1TQJsB296zv1oAkb+HI8qV/JpuHysZhu1Yc5lT3yz/1yGaTlLPTSgXwbeaMITDv0dveD3Y4D7KwyCd5DvyB1SP8= Received: by 10.114.146.1 with SMTP id t1mr3713575wad.1190058230092; Mon, 17 Sep 2007 12:43:50 -0700 (PDT) Received: by 10.114.13.15 with HTTP; Mon, 17 Sep 2007 12:43:50 -0700 (PDT) Message-ID: Date: Mon, 17 Sep 2007 12:43:50 -0700 From: "Kip Macy" To: "Julian Elischer" In-Reply-To: <46EED397.3040700@elischer.org> MIME-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Content-Disposition: inline References: <200708212351.l7LNpi6Q006480@repoman.freebsd.org> <46EEC18F.6000809@elischer.org> <200709172048.29253.zec@icir.org> <46EED397.3040700@elischer.org> Cc: Marko Zec , Perforce Change Reviews 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 19:43:52 -0000 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. -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, > > > >