| Message ID | 20090826154552.GA31910@amit-x200.redhat.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
On Wed, 2009-08-26 at 21:15 +0530, Amit Shah wrote: > > > - 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. Two things here: - First you seem to have completely missed the fact that hvc_poll() can be called from interrupt time :-) Look at hvc_irq.c which is used by some backends. Maybe that can be "fixed" by deferring to a work queue, though it's nice to have the keyboard input have somewhat of a higher priority than anything else here. So unless that's fixed, or I missed something, that's a big NACK for now. - Then, are we certain that there's no case where the tty layer will call us with some lock held or in an atomic context ? To be honest, I've totally lost track of the locking rules in tty land lately so it might well be ok, but something to verify. Cheers, Ben.
On Wed, 2009-08-26 at 21:15 +0530, Amit Shah wrote: > [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. I won't tell Ryan you called his code "pure legacy baggage" if you don't ;) http://git.kernel.org/?p=linux/kernel/git/torvalds/old-2.6-bkcvs.git;a=commitdiff;h=d450b4ae023fb4be175389c18f4f87677da03020 cheers
On (Thu) Aug 27 2009 [14:07:03], Benjamin Herrenschmidt wrote: > On Wed, 2009-08-26 at 21:15 +0530, Amit Shah wrote: > > > > > > - 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. > > Two things here: > > - First you seem to have completely missed the fact that hvc_poll() can > be called from interrupt time :-) Look at hvc_irq.c which is used by Right! That's the obvious one. > some backends. Maybe that can be "fixed" by deferring to a work queue, > though it's nice to have the keyboard input have somewhat of a higher > priority than anything else here. Hm, to maintain the current behaviour of poll() returning some poll_mask, the poll_mask could be made into an atomic variable with khvcd() updating it. But to have read at a higher priority than the other stuff, I don't quite see yet how that can be done. > So unless that's fixed, or I missed something, that's a big NACK for > now. > > - Then, are we certain that there's no case where the tty layer will > call us with some lock held or in an atomic context ? To be honest, I've The other routines are open(), close(), write(), etc., and other kernel context (hvc_instantiate() and the khvcd thread). > totally lost track of the locking rules in tty land lately so it might > well be ok, but something to verify. Yes. Thanks for the response! Amit
On (Thu) Aug 27 2009 [15:04:45], Michael Ellerman wrote: > On Wed, 2009-08-26 at 21:15 +0530, Amit Shah wrote: > > [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. > > I won't tell Ryan you called his code "pure legacy baggage" if you > don't ;) > > http://git.kernel.org/?p=linux/kernel/git/torvalds/old-2.6-bkcvs.git;a=commitdiff;h=d450b4ae023fb4be175389c18f4f87677da03020 Thanks for the link! (and this general area might be the one that doesn't get major upheavals in 5-yr spans :-) Amit
> - Then, are we certain that there's no case where the tty layer will > call us with some lock held or in an atomic context ? To be honest, > I've totally lost track of the locking rules in tty land lately so it > might well be ok, but something to verify. Some of the less well behaved line disciplines do this and always have done. Alan
On Thu, 2009-08-27 at 10:08 +0100, Alan Cox wrote: > > - Then, are we certain that there's no case where the tty layer will > > call us with some lock held or in an atomic context ? To be honest, > > I've totally lost track of the locking rules in tty land lately so it > > might well be ok, but something to verify. > > Some of the less well behaved line disciplines do this and always have > done. That was also my understanding but heh, I though that maybe you may have fixed all of that already :-) So at this stage, I think the reasonably thing to do is to stick to the spinlock, but we can try to make it a bit smarter, and we can definitely attempt to fix the case Amit pointed out where we call resize without a lock while it seems to expect it (though we also need to be careful about re-entrancy I believe). Cheers, Ben.
On Thu, 2009-08-27 at 12:22 +0530, Amit Shah wrote: > On (Thu) Aug 27 2009 [15:04:45], Michael Ellerman wrote: > Ryan you called his code "pure legacy baggage" if you > > don't ;) > > > > http://git.kernel.org/?p=linux/kernel/git/torvalds/old-2.6-bkcvs.git;a=commitdiff;h=d450b4ae023fb4be175389c18f4f87677da03020 > > Thanks for the link! > > (and this general area might be the one that doesn't get major upheavals > in 5-yr spans :-) Actually, quite the opposite... too many cooks in the kitchen in the last 6 years have left this code with some historical oddities. When I added the interrupt context code in the first place I made sure that the driver did NOTHING in the interrupt handler except awaken the working thread. Someone in the subsequent years thought it'd be a good idea to have the interrupt handler actually do the work. Furthermore the front and backends were separated so that new front ends can be used. This was sometime in the last 5 years as well. Let's just say I'm happy to pass the torch on this one. I would suggest that someone perform throughput tests on this driver at some point. At the point where I stopped maintaining it I'd gotten the driver to the point where there was no jerking output or lost data under heavy I/O load. I'm not sure where the driver's at now. Ryan S. Arnold
Alan Cox wrote: > > - Then, are we certain that there's no case where the tty layer will > > call us with some lock held or in an atomic context ? To be honest, > > I've totally lost track of the locking rules in tty land lately so it > > might well be ok, but something to verify. > > Some of the less well behaved line disciplines do this and always have > done. I had a backtrace in my kernel log recently which looked like that, while doing PPP over Bluetooth RFCOMM. Resulted in AppArmor complaining that it's hook was being called in irq context. -- Jamie
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;