From owner-freebsd-current@FreeBSD.ORG Wed Jul 14 15:43:02 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 82BD716A4CE; Wed, 14 Jul 2004 15:43:02 +0000 (GMT) Received: from cell.sick.ru (cell.sick.ru [217.72.144.68]) by mx1.FreeBSD.org (Postfix) with ESMTP id AB7AF43D4C; Wed, 14 Jul 2004 15:42:56 +0000 (GMT) (envelope-from glebius@freebsd.org) Received: from cell.sick.ru (glebius@localhost [127.0.0.1]) by cell.sick.ru (8.12.11/8.12.8) with ESMTP id i6EFgsug010082 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Wed, 14 Jul 2004 19:42:55 +0400 (MSD) (envelope-from glebius@freebsd.org) Received: (from glebius@localhost) by cell.sick.ru (8.12.11/8.12.11/Submit) id i6EFgs0L010081; Wed, 14 Jul 2004 19:42:54 +0400 (MSD) (envelope-from glebius@freebsd.org) X-Authentication-Warning: cell.sick.ru: glebius set sender to glebius@freebsd.org using -f Date: Wed, 14 Jul 2004 19:42:54 +0400 From: Gleb Smirnoff To: Robert Watson , phk@freebsd.org Message-ID: <20040714154254.GB9999@cell.sick.ru> References: <20040714130120.GA7897@cell.sick.ru> Mime-Version: 1.0 Content-Type: text/plain; charset=koi8-r Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.6i 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 15:43:02 -0000 On Wed, Jul 14, 2004 at 10:25:31AM -0400, Robert Watson wrote: R> > On Wed, Jul 14, 2004 at 12:30:40AM -0400, Robert Watson wrote: R> > R> //depot/vendor/freebsd/src/sys/netgraph/ng_eiface.c R> > R> //depot/vendor/freebsd/src/sys/netgraph/ng_fec.c R> > R> //depot/vendor/freebsd/src/sys/netgraph/ng_iface.c R> > R> > Well, these three are quite straightforward and identical. Look fine. R> R> I was somewhat hoping someone would actually give them a try and R> demonstrate that practice matches the theory. :-) I can test ng_iface. Since change is similar we could assume ng_eiface, ng_fec tested, then. Would it be enough to test on UP hardware? R> > R> //depot/vendor/freebsd/src/sys/netgraph/ng_ppp.c R> > R> > Is there any hidden obstacles for merging qsort_r() from libc to R> > libkern? It will help to remove this ugly hack. R> R> My recollection of the quicksort algorithm is extremely vague, but I R> recall that it involves recursion, and recursion is generally bad in the R> kernel due to stack depth. Yes it does. But qsort() already used in ng_ppp is as much recursive as qsort_r() is. It will help us to get rid of global variable. I Cc phk@ to this mail, because he copied qsort() to libkern from libc. R> > R> //depot/vendor/freebsd/src/sys/netgraph/ng_pppoe.c R> > R> > Agreed with comment. Anyway, I'm planning to move this configuration R> > trigger to private data. sysctl's are not very elegant way to set R> > configaration of netgraph node. Moreover, I can imagine setup when you R> > need to serve non-standard PPPoE only on one interface, and normal PPPoE R> > on other interfaces: for example two different networks merged to use R> > one AC. R> R> This sounds good. I've chatted some with Julian about locking down access R> to private data, but I've not had a chance to really digest and think R> through it yet. Is this something you're interested in looking at? :-) Yes, it is. AFAIU, at this moment netgraph locks node in way that only one netgraph callback can run at a time. It means one thread per node. So there is no need to lock private data separately. Do I miss smth? R> > R> //depot/vendor/freebsd/src/sys/netgraph/ng_tty.c R> > R> +/* R> > R> + * XXXRW: ngt_unit is protected by ng_tty_mtx. ngt_ldisc is constant once R> > R> + * ng_tty is initialized. ngt_nodeop_ok is untouched, and might want to be a R> > R> + * sleep lock in the future? R> > R> + */ R> > R> > One question: are any locks held when linesw callbacks (ngt_open, R> > ngt_close, etc..) are called? R> R> Hmm. That is an interesting problem indeed. The tty code currently R> requires Giant, although Poul-Henning is doing cleanup that will hopefully R> lead to locking at some point. In the SLIP code, I currently use a R> taskqueue to defer calls into the tty code so that Giant is not grabbed at R> arbitrary points in the SLIP processing, which may be called into while R> holding protocol layer locks. It looks like a lot of the tty access in R> ng_tty.c is done from the device node calls associated with the tty R> subsystem, and therefore Giant will already be held (which is fine, R> although not 100% desirable). The problematic bit is the call to R> ngt_start() in ngt_rcvdata(), and synchronizing the mbuf queue in the node R> between various consumers. We could very easily use the same trick here I R> use in SLIP by deferring ngt_start() to a Giant-holding task queue. We R> could add a 'struct task' to the node softc and just use that, for R> example. I have to get better understanding of tty code, then make comments :) May be Poul-Henning has some ideas. -- Totus tuus, Glebius. GLEBIUS-RIPN GLEB-RIPE