[Qemu-devel] [PATCH] [RESEND] Make char muxer more robust wrt small FIFOs

[Qemu-devel] [PATCH] [RESEND] Make char muxer more robust wrt small FIFOs

[Alexander Graf]

Virtio-Console can only process one character at a time. Using it on S390
gave me strage "lags" where I got the character I pressed before when
pressing one. So I typed in "abc" and only received "a", then pressed "d"
but the guest received "b" and so on.

While the stdio driver calls a poll function that just processes on its
queue in case virtio-console can't take multiple characters at once, the
muxer does not have such callbacks, so it can't empty its queue.

To work around that limitation, I introduced a new timer that only gets
active when the guest can not receive any more characters. In that case
it polls again after a while to check if the guest is now receiving input.

This patch fixes input when using -nographic on s390 for me.

---

Please consider for stable.
---
 qemu-char.c |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/qemu-char.c b/qemu-char.c
index 05df971..ce9df3a 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -235,6 +235,7 @@ typedef struct {
     IOEventHandler *chr_event[MAX_MUX];
     void *ext_opaque[MAX_MUX];
     CharDriverState *drv;
+    QEMUTimer *accept_timer;
     int focus;
     int mux_cnt;
     int term_got_escape;
@@ -396,6 +397,13 @@ static void mux_chr_accept_input(CharDriverState *chr)
         d->chr_read[m](d->ext_opaque[m],
                        &d->buffer[m][d->cons[m]++ & MUX_BUFFER_MASK], 1);
     }
+
+    /* We're still not able to sync producer and consumer, so let's wait a bit
+       and try again by then. */
+    if (d->prod[m] != d->cons[m]) {
+        qemu_mod_timer(d->accept_timer, qemu_get_clock(vm_clock)
+                                        + (int64_t)100000);
+    }
 }
 
 static int mux_chr_can_read(void *opaque)
@@ -478,6 +486,8 @@ static CharDriverState *qemu_chr_open_mux(CharDriverState *drv)
     chr->opaque = d;
     d->drv = drv;
     d->focus = -1;
+    d->accept_timer = qemu_new_timer(vm_clock,
+                                     (QEMUTimerCB*)mux_chr_accept_input, chr);
     chr->chr_write = mux_chr_write;
     chr->chr_update_read_handler = mux_chr_update_read_handler;
     chr->chr_accept_input = mux_chr_accept_input;
-- 
1.6.0.2

Re: [Qemu-devel] [PATCH] [RESEND] Make char muxer more robust wrt small FIFOs

[Anthony Liguori]

On 04/20/2010 11:56 AM, Alexander Graf wrote:
> Virtio-Console can only process one character at a time. Using it on S390
> gave me strage "lags" where I got the character I pressed before when
> pressing one. So I typed in "abc" and only received "a", then pressed "d"
> but the guest received "b" and so on.
>
> While the stdio driver calls a poll function that just processes on its
> queue in case virtio-console can't take multiple characters at once, the
> muxer does not have such callbacks, so it can't empty its queue.
>
> To work around that limitation, I introduced a new timer that only gets
> active when the guest can not receive any more characters. In that case
> it polls again after a while to check if the guest is now receiving input.
>
> This patch fixes input when using -nographic on s390 for me.
>    

I think this is really a kvm issue.  I assume it's because s390 idles in 
the kernel so you never drop to userspace to repoll the descriptor.

A timer is a hacky solution.  You really need to use an io thread to 
solve this and then you need to switch away from qemu_set_fd_handler2 to 
qemu_set_fd_handler() and make sure that the later breaks select 
whenever it's invoked.

Regards,

Anthony Liguori

> ---
>
> Please consider for stable.
> ---
>   qemu-char.c |   10 ++++++++++
>   1 files changed, 10 insertions(+), 0 deletions(-)
>
> diff --git a/qemu-char.c b/qemu-char.c
> index 05df971..ce9df3a 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -235,6 +235,7 @@ typedef struct {
>       IOEventHandler *chr_event[MAX_MUX];
>       void *ext_opaque[MAX_MUX];
>       CharDriverState *drv;
> +    QEMUTimer *accept_timer;
>       int focus;
>       int mux_cnt;
>       int term_got_escape;
> @@ -396,6 +397,13 @@ static void mux_chr_accept_input(CharDriverState *chr)
>           d->chr_read[m](d->ext_opaque[m],
>                          &d->buffer[m][d->cons[m]++&  MUX_BUFFER_MASK], 1);
>       }
> +
> +    /* We're still not able to sync producer and consumer, so let's wait a bit
> +       and try again by then. */
> +    if (d->prod[m] != d->cons[m]) {
> +        qemu_mod_timer(d->accept_timer, qemu_get_clock(vm_clock)
> +                                        + (int64_t)100000);
> +    }
>   }
>
>   static int mux_chr_can_read(void *opaque)
> @@ -478,6 +486,8 @@ static CharDriverState *qemu_chr_open_mux(CharDriverState *drv)
>       chr->opaque = d;
>       d->drv = drv;
>       d->focus = -1;
> +    d->accept_timer = qemu_new_timer(vm_clock,
> +                                     (QEMUTimerCB*)mux_chr_accept_input, chr);
>       chr->chr_write = mux_chr_write;
>       chr->chr_update_read_handler = mux_chr_update_read_handler;
>       chr->chr_accept_input = mux_chr_accept_input;
>    

Re: [Qemu-devel] [PATCH] [RESEND] Make char muxer more robust wrt small FIFOs

[Alexander Graf]

Am 04.05.2010 um 15:44 schrieb Anthony Liguori <anthony@codemonkey.ws>:

> On 04/20/2010 11:56 AM, Alexander Graf wrote:
>> Virtio-Console can only process one character at a time. Using it  
>> on S390
>> gave me strage "lags" where I got the character I pressed before when
>> pressing one. So I typed in "abc" and only received "a", then  
>> pressed "d"
>> but the guest received "b" and so on.
>>
>> While the stdio driver calls a poll function that just processes on  
>> its
>> queue in case virtio-console can't take multiple characters at  
>> once, the
>> muxer does not have such callbacks, so it can't empty its queue.
>>
>> To work around that limitation, I introduced a new timer that only  
>> gets
>> active when the guest can not receive any more characters. In that  
>> case
>> it polls again after a while to check if the guest is now receiving  
>> input.
>>
>> This patch fixes input when using -nographic on s390 for me.
>>
>
> I think this is really a kvm issue.  I assume it's because s390  
> idles in the kernel so you never drop to userspace to repoll the  
> descriptor.

There is no polling for the muxer. That's why it never knows when  
virtio-console can receive again.

This patch basically adfs timer based polling for that exact case.


Alex

>
> A timer is a hacky solution.  You really need to use an io thread to  
> solve this and then you need to switch away from  
> qemu_set_fd_handler2 to qemu_set_fd_handler() and make sure that the  
> later breaks select whenever it's invoked.
>
> Regards,
>
> Anthony Liguori
>
>> ---
>>
>> Please consider for stable.
>> ---
>>  qemu-char.c |   10 ++++++++++
>>  1 files changed, 10 insertions(+), 0 deletions(-)
>>
>> diff --git a/qemu-char.c b/qemu-char.c
>> index 05df971..ce9df3a 100644
>> --- a/qemu-char.c
>> +++ b/qemu-char.c
>> @@ -235,6 +235,7 @@ typedef struct {
>>      IOEventHandler *chr_event[MAX_MUX];
>>      void *ext_opaque[MAX_MUX];
>>      CharDriverState *drv;
>> +    QEMUTimer *accept_timer;
>>      int focus;
>>      int mux_cnt;
>>      int term_got_escape;
>> @@ -396,6 +397,13 @@ static void mux_chr_accept_input 
>> (CharDriverState *chr)
>>          d->chr_read[m](d->ext_opaque[m],
>>                         &d->buffer[m][d->cons[m]++&   
>> MUX_BUFFER_MASK], 1);
>>      }
>> +
>> +    /* We're still not able to sync producer and consumer, so  
>> let's wait a bit
>> +       and try again by then. */
>> +    if (d->prod[m] != d->cons[m]) {
>> +        qemu_mod_timer(d->accept_timer, qemu_get_clock(vm_clock)
>> +                                        + (int64_t)100000);
>> +    }
>>  }
>>
>>  static int mux_chr_can_read(void *opaque)
>> @@ -478,6 +486,8 @@ static CharDriverState *qemu_chr_open_mux 
>> (CharDriverState *drv)
>>      chr->opaque = d;
>>      d->drv = drv;
>>      d->focus = -1;
>> +    d->accept_timer = qemu_new_timer(vm_clock,
>> +                                     (QEMUTimerCB*) 
>> mux_chr_accept_input, chr);
>>      chr->chr_write = mux_chr_write;
>>      chr->chr_update_read_handler = mux_chr_update_read_handler;
>>      chr->chr_accept_input = mux_chr_accept_input;
>>
>

Re: [Qemu-devel] [PATCH] [RESEND] Make char muxer more robust wrt small FIFOs

[Anthony Liguori]

On 05/04/2010 09:30 AM, Alexander Graf wrote:
>
>
> Am 04.05.2010 um 15:44 schrieb Anthony Liguori <anthony@codemonkey.ws>:
>
>> On 04/20/2010 11:56 AM, Alexander Graf wrote:
>>> Virtio-Console can only process one character at a time. Using it on 
>>> S390
>>> gave me strage "lags" where I got the character I pressed before when
>>> pressing one. So I typed in "abc" and only received "a", then 
>>> pressed "d"
>>> but the guest received "b" and so on.
>>>
>>> While the stdio driver calls a poll function that just processes on its
>>> queue in case virtio-console can't take multiple characters at once, 
>>> the
>>> muxer does not have such callbacks, so it can't empty its queue.
>>>
>>> To work around that limitation, I introduced a new timer that only gets
>>> active when the guest can not receive any more characters. In that case
>>> it polls again after a while to check if the guest is now receiving 
>>> input.
>>>
>>> This patch fixes input when using -nographic on s390 for me.
>>>
>>
>> I think this is really a kvm issue.  I assume it's because s390 idles 
>> in the kernel so you never drop to userspace to repoll the descriptor.
>
> There is no polling for the muxer. That's why it never knows when 
> virtio-console can receive again.

Maybe I'm missing something simple, but it looks to me like the muxer is 
polling.  mux_chr_can_read() is going to eventually poll the muxed 
devices to figure this out.

If the root of the problem is that mux_chr_can_read() isn't being 
invoked for a prolonged period of time, the real issue is the problem I 
described.

Regards,

Anthony Liguori

> This patch basically adfs timer based polling for that exact case.
>
>
> Alex
>
>>
>> A timer is a hacky solution.  You really need to use an io thread to 
>> solve this and then you need to switch away from qemu_set_fd_handler2 
>> to qemu_set_fd_handler() and make sure that the later breaks select 
>> whenever it's invoked.
>>
>> Regards,
>>
>> Anthony Liguori
>>
>>> ---
>>>
>>> Please consider for stable.
>>> ---
>>>  qemu-char.c |   10 ++++++++++
>>>  1 files changed, 10 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/qemu-char.c b/qemu-char.c
>>> index 05df971..ce9df3a 100644
>>> --- a/qemu-char.c
>>> +++ b/qemu-char.c
>>> @@ -235,6 +235,7 @@ typedef struct {
>>>      IOEventHandler *chr_event[MAX_MUX];
>>>      void *ext_opaque[MAX_MUX];
>>>      CharDriverState *drv;
>>> +    QEMUTimer *accept_timer;
>>>      int focus;
>>>      int mux_cnt;
>>>      int term_got_escape;
>>> @@ -396,6 +397,13 @@ static void 
>>> mux_chr_accept_input(CharDriverState *chr)
>>>          d->chr_read[m](d->ext_opaque[m],
>>> &d->buffer[m][d->cons[m]++&  MUX_BUFFER_MASK], 1);
>>>      }
>>> +
>>> +    /* We're still not able to sync producer and consumer, so let's 
>>> wait a bit
>>> +       and try again by then. */
>>> +    if (d->prod[m] != d->cons[m]) {
>>> +        qemu_mod_timer(d->accept_timer, qemu_get_clock(vm_clock)
>>> +                                        + (int64_t)100000);
>>> +    }
>>>  }
>>>
>>>  static int mux_chr_can_read(void *opaque)
>>> @@ -478,6 +486,8 @@ static CharDriverState 
>>> *qemu_chr_open_mux(CharDriverState *drv)
>>>      chr->opaque = d;
>>>      d->drv = drv;
>>>      d->focus = -1;
>>> +    d->accept_timer = qemu_new_timer(vm_clock,
>>> +                                     
>>> (QEMUTimerCB*)mux_chr_accept_input, chr);
>>>      chr->chr_write = mux_chr_write;
>>>      chr->chr_update_read_handler = mux_chr_update_read_handler;
>>>      chr->chr_accept_input = mux_chr_accept_input;
>>>
>>

Re: [Qemu-devel] [PATCH] [RESEND] Make char muxer more robust wrt small FIFOs

[Alexander Graf]

Am 04.05.2010 um 16:34 schrieb Anthony Liguori <anthony@codemonkey.ws>:

> On 05/04/2010 09:30 AM, Alexander Graf wrote:
>>
>>
>> Am 04.05.2010 um 15:44 schrieb Anthony Liguori  
>> <anthony@codemonkey.ws>:
>>
>>> On 04/20/2010 11:56 AM, Alexander Graf wrote:
>>>> Virtio-Console can only process one character at a time. Using it  
>>>> on S390
>>>> gave me strage "lags" where I got the character I pressed before  
>>>> when
>>>> pressing one. So I typed in "abc" and only received "a", then  
>>>> pressed "d"
>>>> but the guest received "b" and so on.
>>>>
>>>> While the stdio driver calls a poll function that just processes  
>>>> on its
>>>> queue in case virtio-console can't take multiple characters at  
>>>> once, the
>>>> muxer does not have such callbacks, so it can't empty its queue.
>>>>
>>>> To work around that limitation, I introduced a new timer that  
>>>> only gets
>>>> active when the guest can not receive any more characters. In  
>>>> that case
>>>> it polls again after a while to check if the guest is now  
>>>> receiving input.
>>>>
>>>> This patch fixes input when using -nographic on s390 for me.
>>>>
>>>
>>> I think this is really a kvm issue.  I assume it's because s390  
>>> idles in the kernel so you never drop to userspace to repoll the  
>>> descriptor.
>>
>> There is no polling for the muxer. That's why it never knows when  
>> virtio-console can receive again.
>
> Maybe I'm missing something simple, but it looks to me like the  
> muxer is polling.  mux_chr_can_read() is going to eventually poll  
> the muxed devices to figure this out.
>
> If the root of the problem is that mux_chr_can_read() isn't being  
> invoked for a prolonged period of time, the real issue is the  
> problem I described.

The problem is that the select list of fds includes the stdio fd, so  
that gets notified and is coupled with virtio-console, but there's  
nothing passing that on to mux and I don't think it'd be clever to  
expose internal data to the muxer to tell it about the backend fds.

Alex

>
> Regards,
>
> Anthony Liguori
>
>> This patch basically adfs timer based polling for that exact case.
>>
>>
>> Alex
>>
>>>
>>> A timer is a hacky solution.  You really need to use an io thread  
>>> to solve this and then you need to switch away from  
>>> qemu_set_fd_handler2 to qemu_set_fd_handler() and make sure that  
>>> the later breaks select whenever it's invoked.
>>>
>>> Regards,
>>>
>>> Anthony Liguori
>>>
>>>> ---
>>>>
>>>> Please consider for stable.
>>>> ---
>>>> qemu-char.c |   10 ++++++++++
>>>> 1 files changed, 10 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/qemu-char.c b/qemu-char.c
>>>> index 05df971..ce9df3a 100644
>>>> --- a/qemu-char.c
>>>> +++ b/qemu-char.c
>>>> @@ -235,6 +235,7 @@ typedef struct {
>>>>     IOEventHandler *chr_event[MAX_MUX];
>>>>     void *ext_opaque[MAX_MUX];
>>>>     CharDriverState *drv;
>>>> +    QEMUTimer *accept_timer;
>>>>     int focus;
>>>>     int mux_cnt;
>>>>     int term_got_escape;
>>>> @@ -396,6 +397,13 @@ static void mux_chr_accept_input 
>>>> (CharDriverState *chr)
>>>>         d->chr_read[m](d->ext_opaque[m],
>>>> &d->buffer[m][d->cons[m]++&  MUX_BUFFER_MASK], 1);
>>>>     }
>>>> +
>>>> +    /* We're still not able to sync producer and consumer, so  
>>>> let's wait a bit
>>>> +       and try again by then. */
>>>> +    if (d->prod[m] != d->cons[m]) {
>>>> +        qemu_mod_timer(d->accept_timer, qemu_get_clock(vm_clock)
>>>> +                                        + (int64_t)100000);
>>>> +    }
>>>> }
>>>>
>>>> static int mux_chr_can_read(void *opaque)
>>>> @@ -478,6 +486,8 @@ static CharDriverState *qemu_chr_open_mux 
>>>> (CharDriverState *drv)
>>>>     chr->opaque = d;
>>>>     d->drv = drv;
>>>>     d->focus = -1;
>>>> +    d->accept_timer = qemu_new_timer(vm_clock,
>>>> +                                     (QEMUTimerCB*) 
>>>> mux_chr_accept_input, chr);
>>>>     chr->chr_write = mux_chr_write;
>>>>     chr->chr_update_read_handler = mux_chr_update_read_handler;
>>>>     chr->chr_accept_input = mux_chr_accept_input;
>>>>
>>>
>

Re: [Qemu-devel] [PATCH] [RESEND] Make char muxer more robust wrt small FIFOs

[Anthony Liguori]

On 05/04/2010 11:01 AM, Alexander Graf wrote:
>
> Am 04.05.2010 um 16:34 schrieb Anthony Liguori <anthony@codemonkey.ws>:
>
>> On 05/04/2010 09:30 AM, Alexander Graf wrote:
>>>
>>>
>>> Am 04.05.2010 um 15:44 schrieb Anthony Liguori <anthony@codemonkey.ws>:
>>>
>>>> On 04/20/2010 11:56 AM, Alexander Graf wrote:
>>>>> Virtio-Console can only process one character at a time. Using it 
>>>>> on S390
>>>>> gave me strage "lags" where I got the character I pressed before when
>>>>> pressing one. So I typed in "abc" and only received "a", then 
>>>>> pressed "d"
>>>>> but the guest received "b" and so on.
>>>>>
>>>>> While the stdio driver calls a poll function that just processes 
>>>>> on its
>>>>> queue in case virtio-console can't take multiple characters at 
>>>>> once, the
>>>>> muxer does not have such callbacks, so it can't empty its queue.
>>>>>
>>>>> To work around that limitation, I introduced a new timer that only 
>>>>> gets
>>>>> active when the guest can not receive any more characters. In that 
>>>>> case
>>>>> it polls again after a while to check if the guest is now 
>>>>> receiving input.
>>>>>
>>>>> This patch fixes input when using -nographic on s390 for me.
>>>>>
>>>>
>>>> I think this is really a kvm issue.  I assume it's because s390 
>>>> idles in the kernel so you never drop to userspace to repoll the 
>>>> descriptor.
>>>
>>> There is no polling for the muxer. That's why it never knows when 
>>> virtio-console can receive again.
>>
>> Maybe I'm missing something simple, but it looks to me like the muxer 
>> is polling.  mux_chr_can_read() is going to eventually poll the muxed 
>> devices to figure this out.
>>
>> If the root of the problem is that mux_chr_can_read() isn't being 
>> invoked for a prolonged period of time, the real issue is the problem 
>> I described.
>
> The problem is that the select list of fds includes the stdio fd, so 
> that gets notified and is coupled with virtio-console, but there's 
> nothing passing that on to mux and I don't think it'd be clever to 
> expose internal data to the muxer to tell it about the backend fds.

When stdio is readable, it should invoke qemu_chr_read() with the read 
data which in turn ought to invoke mux_chr_read().

I'm not sure I understand what signalling is missing.  Jan, does the 
problem Alex describes ring a bell?  I seem to recall you saying that 
mux was still fundamentally broken but ought to work most of the time...

Regards,

Anthony Liguori

> Alex

Re: [Qemu-devel] [PATCH] [RESEND] Make char muxer more robust wrt small FIFOs

[Jan Kiszka]

Anthony Liguori wrote:
> On 05/04/2010 11:01 AM, Alexander Graf wrote:
>> Am 04.05.2010 um 16:34 schrieb Anthony Liguori <anthony@codemonkey.ws>:
>>
>>> On 05/04/2010 09:30 AM, Alexander Graf wrote:
>>>>
>>>> Am 04.05.2010 um 15:44 schrieb Anthony Liguori <anthony@codemonkey.ws>:
>>>>
>>>>> On 04/20/2010 11:56 AM, Alexander Graf wrote:
>>>>>> Virtio-Console can only process one character at a time. Using it 
>>>>>> on S390
>>>>>> gave me strage "lags" where I got the character I pressed before when
>>>>>> pressing one. So I typed in "abc" and only received "a", then 
>>>>>> pressed "d"
>>>>>> but the guest received "b" and so on.
>>>>>>
>>>>>> While the stdio driver calls a poll function that just processes 
>>>>>> on its
>>>>>> queue in case virtio-console can't take multiple characters at 
>>>>>> once, the
>>>>>> muxer does not have such callbacks, so it can't empty its queue.
>>>>>>
>>>>>> To work around that limitation, I introduced a new timer that only 
>>>>>> gets
>>>>>> active when the guest can not receive any more characters. In that 
>>>>>> case
>>>>>> it polls again after a while to check if the guest is now 
>>>>>> receiving input.
>>>>>>
>>>>>> This patch fixes input when using -nographic on s390 for me.
>>>>>>
>>>>> I think this is really a kvm issue.  I assume it's because s390 
>>>>> idles in the kernel so you never drop to userspace to repoll the 
>>>>> descriptor.
>>>> There is no polling for the muxer. That's why it never knows when 
>>>> virtio-console can receive again.
>>> Maybe I'm missing something simple, but it looks to me like the muxer 
>>> is polling.  mux_chr_can_read() is going to eventually poll the muxed 
>>> devices to figure this out.
>>>
>>> If the root of the problem is that mux_chr_can_read() isn't being 
>>> invoked for a prolonged period of time, the real issue is the problem 
>>> I described.
>> The problem is that the select list of fds includes the stdio fd, so 
>> that gets notified and is coupled with virtio-console, but there's 
>> nothing passing that on to mux and I don't think it'd be clever to 
>> expose internal data to the muxer to tell it about the backend fds.
> 
> When stdio is readable, it should invoke qemu_chr_read() with the read 
> data which in turn ought to invoke mux_chr_read().
> 
> I'm not sure I understand what signalling is missing.  Jan, does the 
> problem Alex describes ring a bell?  I seem to recall you saying that 
> mux was still fundamentally broken but ought to work most of the time...

That problem was (and still is) that the muxer needs to accept
characters even if the active front-end device is not in order to filter
out control sequences. Once its queue is full, it will start dropping
those the active device would not if directly connected. Could only be
solved via some peek service on pending front-end data.

I think Alex' problem can be addressed by registering
qemu_set_fd_handler2(..., backend->read_poll, mux_chr_read, ...). That
means the backend has to tell us about its read poll handler (if any).

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

[Qemu-devel] Re: [PATCH] [RESEND] Make char muxer more robust wrt small FIFOs

[Jan Kiszka]

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

Jan Kiszka wrote:
> Anthony Liguori wrote:
>> On 05/04/2010 11:01 AM, Alexander Graf wrote:
>>> Am 04.05.2010 um 16:34 schrieb Anthony Liguori <anthony@codemonkey.ws>:
>>>
>>>> On 05/04/2010 09:30 AM, Alexander Graf wrote:
>>>>> Am 04.05.2010 um 15:44 schrieb Anthony Liguori <anthony@codemonkey.ws>:
>>>>>
>>>>>> On 04/20/2010 11:56 AM, Alexander Graf wrote:
>>>>>>> Virtio-Console can only process one character at a time. Using it 
>>>>>>> on S390
>>>>>>> gave me strage "lags" where I got the character I pressed before when
>>>>>>> pressing one. So I typed in "abc" and only received "a", then 
>>>>>>> pressed "d"
>>>>>>> but the guest received "b" and so on.
>>>>>>>
>>>>>>> While the stdio driver calls a poll function that just processes 
>>>>>>> on its
>>>>>>> queue in case virtio-console can't take multiple characters at 
>>>>>>> once, the
>>>>>>> muxer does not have such callbacks, so it can't empty its queue.
>>>>>>>
>>>>>>> To work around that limitation, I introduced a new timer that only 
>>>>>>> gets
>>>>>>> active when the guest can not receive any more characters. In that 
>>>>>>> case
>>>>>>> it polls again after a while to check if the guest is now 
>>>>>>> receiving input.
>>>>>>>
>>>>>>> This patch fixes input when using -nographic on s390 for me.
>>>>>>>
>>>>>> I think this is really a kvm issue.  I assume it's because s390 
>>>>>> idles in the kernel so you never drop to userspace to repoll the 
>>>>>> descriptor.
>>>>> There is no polling for the muxer. That's why it never knows when 
>>>>> virtio-console can receive again.
>>>> Maybe I'm missing something simple, but it looks to me like the muxer 
>>>> is polling.  mux_chr_can_read() is going to eventually poll the muxed 
>>>> devices to figure this out.
>>>>
>>>> If the root of the problem is that mux_chr_can_read() isn't being 
>>>> invoked for a prolonged period of time, the real issue is the problem 
>>>> I described.
>>> The problem is that the select list of fds includes the stdio fd, so 
>>> that gets notified and is coupled with virtio-console, but there's 
>>> nothing passing that on to mux and I don't think it'd be clever to 
>>> expose internal data to the muxer to tell it about the backend fds.
>> When stdio is readable, it should invoke qemu_chr_read() with the read 
>> data which in turn ought to invoke mux_chr_read().
>>
>> I'm not sure I understand what signalling is missing.  Jan, does the 
>> problem Alex describes ring a bell?  I seem to recall you saying that 
>> mux was still fundamentally broken but ought to work most of the time...
> 
> That problem was (and still is) that the muxer needs to accept
> characters even if the active front-end device is not in order to filter
> out control sequences. Once its queue is full, it will start dropping
> those the active device would not if directly connected. Could only be
> solved via some peek service on pending front-end data.
> 
> I think Alex' problem can be addressed by registering
> qemu_set_fd_handler2(..., backend->read_poll, mux_chr_read, ...). That
> means the backend has to tell us about its read poll handler (if any).

Nonsense.

In fact, the problem is the former issue: As the muxer reads the
character the front-end is currently unable to receive, polling may stop
until as the back-end has some further chars to deliver.

But interestingly, the stdio back-end has a (single-byte) fifo as well.
It just drives it a bit differently.

Alex, does this help as well?

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] &&


I'm trying to reproduce in parallel.

Jan


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

[Qemu-devel] Re: [PATCH] [RESEND] Make char muxer more robust wrt small FIFOs

[Jan Kiszka]

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

Jan Kiszka wrote:
> Alex, does this help as well?
> 
> 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] &&
> 
> 
> I'm trying to reproduce in parallel.

Works for me. Will post as proper patch later.

Jan


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

[Qemu-devel] Re: [PATCH] [RESEND] Make char muxer more robust wrt small FIFOs

[Alexander Graf]

Am 05.05.2010 um 10:08 schrieb Jan Kiszka <jan.kiszka@web.de>:

> Jan Kiszka wrote:
>> Alex, does this help as well?
>>
>> 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] &&
>>
>>
>> I'm trying to reproduce in parallel.
>
> Works for me. Will post as proper patch later.

I'll try it out next week, after my vacation is over :)

Alex

>

[Qemu-devel] [PATCH] char: Flush read buffer in mux_chr_can_read

[Jan Kiszka]

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(-)

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] &&

[Qemu-devel] Re: [PATCH] char: Flush read buffer in mux_chr_can_read

[Alexander Graf]

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

[Qemu-devel] Re: [PATCH] char: Flush read buffer in mux_chr_can_read

[Jan Kiszka]

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

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

[Qemu-devel] Re: [PATCH] char: Flush read buffer in mux_chr_can_read

[Alexander Graf]

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

[Qemu-devel] Re: [PATCH] char: Flush read buffer in mux_chr_can_read

[Jan Kiszka]

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

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


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

[Qemu-devel] Re: [PATCH] char: Flush read buffer in mux_chr_can_read

[Alexander Graf]

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

[Qemu-devel] Re: [PATCH] char: Flush read buffer in mux_chr_can_read

[Jan Kiszka]

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

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

[Qemu-devel] Re: [PATCH] char: Flush read buffer in mux_chr_can_read

[Alexander Graf]

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

[Qemu-devel] Re: [PATCH] char: Flush read buffer in mux_chr_can_read

[Jan Kiszka]

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

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


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

[Qemu-devel] Re: [PATCH] char: Flush read buffer in mux_chr_can_read

[Alexander Graf]

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

[Qemu-devel] Re: [PATCH] char: Flush read buffer in mux_chr_can_read

[Jan Kiszka]

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

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


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