[PATCH] Use tty_schedule in VT code.

[PATCH] Use tty_schedule in VT code.

[James Simmons]

This patch has the VT subsystem use tty_schedule_flip instead of 
con_schedule_flip. There are two ways we can approach this. We can
do the below path or extend tty_schedule_flip to accept a time field.
Comments welcomed.

Signed-Off: James Simmons <jsimmons@infradead.org>

diff --git a/drivers/char/keyboard.c b/drivers/char/keyboard.c
index cb8d691..2db8973 100644
--- a/drivers/char/keyboard.c
+++ b/drivers/char/keyboard.c
@@ -303,7 +303,7 @@ static void put_queue(struct vc_data *vc, int ch)
 
 	if (tty) {
 		tty_insert_flip_char(tty, ch, 0);
-		con_schedule_flip(tty);
+		tty_schedule_flip(tty);
 	}
 }
 
@@ -318,7 +318,7 @@ static void puts_queue(struct vc_data *vc, char *cp)
 		tty_insert_flip_char(tty, *cp, 0);
 		cp++;
 	}
-	con_schedule_flip(tty);
+	tty_schedule_flip(tty);
 }
 
 static void applkey(struct vc_data *vc, int key, char mode)
@@ -549,7 +549,7 @@ static void fn_send_intr(struct vc_data *vc)
 	if (!tty)
 		return;
 	tty_insert_flip_char(tty, 0, TTY_BREAK);
-	con_schedule_flip(tty);
+	tty_schedule_flip(tty);
 }
 
 static void fn_scroll_forw(struct vc_data *vc)
diff --git a/drivers/char/vt.c b/drivers/char/vt.c
index 1bbb45b..c9bbb14 100644
--- a/drivers/char/vt.c
+++ b/drivers/char/vt.c
@@ -1240,7 +1240,7 @@ static void respond_string(const char *p, struct tty_struct *tty)
 		tty_insert_flip_char(tty, *p, 0);
 		p++;
 	}
-	con_schedule_flip(tty);
+	tty_schedule_flip(tty);
 }
 
 static void cursor_report(struct vc_data *vc, struct tty_struct *tty)
diff --git a/include/linux/kbd_kern.h b/include/linux/kbd_kern.h
index 506ad20..7b24a0d 100644
--- a/include/linux/kbd_kern.h
+++ b/include/linux/kbd_kern.h
@@ -149,16 +149,4 @@ void compute_shiftstate(void);
 
 extern unsigned int keymap_count;
 
-/* console.c */
-
-static inline void con_schedule_flip(struct tty_struct *t)
-{
-	unsigned long flags;
-	spin_lock_irqsave(&t->buf.lock, flags);
-	if (t->buf.tail != NULL)
-		t->buf.tail->commit = t->buf.tail->used;
-	spin_unlock_irqrestore(&t->buf.lock, flags);
-	schedule_delayed_work(&t->buf.work, 0);
-}
-
 #endif

Re: [PATCH] Use tty_schedule in VT code.

[Paul Fulghum]

On Tue, 2007-05-08 at 21:10 +0100, James Simmons wrote:
> 
> This patch has the VT subsystem use tty_schedule_flip instead of 
> con_schedule_flip. There are two ways we can approach this. We can
> do the below path or extend tty_schedule_flip to accept a time field.
> Comments welcomed.

This looks reasonable.

I don't think a time field is necessary. In fact, I think the
scheduled_delayed_work() in tty_schedule_flip() should use a
time of 0 just like con_schedule_flip().

As the tty flip buffer code has evolved, that delay value of 1
was carried along. It may have had some historical purpose, but
I can't figure it out and it appears to have no use currently.

It would be better for performance to process the receive data
as soon as possible (delay value of 0).


--
Paul Fulghum
Microgate Systems, Ltd

Re: [PATCH] Use tty_schedule in VT code.

[James Simmons]

> Paul Fulghum wrote:
> > As the tty flip buffer code has evolved, that delay value of 1
> > was carried along. It may have had some historical purpose, but
> > I can't figure it out and it appears to have no use currently.
> 
> I looked further back and in the 2.4 kernels this scheduling
> was done with the timer task queue (process receive data on
> next timer tick).
> 
> I guess the schedule_delayed_work() with a time delay of 1
> was the best approximation of the previous behavior.
> 
> There is no logical reason to delay the first attempt at
> processing receive data so schedule_delayed_work() in
> tty_schedule_flip() should be changed to 0 (as was the
> case for con_schedule_flip).
> 
> The schedule_delayed_work in flush_to_ldisc() will continue
> to use a delay of 1 if the ldisc can't accept more data.
> This allows the user app and ldisc to catch up.
> 
> Subsequent calls to tty_schedule_flip won't affect
> this 'back off' delay because once the work is scheduled
> (with a delay of 1) new scheduling calls are ignored for
> the same work structure.
> 
> I've been testing the change to 0 in tty_schedule_flip()
> under various loads and data rates with no ill effects.

Great!!!! Here is the patch updated with the delay value set to zero.

Signed-Off: James Simmons <jsimmons@infradead.org>

diff --git a/drivers/char/keyboard.c b/drivers/char/keyboard.c
index 1b09450..0027736 100644
--- a/drivers/char/keyboard.c
+++ b/drivers/char/keyboard.c
@@ -277,7 +277,7 @@ static void put_queue(struct vc_data *vc, int ch)
 
 	if (tty) {
 		tty_insert_flip_char(tty, ch, 0);
-		con_schedule_flip(tty);
+		tty_schedule_flip(tty);
 	}
 }
 
@@ -292,7 +292,7 @@ static void puts_queue(struct vc_data *vc, char *cp)
 		tty_insert_flip_char(tty, *cp, 0);
 		cp++;
 	}
-	con_schedule_flip(tty);
+	tty_schedule_flip(tty);
 }
 
 static void applkey(struct vc_data *vc, int key, char mode)
@@ -523,7 +523,7 @@ static void fn_send_intr(struct vc_data *vc)
 	if (!tty)
 		return;
 	tty_insert_flip_char(tty, 0, TTY_BREAK);
-	con_schedule_flip(tty);
+	tty_schedule_flip(tty);
 }
 
 static void fn_scroll_forw(struct vc_data *vc)
diff --git a/drivers/char/tty_io.c b/drivers/char/tty_io.c
index 7710a6a..0174c3f 100644
--- a/drivers/char/tty_io.c
+++ b/drivers/char/tty_io.c
@@ -530,7 +530,7 @@ void tty_schedule_flip(struct tty_struct *tty)
 	if (tty->buf.tail != NULL)
 		tty->buf.tail->commit = tty->buf.tail->used;
 	spin_unlock_irqrestore(&tty->buf.lock, flags);
-	schedule_delayed_work(&tty->buf.work, 1);
+	schedule_delayed_work(&tty->buf.work, 0);
 }
 EXPORT_SYMBOL(tty_schedule_flip);
 
diff --git a/drivers/char/vt.c b/drivers/char/vt.c
index bbd9fc4..283e189 100644
--- a/drivers/char/vt.c
+++ b/drivers/char/vt.c
@@ -1261,7 +1261,7 @@ static void respond_string(const char *p, struct tty_struct *tty)
 		tty_insert_flip_char(tty, *p, 0);
 		p++;
 	}
-	con_schedule_flip(tty);
+	tty_schedule_flip(tty);
 }
 
 static void cursor_report(struct vc_data *vc, struct tty_struct *tty)
diff --git a/include/linux/kbd_kern.h b/include/linux/kbd_kern.h
index 506ad20..7b24a0d 100644
--- a/include/linux/kbd_kern.h
+++ b/include/linux/kbd_kern.h
@@ -149,16 +149,4 @@ void compute_shiftstate(void);
 
 extern unsigned int keymap_count;
 
-/* console.c */
-
-static inline void con_schedule_flip(struct tty_struct *t)
-{
-	unsigned long flags;
-	spin_lock_irqsave(&t->buf.lock, flags);
-	if (t->buf.tail != NULL)
-		t->buf.tail->commit = t->buf.tail->used;
-	spin_unlock_irqrestore(&t->buf.lock, flags);
-	schedule_delayed_work(&t->buf.work, 0);
-}
-
 #endif

Re: [PATCH] Use tty_schedule in VT code.

[Paul Fulghum]

Paul Fulghum wrote:
> As the tty flip buffer code has evolved, that delay value of 1
> was carried along. It may have had some historical purpose, but
> I can't figure it out and it appears to have no use currently.

I looked further back and in the 2.4 kernels this scheduling
was done with the timer task queue (process receive data on
next timer tick).

I guess the schedule_delayed_work() with a time delay of 1
was the best approximation of the previous behavior.

There is no logical reason to delay the first attempt at
processing receive data so schedule_delayed_work() in
tty_schedule_flip() should be changed to 0 (as was the
case for con_schedule_flip).

The schedule_delayed_work in flush_to_ldisc() will continue
to use a delay of 1 if the ldisc can't accept more data.
This allows the user app and ldisc to catch up.

Subsequent calls to tty_schedule_flip won't affect
this 'back off' delay because once the work is scheduled
(with a delay of 1) new scheduling calls are ignored for
the same work structure.

I've been testing the change to 0 in tty_schedule_flip()
under various loads and data rates with no ill effects.

-- 
Paul Fulghum
Microgate Systems, Ltd.

Re: [PATCH] Use tty_schedule in VT code.

[Paul Fulghum]

James Simmons wrote:
> Great!!!! Here is the patch updated with the delay value set to zero.

Acked-by: Paul Fulghum <paulkf@microgate.com>

You should submit this to
Andrew Morton <akpm@linux-foundation.org>
so it can get into the mm tree.

--
Paul

[PATCH] Use tty_schedule in VT code.

[James Simmons]

This patch has the VT subsystem use tty_schedule_flip instead of
con_schedule_flip. The patch has been tested for several weeks 
on my local system and has been in the linuxconsole repository
for some time. Please apply.

Signed-Off: James Simmons <jsimmons@infradead.org>

diff --git a/drivers/char/keyboard.c b/drivers/char/keyboard.c
index 2ce0af1..68eab40 100644
--- a/drivers/char/keyboard.c
+++ b/drivers/char/keyboard.c
@@ -278,7 +278,7 @@ static void put_queue(struct vc_data *vc, int ch)
 
 	if (tty) {
 		tty_insert_flip_char(tty, ch, 0);
-		con_schedule_flip(tty);
+		tty_schedule_flip(tty);
 	}
 }
 
@@ -293,7 +293,7 @@ static void puts_queue(struct vc_data *vc, char *cp)
 		tty_insert_flip_char(tty, *cp, 0);
 		cp++;
 	}
-	con_schedule_flip(tty);
+	tty_schedule_flip(tty);
 }
 
 static void applkey(struct vc_data *vc, int key, char mode)
@@ -533,7 +533,7 @@ static void fn_send_intr(struct vc_data *vc)
 	if (!tty)
 		return;
 	tty_insert_flip_char(tty, 0, TTY_BREAK);
-	con_schedule_flip(tty);
+	tty_schedule_flip(tty);
 }
 
 static void fn_scroll_forw(struct vc_data *vc)
diff --git a/drivers/char/tty_io.c b/drivers/char/tty_io.c
index de37ebc..34894e5 100644
--- a/drivers/char/tty_io.c
+++ b/drivers/char/tty_io.c
@@ -559,7 +559,7 @@ void tty_schedule_flip(struct tty_struct *tty)
 	if (tty->buf.tail != NULL)
 		tty->buf.tail->commit = tty->buf.tail->used;
 	spin_unlock_irqrestore(&tty->buf.lock, flags);
-	schedule_delayed_work(&tty->buf.work, 1);
+	schedule_delayed_work(&tty->buf.work, 0);
 }
 EXPORT_SYMBOL(tty_schedule_flip);
 
diff --git a/drivers/char/vt.c b/drivers/char/vt.c
index edb7002..d17cfcd 100644
--- a/drivers/char/vt.c
+++ b/drivers/char/vt.c
@@ -1260,7 +1260,7 @@ static void respond_string(const char *p, struct tty_struct *tty)
 		tty_insert_flip_char(tty, *p, 0);
 		p++;
 	}
-	con_schedule_flip(tty);
+	tty_schedule_flip(tty);
 }
 
 static void cursor_report(struct vc_data *vc, struct tty_struct *tty)
diff --git a/include/linux/kbd_kern.h b/include/linux/kbd_kern.h
index 506ad20..7b24a0d 100644
--- a/include/linux/kbd_kern.h
+++ b/include/linux/kbd_kern.h
@@ -149,16 +149,4 @@ void compute_shiftstate(void);
 
 extern unsigned int keymap_count;
 
-/* console.c */
-
-static inline void con_schedule_flip(struct tty_struct *t)
-{
-	unsigned long flags;
-	spin_lock_irqsave(&t->buf.lock, flags);
-	if (t->buf.tail != NULL)
-		t->buf.tail->commit = t->buf.tail->used;
-	spin_unlock_irqrestore(&t->buf.lock, flags);
-	schedule_delayed_work(&t->buf.work, 0);
-}
-
 #endif

Re: [PATCH] Use tty_schedule in VT code.

[Samuel Thibault]

James Simmons, le Tue 17 Jul 2007 19:37:57 +0100, a écrit :
> -	schedule_delayed_work(&t->buf.work, 0);

It was schedule_delayed_work(&t->buf.work, 1); in con_schedule_flip() ;
could that matter?

Samuel

Re: [PATCH] Use tty_schedule in VT code.

[James Simmons]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 242 bytes --]


> James Simmons, le Tue 17 Jul 2007 19:37:57 +0100, a écrit :
> > -	schedule_delayed_work(&t->buf.work, 0);
> 
> It was schedule_delayed_work(&t->buf.work, 1); in con_schedule_flip() ;
> could that matter?

I did not detect any regressions.

Re: [PATCH] Use tty_schedule in VT code.

[Linus Torvalds]

[ Alan added to participants list, since he's in charge of tty code ]

On Tue, 17 Jul 2007, James Simmons wrote:
> diff --git a/drivers/char/tty_io.c b/drivers/char/tty_io.c
> index de37ebc..34894e5 100644
> --- a/drivers/char/tty_io.c
> +++ b/drivers/char/tty_io.c
> @@ -559,7 +559,7 @@ void tty_schedule_flip(struct tty_struct *tty)
>  	if (tty->buf.tail != NULL)
>  		tty->buf.tail->commit = tty->buf.tail->used;
>  	spin_unlock_irqrestore(&tty->buf.lock, flags);
> -	schedule_delayed_work(&tty->buf.work, 1);
> +	schedule_delayed_work(&tty->buf.work, 0);

Is there any real reason for this?

I think that patch is bogus. Either it should stay at 1, or the whole work 
should be a non-scheduled one instead.

Do we really need to handle it asap for the console, or is it ok to wait 
for the next tick, like the regular tty case used to?

And if we need to handle it asap, why the "delayed"?

		Linus

Re: [PATCH] Use tty_schedule in VT code.

[Linus Torvalds]

On Tue, 17 Jul 2007, Paul Fulghum wrote:
> 
> The scheduling is to move the processing out of interrupt context.
> The receive data is often extracted from the hardware
> at interrupt time and then queued for processing.

You misunderstand.

If the "delay" is zero, then why are you using "delayed" workqueues at 
all?

A delay of zero tends to be pointless. It's not a delay at all, and you 
could just use the regular NON-DELAYING workqueues.

		Linus

Re: [PATCH] Use tty_schedule in VT code.

[James Simmons]

> On Tue, 17 Jul 2007, Paul Fulghum wrote:
> > 
> > The scheduling is to move the processing out of interrupt context.
> > The receive data is often extracted from the hardware
> > at interrupt time and then queued for processing.
> 
> You misunderstand.
> 
> If the "delay" is zero, then why are you using "delayed" workqueues at 
> all?
> 
> A delay of zero tends to be pointless. It's not a delay at all, and you 
> could just use the regular NON-DELAYING workqueues.

Because sometimes you do want the delay. In other parts of the tty 
code we do delay. What should be done is 

        if (tty->low_latency)
                flush_to_ldisc(&tty->buf.work.work);
        else
                schedule_delayed_work(&tty->buf.work, 1);

Is this acceptable to you?

Re: [PATCH] Use tty_schedule in VT code.

[James Simmons]

> James Simmons wrote:
> > Because sometimes you do want the delay. In other parts of the tty code we
> > do delay. What should be done is 
> 
> Correct, so we must stick with the delayed work structure
> which requires calling the delayed work function.
> 
> >         if (tty->low_latency)
> >                 flush_to_ldisc(&tty->buf.work.work);
> >         else
> >                 schedule_delayed_work(&tty->buf.work, 1);
> > 
> > Is this acceptable to you?
> 
> That does not make sense to me.
> 
> If you are calling from interrupt context, you do not want
> to call flush_to_ldisc() directly regardless of low_latency.
> This used to be the way it was done and it ended up causing
> deadlocks in just that situation.

The low_latency is used by the drivers in the case where its 
not in a interrupt context. Well we are trusting the drivers.
Now if it is true what you said then tty_flip_buffer_push has
a bug. Looking at several drivers including serial devices
they set the low_latency flag.

> And the initial schedule has no reason to add the extra delay.

So do you support a non delay work queue as well?

Re: [PATCH] Use tty_schedule in VT code.

[Paul Fulghum]

James Simmons wrote:
>> James Simmons, le Tue 17 Jul 2007 19:37:57 +0100, a écrit :
>>> -	schedule_delayed_work(&t->buf.work, 0);
>> It was schedule_delayed_work(&t->buf.work, 1); in con_schedule_flip() ;
>> could that matter?
> 
> I did not detect any regressions.

The console behavior stays exactly the same as the patch
changes tty_schedule_flip to use the 0 delay. The change
to tty_schedule_flip() to use 0 delay also is OK. I had
looked at this when James originally posted this patch and found:

I looked further back and in the 2.4 kernels this scheduling
was done with the timer task queue (process receive data on
next timer tick).

I guess the schedule_delayed_work() with a time delay of 1
was the best approximation of the previous behavior.

There is no logical reason to delay the first attempt at
processing receive data so schedule_delayed_work() in
tty_schedule_flip() should be changed to 0 (as was the
case for con_schedule_flip).

The schedule_delayed_work in flush_to_ldisc() will continue
to use a delay of 1 if the ldisc can't accept more data.
This allows the user app and ldisc to catch up.

Subsequent calls to tty_schedule_flip won't affect
this 'back off' delay because once the work is scheduled
(with a delay of 1) new scheduling calls are ignored for
the same work structure.

I've been testing the change to 0 in tty_schedule_flip()
under various loads and data rates with no ill effects.


-- 
Paul Fulghum
Microgate Systems, Ltd.

Re: [PATCH] Use tty_schedule in VT code.

[Paul Fulghum]

Linus Torvalds wrote:
>> -	schedule_delayed_work(&tty->buf.work, 1);
>> +	schedule_delayed_work(&tty->buf.work, 0);
> 
> Is there any real reason for this?
> 
> I think that patch is bogus. Either it should stay at 1, or the whole work 
> should be a non-scheduled one instead.
> 
> Do we really need to handle it asap for the console, or is it ok to wait 
> for the next tick, like the regular tty case used to?
> 
> And if we need to handle it asap, why the "delayed"?

The scheduling is to move the processing out of interrupt context.
The receive data is often extracted from the hardware
at interrupt time and then queued for processing.

-- 
Paul Fulghum
Microgate Systems, Ltd.

Re: [PATCH] Use tty_schedule in VT code.

[Linus Torvalds]

On Tue, 17 Jul 2007, James Simmons wrote:
> 
> Because sometimes you do want the delay. In other parts of the tty 
> code we do delay.

Ahh, ok, in that it's ok by me.

> What should be done is 
> 
>         if (tty->low_latency)
>                 flush_to_ldisc(&tty->buf.work.work);
>         else
>                 schedule_delayed_work(&tty->buf.work, 1);
> 
> Is this acceptable to you?

In that case, we might as well just always do the scheduled_delayed_work() 
with a zero timeout as per the earlier patch. I'd still like to know who 
*cares*, though? Why not leave it at 1?

		Linus

Re: [PATCH] Use tty_schedule in VT code.

[Paul Fulghum]

James Simmons wrote:
> Because sometimes you do want the delay. In other parts of the tty 
> code we do delay. What should be done is 

Correct, so we must stick with the delayed work structure
which requires calling the delayed work function.

>         if (tty->low_latency)
>                 flush_to_ldisc(&tty->buf.work.work);
>         else
>                 schedule_delayed_work(&tty->buf.work, 1);
> 
> Is this acceptable to you?

That does not make sense to me.

If you are calling from interrupt context, you do not want
to call flush_to_ldisc() directly regardless of low_latency.
This used to be the way it was done and it ended up causing
deadlocks in just that situation.

And the initial schedule has no reason to add the extra delay.

-- 
Paul Fulghum
Microgate Systems, Ltd.

Re: [PATCH] Use tty_schedule in VT code.

[Paul Fulghum]

James Simmons wrote:
> The low_latency is used by the drivers in the case where its 
> not in a interrupt context. Well we are trusting the drivers.
> Now if it is true what you said then tty_flip_buffer_push has
> a bug. Looking at several drivers including serial devices
> they set the low_latency flag.

The generic serial driver (8250) is the one that was
dead locking when that code originally existed.
It was setting low_latency and calling from interrupt context.

>> And the initial schedule has no reason to add the extra delay.
> 
> So do you support a non delay work queue as well?

No, the delay work must be used for flush_to_ldisc()
so it makes no sense to define two different work queues
(one delayed and one not) for the same work.

I support your patch.

The current stuff works and your patch works.
With your patch, you actually reduce initial
latency for processing receive data.

Whichever way everyone else wants to go.


-- 
Paul Fulghum
Microgate Systems, Ltd.

Re: [PATCH] Use tty_schedule in VT code.

[Alan Cox]

O> In that case, we might as well just always do the scheduled_delayed_work() 
> with a zero timeout as per the earlier patch. I'd still like to know who 
> *cares*, though? Why not leave it at 1?

I don't think it really matters too much on modern systems so long as we
keep the flush_to_ldisc out of IRQ context. Historically we tried to
batch the ldisc processing but I doubt any modern box cares too much

Re: [PATCH] Use tty_schedule in VT code.

[James Simmons]

> James Simmons wrote:
> > The low_latency is used by the drivers in the case where its not in a
> > interrupt context. Well we are trusting the drivers.
> > Now if it is true what you said then tty_flip_buffer_push has
> > a bug. Looking at several drivers including serial devices
> > they set the low_latency flag.
> 
> The generic serial driver (8250) is the one that was
> dead locking when that code originally existed.
> It was setting low_latency and calling from interrupt context.

serial8250_interrupt -> serial8250_handle_port ->
receive_chars -> tty_flip_buffer_push

It could still dead lock. The low_latency flag is too weak of a test.
I patched tty_flip_buffer_push to fix this.

> > > And the initial schedule has no reason to add the extra delay.
> > 
> > So do you support a non delay work queue as well?
> 
> No, the delay work must be used for flush_to_ldisc()
> so it makes no sense to define two different work queues
> (one delayed and one not) for the same work.

Sorry I mean move to a just a non delay work queue only.
Which we won't anyways.

> I support your patch.

> The current stuff works and your patch works.
> With your patch, you actually reduce initial
> latency for processing receive data.

Thanks.

Re: [PATCH] Use tty_schedule in VT code.

[James Simmons]

> > What should be done is 
> > 
> >         if (tty->low_latency)
> >                 flush_to_ldisc(&tty->buf.work.work);
> >         else
> >                 schedule_delayed_work(&tty->buf.work, 1);
> > 
> > Is this acceptable to you?
> 
> In that case, we might as well just always do the scheduled_delayed_work() 
> with a zero timeout as per the earlier patch. I'd still like to know who 
> *cares*, though? Why not leave it at 1?

I have no problem leaving at one. Here is the new patch. I did address the 
problem with tty_flip_buffer_push in this patch. It is possible for a 
driver to call tty_flip_buffer_push within a interrupt context if they
set the low_latency flag.

diff --git a/drivers/char/keyboard.c b/drivers/char/keyboard.c
index 2ce0af1..68eab40 100644
--- a/drivers/char/keyboard.c
+++ b/drivers/char/keyboard.c
@@ -278,7 +278,7 @@ static void put_queue(struct vc_data *vc, int ch)
 
 	if (tty) {
 		tty_insert_flip_char(tty, ch, 0);
-		con_schedule_flip(tty);
+		tty_schedule_flip(tty);
 	}
 }
 
@@ -293,7 +293,7 @@ static void puts_queue(struct vc_data *vc, char *cp)
 		tty_insert_flip_char(tty, *cp, 0);
 		cp++;
 	}
-	con_schedule_flip(tty);
+	tty_schedule_flip(tty);
 }
 
 static void applkey(struct vc_data *vc, int key, char mode)
@@ -533,7 +533,7 @@ static void fn_send_intr(struct vc_data *vc)
 	if (!tty)
 		return;
 	tty_insert_flip_char(tty, 0, TTY_BREAK);
-	con_schedule_flip(tty);
+	tty_schedule_flip(tty);
 }
 
 static void fn_scroll_forw(struct vc_data *vc)
diff --git a/drivers/char/tty_io.c b/drivers/char/tty_io.c
index de37ebc..8533173 100644
--- a/drivers/char/tty_io.c
+++ b/drivers/char/tty_io.c
@@ -3647,11 +3647,7 @@ void tty_flip_buffer_push(struct tty_struct *tty)
 	if (tty->buf.tail != NULL)
 		tty->buf.tail->commit = tty->buf.tail->used;
 	spin_unlock_irqrestore(&tty->buf.lock, flags);
-
-	if (tty->low_latency)
-		flush_to_ldisc(&tty->buf.work.work);
-	else
-		schedule_delayed_work(&tty->buf.work, 1);
+	schedule_delayed_work(&tty->buf.work, 1);
 }
 
 EXPORT_SYMBOL(tty_flip_buffer_push);
diff --git a/drivers/char/vt.c b/drivers/char/vt.c
index edb7002..d17cfcd 100644
--- a/drivers/char/vt.c
+++ b/drivers/char/vt.c
@@ -1260,7 +1260,7 @@ static void respond_string(const char *p, struct tty_struct *tty)
 		tty_insert_flip_char(tty, *p, 0);
 		p++;
 	}
-	con_schedule_flip(tty);
+	tty_schedule_flip(tty);
 }
 
 static void cursor_report(struct vc_data *vc, struct tty_struct *tty)
diff --git a/include/linux/kbd_kern.h b/include/linux/kbd_kern.h
index 506ad20..7b24a0d 100644
--- a/include/linux/kbd_kern.h
+++ b/include/linux/kbd_kern.h
@@ -149,16 +149,4 @@ void compute_shiftstate(void);
 
 extern unsigned int keymap_count;
 
-/* console.c */
-
-static inline void con_schedule_flip(struct tty_struct *t)
-{
-	unsigned long flags;
-	spin_lock_irqsave(&t->buf.lock, flags);
-	if (t->buf.tail != NULL)
-		t->buf.tail->commit = t->buf.tail->used;
-	spin_unlock_irqrestore(&t->buf.lock, flags);
-	schedule_delayed_work(&t->buf.work, 0);
-}
-
 #endif

Re: [PATCH] Use tty_schedule in VT code.

[Paul Fulghum]

On Wed, 2007-07-18 at 19:17 +0100, James Simmons wrote:

> I have no problem leaving at one. Here is the new patch. I did address the 
> problem with tty_flip_buffer_push in this patch. It is possible for a 
> driver to call tty_flip_buffer_push within a interrupt context if they
> set the low_latency flag.
> --- a/drivers/char/tty_io.c
> +++ b/drivers/char/tty_io.c
> @@ -3647,11 +3647,7 @@ void tty_flip_buffer_push(struct tty_struct *tty)
>  	if (tty->buf.tail != NULL)
>  		tty->buf.tail->commit = tty->buf.tail->used;
>  	spin_unlock_irqrestore(&tty->buf.lock, flags);
> -
> -	if (tty->low_latency)
> -		flush_to_ldisc(&tty->buf.work.work);
> -	else
> -		schedule_delayed_work(&tty->buf.work, 1);
> +	schedule_delayed_work(&tty->buf.work, 1);
>  }

While I have no problem with this, it would be a significant
behavior change (more so than changing the initial delay to 0).

IIRC, when the serial_core dead lock was being debugged
(by Russel King with some Dell guy who reported it 1-2 years ago) 
this change was suggested and rejected because some callers
are not running at IRQ context and want to use low_latency.
In the end, I think they decided to leave the correct use of
low_latency WRT running in IRQ context to the caller.

It might be safest to drop this portion so you can get the
obvious part of the patch accepted (consolidating
the redundant xxx_schedule_flip functions).
 
--
Paul Fulghum
Microgate Systems, Ltd

Re: [PATCH] Use tty_schedule in VT code.

[James Simmons]

> While I have no problem with this, it would be a significant
> behavior change (more so than changing the initial delay to 0).
> 
> IIRC, when the serial_core dead lock was being debugged
> (by Russel King with some Dell guy who reported it 1-2 years ago) 
> this change was suggested and rejected because some callers
> are not running at IRQ context and want to use low_latency.
> In the end, I think they decided to leave the correct use of
> low_latency WRT running in IRQ context to the caller.
> 
> It might be safest to drop this portion so you can get the
> obvious part of the patch accepted (consolidating
> the redundant xxx_schedule_flip functions).

Done. I still smell a dead lock issue tho.

diff --git a/drivers/char/keyboard.c b/drivers/char/keyboard.c
index 2ce0af1..68eab40 100644
--- a/drivers/char/keyboard.c
+++ b/drivers/char/keyboard.c
@@ -278,7 +278,7 @@ static void put_queue(struct vc_data *vc, int ch)
 
 	if (tty) {
 		tty_insert_flip_char(tty, ch, 0);
-		con_schedule_flip(tty);
+		tty_schedule_flip(tty);
 	}
 }
 
@@ -293,7 +293,7 @@ static void puts_queue(struct vc_data *vc, char *cp)
 		tty_insert_flip_char(tty, *cp, 0);
 		cp++;
 	}
-	con_schedule_flip(tty);
+	tty_schedule_flip(tty);
 }
 
 static void applkey(struct vc_data *vc, int key, char mode)
@@ -533,7 +533,7 @@ static void fn_send_intr(struct vc_data *vc)
 	if (!tty)
 		return;
 	tty_insert_flip_char(tty, 0, TTY_BREAK);
-	con_schedule_flip(tty);
+	tty_schedule_flip(tty);
 }
 
 static void fn_scroll_forw(struct vc_data *vc)
diff --git a/drivers/char/vt.c b/drivers/char/vt.c
index edb7002..d17cfcd 100644
--- a/drivers/char/vt.c
+++ b/drivers/char/vt.c
@@ -1260,7 +1260,7 @@ static void respond_string(const char *p, struct tty_struct *tty)
 		tty_insert_flip_char(tty, *p, 0);
 		p++;
 	}
-	con_schedule_flip(tty);
+	tty_schedule_flip(tty);
 }
 
 static void cursor_report(struct vc_data *vc, struct tty_struct *tty)
diff --git a/include/linux/kbd_kern.h b/include/linux/kbd_kern.h
index 506ad20..7b24a0d 100644
--- a/include/linux/kbd_kern.h
+++ b/include/linux/kbd_kern.h
@@ -149,16 +149,4 @@ void compute_shiftstate(void);
 
 extern unsigned int keymap_count;
 
-/* console.c */
-
-static inline void con_schedule_flip(struct tty_struct *t)
-{
-	unsigned long flags;
-	spin_lock_irqsave(&t->buf.lock, flags);
-	if (t->buf.tail != NULL)
-		t->buf.tail->commit = t->buf.tail->used;
-	spin_unlock_irqrestore(&t->buf.lock, flags);
-	schedule_delayed_work(&t->buf.work, 0);
-}
-
 #endif

Re: [PATCH] Use tty_schedule in VT code.

[Linus Torvalds]

On Wed, 18 Jul 2007, Paul Fulghum wrote:
> 
> It might be safest to drop this portion so you can get the
> obvious part of the patch accepted (consolidating
> the redundant xxx_schedule_flip functions).

But wasn't the whole _point_ that con_schedule_flip() potentially gets 
called from interrupt context, and thus that part is wrong. It's why 
con_schedule_flip() was different from tty_schedule_flip() to begin with, 
no?

If a tty driver has marked itself low-latency, it's still wrong to do the 
flush_to_ldisc() from interrupt context if a console event happens in 
interrupt context.

I thought that was the whole *point* of the difference between 
"tty_schedule_flip()" and "con_schedule_flip()", as far as I know. The 
"con_schedule_flip()" can be called from any context (console messages), 
while "tty_schedule_flip()" is only called from well-behaved tty layer.

But I really don't know. I used to be involved with the tty layer, these 
days I'd rather avoid it. This "simple" patch seems to be anything but, 
and I'd like somebody to just make sure that all the issues are taken care 
of.

Alan?

		Linus

Re: [PATCH] Use tty_schedule in VT code.

[Paul Fulghum]

James Simmons wrote:
> Done. I still smell a dead lock issue tho.

Yes, but it is an existing problem that was kicked
about with no real resolution.

No one can blame you for that! :-)

-- 
Paul Fulghum
Microgate Systems, Ltd.

Re: [PATCH] Use tty_schedule in VT code.

[Alan Cox]

> If a tty driver has marked itself low-latency, it's still wrong to do the 
> flush_to_ldisc() from interrupt context if a console event happens in 
> interrupt context.

It should be ok nowdays. Calling ld->receive() paths directly from an IRQ
is verboted however.

> I thought that was the whole *point* of the difference between 
> "tty_schedule_flip()" and "con_schedule_flip()", as far as I know. The 
> "con_schedule_flip()" can be called from any context (console messages), 
> while "tty_schedule_flip()" is only called from well-behaved tty layer.

I have no idea of the history of con_schedule_flip and I leave it alone 8)
> 
> But I really don't know. I used to be involved with the tty layer, these 
> days I'd rather avoid it. This "simple" patch seems to be anything but, 
> and I'd like somebody to just make sure that all the issues are taken care 
> of.
> 
> Alan?

For tty it is ok to call tty_flip_buffer_push from an IRQ. It is also ok
for this to do stuff based on the low latency flag. Any private internal
knowledge of that happens is at the drivers peril and may change.

We *need* low latency to do MIDI, Digitrax DCC and a few other things.

A driver which sets ->low_latency must allow its own write() method to be
called back as a result of calling tty_flip_buffer_push, and that
requires care on driver coding. (Arguably we should have a ->echo()
method for this but that opens other cans of worms)

Alan