[Devel] [RFC rh7 v2] ve/tty: vt -- Implement per VE support for virtual consoles

[Cyrill Gorcunov]

On Thu, Jul 30, 2015 at 07:33:47PM +0300, Vladimir Davydov wrote:
> On Thu, Jul 30, 2015 at 03:51:55PM +0300, Cyrill Gorcunov wrote:
> 
> > Index: linux-pcs7.git/drivers/tty/tty_io.c
> > ===================================================================
> > --- linux-pcs7.git.orig/drivers/tty/tty_io.c
> > +++ linux-pcs7.git/drivers/tty/tty_io.c
> > @@ -104,6 +104,7 @@
> >  #include <linux/kmod.h>
> >  #include <linux/nsproxy.h>
> >  #include <linux/ve.h>
> > +#include <uapi/linux/vzt.h>
> 
> For me, vzt is inevitably associated with our test suit. A test for
> testing it would be called vzt-vzt I suppose :-) May be we'd better call
> it vtty to avoid confusion?

Whatever you like ;) I'll rename, sure.

> > Index: linux-pcs7.git/include/uapi/linux/vzt.h
> > ===================================================================
> > --- /dev/null
> > +++ linux-pcs7.git/include/uapi/linux/vzt.h
> > @@ -0,0 +1,30 @@
> > +#ifndef _UAPI_LINUX_VZT_H
> > +#define _UAPI_LINUX_VZT_H
> > +
> > +#define MAX_NR_VZT_CONSOLES		(12)
> > +
> > +#define VZT_MASTER_DRIVER_NAME		"vzt_master"
> > +#define VZT_MASTER_NAME			"vztm"
> > +
> > +#define VZT_SLAVE_DRIVER_NAME		"vzt_slave"
> > +#define VZT_SLAVE_NAME			"vzts"
> 
> I don't think we need this in uapi. Both "ptmx" and "tty" are hardcoded,
> why should we be different?

Because it's right thing, ptmx called this way simply for hist
reasons and didn't exported for the same. But for our driver
I think it's a good habit to show what's the names we're
exporting via kernel header, instead of hardcoding it into
the kernel. Same applies to MAX_NR_VZT_CONSOLES.

> > Index: linux-pcs7.git/kernel/ve/vzt.c
> > ===================================================================
> > --- /dev/null
> > +++ linux-pcs7.git/kernel/ve/vzt.c
> > +
> > +#define driver_is_master(d)	((d) == vztm_driver)
> > +#define driver_is_slave(d)	((d) == vzts_driver)
> 
> No need in this macro - you can use !driver_is_master instead.
> 
> > +#define tty_is_master(t)	driver_is_master((t)->driver)
> > +#define tty_is_slave(t)		driver_is_slave((t)->driver)
> 
> ditto

master/slave is easier to read I think but ok, will do

> > +static tty_map_t *tty_map_alloc(envid_t veid)
> > +{
> > +	tty_map_t *map = kzalloc(sizeof(*map), GFP_KERNEL);
> > +
> > +	if (map) {
> > +		map->veid = veid;
> > +		veid = idr_alloc(&vzt_idr, map, veid, veid + 1, GFP_KERNEL);
> > +		if (veid < 0) {
> > +			kfree(map);
> > +			return ERR_PTR(veid);
> > +		}
> 
> > +		idr_replace(&vzt_idr, map, veid);
> 
> What is this for? vzt_idr[veid] must already equal map.

Yeah, redundant, thanks.

> > +static struct tty_struct *vzt_tty_lookup(struct tty_driver *driver,
> > +					struct inode *inode, int idx)
> > +{
> > +	tty_map_t *map = tty_map_lookup(get_exec_env()->veid);
> > +	struct tty_struct *tty;
> > +
> > +	if (idx < 0 || idx >= MAX_NR_VZT_CONSOLES)
> > +		return ERR_PTR(-EIO);
> 
> if (!map)
> 	return ERR_PTR(-ENXIO);
> 
> will free you from the responsibility to check if map == NULL below.

No, map = NULL here is perfectly fine, it means no tty has ever
being allocated yet, so that lookup return NULL and install()
will be called to create new map and tty pairs.

Here is a difference between our vtty code and pty -- in pty only
one master peer may exist but a number of slave peers might be
opened, but in our vtty many masters and many slaves may be
opened so we have to: lookup over existing maps to find if
tty already allocated and both peers are not in "closing"
state, if any of peer is closing -- we should exit with
-ENXIO, if there is no peers at all -- create new pair.


> > +	if (driver_is_slave(driver)) {
> > +		struct tty_struct *peer = map ? map->master[idx] : NULL;
> > +		struct tty_struct *me = map ? map->slave[idx] : NULL;
> > +
> > +		tty = (peer && !tty_is_exiting(peer)) ?
> > +			(me && !tty_is_exiting(me) ?
> > +			 me : ERR_PTR(-ENXIO)) :
> > +			ERR_PTR(-ENXIO);
> 
> It's difficult to read. Rewrite it w/o using ?: please.

OK

> 
> > +	} else {
> > +		tty = map ? map->master[idx] : NULL;
> > +		if (tty && tty_is_exiting(tty))
> 
> if (!tty || tty_is_exiting(tty))
> 
> ?
> 
> > +			tty = ERR_PTR(-ENXIO);

Nope, already explained.

> > +static int vzt_tty_install(struct tty_driver *driver, struct tty_struct *tty)
> > +{
> > +	envid_t veid = get_exec_env()->veid;
> > +	struct tty_struct *peer;
> > +	tty_map_t *map;
> > +
> > +	map = tty_map_lookup(veid);
> > +	if (!map) {
> > +		map = tty_map_alloc(veid);
> > +		if (IS_ERR_OR_NULL(tty))
> 
> IS_ERR(map)
> 
> ?

yea, typo, thanks!

> > +
> > +static void vzt_tty_shutdown(struct tty_struct *tty)
> > +{
> > +	mutex_lock(&vzt_mutex);
> 
> AFAIU this function is always called under tty_mutex. What's the point
> in the vzt_mutex then?

tty_mutex locks only one peer, but we need to make shutdown
procedure be ordered (they may start simultaneously)

> > +	if (tty_is_slave(tty)) {
> > +		if (!test_bit(TTY_VT_OPEN, &tty->flags)) {
> > +			set_bit(TTY_VT_OPEN, &tty->flags);
> > +			tty_kref_put(tty);
> > +		}
> > +	} else {
> > +		if (!test_bit(TTY_VT_OPEN, &tty->link->flags)) {
> > +			set_bit(TTY_VT_OPEN, &tty->link->flags);
> > +			tty_kref_put(tty->link);
> > +		}
> > +	}
> > +
> > +	tty->port->itty = NULL;
> > +	tty->link->port->itty = NULL;
> > +
> > +	tty_driver_remove_tty(tty->driver, tty);
> > +	tty_driver_remove_tty(vzt_other(tty->driver), tty);
> > +
> > +	mutex_unlock(&vzt_mutex);
> > +}
> > +
> > +static void vzt_tty_cleanup(struct tty_struct *tty)
> > +{
> > +	if (tty_is_slave(tty)) {
> > +		smp_mb__before_clear_bit();
> 
> Why?
> 
> And please comment what you're trying to achieve here.

It's used for locking purpose and doesn't have any
memory barries, so at least smp_ barries needed because
cleanup can be called in any order (for master and
slave) but we need to make sure the master is closed
after the slave.

> 
> > +		clear_bit(TTY_VT_OPEN, &tty->link->flags);
> > +		smp_mb__after_clear_bit();
> > +		wake_up_bit(&tty->link->flags, TTY_VT_OPEN);
> > +
> > +		tty_free_termios(tty);
> > +		cancel_work_sync(&tty->port->buf.work);
> > +	} else {
> > +		wait_on_bit(&tty->flags, TTY_VT_OPEN,
> > +			    vzt_tty_sleep_fn, TASK_KILLABLE);
> 
> Why is TASK_KILLABLE safe?

I think so. In worst scenario (say slave peer never closed,
or task killed before slave finished it work) the pair
simply become unopenable (as far as I can say) but it
doesn't lock the whole system. Is there something I'm missing?

> 
> > +
> > +		tty_free_termios(tty);
> > +		cancel_work_sync(&tty->port->buf.work);
> > +	}
> > +
> > +	WARN_ON_ONCE(test_bit(TTY_LDISC, &tty->flags));
> > +	WARN_ON_ONCE(!test_bit(TTY_LDISC_HALTED, &tty->flags));
> > +
> > +	tty_port_put(tty->port);
> > +}
> > +
> > +static int vzt_tty_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
> > +			tty_perform_flush(peer, TCIFLUSH);
> 
> A comment explaining why you perform flush here would be nice to have.

ok

> > +
> > +static int vzt_tty_write_room(struct tty_struct *tty)
> > +{
> > +	struct tty_struct *peer = tty->link;
> > +	int n;
> > +
> > +	if (tty->stopped)
> > +		return 0;
> > +
> > +	if (peer->count < tty_is_master(tty) ? 1 : 2)
> 
> Please add a comment explaining why you distinguish master tty.
> 
> Also, a general comment on what you're trying to achieve here would be
> good.

ok

> 
> > +		return TTY_BUFFER_PAGE;
> > +
> > +	n = TTY_BUFFER_PAGE - peer->port->buf.memory_used;
> > +	return n < 0 ? 0 : n;
> > +}
> > +
> > +static void vzt_tty_set_termios(struct tty_struct *tty, struct ktermios * old)
> > +{
> > +	tty->termios.c_cflag &= ~(CSIZE | PARENB);
> > +	tty->termios.c_cflag |= (CS8 | CREAD);
> 
> OK, this is copied from pty_set_termios. Please mention it here and
> everywhere you copy-n-paste from pty.c. BTW, wouldn't it be better to
> keep the code in pty.c in order to avoid these code duplications?

I don't think so. I tried all my best to make this module as a separate
one, previous vtty code is really-really hard to support. I'll rather
add comments here why these bits are present. OK?

> > +
> > +struct tty_driver *vzt_console_driver(int *index)
> > +{
> > +	*index = 0;
> > +	return vztm_driver;
> > +}
> 
> > +EXPORT_SYMBOL_GPL(vzt_console_driver);
> 
> Why do you export it?

Because I've been intended to make it a loadable module
but as you pointed below it can't be. I'll rip it off,
tahnks!

> > +
> > +static void ve_vzt_fini(void *data)
> > +{
> > +	struct ve_struct *ve = data;
> > +	tty_map_remove(tty_map_lookup(ve->veid));
> 
> I think we'd better make tty_map_t ref-countable and free it once the
> last tty using it is destroyed.

You know, this map keeps only ~196 bytes and at first I thought about
keeping it inside ve structure, but then thought it gonna be a bad
idea that such data lives outside of the module code. IOW I didn't
want to free it at all at first ;) Adding ref only make code more
complex I think (and make it very close to venet accounting :)
Lets better stick with simplier?

> > +
> > +static void __exit vzt_module_fini(void)
> > +{
> > +	ve_hook_unregister(&vzt_hook);
> > +	tty_unregister_driver(vzts_driver);
> > +	tty_unregister_driver(vztm_driver);
> > +	put_tty_driver(vzts_driver);
> > +	put_tty_driver(vztm_driver);
> > +}
> > +module_exit(vzt_module_fini);
> 
> It can't be a module, because tty_io.c depends on it.

True. Thanks.