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

[Cyrill Gorcunov]

On Mon, Aug 17, 2015 at 07:33:36PM +0300, Vladimir Davydov wrote:
...
> 
> Nit: It would be nice to have a comment explaining synchronization rules
> here. If vtty_idr is protected by a mutex (tty_mutex?), I would also
> recommend inserting lockdep_assert_held to all the helpers for
> accessing/modifying vtty_idr below.

Yes, it's protected by tty_mutex. Sure, I'll add comments and
lockdep_assert_held helpers where needed. Thanks!
> 
> Nit:
> 
> if (A)
> 	vtt_printk(...);
> 
> won't compile if !VTTY_DEBUG. Use do { } while (0) stubs to overcome
> this.

if !VTTY_DEBUG defined then you'll get

	if (a)
		;

which is prefectly fine :)
...
> > +static struct tty_struct *vtty_install_peer(vtty_map_t *map, struct tty_driver *driver, int index)
> > +{
> > +	struct tty_struct *tty;
> > +
> > +	tty = alloc_tty_struct();
> > +	if (!tty)
> > +		return ERR_PTR(-ENOMEM);
> > +	initialize_tty_struct(tty, driver, index);
> > +
> > +	tty->port = kzalloc(sizeof(*tty->port), GFP_KERNEL);
> > +	if (!tty->port) {
> > +		deinitialize_tty_struct(tty);
> 
> Nit: Move port allocation before initialize_tty_struct and you won't
> have to call deinitialize_tty_struct on error path.

ok
...
> > +static int vtty_install(struct tty_driver *driver, struct tty_struct *tty)
> > +{
> > +	envid_t veid = vtty_get_context();
> > +	struct tty_struct *peer;
> > +	vtty_map_t *map;
> > +
> > +	WARN_ON_ONCE(driver != vttys_driver);
> > +
> > +	map = vtty_map_lookup(veid);
> > +	if (!map) {
> > +		map = vtty_map_alloc(veid);
> > +		if (IS_ERR(map))
> > +			return PTR_ERR(map);
> > +	}
> > +
> > +	tty->port = kzalloc(sizeof(*tty->port), GFP_KERNEL);
> > +	if (!tty->port)
> > +		return -ENOMEM;
> > +
> > +	peer = vtty_install_peer(map, vttym_driver, tty->index);
> > +	if (IS_ERR(peer)) {
> > +		vtty_map_del(map, tty, tty->index);
> > +		kfree(tty->port);
> > +		return PTR_ERR(peer);
> > +	}
> > +	set_bit(TTY_EXTRA_REF, &peer->flags);
> > +
> > +	vtty_standard_install(map, vttys_driver, tty);
> > +	vtty_map_set(map, tty, tty->index);
> > +
> > +	tty->link = peer;
> > +	peer->link = tty;
> > +
> 
> tty->count++;
> ?

No. After this call tty->count = 1, which matches the number of files opened.
The TTY_EXTRA_REF set on the peer is a sign to the rest of tty layer that
count != fd on the peer, so check won't complain.

> > +	if (test_bit(TTY_EXTRA_REF, &peer->flags)) {
> > +		clear_bit(TTY_EXTRA_REF, &peer->flags);
> > +		peer->count--;
> > +
> > +		tty_unlock(tty);
> > +		tty_vhangup(peer);
> > +		tty_lock(tty);
> 
> Why do you think we need to do this?

The peer should know that the master link is down,
delivering appropriate signal to an application.
Otherwise the application may not notice that the
master peer is off and continue use terminal which
barely writes everything into the void.

> > +
> > +static void vtty_cleanup(struct tty_struct *tty)
> 
> I'd use pty_cleanup instead.

Well, I simply needed some tracer (for vtty_printk_one
call). But sure will do.
...
> > +static int vtty_write(struct tty_struct *tty, const unsigned char *buf, int count)
> > +{
> > +	struct tty_struct *peer = tty->link;
> > +
> > +	if (tty->stopped)
> > +		return 0;
> > +
> > +	if (count > 0) {
> > +		count = tty_insert_flip_string(peer->port, buf, count);
> > +		if (count) {
> > +			tty_flip_buffer_push(peer->port);
> > +			tty_wakeup(tty);
> > +		} else {
> > +			/*
> > +			 * Flush the slave reader if noone
> > +			 * is actually hooked on. Otherwise
> > +			 * wait until reader fetch all data.
> > +			 */
> > +			if (peer->count < 2)
> 
> I.e. we always perform flush on master to slave writes here, right? If
> so, this is incorrect. Missed a slave reference in tty_install?

Yes, if noone connected to another end of the tty pair and
there is no space left in buffer, we flush it out. Why
it's incorrect? It's done in purpose -- we can't block
writes.

> > +
> > +static int vtty_write_room(struct tty_struct *tty)
> > +{
> > +	struct tty_struct *peer = tty->link;
> > +
> > +	if (peer->stopped)
> 
> tty->stopped

yea, thanks, typo

> 
> > +		return 0;
> > +
> > +	if (peer->count < 2)
> > +		return TTY_BUFFER_PAGE;
> 
> TTY_BUFFER_PAGE looks confusing here, because our write_room method has
> nothing to do with it. Please use a numeric constant (e.g. 8192).

Where this magic two-pages constant came from? I was always wondering
why on earth there a some constant which simply taken hell knows
from where.

In turn, if you look into TTY_BUFFER_PAGE references then you will
notice that the layer tries to minimize impact on the buffer writting
up to one page at once. So no, I think it's good idea to use TTY_BUFFER_PAGE
here.

> > +static void ve_vtty_fini(void *data)
> > +{
> > +	struct ve_struct *ve = data;
> > +	vtty_map_t *map = vtty_map_lookup(ve->veid);
> > +	int i;
> > +
> > +	mutex_lock(&tty_mutex);
> > +	for (i = 0; i < MAX_NR_VTTY_CONSOLES; i++) {
> > +		struct tty_struct *tty = map->vttys[i];
> > +		if (!tty)
> > +			continue;
> > +		vtty_printk("active tty at %d\n", i);
> > +		vtty_printk_pair(tty);
> > +		tty->driver_data = tty->link->driver_data = NULL;
> > +	}
> > +	mutex_unlock(&tty_mutex);
> > +	vtty_map_free(map);
> 
> What protects vtty_idr here?

Good question ;) We can't allocate same veid once this is inside
"fini" hook I think. So if I'm not missing something obvious
we don't need any lock here.

...
> > +int vtty_open_master(envid_t veid, int idx)
> > +{
> > +	struct tty_struct *tty;
> > +	struct file *file;
> > +	char devname[64];
> > +	int fd, ret;
> > +
> > +	if (!vtty_match_index(idx))
> > +		return -EIO;
> > +
> > +	vtty_printk("veid %d index %d\n", (int)veid, idx);
> > +
> > +	fd = get_unused_fd_flags(0);
> > +	if (fd < 0)
> > +		return fd;
> > +
> > +	snprintf(devname, sizeof(devname), "v%utty%d", veid, idx);
> > +	file = anon_inode_getfile(devname, &vtty_fops, NULL, O_RDWR);
> > +	if (IS_ERR(file)) {
> > +		ret = PTR_ERR(file);
> > +		goto err_put_unused_fd;
> > +	}
> > +	nonseekable_open(NULL, file);
> > +
> > +	ret = tty_alloc_file(file);
> > +	if (ret)
> > +		goto err_fput;
> > +
> > +	/*
> > +	 * Opening comes from ve0 context so
> > +	 * setup VE's context until master fetched.
> > +	 * This is done under @tty_mutex so noone
> > +	 * else would access it while we're hilding
> > +	 * the lock.
> > +	 */
> > +	mutex_lock(&tty_mutex);
> > +	vtty_set_context(veid);
> > +
> > +	tty = vtty_lookup(vttym_driver, NULL, idx);
> > +	if (IS_ERR(tty)) {
> > +		ret = -ENODEV;
> > +		goto err_install;
> > +	} else if (!tty) {
> > +		/*
> > +		 * Allocate a new tty pair, the slave
> > +		 * won't be having @count here ecause
> > +		 * it's not opened by anyone yet.
> > +		 */
> 
> Hey, wait! It's opened by us, isn't it? Where is the reference?

Not always. The init inside container may open, or may
not yet open any tty (including /dev/console), so we
have 2 scenarios -- if noone has opened yet tty we need
to open both pairs give us back the master peer here
and a slave peer then gonna be opened by some application
inside container (or may not). As far as I understand
pcs6 code does the same -- you can literally open any
console from the node, even if there no client inside
container. Say

	vzctl console $ctid 7

but there is no [a]getty listening on /dev/tty6 so you'll
be hanging aroud until client get connected.

> > +		tty = tty_init_dev(vttys_driver, idx);
> > +		if (IS_ERR(tty))
> > +			goto err_install;
> > +		tty->count--;
> > +		tty_unlock(tty);
> > +		tty = tty->link;
> > +	}
> > +
> > +	/*
> > +	 * Make sure the slave peer won't disappear
> > +	 * while we're keeping the tty opened.
> > +	 */
> > +	set_bit(TTY_EXTRA_REF, &tty->link->flags);
> > +	tty->link->count++;
> > +
> > +	vtty_drop_context();
> > +
> > +	WARN_ON(!test_bit(TTY_LDISC, &tty->flags));
> > +
> > +	clear_bit(TTY_EXTRA_REF, &tty->flags);
> 
> This juggling with TTY_EXTRA_REF looks suspicious. AFAIU we only need
> TTY_EXTRA_REF set for master, so that it won't pass away on close unless
> its slave is dead too, and master must always pin its slave, no?

This all is due to ability to open master via this call without any
real slave open inside container. So imagine master is opened but
slave never was and never been, we need to cleanup all this things
somehow and here TTY_EXTRA_REF comes into a game.

1) Slave opened first from inside container

	- master set @tty->count = 1 and TTY_EXTRA_REF, so
	  the rest of tty code won't complan if there is
	  no files opened on the slave.

	- master get closed -> TTY_EXTRA_REF dropped, count--,
	  so tty layer may free the associated tty structure

	(if both slave and master is opened, master won't be
	 dropped until slave is closed)

2) Master opened via ioctl, but slave peer has not yet been opened
   so we're allocating it and give it to vtty map. Next open from inside
   of container will find this allocated tty and both ends get connected.

   But for such scenario there is a bit of difference: we change the meaning
   of master-slave in compare with scenario (1): now master peer opened from
   the node is controlling the slave peer. While it's opened the slave peer
   is valid and associated tty is alive. Once master peer get closed then
   slave become free.

I know it's somehow vague, but that's because of a case where master
is opened and noone open slave peer from inside the container but
we still need to cleanup both ends on master peer close.

That said, if I'm missing something in "general idea", please share.

Thanks a huge for review!