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

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?



I'm trying to reproduce in parallel.

Jan

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

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

>