[Devel] [RFC rh7 v5] ve/tty: vt -- Implement per VE support for console and terminals

[Vladimir Davydov]

On Thu, Aug 27, 2015 at 06:40:32PM +0300, Cyrill Gorcunov wrote:
> On Thu, Aug 27, 2015 at 06:00:28PM +0300, Vladimir Davydov wrote:
> > > +
> > > +static struct file_operations vtty_fops;
> > > +
> > > +#define vtty_match_index(idx)		((idx) >= 0 && (idx) < MAX_NR_VTTY_CONSOLES)
> > 
> > nit: this line is longer than 80 symbols
> 
> yes, but recall that there is no 80 syms limit. The last discussion/flame
> over line length I saw was calmed down with agreemen that 90 symbols are

Hmm, checkpatch still has max_line_length set to 80. Could you please
share a link to this agreement?

> fine. Wonder, do you really still sit on 80 chars terminal?

I use a 12" laptop. With the window vertically split into two panes, I
have only ~80 characters per each pane.

...
> > > +static void vtty_set_context(envid_t veid)
> > > +{
> > 
> > nit: WARN_ON(veid)
> 
> I believe you meant veid == 0

Right.

...
> > > +static void vtty_close(struct tty_struct *tty, struct file *filp)
> > > +{
> > > +	struct tty_struct *peer = tty->link;
> > > +	vtty_map_t *map = tty->driver_data;
> > > +
> > > +	spin_lock(&map->close_lock);
> > 
> > This spinlock doesn't protect us from concurrent modifications of
> > peer->count from e.g. tty_reopen. What about taking tty_mutex instead?
> > 
> > On the second thought, this wouldn't help us if we patched mainstream
> > kernel, because tty_mutex is not held for tty->count modifications there
> > anymore. That said, if we simply substitute the spinlock with tty_mutex,
> > we will surely have problems sooner or later...
> > 
> > May be, we'd better move our ->count tweaking to the generic path, i.e.
> > tty_release, similarly to pty? We could distinguish vtty from pty by
> > checking TTY_EXTRA_REF. What do you think?
> 
> you know, i really don't wanna put our code into generic one, its gonna
> be hard to support. maybe we should lock the peer manually inside

Yes, you're right :-(

OTOH, this would reveal potential problems on build stage, while playing
with locks can end up with a dead lock after rebase :-/

> our close routine, so in this way we would have to take own lock, then
> take peer lock under it, which would protect from abba deadlock. Hm?
> 
> vtty_close
> 	spin_lock(&map->close_lock);
> 	tty_lock(tty->link);
> 	...
> 	tty_unlock(tty->link);
> 	spin_unlock(&map->close_lock);
> 
> and instead of spinlock I would use mutex here. Won't this work?

Hmm, ->close is called under tty_lock(tty). That means we can't call
tty_lock(tty->link) here, neither can we acquire tty_mutex. OTOH, hand
if we pull patches bringing in tty_lock_slave, this should work. Will it
be difficult to pull them?