From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:43160) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SyL0X-0000B7-GM for qemu-devel@nongnu.org; Mon, 06 Aug 2012 06:57:58 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SyL0W-0001U8-5C for qemu-devel@nongnu.org; Mon, 06 Aug 2012 06:57:57 -0400 Received: from e23smtp01.au.ibm.com ([202.81.31.143]:35668) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SyL0V-0001U2-Ak for qemu-devel@nongnu.org; Mon, 06 Aug 2012 06:57:56 -0400 Received: from /spool/local by e23smtp01.au.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Mon, 6 Aug 2012 20:57:22 +1000 Received: from d23av03.au.ibm.com (d23av03.au.ibm.com [9.190.234.97]) by d23relay04.au.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id q76AnJbw8847582 for ; Mon, 6 Aug 2012 20:49:19 +1000 Received: from d23av03.au.ibm.com (loopback [127.0.0.1]) by d23av03.au.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id q76AvgnR008840 for ; Mon, 6 Aug 2012 20:57:42 +1000 Message-ID: <501FA321.8050902@linux.vnet.ibm.com> Date: Mon, 06 Aug 2012 18:57:37 +0800 From: Lei Li MIME-Version: 1.0 References: <1343814538-27591-1-git-send-email-lilei@linux.vnet.ibm.com> <1343814538-27591-2-git-send-email-lilei@linux.vnet.ibm.com> <87obmuti50.fsf@codemonkey.ws> In-Reply-To: <87obmuti50.fsf@codemonkey.ws> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [RFC PATCH 1/4] qemu-char: Convert MemCharDriver to circular buffer List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Anthony Liguori , qemu-devel@nongnu.org Cc: stefanha@linux.vnet.ibm.com, Lei Li On 08/02/2012 05:30 AM, Anthony Liguori wrote: > Lei Li writes: > >> Signed-off-by: Lei Li >> --- >> qemu-char.c | 96 +++++++++++++++++++++++++++++++++++++++++++++++----------- >> qemu-char.h | 2 +- >> 2 files changed, 78 insertions(+), 20 deletions(-) >> >> diff --git a/qemu-char.c b/qemu-char.c >> index c2aaaee..087c92d 100644 >> --- a/qemu-char.c >> +++ b/qemu-char.c >> @@ -2517,38 +2517,96 @@ static CharDriverState *qemu_chr_open_socket(QemuOpts *opts) >> /***********************************************************/ >> /* Memory chardev */ >> typedef struct { >> - size_t outbuf_size; >> - size_t outbuf_capacity; >> - uint8_t *outbuf; >> + size_t cbuf_capacity; >> + size_t cbuf_in; >> + size_t cbuf_out; >> + size_t cbuf_count; >> + uint8_t *cbuf; >> } MemoryDriver; > Probably should move the buffer into a separate structure and then you > can drop the cbuf_ prefixes. > >> +static int mem_chr_is_empty(CharDriverState *chr) >> +{ >> + MemoryDriver *d = chr->opaque; >> + >> + return d->cbuf_count == 0; >> +} >> + >> +static int mem_chr_is_full(CharDriverState *chr) >> +{ >> + MemoryDriver *d = chr->opaque; >> + >> + return d->cbuf_count == d->cbuf_capacity; >> +} >> + > Typically, you would use a producer and a consumer index. To test for > empty, you would check if (consumer == producer). > > To check for full, you would check if ((producer - consumer) == size). > > To get the actual index, you always modulus the indexes with size. This > only works if size is a power of 2 but that's a reasonable restriction. > >> static int mem_chr_write(CharDriverState *chr, const uint8_t *buf, int len) >> { >> MemoryDriver *d = chr->opaque; >> + int left; >> >> - /* TODO: the QString implementation has the same code, we should >> - * introduce a generic way to do this in cutils.c */ >> - if (d->outbuf_capacity < d->outbuf_size + len) { >> - /* grow outbuf */ >> - d->outbuf_capacity += len; >> - d->outbuf_capacity *= 2; >> - d->outbuf = g_realloc(d->outbuf, d->outbuf_capacity); >> + if (d->cbuf_capacity < len) { >> + return -1; >> } >> >> - memcpy(d->outbuf + d->outbuf_size, buf, len); >> - d->outbuf_size += len; >> + left = d->cbuf_capacity - d->cbuf_count % d->cbuf_capacity; >> + >> + /* Some of cbuf need to be overwrited */ >> + if (left < len) { >> + memcpy(d->cbuf + d->cbuf_in, buf, left); >> + memcpy(d->cbuf + d->cbuf_out, buf + left, len - left); >> + d->cbuf_out = (d->cbuf_out + len - left) % d->cbuf_capacity; >> + d->cbuf_count = d->cbuf_count + left; > Doing a mempcy() like this may work, but seems inefficient to me. I > think reading like a ring queue works a bit nicer. Hi Anthony, What do you mean "reading like a ring queue"? I am a little confused here. Could you please give more details? And thanks for your suggestions. :) >> + } else { >> + /* Completely overwrite */ >> + if (mem_chr_is_full(chr)) { >> + d->cbuf_out = (d->cbuf_out + len) % d->cbuf_capacity; >> + } else { >> + /* Enough cbuf to write */ >> + memcpy(d->cbuf + d->cbuf_in, buf, len); >> + d->cbuf_count += len; >> + } > Looks like indenting is off here. > > Regards, > > Anthony Liguori > >> + } >> + >> + d->cbuf_in = (d->cbuf_in + len) % d->cbuf_capacity; >> >> return len; >> } >> >> -void qemu_chr_init_mem(CharDriverState *chr) >> +static void mem_chr_read(CharDriverState *chr, uint8_t *buf, int len) >> +{ >> + MemoryDriver *d = chr->opaque; >> + int left; >> + >> + if (mem_chr_is_empty(chr)) { >> + return; >> + } >> + >> + left = d->cbuf_capacity - d->cbuf_count % d->cbuf_capacity; >> + >> + if (d->cbuf_capacity < len) { >> + len = d->cbuf_capacity; >> + } >> + >> + if (left < len) { >> + memcpy(buf, d->cbuf + d->cbuf_out, left); >> + memcpy(buf + left, d->cbuf + d->cbuf_out + left, len - left); >> + } else { >> + memcpy(buf, d->cbuf + d->cbuf_out, len); >> + } >> + >> + d->cbuf_out = (d->cbuf_out + len) % d->cbuf_capacity; >> + d->cbuf_count -= len; >> +} >> + >> +void qemu_chr_init_mem(CharDriverState *chr, size_t size) >> { >> MemoryDriver *d; >> >> d = g_malloc(sizeof(*d)); >> - d->outbuf_size = 0; >> - d->outbuf_capacity = 4096; >> - d->outbuf = g_malloc0(d->outbuf_capacity); >> + d->cbuf_capacity = size; >> + d->cbuf_in = 0; >> + d->cbuf_out = 0; >> + d->cbuf_count = 0; >> + d->cbuf = g_malloc0(d->cbuf_capacity); >> >> memset(chr, 0, sizeof(*chr)); >> chr->opaque = d; >> @@ -2558,7 +2616,7 @@ void qemu_chr_init_mem(CharDriverState *chr) >> QString *qemu_chr_mem_to_qs(CharDriverState *chr) >> { >> MemoryDriver *d = chr->opaque; >> - return qstring_from_substr((char *) d->outbuf, 0, d->outbuf_size - 1); >> + return qstring_from_substr((char *) d->cbuf, 0, d->cbuf_count - 1); >> } >> >> /* NOTE: this driver can not be closed with qemu_chr_delete()! */ >> @@ -2566,7 +2624,7 @@ void qemu_chr_close_mem(CharDriverState *chr) >> { >> MemoryDriver *d = chr->opaque; >> >> - g_free(d->outbuf); >> + g_free(d->cbuf); >> g_free(chr->opaque); >> chr->opaque = NULL; >> chr->chr_write = NULL; >> @@ -2575,7 +2633,7 @@ void qemu_chr_close_mem(CharDriverState *chr) >> size_t qemu_chr_mem_osize(const CharDriverState *chr) >> { >> const MemoryDriver *d = chr->opaque; >> - return d->outbuf_size; >> + return d->cbuf_count; >> } >> >> QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename) >> diff --git a/qemu-char.h b/qemu-char.h >> index 486644b..d8d90cc 100644 >> --- a/qemu-char.h >> +++ b/qemu-char.h >> @@ -243,7 +243,7 @@ CharDriverState *qemu_chr_open_eventfd(int eventfd); >> extern int term_escape_char; >> >> /* memory chardev */ >> -void qemu_chr_init_mem(CharDriverState *chr); >> +void qemu_chr_init_mem(CharDriverState *chr, size_t size); >> void qemu_chr_close_mem(CharDriverState *chr); >> QString *qemu_chr_mem_to_qs(CharDriverState *chr); >> size_t qemu_chr_mem_osize(const CharDriverState *chr); >> -- >> 1.7.7.6 -- Lei