Date: Wed, 14 Jul 2004 19:42:54 +0400 From: Gleb Smirnoff <glebius@freebsd.org> To: Robert Watson <rwatson@freebsd.org>, phk@freebsd.org Cc: current@freebsd.org Subject: Re: Some netgraph node global locking patches Message-ID: <20040714154254.GB9999@cell.sick.ru> In-Reply-To: <Pine.NEB.3.96L.1040714101817.83353G-100000@fledge.watson.org> References: <20040714130120.GA7897@cell.sick.ru> <Pine.NEB.3.96L.1040714101817.83353G-100000@fledge.watson.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20040714154254.GB9999>