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

[Amit Shah]

[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;