From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:59651) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UL3Qs-0006Vi-RW for qemu-devel@nongnu.org; Wed, 27 Mar 2013 23:23:22 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UL3Qo-0007TA-DR for qemu-devel@nongnu.org; Wed, 27 Mar 2013 23:23:18 -0400 Received: from e28smtp03.in.ibm.com ([122.248.162.3]:47720) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UL3Qn-0007Sk-Ns for qemu-devel@nongnu.org; Wed, 27 Mar 2013 23:23:14 -0400 Received: from /spool/local by e28smtp03.in.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Thu, 28 Mar 2013 08:49:43 +0530 Received: from d28relay01.in.ibm.com (d28relay01.in.ibm.com [9.184.220.58]) by d28dlp01.in.ibm.com (Postfix) with ESMTP id DFEFEE002D for ; Thu, 28 Mar 2013 08:54:42 +0530 (IST) Received: from d28av01.in.ibm.com (d28av01.in.ibm.com [9.184.220.63]) by d28relay01.in.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id r2S3N1C23473778 for ; Thu, 28 Mar 2013 08:53:01 +0530 Received: from d28av01.in.ibm.com (loopback [127.0.0.1]) by d28av01.in.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id r2S3N527023732 for ; Thu, 28 Mar 2013 03:23:05 GMT Message-ID: <5153B744.6070302@linux.vnet.ibm.com> Date: Thu, 28 Mar 2013 11:21:40 +0800 From: Wenchao Xia MIME-Version: 1.0 References: <1364240439-23450-1-git-send-email-lcapitulino@redhat.com> <1364240439-23450-3-git-send-email-lcapitulino@redhat.com> <515295A1.8030705@linux.vnet.ibm.com> <20130327082756.212b15f7@redhat.com> In-Reply-To: <20130327082756.212b15f7@redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH 2/2] Monitor: Make output buffer dynamic List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Luiz Capitulino Cc: kraxel@redhat.com, qemu-devel@nongnu.org, fred.konrad@greensocs.com 于 2013-3-27 20:27, Luiz Capitulino 写道: > On Wed, 27 Mar 2013 14:45:53 +0800 > Wenchao Xia 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 >>> --- >>> monitor.c | 42 +++++++++++++++++++++++++----------------- >>> 1 file changed, 25 insertions(+), 17 deletions(-) >>> >> > > -- Best Regards Wenchao Xia