Re: Some netgraph node global locking patches

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_at_ 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