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

[Vladimir Davydov]

On Mon, Aug 17, 2015 at 09:11:03PM +0300, Cyrill Gorcunov wrote:
...
> > 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 :)

Hmm, you're right. I thought it wouldn't. Never mind.

...
> > > +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.

OK, I see. I missed juggling with TTY_EXTRA_REF when a master is opened.

> 
> > > +	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.

I don't think so. AFAIU if the master is closed, the process inside
container should continue using the slave as usual, and vice-versa. The
tty pair dies only when both master and slave are closed, in which case
there is no one to send SIGHUP.

> Otherwise the application may not notice that the
> master peer is off and continue use terminal which
> barely writes everything into the void.

That's what we want to achieve, no?

> 
> > > +
> > > +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.

Ah, if you need to trace it, then it's OK.

> ...
> > > +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.

Ah, I missed that you increment slave->count when the master is opened.

> 
> > > +
> > > +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.

We just want to report that there is always some space in our tty
buffer. It does not make much sense to use tty_buffer.c private constant
for that (yes, it's now private - check vanilla kernel). If you don't
like 8192, use 4096, or 2048 (BTW tty_write_room() defaults to 2048).

> 
> > > +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.

idr is a layered structure, so there may be racing allocations from the
same layer. So please move vtty_map_free before tty_mutex unlock.

> 
> ...
> > > +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.

Ah, I missed that you increment tty->link->link->count (sic!), i.e.
tty->count, right below.

> 
> > > +		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.

Let's simplify the "general idea", so that we won't have to play with
EXTRA_REF:

1. When a slave is opened it has incremented ->count (think of it as
   master's reference). I.e. When you call install(slave) you get
   master->count=1, slave->count=2 (file ref + master ref). Since master
   is not opened by anybody it should have EXTRA_REF flag set. You'll
   have to use TTY_DRIVER_TYPE_PTY and PTY_TYPE_{MASTER,SLAVE} for vtty
   driver type and subtype then, but I don't see any problems with that.

2. When a master is opened (via ioctl), it increments master->count. If
   slave does not exist, it creates it and decrements its reference,
   i.e. we have master->count=2, slave->count=(1+nr_users).

3. When a slave is closed by the last user (on close, count=2), it
   checks master->count. If it is 1, then the master is not opened and
   it releases both itself and master, clearing master's EXTRA_REF.

4. When a master is closed by the last user (on close, count=2), it
   checks slave->count. If it is 1, then the slave is not opened and it
   releases both itself and slave, clearing self EXTRA_REF.

This is so much closer to the pty design and so easier to understand
IMO. BTW, we seem to have the same ref counting scheme in PCS6. Why did
you decide to turn away from it?