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

[Vladimir Davydov]

On Thu, Jul 30, 2015 at 08:27:21PM +0300, Cyrill Gorcunov wrote:
> 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.

I'd prefer to stick to names from PCS6, vtty and vttm, but it's up to
you.

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

None of tty drivers exports names to userspace. Are they all wrong?

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

I'm afraid I still don't understand what you mean.

tty's ->shutdown method is always called with the global tty_mutex held,
so what's the point in taking the vzt_mutex, which you never take
anywhere else?

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

Please explain me what can go wrong if you remove
smp_mb__before_clear_bit.

> slave) but we need to make sure the master is closed
> after the slave.

Why is that?

> 
> > 
> > > +		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);

BTW, do we really need to call tty_free_termios here? Isn't it freed in
release_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

Do we really need to perform flush if both master and slave are open?

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

Please explain me now what "tty_is_master(tty) ? 1 : 2" means.

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

I don't ask you to hack into the pty code. You just place your code
wrapped in CONFIG_VE in drivers/char/pty.c and reuse only those pty
functions that do not need modification, like set_termios. Why is that
worse than copy-n-pasting?

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

I have a bad feeling about it. What if tty slave is still open? Panic?