Re: Some netgraph node global locking patches

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_at_fledge.watson.org      Principal Research Scientist, McAfee Research