Re: [PATCH 07/10] TTY: switch tty_schedule_flip

[Peter Hurley <peter@hurleysoftware.com>]

On Tue, 2013-02-05 at 12:40 +0100, Jiri Slaby wrote:
> On 02/01/2013 09:39 PM, Peter Hurley wrote:
> > On Fri, 2013-02-01 at 16:06 +0100, Jiri Slaby wrote:
> >> On 02/01/2013 01:37 PM, Peter Hurley wrote:
> >>> On Thu, 2013-01-03 at 15:53 +0100, Jiri Slaby wrote:
> >>>> Now, we start converting tty buffer functions to actually use
> >>>> tty_port. This will allow us to get rid of the need of tty in many
> >>>> call sites. Only tty_port will needed and hence no more
> >>>> tty_port_tty_get in those paths.
> >>>>
> >>>> This is the last one: tty_schedule_flip
> >>>
> >>> [snip]
> >>>
> >>>> diff --git a/drivers/tty/vt/keyboard.c b/drivers/tty/vt/keyboard.c
> >>>> index 5aace4d..a9af1b9a 100644
> >>>> --- a/drivers/tty/vt/keyboard.c
> >>>> +++ b/drivers/tty/vt/keyboard.c
> >>>> @@ -307,26 +307,17 @@ int kbd_rate(struct kbd_repeat *rep)
> >>>>   */
> >>>>  static void put_queue(struct vc_data *vc, int ch)
> >>>>  {
> >>>> -	struct tty_struct *tty = vc->port.tty;
> >>>> -
> >>>>  	tty_insert_flip_char(&vc->port, ch, 0);
> >>>> -	if (tty) {
> >>>> -		tty_schedule_flip(tty);
> >>>> -	}
> >>>> +	tty_schedule_flip(&vc->port);
> >>>>  }
> >>>>  
> >>>>  static void puts_queue(struct vc_data *vc, char *cp)
> >>>>  {
> >>>> -	struct tty_struct *tty = vc->port.tty;
> >>>> -
> >>>> -	if (!tty)
> >>>> -		return;
> >>>> -
> >>>>  	while (*cp) {
> >>>>  		tty_insert_flip_char(&vc->port, *cp, 0);
> >>>>  		cp++;
> >>>>  	}
> >>>> -	tty_schedule_flip(tty);
> >>>> +	tty_schedule_flip(&vc->port);
> >>>>  }
> >>>
> >>> Umm. So even though the vt driver knows the tty has been shutdown,
> >>> keystrokes will still be buffered? And then fed to whichever tty happens
> >>> to next get installed on the same port?
> >>
> >> Unless I completely missed something, they should be flushed. If that's
> >> not the case, that's a bug. I will check next week.
> > 
> > Ok.
> > 
> > I'm fairly sure this is happening. Try repeatedly tapping arrow-down
> > while booting (I suggest a VM) and you won't be able to login at tty1
> > (ie, text mode). Repeatable 1 time in 5 or so.
> 
> I can reproduce... with 3.0! Are you sure this is new and introduced by
> the tty buffers switch?

No, I'm not sure this is new.

> > FWIW, I don't agree that the best way is to flush the flip buffers. Why
> > buffer and then schedule the cpu to do work which we already know it
> > can't do and which we're going to discard anyway?
> 
> Because the drivers needn't care whether there is any tty behind and
> listening. This simplifies and speeds up the paths _a lot_. No spin
> locks, no ttys around, no two code paths etc.

Let's defer this part of the discussion until after everything is
running solid.

> > In any event, since -next is already carrying these patches, and
> > by-design, these patches _expect_ the tty to be NULL, are you going to
> > remove the warning in flush_to_ldisc() or shall I?
> 
> I think I don't understand you here, could you elaborate?

         - CPU 0 -          |          - CPU 1 -
                            |
release_tty()               |
   .                        |
   tty->ops->shutdown()     |
      con_shutdown()        |
      .                     |
      vc->port.tty = NULL;  |
      .                     |
   tty->port->itty = NULL;  |
   .                        | puts_queue()
   .                        |    .
   .                        |    tty_schedule_flip(&vc->port);
   .                        |    .
                            |
                            | flush_to_ldisc() runs & WARNs

If driver i/o paths don't need to worry about the lifetime of the tty,
then why warn when they inevitably schedule work to a dead tty (as
above)?

Also, what ensures that flush_to_ldisc() has a valid tty when obtaining
an ldisc ref?  IOW,

        - CPU 0 -           |          - CPU 1 -
                            |
                            | flush_to_ldisc()
                            |    tty = port->itty;
                            | < scheduled away - IRQ servicing ...
tty->port->itty = NULL;     |    .
...                         |    .
free_tty_struct(tty);       |    .
                            | ... returns from IRQ service >
                            | ldisc = tty_ldisc_ref(tty);

Regards,
Peter Hurley