[Qemu-devel] [PATCH] 8250: more realistic TX-done IRQ rate

[Qemu-devel] [PATCH] 8250: more realistic TX-done IRQ rate

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 3648 bytes --]

The 8250 UART emulation currently raises a TX-done IRQ immediately when the guest writes out some character. This is problematic for guests like Linux which may flush its output buffer in a loop from IRQ context, because they may then enter a tight loop with IRQs disabled. In fact, Linux breaks out of this loop after some iterations and issue the well-known [1] "too much work for irq..." warning. And in case the console output is on the very same serial port, the console output is utterly corrupted.

Patch below addresses the issue by delaying the TX-done IRQ more realistically, ie. according to the currently set baudrate.

Jan

[1] http://lkml.org/lkml/2008/1/12/135

Signed-off-by: Jan Kiszka <jan.kiszka@web.de>

---
 hw/serial.c |   39 ++++++++++++++++++++++++++++-----------
 1 file changed, 28 insertions(+), 11 deletions(-)

Index: b/hw/serial.c
===================================================================
--- a/hw/serial.c
+++ b/hw/serial.c
@@ -25,6 +25,7 @@
 #include "qemu-char.h"
 #include "isa.h"
 #include "pc.h"
+#include "qemu-timer.h"
 
 //#define DEBUG_SERIAL
 
@@ -91,6 +92,8 @@ struct SerialState {
     int last_break_enable;
     target_phys_addr_t base;
     int it_shift;
+    QEMUTimer *tx_timer;
+    char tx_buf;
 };
 
 static void serial_receive_byte(SerialState *s, int ch);
@@ -111,6 +114,20 @@ static void serial_update_irq(SerialStat
     }
 }
 
+static void serial_tx_done(void *opaque)
+{
+    SerialState *s = opaque;
+
+    s->thr_ipending = 1;
+    s->lsr |= UART_LSR_THRE;
+    s->lsr |= UART_LSR_TEMT;
+    serial_update_irq(s);
+    if (s->mcr & UART_MCR_LOOP) {
+        /* in loopback mode, say that we just received a char */
+        serial_receive_byte(s, s->tx_buf);
+    }
+}
+
 static void serial_update_parameters(SerialState *s)
 {
     int speed, parity, data_bits, stop_bits;
@@ -146,7 +163,6 @@ static void serial_update_parameters(Ser
 static void serial_ioport_write(void *opaque, uint32_t addr, uint32_t val)
 {
     SerialState *s = opaque;
-    unsigned char ch;
 
     addr &= 7;
 #ifdef DEBUG_SERIAL
@@ -162,19 +178,12 @@ static void serial_ioport_write(void *op
             s->thr_ipending = 0;
             s->lsr &= ~UART_LSR_THRE;
             serial_update_irq(s);
-            ch = val;
+            s->tx_buf = val;
             if (!(s->mcr & UART_MCR_LOOP)) {
                 /* when not in loopback mode, send the char */
-                qemu_chr_write(s->chr, &ch, 1);
-            }
-            s->thr_ipending = 1;
-            s->lsr |= UART_LSR_THRE;
-            s->lsr |= UART_LSR_TEMT;
-            serial_update_irq(s);
-            if (s->mcr & UART_MCR_LOOP) {
-                /* in loopback mode, say that we just received a char */
-                serial_receive_byte(s, ch);
+                qemu_chr_write(s->chr, &s->tx_buf, 1);
             }
+            qemu_mod_timer(s->tx_timer, 1000 / (11520 / s->divider));
         }
         break;
     case 1:
@@ -387,6 +396,10 @@ SerialState *serial_init(int base, qemu_
         return NULL;
     s->irq = irq;
 
+    s->tx_timer = qemu_new_timer(vm_clock, serial_tx_done, s);
+    if (!s->tx_timer)
+        return NULL;
+
     qemu_register_reset(serial_reset, s);
     serial_reset(s);
 
@@ -486,6 +499,10 @@ SerialState *serial_mm_init (target_phys
     s->base = base;
     s->it_shift = it_shift;
 
+    s->tx_timer = qemu_new_timer(vm_clock, serial_tx_done, s);
+    if (!s->tx_timer)
+        return NULL;
+
     qemu_register_reset(serial_reset, s);
     serial_reset(s);
 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 254 bytes --]

[Qemu-devel] Re: [PATCH] 8250: more realistic TX-done IRQ rate

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 954 bytes --]

Jan Kiszka wrote:
> The 8250 UART emulation currently raises a TX-done IRQ immediately when 
> the guest writes out some character. This is problematic for guests like 
> Linux which may flush its output buffer in a loop from IRQ context, 
> because they may then enter a tight loop with IRQs disabled. In fact, 
> Linux breaks out of this loop after some iterations and issue the 
> well-known [1] "too much work for irq..." warning. And in case the 
> console output is on the very same serial port, the console output is 
> utterly corrupted.
> 
> Patch below addresses the issue by delaying the TX-done IRQ more 
> realistically, ie. according to the currently set baudrate.
> 
> Jan
> 
> [1] http://lkml.org/lkml/2008/1/12/135
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@web.de>
> 

Sorry, my TB obviously "gained" some regression, now wrecking inline 
patches even in do-not-wrap mode. That used to work for ages. Dreck.

Clean patch attached.

Jan

[-- Attachment #2: throttle-8250-tx.patch --]
[-- Type: text/x-patch, Size: 2813 bytes --]

---
 hw/serial.c |   39 ++++++++++++++++++++++++++++-----------
 1 file changed, 28 insertions(+), 11 deletions(-)

Index: b/hw/serial.c
===================================================================
--- a/hw/serial.c
+++ b/hw/serial.c
@@ -25,6 +25,7 @@
 #include "qemu-char.h"
 #include "isa.h"
 #include "pc.h"
+#include "qemu-timer.h"
 
 //#define DEBUG_SERIAL
 
@@ -91,6 +92,8 @@ struct SerialState {
     int last_break_enable;
     target_phys_addr_t base;
     int it_shift;
+    QEMUTimer *tx_timer;
+    char tx_buf;
 };
 
 static void serial_receive_byte(SerialState *s, int ch);
@@ -111,6 +114,20 @@ static void serial_update_irq(SerialStat
     }
 }
 
+static void serial_tx_done(void *opaque)
+{
+    SerialState *s = opaque;
+
+    s->thr_ipending = 1;
+    s->lsr |= UART_LSR_THRE;
+    s->lsr |= UART_LSR_TEMT;
+    serial_update_irq(s);
+    if (s->mcr & UART_MCR_LOOP) {
+        /* in loopback mode, say that we just received a char */
+        serial_receive_byte(s, s->tx_buf);
+    }
+}
+
 static void serial_update_parameters(SerialState *s)
 {
     int speed, parity, data_bits, stop_bits;
@@ -146,7 +163,6 @@ static void serial_update_parameters(Ser
 static void serial_ioport_write(void *opaque, uint32_t addr, uint32_t val)
 {
     SerialState *s = opaque;
-    unsigned char ch;
 
     addr &= 7;
 #ifdef DEBUG_SERIAL
@@ -162,19 +178,12 @@ static void serial_ioport_write(void *op
             s->thr_ipending = 0;
             s->lsr &= ~UART_LSR_THRE;
             serial_update_irq(s);
-            ch = val;
+            s->tx_buf = val;
             if (!(s->mcr & UART_MCR_LOOP)) {
                 /* when not in loopback mode, send the char */
-                qemu_chr_write(s->chr, &ch, 1);
-            }
-            s->thr_ipending = 1;
-            s->lsr |= UART_LSR_THRE;
-            s->lsr |= UART_LSR_TEMT;
-            serial_update_irq(s);
-            if (s->mcr & UART_MCR_LOOP) {
-                /* in loopback mode, say that we just received a char */
-                serial_receive_byte(s, ch);
+                qemu_chr_write(s->chr, &s->tx_buf, 1);
             }
+            qemu_mod_timer(s->tx_timer, 1000 / (11520 / s->divider));
         }
         break;
     case 1:
@@ -387,6 +396,10 @@ SerialState *serial_init(int base, qemu_
         return NULL;
     s->irq = irq;
 
+    s->tx_timer = qemu_new_timer(vm_clock, serial_tx_done, s);
+    if (!s->tx_timer)
+        return NULL;
+
     qemu_register_reset(serial_reset, s);
     serial_reset(s);
 
@@ -486,6 +499,10 @@ SerialState *serial_mm_init (target_phys
     s->base = base;
     s->it_shift = it_shift;
 
+    s->tx_timer = qemu_new_timer(vm_clock, serial_tx_done, s);
+    if (!s->tx_timer)
+        return NULL;
+
     qemu_register_reset(serial_reset, s);
     serial_reset(s);
 

Re: [Qemu-devel] [PATCH] 8250: more realistic TX-done IRQ rate

[Paul Brook]

On Saturday 12 April 2008, Jan Kiszka wrote:
> The 8250 UART emulation currently raises a TX-done IRQ immediately when the
> guest writes out some character. This is problematic for guests like Linux
> which may flush its output buffer in a loop from IRQ context, because they
> may then enter a tight loop with IRQs disabled. In fact, Linux breaks out
> of this loop after some iterations and issue the well-known [1] "too much
> work for irq..." warning. And in case the console output is on the very
> same serial port, the console output is utterly corrupted.

Please see previous threads on this topic.

> Patch below addresses the issue by delaying the TX-done IRQ more
> realistically, ie. according to the currently set baudrate.

Unless the serial baud rate is extremely low (<1kbaud) this isn't going to 
work with any sort of reliability. You have to fix this in a way that doesn't 
require high resolution realtime response.

I'd also expect you to have the same problem with the RX queue. If you don't 
it's a bug elsewhere in qemu.

> +            qemu_mod_timer(s->tx_timer, 1000 / (11520 / s->divider));

This looks bogus. I think you're a few orders of magnitude out in your timing 
calculations. As mentioned above, in practice you unlikely to get anywhere 
near the necessary realtime performance out of qemu.

Paul

Re: [Qemu-devel] [PATCH] 8250: more realistic TX-done IRQ rate

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 2266 bytes --]

Paul Brook wrote:
> On Saturday 12 April 2008, Jan Kiszka wrote:
>> The 8250 UART emulation currently raises a TX-done IRQ immediately when the
>> guest writes out some character. This is problematic for guests like Linux
>> which may flush its output buffer in a loop from IRQ context, because they
>> may then enter a tight loop with IRQs disabled. In fact, Linux breaks out
>> of this loop after some iterations and issue the well-known [1] "too much
>> work for irq..." warning. And in case the console output is on the very
>> same serial port, the console output is utterly corrupted.
> 
> Please see previous threads on this topic.

Some keyword or reference at hand? I did try hpa's patch which was also 
posted here, but his fix didn't work in my case (some ARM board 
emulation on recent x86-64 Linux host).

> 
>> Patch below addresses the issue by delaying the TX-done IRQ more
>> realistically, ie. according to the currently set baudrate.
> 
> Unless the serial baud rate is extremely low (<1kbaud) this isn't going to 
> work with any sort of reliability.  You have to fix this in a way that doesn't
> require high resolution realtime response.

OK, having no highres clock underneath may cause much heavier delays 
with this patch than desired or acceptable. I guess I have to think 
about some accumulative approach that throttles every n characters for a 
few miliseconds. Even if that would mean delaying, e.g., 10 ms due to a 
slow host with tick-based timing (thus about 100 characters @115k), that 
should still be fine to address the original issue.

> I'd also expect you to have the same problem with the RX queue. If you don't 
> it's a bug elsewhere in qemu.
> 
>> +            qemu_mod_timer(s->tx_timer, 1000 / (11520 / s->divider));
> 
> This looks bogus. I think you're a few orders of magnitude out in your timing 
> calculations.  As mentioned above, in practice you unlikely to get anywhere
> near the necessary realtime performance out of qemu.

That depends. Current Linux distros typically have 
CONFIG_HIGH_RES_TIMERS enabled. But, of course, not many OSes QEMU works 
on have such support at all, and even Linux may decided to work without 
it for various reasons.

Thanks,
Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 254 bytes --]

Re: [Qemu-devel] [PATCH] 8250: more realistic TX-done IRQ rate

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1053 bytes --]

Jan Kiszka wrote:
> Paul Brook wrote:
>> On Saturday 12 April 2008, Jan Kiszka wrote:
>> I'd also expect you to have the same problem with the RX queue. If you
>> don't it's a bug elsewhere in qemu.
>>
>>> +            qemu_mod_timer(s->tx_timer, 1000 / (11520 / s->divider));
>>
>> This looks bogus. I think you're a few orders of magnitude out in your
>> timing calculations.  As mentioned above, in practice you unlikely to
>> get anywhere
>> near the necessary realtime performance out of qemu.
> 
> That depends. Current Linux distros typically have
> CONFIG_HIGH_RES_TIMERS enabled. But, of course, not many OSes QEMU works
> on have such support at all, and even Linux may decided to work without
> it for various reasons.

OK, that line was totally bogus. Somehow I once assumed that
qemu_mod_timer takes relative timeout (though I've used it with absolute
dates before). A new version will follow soon, which in fact triggered a
second patch for another issue of the current emulation - the hard-coded
baudbase.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 254 bytes --]