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

[Vladimir Davydov]

On Fri, Jul 31, 2015 at 12:54:42PM +0300, Cyrill Gorcunov wrote:

> > > > > +#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?
> 
> They all export it, implicitly. Any change in naming will break
> every userspace tool with works with them. For example ipwireless

It'd break them anyway (need recompilation).

> tty module carries IPWIRELESS_PCCARD_NAME in header. Yes, it's
> not exported into userspace but I'm pretty sure it's a bad practice.

If you think so, please fix it upstream ;-)

> We have (have we?) to export number of consoles, and exporting consoles names
> is good choise as well I think. That said, if you still prefer
> to hide them I will do (moreover, I think there would be no much
> sense in this header at all because exporting only a number of
> consoles won't make situation anyhow better).

Please.

> > > > > +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.
> 
> /**
>  * clear_bit - Clears a bit in memory
>  * @nr: Bit to clear
>  * @addr: Address to start counting from
>  *
>  * clear_bit() is atomic and may not be reordered.  However, it does
>  * not contain a memory barrier, so if it is used for locking purposes,
>  * you should call smp_mb__before_clear_bit() and/or smp_mb__after_clear_bit()
>  * in order to ensure changes are visible on other processors.
>  */
> 
> We use it as ordering point for cleanup routine, so we've
> to be sure that other cpus are seeing our changes here.
> If we don't put these routines here I dont expect any
> tragedy because due to mesi other cpus will notify the
> change in any case and waiter still run (and because master peer
> and slave peer are carrying unbound data structures which
> are gonna be freed, any delay or reorded won't hurt the only
> matter here to be sure that structures are freed),
> but I'm not that confident on other archs and this code
> is general code which supposed to work on any arch even
> if we don't build them ever.

What *exactly* can go wrong on any arch of your preference if you remove
this smp_mb__before_clear_bit?

> 
> > > slave) but we need to make sure the master is closed
> > > after the slave.
> > 
> > Why is that?
> 
> Because you may open master and never open slave but master
> carries slave reference, so that there no tty_release ever
> been called on slave peer, but it gonna be cleaned up
> via kref decrement code.
> 
> IOW, slave peer may be cleaned up via two cases: via
> explicit release on file close (if it has been opened),
> or via kref-put (if it has not been opened, but master
> slave is closing decrementing slave peer's kref and
> kernel schedule to call cleanup() on both peers but
> cleanup() routine can be called in any order, the
> kernel chooses on its own which one to clean first).

All your cleanup consists of tty_port_put. Why is it bad to call
tty_port_put for master before tty_port_put for slave? Both ttys are
already dead, so why should we care?

> 
> Thus we need to bring own order here, and that's
> done via wait-bit.

> > > > > +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?
> 
> Yes. Open state only mean that there is a reader, but reader might not
> yet be ready to fetch all data, so the base idea is to not block inside
> write ever.

That's a wrong idea. We must block writes if there is a reader. Think
what will happen if our VNC viewer does not manage to read data in time.

> 
> > > 
> > > ok
> > 
> > Please explain me now what "tty_is_master(tty) ? 1 : 2" means.
> 
> I'll add a comment. In short -- master peer always carry an
> aditional count reference on the peer, so the kernel wont
> free slave peer structure until it's explicitly done on
> master peer release.

I think we should drop data only if we write to master and there is no
slave, i.e. if peer->count < 2.

> 
> > > > > +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?
> 
> Simply because I didn't use much code from pty. The only real copy
> is set termios. I really don't wanna uglify pty.c with our CONFIG_VE
> just for this piece.

Why "uglify"? I said you don't mess with pty code. You only add your
code in this file accurately wrapped in CONFIG_VE. Why do you think it's
ugly?

Seriously, pty and vtty look alike. IMO it's better to keep them
together.

> 
> > > > > +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?
> 
> On exit files are closed before the ve get vanished as far as I remember,
> so slave peer get released before we free the map.
> 

OK, I surrender this point for now.