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

[Cyrill Gorcunov]

On Tue, Aug 18, 2015 at 11:51:20AM +0300, Vladimir Davydov wrote:
...
> > > > +	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.

No :-) Look, the close() reaches this point when this peer (not pair)
is dead, it's about to release including all associated resources so
the application on the link may continue calling write() or whatever
but there is no any reason to dive into lower layer (ie real tty
buffer writes and so on), nobody will be able to fetch the data
writen into such peer.

IOW, imagine we have both ends connected and alive

 slave (container)	master (node)
 close() -
          \
           ->	tty_vhangup()
                 filp->f_op = &hung_up_tty_fops;
                 tty_ldisc_hangup

                after that user may continue write/read
                data but it simply become a nop

I introduced this for vtty in pcs6 because otherwise
we've been noticing problems with ldisk layer when
container intensively writes data into the tty but
then get force stopped (as far as I remember). There
were a bug in jira for it (about a year ago).

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

I think we should notify the readers that peer is down.
Lets do a deal: I'll test the containers without this
hangup and check if it brings some downside, ok?

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

Just dropped ;)

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

Hmm, ok.

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

ok

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

Yeah, I thought about TTY_DRIVER_TYPE_PTY but when been trying it raised
problems with some tty checking code, letme try again, maybe I've been
missing something.

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

In pcs6 we've type=pty which brought problems in current tty layer
iirc. But letme try again to swotch to pty type, maybe I've been
missing something there. Will ping. Thanks!

	Cyrill