Re: [PATCH] Use tty_schedule in VT code.
> 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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Use tty_schedule in VT code.
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. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Use tty_schedule in VT code.
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Use tty_schedule in VT code.
> 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(>buf.lock, flags);
- if (t->buf.tail != NULL)
- t->buf.tail->commit = t->buf.tail->used;
- spin_unlock_irqrestore(>buf.lock, flags);
- schedule_delayed_work(>buf.work, 0);
-}
-
#endif
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Use tty_schedule in VT code.
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(>buf.lock, flags); > - > - if (tty->low_latency) > - flush_to_ldisc(>buf.work.work); > - else > - schedule_delayed_work(>buf.work, 1); > + schedule_delayed_work(>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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Use tty_schedule in VT code.
> > What should be done is
> >
> > if (tty->low_latency)
> > flush_to_ldisc(>buf.work.work);
> > else
> > schedule_delayed_work(>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(>buf.lock, flags);
-
- if (tty->low_latency)
- flush_to_ldisc(>buf.work.work);
- else
- schedule_delayed_work(>buf.work, 1);
+ schedule_delayed_work(>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(>buf.lock, flags);
- if (t->buf.tail != NULL)
- t->buf.tail->commit = t->buf.tail->used;
- spin_unlock_irqrestore(>buf.lock, flags);
- schedule_delayed_work(>buf.work, 0);
-}
-
#endif
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Use tty_schedule in VT code.
> 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. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Use tty_schedule in VT code.
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. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Use tty_schedule in VT code.
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
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Use tty_schedule in VT code.
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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Use tty_schedule in VT code.
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
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Use tty_schedule in VT code.
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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Use tty_schedule in VT code.
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. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Use tty_schedule in VT code.
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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Use tty_schedule in VT code.
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Use tty_schedule in VT code.
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(>buf.work.work); > else > schedule_delayed_work(>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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Use tty_schedule in VT code.
James Simmons wrote: James Simmons, le Tue 17 Jul 2007 19:37:57 +0100, a écrit : - schedule_delayed_work(>buf.work, 0); It was schedule_delayed_work(>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. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Use tty_schedule in VT code.
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. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Use tty_schedule in VT code.
Linus Torvalds wrote: - schedule_delayed_work(>buf.work, 1); + schedule_delayed_work(>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. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Use tty_schedule in VT code.
> 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(>buf.work.work); > > else > > schedule_delayed_work(>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? - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Use tty_schedule in VT code.
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(>buf.work.work); else schedule_delayed_work(>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. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Use tty_schedule in VT code.
> 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(>buf.work.work); else schedule_delayed_work(>buf.work, 1); Is this acceptable to you? - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Use tty_schedule in VT code.
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Use tty_schedule in VT code.
[ 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(>buf.lock, flags); > - schedule_delayed_work(>buf.work, 1); > + schedule_delayed_work(>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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Use tty_schedule in VT code.
> James Simmons, le Tue 17 Jul 2007 19:37:57 +0100, a écrit : > > - schedule_delayed_work(>buf.work, 0); > > It was schedule_delayed_work(>buf.work, 1); in con_schedule_flip() ; > could that matter? I did not detect any regressions.
Re: [PATCH] Use tty_schedule in VT code.
James Simmons, le Tue 17 Jul 2007 19:37:57 +0100, a écrit : > - schedule_delayed_work(>buf.work, 0); It was schedule_delayed_work(>buf.work, 1); in con_schedule_flip() ; could that matter? Samuel - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] Use tty_schedule in VT code.
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 <[EMAIL PROTECTED]>
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(>buf.lock, flags);
- schedule_delayed_work(>buf.work, 1);
+ schedule_delayed_work(>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(>buf.lock, flags);
- if (t->buf.tail != NULL)
- t->buf.tail->commit = t->buf.tail->used;
- spin_unlock_irqrestore(>buf.lock, flags);
- schedule_delayed_work(>buf.work, 0);
-}
-
#endif
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] Use tty_schedule in VT code.
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 [EMAIL PROTECTED]
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
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Use tty_schedule in VT code.
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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Use tty_schedule in VT code.
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.
[ 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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Use tty_schedule in VT code.
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? - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Use tty_schedule in VT code.
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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Use tty_schedule in VT code.
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. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Use tty_schedule in VT code.
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? - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Use tty_schedule in VT code.
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. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Use tty_schedule in VT code.
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. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Use tty_schedule in VT code.
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. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Use tty_schedule in VT code.
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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Use tty_schedule in VT code.
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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Use tty_schedule in VT code.
James Simmons wrote: Great Here is the patch updated with the delay value set to zero. Acked-by: Paul Fulghum <[EMAIL PROTECTED]> You should submit this to Andrew Morton <[EMAIL PROTECTED]> so it can get into the mm tree. -- Paul - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Use tty_schedule in VT code.
> 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 <[EMAIL PROTECTED]>
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(>buf.lock, flags);
- schedule_delayed_work(>buf.work, 1);
+ schedule_delayed_work(>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(>buf.lock, flags);
- if (t->buf.tail != NULL)
- t->buf.tail->commit = t->buf.tail->used;
- spin_unlock_irqrestore(>buf.lock, flags);
- schedule_delayed_work(>buf.work, 0);
-}
-
#endif
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Use tty_schedule in VT code.
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. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Use tty_schedule in VT code.
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. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Use tty_schedule in VT code.
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 [EMAIL PROTECTED]
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
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Use tty_schedule in VT code.
James Simmons wrote: Great Here is the patch updated with the delay value set to zero. Acked-by: Paul Fulghum [EMAIL PROTECTED] You should submit this to Andrew Morton [EMAIL PROTECTED] so it can get into the mm tree. -- Paul - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Use tty_schedule in VT code.
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] Use tty_schedule in VT code.
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 <[EMAIL PROTECTED]>
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(>buf.lock, flags);
- if (t->buf.tail != NULL)
- t->buf.tail->commit = t->buf.tail->used;
- spin_unlock_irqrestore(>buf.lock, flags);
- schedule_delayed_work(>buf.work, 0);
-}
-
#endif
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] Use tty_schedule in VT code.
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 [EMAIL PROTECTED]
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
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Use tty_schedule in VT code.
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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
