[Qemu-devel] Re: Extending virtio_console to support multiple ports

[Amit Shah <amit.shah@redhat.com>]

[cc'ing some people who have made some commits in hvc_console.c]

On (Wed) Aug 26 2009 [16:57:18], Amit Shah wrote:
> On (Tue) Aug 25 2009 [11:47:20], Amit Shah wrote:
> > 
> > Hello all,
> > 
> > Here is a new iteration of the patch series that implements a
> > transport for guest and host communications.
> > 
> > The code has been updated to reuse the virtio-console device instead
> > of creating a new virtio-serial device.
> 
> And the problem now is that hvc calls the put_chars function with
> spinlocks held and we now allocate pages in send_buf(), called from
> put_chars.
> 
> A few solutions:

[snip]

> - Convert hvc's usage of spinlocks to mutexes. I've no idea how this
>   will play out; I'm no expert here. But I did try doing this and so far
>   it all looks OK. No lockups, lockdep warnings, nothing. I have full
>   debugging enabled. But this doesn't mean it's right.

So just to test this further I added the capability to have more than
one hvc console spawn from virtio_console, created two consoles and did
a 'cat' of a file in each of the virtio-consoles. It's been running for
half an hour now without any badness. No spew in debug logs too.

I also checked the code in hvc_console.c that takes the spin_locks.
Nothing there that runs from (or needs to run from) interrupt context.
So the change to mutexes does seem reasonable. Also, the spinlock code
was added really long back -- git blame shows Linus' first git commit
introduced them in the git history, so it's pure legacy baggage.

Also found a bug: hvc_resize() wants to be called with a lock held
(hp->lock) but virtio_console just calls it directly.

Anyway I'm wondering whether all those locks are needed.

		Amit


diff --git a/drivers/char/hvc_console.c b/drivers/char/hvc_console.c
index d97779e..51078a3 100644
--- a/drivers/char/hvc_console.c
+++ b/drivers/char/hvc_console.c
@@ -35,7 +35,7 @@
 #include <linux/tty.h>
 #include <linux/tty_flip.h>
 #include <linux/sched.h>
-#include <linux/spinlock.h>
+#include <linux/mutex.h>
 #include <linux/delay.h>
 #include <linux/freezer.h>
 
@@ -81,7 +81,7 @@ static LIST_HEAD(hvc_structs);
  * Protect the list of hvc_struct instances from inserts and removals during
  * list traversal.
  */
-static DEFINE_SPINLOCK(hvc_structs_lock);
+static DEFINE_MUTEX(hvc_structs_lock);
 
 /*
  * This value is used to assign a tty->index value to a hvc_struct based
@@ -98,23 +98,22 @@ static int last_hvc = -1;
 static struct hvc_struct *hvc_get_by_index(int index)
 {
 	struct hvc_struct *hp;
-	unsigned long flags;
 
-	spin_lock(&hvc_structs_lock);
+	mutex_lock(&hvc_structs_lock);
 
 	list_for_each_entry(hp, &hvc_structs, next) {
-		spin_lock_irqsave(&hp->lock, flags);
+		mutex_lock(&hp->lock);
 		if (hp->index == index) {
 			kref_get(&hp->kref);
-			spin_unlock_irqrestore(&hp->lock, flags);
-			spin_unlock(&hvc_structs_lock);
+			mutex_unlock(&hp->lock);
+			mutex_unlock(&hvc_structs_lock);
 			return hp;
 		}
-		spin_unlock_irqrestore(&hp->lock, flags);
+		mutex_unlock(&hp->lock);
 	}
 	hp = NULL;
 
-	spin_unlock(&hvc_structs_lock);
+	mutex_unlock(&hvc_structs_lock);
 	return hp;
 }
 
@@ -228,15 +227,14 @@ console_initcall(hvc_console_init);
 static void destroy_hvc_struct(struct kref *kref)
 {
 	struct hvc_struct *hp = container_of(kref, struct hvc_struct, kref);
-	unsigned long flags;
 
-	spin_lock(&hvc_structs_lock);
+	mutex_lock(&hvc_structs_lock);
 
-	spin_lock_irqsave(&hp->lock, flags);
+	mutex_lock(&hp->lock);
 	list_del(&(hp->next));
-	spin_unlock_irqrestore(&hp->lock, flags);
+	mutex_unlock(&hp->lock);
 
-	spin_unlock(&hvc_structs_lock);
+	mutex_unlock(&hvc_structs_lock);
 
 	kfree(hp);
 }
@@ -302,17 +300,16 @@ static void hvc_unthrottle(struct tty_struct *tty)
 static int hvc_open(struct tty_struct *tty, struct file * filp)
 {
 	struct hvc_struct *hp;
-	unsigned long flags;
 	int rc = 0;
 
 	/* Auto increments kref reference if found. */
 	if (!(hp = hvc_get_by_index(tty->index)))
 		return -ENODEV;
 
-	spin_lock_irqsave(&hp->lock, flags);
+	mutex_lock(&hp->lock);
 	/* Check and then increment for fast path open. */
 	if (hp->count++ > 0) {
-		spin_unlock_irqrestore(&hp->lock, flags);
+		mutex_unlock(&hp->lock);
 		hvc_kick();
 		return 0;
 	} /* else count == 0 */
@@ -321,7 +318,7 @@ static int hvc_open(struct tty_struct *tty, struct file * filp)
 
 	hp->tty = tty;
 
-	spin_unlock_irqrestore(&hp->lock, flags);
+	mutex_unlock(&hp->lock);
 
 	if (hp->ops->notifier_add)
 		rc = hp->ops->notifier_add(hp, hp->data);
@@ -333,9 +330,9 @@ static int hvc_open(struct tty_struct *tty, struct file * filp)
 	 * tty fields and return the kref reference.
 	 */
 	if (rc) {
-		spin_lock_irqsave(&hp->lock, flags);
+		mutex_lock(&hp->lock);
 		hp->tty = NULL;
-		spin_unlock_irqrestore(&hp->lock, flags);
+		mutex_unlock(&hp->lock);
 		tty->driver_data = NULL;
 		kref_put(&hp->kref, destroy_hvc_struct);
 		printk(KERN_ERR "hvc_open: request_irq failed with rc %d.\n", rc);
@@ -349,7 +346,6 @@ static int hvc_open(struct tty_struct *tty, struct file * filp)
 static void hvc_close(struct tty_struct *tty, struct file * filp)
 {
 	struct hvc_struct *hp;
-	unsigned long flags;
 
 	if (tty_hung_up_p(filp))
 		return;
@@ -363,12 +359,12 @@ static void hvc_close(struct tty_struct *tty, struct file * filp)
 		return;
 
 	hp = tty->driver_data;
-	spin_lock_irqsave(&hp->lock, flags);
+	mutex_lock(&hp->lock);
 
 	if (--hp->count == 0) {
 		/* We are done with the tty pointer now. */
 		hp->tty = NULL;
-		spin_unlock_irqrestore(&hp->lock, flags);
+		mutex_unlock(&hp->lock);
 
 		if (hp->ops->notifier_del)
 			hp->ops->notifier_del(hp, hp->data);
@@ -386,7 +382,7 @@ static void hvc_close(struct tty_struct *tty, struct file * filp)
 		if (hp->count < 0)
 			printk(KERN_ERR "hvc_close %X: oops, count is %d\n",
 				hp->vtermno, hp->count);
-		spin_unlock_irqrestore(&hp->lock, flags);
+		mutex_unlock(&hp->lock);
 	}
 
 	kref_put(&hp->kref, destroy_hvc_struct);
@@ -395,7 +391,6 @@ static void hvc_close(struct tty_struct *tty, struct file * filp)
 static void hvc_hangup(struct tty_struct *tty)
 {
 	struct hvc_struct *hp = tty->driver_data;
-	unsigned long flags;
 	int temp_open_count;
 
 	if (!hp)
@@ -404,7 +399,7 @@ static void hvc_hangup(struct tty_struct *tty)
 	/* cancel pending tty resize work */
 	cancel_work_sync(&hp->tty_resize);
 
-	spin_lock_irqsave(&hp->lock, flags);
+	mutex_lock(&hp->lock);
 
 	/*
 	 * The N_TTY line discipline has problems such that in a close vs
@@ -412,7 +407,7 @@ static void hvc_hangup(struct tty_struct *tty)
 	 * that from happening for now.
 	 */
 	if (hp->count <= 0) {
-		spin_unlock_irqrestore(&hp->lock, flags);
+		mutex_unlock(&hp->lock);
 		return;
 	}
 
@@ -421,7 +416,7 @@ static void hvc_hangup(struct tty_struct *tty)
 	hp->n_outbuf = 0;
 	hp->tty = NULL;
 
-	spin_unlock_irqrestore(&hp->lock, flags);
+	mutex_unlock(&hp->lock);
 
 	if (hp->ops->notifier_hangup)
 			hp->ops->notifier_hangup(hp, hp->data);
@@ -462,7 +457,6 @@ static int hvc_push(struct hvc_struct *hp)
 static int hvc_write(struct tty_struct *tty, const unsigned char *buf, int count)
 {
 	struct hvc_struct *hp = tty->driver_data;
-	unsigned long flags;
 	int rsize, written = 0;
 
 	/* This write was probably executed during a tty close. */
@@ -472,7 +466,7 @@ static int hvc_write(struct tty_struct *tty, const unsigned char *buf, int count
 	if (hp->count <= 0)
 		return -EIO;
 
-	spin_lock_irqsave(&hp->lock, flags);
+	mutex_lock(&hp->lock);
 
 	/* Push pending writes */
 	if (hp->n_outbuf > 0)
@@ -488,7 +482,7 @@ static int hvc_write(struct tty_struct *tty, const unsigned char *buf, int count
 		written += rsize;
 		hvc_push(hp);
 	}
-	spin_unlock_irqrestore(&hp->lock, flags);
+	mutex_unlock(&hp->lock);
 
 	/*
 	 * Racy, but harmless, kick thread if there is still pending data.
@@ -511,7 +505,6 @@ static int hvc_write(struct tty_struct *tty, const unsigned char *buf, int count
 static void hvc_set_winsz(struct work_struct *work)
 {
 	struct hvc_struct *hp;
-	unsigned long hvc_flags;
 	struct tty_struct *tty;
 	struct winsize ws;
 
@@ -519,14 +512,14 @@ static void hvc_set_winsz(struct work_struct *work)
 	if (!hp)
 		return;
 
-	spin_lock_irqsave(&hp->lock, hvc_flags);
+	mutex_lock(&hp->lock);
 	if (!hp->tty) {
-		spin_unlock_irqrestore(&hp->lock, hvc_flags);
+		mutex_unlock(&hp->lock);
 		return;
 	}
 	ws  = hp->ws;
 	tty = tty_kref_get(hp->tty);
-	spin_unlock_irqrestore(&hp->lock, hvc_flags);
+	mutex_unlock(&hp->lock);
 
 	tty_do_resize(tty, &ws);
 	tty_kref_put(tty);
@@ -576,11 +569,10 @@ int hvc_poll(struct hvc_struct *hp)
 	struct tty_struct *tty;
 	int i, n, poll_mask = 0;
 	char buf[N_INBUF] __ALIGNED__;
-	unsigned long flags;
 	int read_total = 0;
 	int written_total = 0;
 
-	spin_lock_irqsave(&hp->lock, flags);
+	mutex_lock(&hp->lock);
 
 	/* Push pending writes */
 	if (hp->n_outbuf > 0)
@@ -622,9 +614,9 @@ int hvc_poll(struct hvc_struct *hp)
 		if (n <= 0) {
 			/* Hangup the tty when disconnected from host */
 			if (n == -EPIPE) {
-				spin_unlock_irqrestore(&hp->lock, flags);
+				mutex_unlock(&hp->lock);
 				tty_hangup(tty);
-				spin_lock_irqsave(&hp->lock, flags);
+				mutex_lock(&hp->lock);
 			} else if ( n == -EAGAIN ) {
 				/*
 				 * Some back-ends can only ensure a certain min
@@ -665,7 +657,7 @@ int hvc_poll(struct hvc_struct *hp)
 		tty_wakeup(tty);
 	}
  bail:
-	spin_unlock_irqrestore(&hp->lock, flags);
+	mutex_unlock(&hp->lock);
 
 	if (read_total) {
 		/* Activity is occurring, so reset the polling backoff value to
@@ -714,11 +706,11 @@ static int khvcd(void *unused)
 		try_to_freeze();
 		wmb();
 		if (!cpus_are_in_xmon()) {
-			spin_lock(&hvc_structs_lock);
+			mutex_lock(&hvc_structs_lock);
 			list_for_each_entry(hp, &hvc_structs, next) {
 				poll_mask |= hvc_poll(hp);
 			}
-			spin_unlock(&hvc_structs_lock);
+			mutex_unlock(&hvc_structs_lock);
 		} else
 			poll_mask |= HVC_POLL_READ;
 		if (hvc_kicked)
@@ -777,8 +769,8 @@ struct hvc_struct __devinit *hvc_alloc(uint32_t vtermno, int data,
 	kref_init(&hp->kref);
 
 	INIT_WORK(&hp->tty_resize, hvc_set_winsz);
-	spin_lock_init(&hp->lock);
-	spin_lock(&hvc_structs_lock);
+	mutex_init(&hp->lock);
+	mutex_lock(&hvc_structs_lock);
 
 	/*
 	 * find index to use:
@@ -796,7 +788,7 @@ struct hvc_struct __devinit *hvc_alloc(uint32_t vtermno, int data,
 	hp->index = i;
 
 	list_add_tail(&(hp->next), &hvc_structs);
-	spin_unlock(&hvc_structs_lock);
+	mutex_unlock(&hvc_structs_lock);
 
 	return hp;
 }
@@ -804,10 +796,9 @@ EXPORT_SYMBOL_GPL(hvc_alloc);
 
 int hvc_remove(struct hvc_struct *hp)
 {
-	unsigned long flags;
 	struct tty_struct *tty;
 
-	spin_lock_irqsave(&hp->lock, flags);
+	mutex_lock(&hp->lock);
 	tty = hp->tty;
 
 	if (hp->index < MAX_NR_HVC_CONSOLES)
@@ -815,7 +806,7 @@ int hvc_remove(struct hvc_struct *hp)
 
 	/* Don't whack hp->irq because tty_hangup() will need to free the irq. */
 
-	spin_unlock_irqrestore(&hp->lock, flags);
+	mutex_unlock(&hp->lock);
 
 	/*
 	 * We 'put' the instance that was grabbed when the kref instance
diff --git a/drivers/char/hvc_console.h b/drivers/char/hvc_console.h
index 3c85d78..3c086f8 100644
--- a/drivers/char/hvc_console.h
+++ b/drivers/char/hvc_console.h
@@ -45,7 +45,7 @@
 #define HVC_ALLOC_TTY_ADAPTERS	8
 
 struct hvc_struct {
-	spinlock_t lock;
+	struct mutex lock;
 	int index;
 	struct tty_struct *tty;
 	int count;