Re: [Qemu-devel] [RFC PATCH 1/4] qemu-char: Convert MemCharDriver to circular buffer

[Lei Li]

On 08/02/2012 05:30 AM, Anthony Liguori wrote:
> Lei Li <address@hidden> writes:
>
> > Signed-off-by: Lei Li <address@hidden>
> > ---
> >   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