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

[Cyrill Gorcunov]

On Fri, Jul 31, 2015 at 12:04:46PM +0300, Vladimir Davydov wrote:
> > > 
> > > 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.

Already done in update, didn't send it yet.

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

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

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

Replied in another email.

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

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

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

> 
> > > > +		tty_free_termios(tty);
> 
> BTW, do we really need to call tty_free_termios here? Isn't it freed in
> release_tty?

Actually no, in case if slave peer has not been opened it won't
be freed via release_tty. But you know I think it's redundant:
we don't carry per-line termios in driver, so better drop it
off. Thanks.

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

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

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

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