Messages in this thread | | | From | Blaisorblade <> | Subject | Re: [uml-devel] [PATCH 1/6] UML - Console locking fixes | Date | Wed, 3 Jan 2007 16:07:34 +0100 |
| |
On Saturday 30 December 2006 00:41, Jeff Dike wrote: > Clean up the console driver locking. There are various problems here, > including sleeping under a spinlock and spinlock recursion, some of > which are fixed here. This patch deals with the locking involved with > opens and closes. The problem is that an mconsole request to change a > console's configuration can race with an open. Changing a > configuration should only be done when a console isn't opened. Also, > an open must be looking at a stable configuration. In addition, a get > configuration request must observe the same locking since it must also > see a stable configuration. With the old locking, it was possible for > this to hang indefinitely in some cases because open would block for a > long time waiting for a connection from the host while holding the > lock needed by the mconsole request. > > As explained in the long comment, this is fixed by adding a spinlock > for the use count and configuration and a mutex for the actual open > and close. > > Signed-off-by: Jeff Dike <jdike@addtoit.com>
> + > int line_open(struct line *lines, struct tty_struct *tty) > { > - struct line *line; > + struct line *line = &lines[tty->index]; > int err = -ENODEV; > > - line = &lines[tty->index]; > - tty->driver_data = line; > + spin_lock(&line->count_lock); > + if(!line->valid) > + goto out_unlock; > + > + err = 0; > + if(tty->count > 1) > + goto out_unlock; > > - /* The IRQ which takes this lock is not yet enabled and won't be run > - * before the end, so we don't need to use spin_lock_irq.*/ > - spin_lock(&line->lock); > + mutex_lock(&line->open_mutex); > + spin_unlock(&line->count_lock);
This is an obnoxious thing to do unless you specifically prove otherwise. You cannot take a mutex (and possibly sleep) while holding a spinlock.
You must have either: + spin_unlock(&line->count_lock); + mutex_lock(&line->open_mutex);
or take count_lock inside open_mutex (which looks like being correct here).
In the first solution, you can create a OPENING flag (via a state variable), and add the rule that (unlike the count) nobody but the original setter is allowed to change it, and that who finds it set (say a concurrent open) must return without touching it.
The state diagram is like: CLOSED -> OPENING -> OPEN (only the function which triggered the transition from CLOSED to OPENING can trigger the transition from OPENING to OPEN). It can probably be simplified to OPENING <-> ! OPENING. -- Inform me of my mistakes, so I can add them to my list! Paolo Giarrusso, aka Blaisorblade http://www.user-mode-linux.org/~blaisorblade
Chiacchiera con i tuoi amici in tempo reale! http://it.yahoo.com/mail_it/foot/*http://it.messenger.yahoo.com
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |