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

Cyrill Gorcunov gorcunov at virtuozzo.com
Fri Jul 31 04:48:54 PDT 2015


On Fri, Jul 31, 2015 at 01:51:32PM +0300, Vladimir Davydov wrote:
...
> > 
> > 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 ;-)

No way ;)

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

OK

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

In worst scenario the wake will be delayed for some time,
until change get propagated into other cpu (it will eventually
that's why I said that I don't expect tragedy here, and actually
on cpu with really weak ordering such as alpha it will propagate
as well). So by "wrong" here I mean a delay which I think we
can eliminate sith smp_mb helpers.

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

This is not about tty_port_put, we have to be sure that cleanup
for slave is finished _before_ the master. Wait please, I'll reply
via another mail, because this is long.

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

It would miss some portion of data, that's how "console" works
and that's how our pcs6 vtty code worked. I can test the number
of real readers but I thought we're doing quite the opposite:
don't block ever.

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

I can change it, but see my quite above. If we really gonna block
writes when there is a reader, any slow reader (such as "slow" vnc
client) may put writer into a state close to hanging task, waiting
forwrite to complete.

Again, initially I've beed said that this code is supposed to not
block ever if no room left on receiver side. Pavel?

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

Because it is ugly. Instead of carrying own self-containing module
we jump into general code :/ I don;t like this idea.

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

I do not agree, but fine, will do.



More information about the Devel mailing list