New TTY layer: condvar(9) and Giant

[John Baldwin]

On Thursday 13 March 2008 09:50:35 am Ed Schouten wrote:
> Hello everyone,
>
> Almost a month ago I started working on my assignment for my internship,
> to reimplement a new TTY layer that fixes a lot of architectural
> problems. So far, things are going quite fast:
>
> - I've already implemented a basic TTY layer, which has support for
>   canonical and non-canonical mode. It still misses important features
>   including flow control, but it seems to work quite good. Unlike the
>   old layer, it doesn't buffer data as much, which should hopefully mean
>   it's a bit faster.
> - I'm using a new PTY driver called pts(4). It works quite good, but it
>   misses the compatibility bits, which we'll need to have to support
>   older FreeBSD or Linux binaries.
> - Some of you may have read I'm working on syscons now. I've got syscons
>   working with the new TTY layer; I'm typing this message through
>   syscons. ;-)
>
> A lot of drivers that are used by the old TTY layer aren't mpsafe yet.
> Of course, I'm willing to fix this, but this cannot be done in the
> nearby future. This is why the new TTY layer should still allow TTY's to
> be run under Giant.
>
> In my initial implementation, each TTY device had its own mutex. In
> theory, this is great. The PTY driver already uses this and it works
> fine. There will be a lot of drivers, however, that want to use a
> per-class mutex to lock all related TTY devices down at once (i.e.
> syscons, which allocates 16 virtual TTY's). This is why I introduced a
> per-class lock. When set to Giant, all TTY instances will lock down the
> Giant lock when entering the TTY layer.
>
> Unfortunately, I discovered condvar(9) can't properly unlock/lock the
> Giant, which causes the system to panic. The condvar routines already
> call DROP_GIANT before unlocking the lock itself.
>
> I've attached a patch that adds support for Giant to condvar(9). I had
> to patch sys/mutex.h a little, because we now only need to call
> DROP_GIANT() under certain conditions. The macro's didn't allow that,
> because DROP_GIANT starts a new code block.
>
> I'm sending this to arch@, because I want to know if I'm doing something
> silly. It seems to work properly on my machine, but I'm not an SMP
> expert. ;-)

In general this sort of thing is discouraged as explicit use of Giant is 
discouraged.  It's magical properties (being implicitly dropped in places) 
can make it unsuitable for use as a regular mutex (though in practice any 
regular mutex would need to be dropped in the same places to avoid problems).  
In other driver locking cases the need for this has been avoided, although 
probably what I sort of forced CAM to do maybe isn't quite right.

Also, your patches won't work in the case of Giant being recursed (it will 
only drop Giant once and the sleeping thread will still own Giant).  If you 
do want to make this work my suggestion would be to make the lc_unlock and 
lc_lock not do anything for Giant.  You could either do this by 1) patching 
kern_convar.c so it does something like this:

	if (lock != &Giant.lo_object)
		cookie = class->lc_unlock(lock);

or instead patch the lc_lock/lc_unlock routines to just not do anything for 
Giant like so:

Index: kern_mutex.c
===================================================================
RCS file: /host/cvs/usr/cvs/src/sys/kern/kern_mutex.c,v
retrieving revision 1.205
diff -u -r1.205 kern_mutex.c
--- kern_mutex.c        13 Feb 2008 23:39:05 -0000      1.205
+++ kern_mutex.c        13 Mar 2008 14:49:04 -0000
@@ -134,6 +134,8 @@
 lock_mtx(struct lock_object *lock, int how)
 {

+       if (lock == &Giant.lo_object)
+               return;
        mtx_lock((struct mtx *)lock);
 }

@@ -149,6 +151,8 @@
 {
        struct mtx *m;

+       if (lock == &Giant.lo_object)
+               return (0);
        m = (struct mtx *)lock;
        mtx_assert(m, MA_OWNED | MA_NOTRECURSED);
        mtx_unlock(m);

I still don't like the idea of letting Giant work with msleep/cv_*wait*() 
because I think it will be abused.

-- 
John Baldwin