From owner-freebsd-arch@FreeBSD.ORG Thu Mar 13 14:50:40 2008 Return-Path: Delivered-To: freebsd-arch@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id B0AB41065671 for ; Thu, 13 Mar 2008 14:50:40 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from elvis.mu.org (elvis.mu.org [192.203.228.196]) by mx1.freebsd.org (Postfix) with ESMTP id 968B68FC16 for ; Thu, 13 Mar 2008 14:50:40 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from zion.baldwin.cx (66-23-211-162.clients.speedfactory.net [66.23.211.162]) by elvis.mu.org (Postfix) with ESMTP id C99201A4D7C; Thu, 13 Mar 2008 07:49:43 -0700 (PDT) From: John Baldwin To: freebsd-arch@freebsd.org Date: Thu, 13 Mar 2008 10:49:57 -0400 User-Agent: KMail/1.9.7 References: <20080313135035.GB80576@hoeg.nl> In-Reply-To: <20080313135035.GB80576@hoeg.nl> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-15" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-Id: <200803131049.58051.jhb@freebsd.org> Cc: Ed Schouten Subject: Re: New TTY layer: condvar(9) and Giant X-BeenThere: freebsd-arch@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Discussion related to FreeBSD architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 13 Mar 2008 14:50:40 -0000 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