Skip site navigation (1)Skip section navigation (2)
Date:      Thu, 13 Mar 2008 14:50:35 +0100
From:      Ed Schouten <ed@80386.nl>
To:        FreeBSD Arch <arch@freebsd.org>
Subject:   New TTY layer: condvar(9) and Giant
Message-ID:  <20080313135035.GB80576@hoeg.nl>

next in thread | raw e-mail | index | archive | help

--ht9V8wKec6a3w1Ef
Content-Type: multipart/mixed; boundary="FexDM9E/OpjgUmaq"
Content-Disposition: inline


--FexDM9E/OpjgUmaq
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

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. ;-)

--=20
 Ed Schouten <ed@fxq.nl>
 WWW: http://g-rave.nl/

--FexDM9E/OpjgUmaq
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="condvar-giant.diff"
Content-Transfer-Encoding: quoted-printable

--- sys/kern/kern_condvar.c
+++ sys/kern/kern_condvar.c
@@ -95,6 +95,7 @@
 _cv_wait(struct cv *cvp, struct lock_object *lock)
 {
 	WITNESS_SAVE_DECL(lock_witness);
+	PARTIAL_DROP_GIANT_DECL();
 	struct lock_class *class;
 	struct thread *td;
 	int lock_state;
@@ -123,7 +124,8 @@
 	sleepq_lock(cvp);
=20
 	cvp->cv_waiters++;
-	DROP_GIANT();
+	if (lock !=3D (struct lock_object *)&Giant)
+		PARTIAL_DROP_GIANT();
=20
 	sleepq_add(cvp, lock, cvp->cv_description, SLEEPQ_CONDVAR, 0);
 	if (class->lc_flags & LC_SLEEPABLE)
@@ -137,7 +139,8 @@
 	if (KTRPOINT(td, KTR_CSW))
 		ktrcsw(0, 0);
 #endif
-	PICKUP_GIANT();
+	if (lock !=3D (struct lock_object *)&Giant)
+		PARTIAL_PICKUP_GIANT();
 	class->lc_lock(lock, lock_state);
 	WITNESS_RESTORE(lock, lock_witness);
 }
@@ -149,6 +152,7 @@
 void
 _cv_wait_unlock(struct cv *cvp, struct lock_object *lock)
 {
+	PARTIAL_DROP_GIANT_DECL();
 	struct lock_class *class;
 	struct thread *td;
=20
@@ -176,7 +180,8 @@
 	sleepq_lock(cvp);
=20
 	cvp->cv_waiters++;
-	DROP_GIANT();
+	if (lock !=3D (struct lock_object *)&Giant)
+		PARTIAL_DROP_GIANT();
=20
 	sleepq_add(cvp, lock, cvp->cv_description, SLEEPQ_CONDVAR, 0);
 	if (class->lc_flags & LC_SLEEPABLE)
@@ -190,7 +195,8 @@
 	if (KTRPOINT(td, KTR_CSW))
 		ktrcsw(0, 0);
 #endif
-	PICKUP_GIANT();
+	if (lock !=3D (struct lock_object *)&Giant)
+		PARTIAL_PICKUP_GIANT();
 }
=20
 /*
@@ -203,6 +209,7 @@
 _cv_wait_sig(struct cv *cvp, struct lock_object *lock)
 {
 	WITNESS_SAVE_DECL(lock_witness);
+	PARTIAL_DROP_GIANT_DECL();
 	struct lock_class *class;
 	struct thread *td;
 	struct proc *p;
@@ -233,7 +240,8 @@
 	sleepq_lock(cvp);
=20
 	cvp->cv_waiters++;
-	DROP_GIANT();
+	if (lock !=3D (struct lock_object *)&Giant)
+		PARTIAL_DROP_GIANT();
=20
 	sleepq_add(cvp, lock, cvp->cv_description, SLEEPQ_CONDVAR |
 	    SLEEPQ_INTERRUPTIBLE, 0);
@@ -248,7 +256,8 @@
 	if (KTRPOINT(td, KTR_CSW))
 		ktrcsw(0, 0);
 #endif
-	PICKUP_GIANT();
+	if (lock !=3D (struct lock_object *)&Giant)
+		PARTIAL_PICKUP_GIANT();
 	class->lc_lock(lock, lock_state);
 	WITNESS_RESTORE(lock, lock_witness);
=20
@@ -264,6 +273,7 @@
 _cv_timedwait(struct cv *cvp, struct lock_object *lock, int timo)
 {
 	WITNESS_SAVE_DECL(lock_witness);
+	PARTIAL_DROP_GIANT_DECL();
 	struct lock_class *class;
 	struct thread *td;
 	int lock_state, rval;
@@ -293,7 +303,8 @@
 	sleepq_lock(cvp);
=20
 	cvp->cv_waiters++;
-	DROP_GIANT();
+	if (lock !=3D (struct lock_object *)&Giant)
+		PARTIAL_DROP_GIANT();
=20
 	sleepq_add(cvp, lock, cvp->cv_description, SLEEPQ_CONDVAR, 0);
 	sleepq_set_timeout(cvp, timo);
@@ -308,7 +319,8 @@
 	if (KTRPOINT(td, KTR_CSW))
 		ktrcsw(0, 0);
 #endif
-	PICKUP_GIANT();
+	if (lock !=3D (struct lock_object *)&Giant)
+		PARTIAL_PICKUP_GIANT();
 	class->lc_lock(lock, lock_state);
 	WITNESS_RESTORE(lock, lock_witness);
=20
@@ -325,6 +337,7 @@
 _cv_timedwait_sig(struct cv *cvp, struct lock_object *lock, int timo)
 {
 	WITNESS_SAVE_DECL(lock_witness);
+	PARTIAL_DROP_GIANT_DECL();
 	struct lock_class *class;
 	struct thread *td;
 	struct proc *p;
@@ -356,7 +369,8 @@
 	sleepq_lock(cvp);
=20
 	cvp->cv_waiters++;
-	DROP_GIANT();
+	if (lock !=3D (struct lock_object *)&Giant)
+		PARTIAL_DROP_GIANT();
=20
 	sleepq_add(cvp, lock, cvp->cv_description, SLEEPQ_CONDVAR |
 	    SLEEPQ_INTERRUPTIBLE, 0);
@@ -372,7 +386,8 @@
 	if (KTRPOINT(td, KTR_CSW))
 		ktrcsw(0, 0);
 #endif
-	PICKUP_GIANT();
+	if (lock !=3D (struct lock_object *)&Giant)
+		PARTIAL_PICKUP_GIANT();
 	class->lc_lock(lock, lock_state);
 	WITNESS_RESTORE(lock, lock_witness);
=20
--- sys/sys/mutex.h
+++ sys/sys/mutex.h
@@ -368,26 +368,33 @@
 #ifndef DROP_GIANT
 #define DROP_GIANT()							\
 do {									\
+	PARTIAL_DROP_GIANT_DECL();					\
+	PARTIAL_DROP_GIANT();
+
+#define PARTIAL_DROP_GIANT_DECL()					\
 	int _giantcnt =3D 0;						\
-	WITNESS_SAVE_DECL(Giant);					\
-									\
+	WITNESS_SAVE_DECL(Giant);
+
+#define PARTIAL_DROP_GIANT() do {					\
 	if (mtx_owned(&Giant)) {					\
 		WITNESS_SAVE(&Giant.lock_object, Giant);		\
 		for (_giantcnt =3D 0; mtx_owned(&Giant); _giantcnt++)	\
 			mtx_unlock(&Giant);				\
-	}
+	}								\
+} while (0)
=20
 #define PICKUP_GIANT()							\
 	PARTIAL_PICKUP_GIANT();						\
 } while (0)
=20
-#define PARTIAL_PICKUP_GIANT()						\
+#define PARTIAL_PICKUP_GIANT() do {					\
 	mtx_assert(&Giant, MA_NOTOWNED);				\
 	if (_giantcnt > 0) {						\
 		while (_giantcnt--)					\
 			mtx_lock(&Giant);				\
 		WITNESS_RESTORE(&Giant.lock_object, Giant);		\
-	}
+	}								\
+} while(0)
 #endif
=20
 #define	UGAR(rval) do {							\

--FexDM9E/OpjgUmaq--

--ht9V8wKec6a3w1Ef
Content-Type: application/pgp-signature
Content-Disposition: inline

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.8 (FreeBSD)

iEYEARECAAYFAkfZMSsACgkQ52SDGA2eCwXMFQCfYwaKBVHU7xCBZv/D+yglHfmk
7dEAn1mXTouuc66FGFTiiVnM6ylfLri5
=5MNx
-----END PGP SIGNATURE-----

--ht9V8wKec6a3w1Ef--



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20080313135035.GB80576>