From owner-freebsd-current@FreeBSD.ORG Wed Jul 14 14:25:49 2004 Return-Path: Delivered-To: freebsd-current@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 90ADC16A4CE; Wed, 14 Jul 2004 14:25:49 +0000 (GMT) Received: from fledge.watson.org (fledge.watson.org [204.156.12.50]) by mx1.FreeBSD.org (Postfix) with ESMTP id 2464343D31; Wed, 14 Jul 2004 14:25:49 +0000 (GMT) (envelope-from robert@fledge.watson.org) Received: from fledge.watson.org (localhost [127.0.0.1]) by fledge.watson.org (8.12.11/8.12.11) with ESMTP id i6EEPVD7052653; Wed, 14 Jul 2004 10:25:31 -0400 (EDT) (envelope-from robert@fledge.watson.org) Received: from localhost (robert@localhost)i6EEPVgU052650; Wed, 14 Jul 2004 10:25:31 -0400 (EDT) (envelope-from robert@fledge.watson.org) Date: Wed, 14 Jul 2004 10:25:31 -0400 (EDT) From: Robert Watson X-Sender: robert@fledge.watson.org To: Gleb Smirnoff In-Reply-To: <20040714130120.GA7897@cell.sick.ru> Message-ID: MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII cc: current@freebsd.org Subject: Re: Some netgraph node global locking patches X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.1 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 14 Jul 2004 14:25:49 -0000 On Wed, 14 Jul 2004, Gleb Smirnoff wrote: > On Wed, Jul 14, 2004 at 12:30:40AM -0400, Robert Watson wrote: > R> //depot/vendor/freebsd/src/sys/netgraph/ng_eiface.c > R> //depot/vendor/freebsd/src/sys/netgraph/ng_fec.c > R> //depot/vendor/freebsd/src/sys/netgraph/ng_iface.c > > Well, these three are quite straightforward and identical. Look fine. I was somewhat hoping someone would actually give them a try and demonstrate that practice matches the theory. :-) > R> //depot/vendor/freebsd/src/sys/netgraph/ng_ppp.c > > Is there any hidden obstacles for merging qsort_r() from libc to > libkern? It will help to remove this ugly hack. My recollection of the quicksort algorithm is extremely vague, but I recall that it involves recursion, and recursion is generally bad in the kernel due to stack depth. > R> //depot/vendor/freebsd/src/sys/netgraph/ng_pppoe.c > > Agreed with comment. Anyway, I'm planning to move this configuration > trigger to private data. sysctl's are not very elegant way to set > configaration of netgraph node. Moreover, I can imagine setup when you > need to serve non-standard PPPoE only on one interface, and normal PPPoE > on other interfaces: for example two different networks merged to use > one AC. This sounds good. I've chatted some with Julian about locking down access to private data, but I've not had a chance to really digest and think through it yet. Is this something you're interested in looking at? :-) > R> //depot/vendor/freebsd/src/sys/netgraph/ng_tty.c > R> +/* > R> + * XXXRW: ngt_unit is protected by ng_tty_mtx. ngt_ldisc is constant once > R> + * ng_tty is initialized. ngt_nodeop_ok is untouched, and might want to be a > R> + * sleep lock in the future? > R> + */ > > One question: are any locks held when linesw callbacks (ngt_open, > ngt_close, etc..) are called? Hmm. That is an interesting problem indeed. The tty code currently requires Giant, although Poul-Henning is doing cleanup that will hopefully lead to locking at some point. In the SLIP code, I currently use a taskqueue to defer calls into the tty code so that Giant is not grabbed at arbitrary points in the SLIP processing, which may be called into while holding protocol layer locks. It looks like a lot of the tty access in ng_tty.c is done from the device node calls associated with the tty subsystem, and therefore Giant will already be held (which is fine, although not 100% desirable). The problematic bit is the call to ngt_start() in ngt_rcvdata(), and synchronizing the mbuf queue in the node between various consumers. We could very easily use the same trick here I use in SLIP by deferring ngt_start() to a Giant-holding task queue. We could add a 'struct task' to the node softc and just use that, for example. Robert N M Watson FreeBSD Core Team, TrustedBSD Projects robert@fledge.watson.org Principal Research Scientist, McAfee Research