Patchwork char: Flush read buffer in mux_chr_can_read

login
register
mail settings
Submitter Jan Kiszka
Date May 5, 2010, 3:27 p.m.
Message ID <4BE18E60.8070709@siemens.com>
Download mbox | patch
Permalink /patch/51739/
State New
Headers show

Comments

Jan Kiszka - May 5, 2010, 3:27 p.m.
Move the buffer flush from mux_chr_read to mux_chr_can_read. While the
latter is called periodically, the former will only be invoked when new
characters arrive at the back-end. This caused problems to front-end
drivers whenever they were unable to read data immediately, e.g.
virtio-console attached to stdio.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 qemu-char.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
Alexander Graf - May 11, 2010, 4:22 p.m.
Jan Kiszka wrote:
> Move the buffer flush from mux_chr_read to mux_chr_can_read. While the
> latter is called periodically, the former will only be invoked when new
> characters arrive at the back-end. This caused problems to front-end
> drivers whenever they were unable to read data immediately, e.g.
> virtio-console attached to stdio.
>   

I don't see this patch applied, but also don't see any issues with
virtio-console anymore on today's git. Odd.

Alex
Jan Kiszka - May 11, 2010, 4:29 p.m.
Alexander Graf wrote:
> Jan Kiszka wrote:
>> Move the buffer flush from mux_chr_read to mux_chr_can_read. While the
>> latter is called periodically, the former will only be invoked when new
>> characters arrive at the back-end. This caused problems to front-end
>> drivers whenever they were unable to read data immediately, e.g.
>> virtio-console attached to stdio.
>>   
> 
> I don't see this patch applied, but also don't see any issues with
> virtio-console anymore on today's git. Odd.
> 

Hmm, I had a clear before-after experience using virtio console with an
x86 Linux guest. I still think my patch is correct and required. Maybe
you can bisect this positive "regression"? Something might paper over
the core issue now.

Jan
Alexander Graf - May 11, 2010, 4:35 p.m.
Jan Kiszka wrote:
> Alexander Graf wrote:
>   
>> Jan Kiszka wrote:
>>     
>>> Move the buffer flush from mux_chr_read to mux_chr_can_read. While the
>>> latter is called periodically, the former will only be invoked when new
>>> characters arrive at the back-end. This caused problems to front-end
>>> drivers whenever they were unable to read data immediately, e.g.
>>> virtio-console attached to stdio.
>>>   
>>>       
>> I don't see this patch applied, but also don't see any issues with
>> virtio-console anymore on today's git. Odd.
>>
>>     
>
> Hmm, I had a clear before-after experience using virtio console with an
> x86 Linux guest. I still think my patch is correct and required. Maybe
> you can bisect this positive "regression"? Something might paper over
> the core issue now.
>   

I just did a git reset --hard on
baf0b55a9e57b909b1f8b0f732c0b10242867418 and it worked! What the ...

Alex
Jan Kiszka - May 12, 2010, 6:51 p.m.
Alexander Graf wrote:
> Jan Kiszka wrote:
>> Alexander Graf wrote:
>>   
>>> Jan Kiszka wrote:
>>>     
>>>> Move the buffer flush from mux_chr_read to mux_chr_can_read. While the
>>>> latter is called periodically, the former will only be invoked when new
>>>> characters arrive at the back-end. This caused problems to front-end
>>>> drivers whenever they were unable to read data immediately, e.g.
>>>> virtio-console attached to stdio.
>>>>   
>>>>       
>>> I don't see this patch applied, but also don't see any issues with
>>> virtio-console anymore on today's git. Odd.
>>>
>>>     
>> Hmm, I had a clear before-after experience using virtio console with an
>> x86 Linux guest. I still think my patch is correct and required. Maybe
>> you can bisect this positive "regression"? Something might paper over
>> the core issue now.
>>   
> 
> I just did a git reset --hard on
> baf0b55a9e57b909b1f8b0f732c0b10242867418 and it worked! What the ...

Whatever "worked" now means (I'm slightly confused), I just rechecked
the situation over current git head ("qemu linux-guest.img -chardev
stdio,id=cons,mux=on -device virtio-serial -device
virtconsole,chardev=cons -mon chardev=cons", "cat /dev/hvc0" in the
guest, typing some chars on the host console): The problem still
persists, my patch still solves it. Can you confirm this, ideally also
for s390?

Jan
Alexander Graf - May 14, 2010, 4 p.m.
On 12.05.2010, at 20:51, Jan Kiszka wrote:

> Alexander Graf wrote:
>> Jan Kiszka wrote:
>>> Alexander Graf wrote:
>>> 
>>>> Jan Kiszka wrote:
>>>> 
>>>>> Move the buffer flush from mux_chr_read to mux_chr_can_read. While the
>>>>> latter is called periodically, the former will only be invoked when new
>>>>> characters arrive at the back-end. This caused problems to front-end
>>>>> drivers whenever they were unable to read data immediately, e.g.
>>>>> virtio-console attached to stdio.
>>>>> 
>>>>> 
>>>> I don't see this patch applied, but also don't see any issues with
>>>> virtio-console anymore on today's git. Odd.
>>>> 
>>>> 
>>> Hmm, I had a clear before-after experience using virtio console with an
>>> x86 Linux guest. I still think my patch is correct and required. Maybe
>>> you can bisect this positive "regression"? Something might paper over
>>> the core issue now.
>>> 
>> 
>> I just did a git reset --hard on
>> baf0b55a9e57b909b1f8b0f732c0b10242867418 and it worked! What the ...
> 
> Whatever "worked" now means (I'm slightly confused), I just rechecked
> the situation over current git head ("qemu linux-guest.img -chardev
> stdio,id=cons,mux=on -device virtio-serial -device
> virtconsole,chardev=cons -mon chardev=cons", "cat /dev/hvc0" in the
> guest, typing some chars on the host console): The problem still
> persists, my patch still solves it. Can you confirm this, ideally also
> for s390?

Now that I can finally reproduce the bug with --enable-io-thread, I can verify that it does *not* fix the issue.

Alex
Jan Kiszka - May 14, 2010, 4:17 p.m.
Alexander Graf wrote:
> On 12.05.2010, at 20:51, Jan Kiszka wrote:
> 
>> Alexander Graf wrote:
>>> Jan Kiszka wrote:
>>>> Alexander Graf wrote:
>>>>
>>>>> Jan Kiszka wrote:
>>>>>
>>>>>> Move the buffer flush from mux_chr_read to mux_chr_can_read. While the
>>>>>> latter is called periodically, the former will only be invoked when new
>>>>>> characters arrive at the back-end. This caused problems to front-end
>>>>>> drivers whenever they were unable to read data immediately, e.g.
>>>>>> virtio-console attached to stdio.
>>>>>>
>>>>>>
>>>>> I don't see this patch applied, but also don't see any issues with
>>>>> virtio-console anymore on today's git. Odd.
>>>>>
>>>>>
>>>> Hmm, I had a clear before-after experience using virtio console with an
>>>> x86 Linux guest. I still think my patch is correct and required. Maybe
>>>> you can bisect this positive "regression"? Something might paper over
>>>> the core issue now.
>>>>
>>> I just did a git reset --hard on
>>> baf0b55a9e57b909b1f8b0f732c0b10242867418 and it worked! What the ...
>> Whatever "worked" now means (I'm slightly confused), I just rechecked
>> the situation over current git head ("qemu linux-guest.img -chardev
>> stdio,id=cons,mux=on -device virtio-serial -device
>> virtconsole,chardev=cons -mon chardev=cons", "cat /dev/hvc0" in the
>> guest, typing some chars on the host console): The problem still
>> persists, my patch still solves it. Can you confirm this, ideally also
>> for s390?
> 
> Now that I can finally reproduce the bug with --enable-io-thread, I can verify that it does *not* fix the issue.

I do not trust your tests. :p

I just tried to reproduce with --enable-io-thread and the setup I
described above - all still fine here. Can you reproduce with an x86 guest?

Jan
Alexander Graf - May 15, 2010, 5:31 a.m.
On 14.05.2010, at 18:17, Jan Kiszka wrote:

> Alexander Graf wrote:
>> On 12.05.2010, at 20:51, Jan Kiszka wrote:
>> 
>>> Alexander Graf wrote:
>>>> Jan Kiszka wrote:
>>>>> Alexander Graf wrote:
>>>>> 
>>>>>> Jan Kiszka wrote:
>>>>>> 
>>>>>>> Move the buffer flush from mux_chr_read to mux_chr_can_read. While the
>>>>>>> latter is called periodically, the former will only be invoked when new
>>>>>>> characters arrive at the back-end. This caused problems to front-end
>>>>>>> drivers whenever they were unable to read data immediately, e.g.
>>>>>>> virtio-console attached to stdio.
>>>>>>> 
>>>>>>> 
>>>>>> I don't see this patch applied, but also don't see any issues with
>>>>>> virtio-console anymore on today's git. Odd.
>>>>>> 
>>>>>> 
>>>>> Hmm, I had a clear before-after experience using virtio console with an
>>>>> x86 Linux guest. I still think my patch is correct and required. Maybe
>>>>> you can bisect this positive "regression"? Something might paper over
>>>>> the core issue now.
>>>>> 
>>>> I just did a git reset --hard on
>>>> baf0b55a9e57b909b1f8b0f732c0b10242867418 and it worked! What the ...
>>> Whatever "worked" now means (I'm slightly confused), I just rechecked
>>> the situation over current git head ("qemu linux-guest.img -chardev
>>> stdio,id=cons,mux=on -device virtio-serial -device
>>> virtconsole,chardev=cons -mon chardev=cons", "cat /dev/hvc0" in the
>>> guest, typing some chars on the host console): The problem still
>>> persists, my patch still solves it. Can you confirm this, ideally also
>>> for s390?
>> 
>> Now that I can finally reproduce the bug with --enable-io-thread, I can verify that it does *not* fix the issue.
> 
> I do not trust your tests. :p
> 
> I just tried to reproduce with --enable-io-thread and the setup I
> described above - all still fine here. Can you reproduce with an x86 guest?

Nope, it works fine with an x86 guest. I put the following in /etc/inittab of my guest:

11:2345:respawn:/sbin/mingetty --noclear /dev/hvc0 linux

One major thing to keep in mind when talking about s390 guests is that we almost never trap into userspace.


Alex
Jan Kiszka - May 15, 2010, 8:36 a.m.
Alexander Graf wrote:
> On 14.05.2010, at 18:17, Jan Kiszka wrote:
>> Alexander Graf wrote:
>>> Now that I can finally reproduce the bug with --enable-io-thread, I can verify that it does *not* fix the issue.
>> I do not trust your tests. :p
>>
>> I just tried to reproduce with --enable-io-thread and the setup I
>> described above - all still fine here. Can you reproduce with an x86 guest?
> 
> Nope, it works fine with an x86 guest. I put the following in /etc/inittab of my guest:
> 
> 11:2345:respawn:/sbin/mingetty --noclear /dev/hvc0 linux
> 
> One major thing to keep in mind when talking about s390 guests is that we almost never trap into userspace.

Do you have the same problem when using an unbuffered backend, e.g. pty?
Maybe that single-byte "fifo" of stdio makes the difference, though I
wonder why only on s390 (guest exists should not block the polling done
by the io-thread, just accelerate it).

Jan
Alexander Graf - May 15, 2010, 8:37 a.m.
On 15.05.2010, at 10:36, Jan Kiszka wrote:

> Alexander Graf wrote:
>> On 14.05.2010, at 18:17, Jan Kiszka wrote:
>>> Alexander Graf wrote:
>>>> Now that I can finally reproduce the bug with --enable-io-thread, I can verify that it does *not* fix the issue.
>>> I do not trust your tests. :p
>>> 
>>> I just tried to reproduce with --enable-io-thread and the setup I
>>> described above - all still fine here. Can you reproduce with an x86 guest?
>> 
>> Nope, it works fine with an x86 guest. I put the following in /etc/inittab of my guest:
>> 
>> 11:2345:respawn:/sbin/mingetty --noclear /dev/hvc0 linux
>> 
>> One major thing to keep in mind when talking about s390 guests is that we almost never trap into userspace.
> 
> Do you have the same problem when using an unbuffered backend, e.g. pty?
> Maybe that single-byte "fifo" of stdio makes the difference, though I
> wonder why only on s390 (guest exists should not block the polling done
> by the io-thread, just accelerate it).

Mind to give me a small example on how to use that? :)

Alex
Jan Kiszka - May 15, 2010, 8:54 a.m.
Alexander Graf wrote:
> On 15.05.2010, at 10:36, Jan Kiszka wrote:
> 
>> Alexander Graf wrote:
>>> On 14.05.2010, at 18:17, Jan Kiszka wrote:
>>>> Alexander Graf wrote:
>>>>> Now that I can finally reproduce the bug with --enable-io-thread, I can verify that it does *not* fix the issue.
>>>> I do not trust your tests. :p
>>>>
>>>> I just tried to reproduce with --enable-io-thread and the setup I
>>>> described above - all still fine here. Can you reproduce with an x86 guest?
>>> Nope, it works fine with an x86 guest. I put the following in /etc/inittab of my guest:
>>>
>>> 11:2345:respawn:/sbin/mingetty --noclear /dev/hvc0 linux
>>>
>>> One major thing to keep in mind when talking about s390 guests is that we almost never trap into userspace.
>> Do you have the same problem when using an unbuffered backend, e.g. pty?
>> Maybe that single-byte "fifo" of stdio makes the difference, though I
>> wonder why only on s390 (guest exists should not block the polling done
>> by the io-thread, just accelerate it).
> 
> Mind to give me a small example on how to use that? :)

-chardev pty,...

echo whatever > /dev/pts/<n>

Or you set up a telnet server.

Jan

Patch

diff --git a/qemu-char.c b/qemu-char.c
index ac65a1c..2b115a4 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -404,6 +404,8 @@  static int mux_chr_can_read(void *opaque)
     MuxDriver *d = chr->opaque;
     int m = d->focus;
 
+    mux_chr_accept_input(opaque);
+
     if ((d->prod[m] - d->cons[m]) < MUX_BUFFER_SIZE)
         return 1;
     if (d->chr_can_read[m])
@@ -418,8 +420,6 @@  static void mux_chr_read(void *opaque, const uint8_t *buf, int size)
     int m = d->focus;
     int i;
 
-    mux_chr_accept_input (opaque);
-
     for(i = 0; i < size; i++)
         if (mux_proc_byte(chr, d, buf[i])) {
             if (d->prod[m] == d->cons[m] &&