Patchwork [RFC,1/4] qemu-char: Convert MemCharDriver to circular buffer

login
register
mail settings
Submitter Lei Li
Date Aug. 1, 2012, 9:48 a.m.
Message ID <1343814538-27591-2-git-send-email-lilei@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/174426/
State New
Headers show

Comments

Lei Li - Aug. 1, 2012, 9:48 a.m.
Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
---
 qemu-char.c |   96 +++++++++++++++++++++++++++++++++++++++++++++++-----------
 qemu-char.h |    2 +-
 2 files changed, 78 insertions(+), 20 deletions(-)
Anthony Liguori - Aug. 1, 2012, 9:30 p.m.
Lei Li <lilei@linux.vnet.ibm.com> writes:

> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
> ---
>  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.

> +    } 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 Li - Aug. 6, 2012, 10:57 a.m.
On 08/02/2012 05:30 AM, Anthony Liguori wrote:
> Lei Li <lilei@linux.vnet.ibm.com> writes:
>
>> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
>> ---
>>   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

Patch

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;
 
+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;
+}
+
 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;
+    } 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;
+        }
+    }
+
+    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);