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

[Vladimir Davydov]

On Fri, Jul 31, 2015 at 02:48:54PM +0300, Cyrill Gorcunov wrote:

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

I don't mind *smp_mb__after_clear_bit* - it is necessary, but I do
object against *smp_mb__before_clear_bit*, because I still don't see any
point in it and your attempts to explain it only confuse me furthermore
:-) That's why I keep asking you to describe what exactly will go wrong
w/o it - I mean something like:

CPU1		CPU2
----		----
BLAH
		BLAH-BLAH
BUG

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

AFAICS in PCS6 we only drop data if there is no slave - see vtty_write.

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

We are constantly doing it :-)

My point is that the code will still be isolated and therefore easy to
rebase. You can think of it as of a separate file folded into another
file :-) At the same time, you'll be able to reuse pty functions, which
is nice.

> 
> > Seriously, pty and vtty look alike. IMO it's better to keep them
> > together.
> 
> I do not agree, but fine, will do.
>