Re: [Qemu-devel] [PATCH 2/2] Monitor: Make output buffer dynamic

[Wenchao Xia <xiawenc@linux.vnet.ibm.com>]

于 2013-3-27 20:27, Luiz Capitulino 写道:
> On Wed, 27 Mar 2013 14:45:53 +0800
> Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
>
>> Hi, Luiz
>>    Personally I hope reduce the dynamic allocated buffer which brings
>> fragments and unexpected memory grow. Instead, how about sacrifice
>> some time to wait output complete, since monitor is not time critical?
>> in this case static buffer's size can decide how many work can be
>> postponded. Following is my suggestion:
>
> I'd be fine with it if you find a good way to postpone work, but sleeping
> on random timers is not a good way.
>
> Also, QEMU allocates fragments of memory in so many places that I doubt
> this is an valid argument. You're right that long QMP output can cause
> unexpected memory growth, but I expected this to be really rare and if
> this really bother us, we can ask monitor_puts() to flush at, say, every
> 4096 bytes.
>
   If the point was async call, you are right it should not sleep. My
thoughts is reducing dynamic allocation when possible. Two alternative
are: make static buffer larger as 4096 bytes and drop the remain if
buffer is full, or dynamic increase the buffer size with a fixed
number when buffer overflow avoiding dynamic allocation every time.
It is a bit overkill, so I am also OK with your solution.

>>
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -293,17 +293,28 @@ static void monitor_puts(Monitor *mon, const char
>> *str)
>>   {
>>       char c;
>>
>> +    /* if mux do not put in any thing to buffer */
>> +    if (mon->mux_out) {
>> +        return;
>> +    }
>> +
>>       for(;;) {
>> -        assert(mon->outbuf_index < sizeof(mon->outbuf) - 1);
>> +        if (mon->outbuf_index >= sizeof(mon->outbuf) - 1) {
>> +            /* when buffer is full, flush it and retry. If buffer is
>> bigger, more
>> +               work can be postponed. */
>> +            monitor_flush(mon);
>> +            usleep(1);
>> +            continue;
>> +        }
>>           c = *str++;
>>           if (c == '\0')
>>               break;
>>           if (c == '\n')
>>               mon->outbuf[mon->outbuf_index++] = '\r';
>>           mon->outbuf[mon->outbuf_index++] = c;
>> -        if (mon->outbuf_index >= (sizeof(mon->outbuf) - 1)
>> -            || c == '\n')
>> +        if (c == '\n') {
>>               monitor_flush(mon);
>> +        }
>>       }
>>   }
>>
>>> Commit f628926bb423fa8a7e0b114511400ea9df38b76a changed monitor_flush()
>>> to retry on qemu_chr_fe_write() errors. However, the Monitor's output
>>> buffer can keep growing while the retry is not issued and this can
>>> cause the buffer to overflow.
>>>
>>> To reproduce this issue, just start qemu and type on the Monitor:
>>>
>>> (qemu) ?
>>>
>>> This will cause the assertion to trig.
>>>
>>> To fix this problem this commit makes the Monitor buffer dynamic,
>>> which means that it can grow as much as needed.
>>>
>>> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
>>> ---
>>>    monitor.c | 42 +++++++++++++++++++++++++-----------------
>>>    1 file changed, 25 insertions(+), 17 deletions(-)
>>>
>>
>
>


-- 
Best Regards

Wenchao Xia