[Qemu-devel] [PATCH v2 0/4] qtest: base64 r/w and faster memset

[Qemu-devel] [PATCH v2 0/4] qtest: base64 r/w and faster memset

[John Snow]

Adds new qtest protocol commands for base64 reads and writes,
as well as a proper command for memset instead of faking it
via write.

This improves the ahci-test performance on my machine from about
14 seconds to about 4.

==
For convenience, this branch is available at:
https://github.com/jnsnow/qemu.git branch qtest_protocol
https://github.com/jnsnow/qemu/tree/qtest_protocol

This version is tagged qtest_protocol-v2:
https://github.com/jnsnow/qemu/releases/tag/qtest_protocol-v2
==

John Snow (4):
  qtest: allow arbitrarily long sends
  qtest: Add base64 encoded read/write
  qtest: add memset to qtest protocol
  libqos/ahci: Swap memread/write with bufread/write

 qtest.c             | 129 ++++++++++++++++++++++++++++++++++++++++++++++------
 tests/ahci-test.c   |   8 ++--
 tests/libqos/ahci.c |   4 +-
 tests/libqtest.c    |  39 +++++++++++++---
 tests/libqtest.h    |  49 ++++++++++++++++++++
 5 files changed, 201 insertions(+), 28 deletions(-)

-- 
2.1.0

[Qemu-devel] [PATCH v2 1/4] qtest: allow arbitrarily long sends

[John Snow]

qtest currently has a static buffer of size 1024 that if we
overflow, ignores the additional data silently which leads
to hangs or stream failures.

Use glib's string facilities to allow arbitrarily long data,
but split this off into a new function, qtest_sendf.

Static data can still be sent using qtest_send, which avoids
the malloc/copy overflow.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 qtest.c | 46 ++++++++++++++++++++++++++++------------------
 1 file changed, 28 insertions(+), 18 deletions(-)

diff --git a/qtest.c b/qtest.c
index 8d1e66c..f635a48 100644
--- a/qtest.c
+++ b/qtest.c
@@ -182,21 +182,29 @@ static void qtest_send_prefix(CharDriverState *chr)
             (long) tv.tv_sec, (long) tv.tv_usec);
 }
 
-static void GCC_FMT_ATTR(2, 3) qtest_send(CharDriverState *chr,
-                                          const char *fmt, ...)
+static void do_qtest_send(CharDriverState *chr, const char *str, size_t len)
+{
+    qemu_chr_fe_write_all(chr, (uint8_t *)str, len);
+    if (qtest_log_fp && qtest_opened) {
+        fprintf(qtest_log_fp, "%s", str);
+    }
+}
+
+static void qtest_send(CharDriverState *chr, const char *str)
+{
+    do_qtest_send(chr, str, strlen(str));
+}
+
+static void GCC_FMT_ATTR(2, 3) qtest_sendf(CharDriverState *chr,
+                                           const char *fmt, ...)
 {
     va_list ap;
-    char buffer[1024];
-    size_t len;
+    gchar *buffer;
 
     va_start(ap, fmt);
-    len = vsnprintf(buffer, sizeof(buffer), fmt, ap);
+    buffer = g_strdup_vprintf(fmt, ap);
+    qtest_send(chr, buffer);
     va_end(ap);
-
-    qemu_chr_fe_write_all(chr, (uint8_t *)buffer, len);
-    if (qtest_log_fp && qtest_opened) {
-        fprintf(qtest_log_fp, "%s", buffer);
-    }
 }
 
 static void qtest_irq_handler(void *opaque, int n, int level)
@@ -208,8 +216,8 @@ static void qtest_irq_handler(void *opaque, int n, int level)
         CharDriverState *chr = qtest_chr;
         irq_levels[n] = level;
         qtest_send_prefix(chr);
-        qtest_send(chr, "IRQ %s %d\n",
-                   level ? "raise" : "lower", n);
+        qtest_sendf(chr, "IRQ %s %d\n",
+                    level ? "raise" : "lower", n);
     }
 }
 
@@ -318,7 +326,7 @@ static void qtest_process_command(CharDriverState *chr, gchar **words)
             value = cpu_inl(addr);
         }
         qtest_send_prefix(chr);
-        qtest_send(chr, "OK 0x%04x\n", value);
+        qtest_sendf(chr, "OK 0x%04x\n", value);
     } else if (strcmp(words[0], "writeb") == 0 ||
                strcmp(words[0], "writew") == 0 ||
                strcmp(words[0], "writel") == 0 ||
@@ -375,7 +383,7 @@ static void qtest_process_command(CharDriverState *chr, gchar **words)
             tswap64s(&value);
         }
         qtest_send_prefix(chr);
-        qtest_send(chr, "OK 0x%016" PRIx64 "\n", value);
+        qtest_sendf(chr, "OK 0x%016" PRIx64 "\n", value);
     } else if (strcmp(words[0], "read") == 0) {
         uint64_t addr, len, i;
         uint8_t *data;
@@ -390,7 +398,7 @@ static void qtest_process_command(CharDriverState *chr, gchar **words)
         qtest_send_prefix(chr);
         qtest_send(chr, "OK 0x");
         for (i = 0; i < len; i++) {
-            qtest_send(chr, "%02x", data[i]);
+            qtest_sendf(chr, "%02x", data[i]);
         }
         qtest_send(chr, "\n");
 
@@ -434,7 +442,8 @@ static void qtest_process_command(CharDriverState *chr, gchar **words)
         }
         qtest_clock_warp(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + ns);
         qtest_send_prefix(chr);
-        qtest_send(chr, "OK %"PRIi64"\n", (int64_t)qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
+        qtest_sendf(chr, "OK %"PRIi64"\n",
+                    (int64_t)qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
     } else if (qtest_enabled() && strcmp(words[0], "clock_set") == 0) {
         int64_t ns;
 
@@ -442,10 +451,11 @@ static void qtest_process_command(CharDriverState *chr, gchar **words)
         ns = strtoll(words[1], NULL, 0);
         qtest_clock_warp(ns);
         qtest_send_prefix(chr);
-        qtest_send(chr, "OK %"PRIi64"\n", (int64_t)qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
+        qtest_sendf(chr, "OK %"PRIi64"\n",
+                    (int64_t)qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
     } else {
         qtest_send_prefix(chr);
-        qtest_send(chr, "FAIL Unknown command `%s'\n", words[0]);
+        qtest_sendf(chr, "FAIL Unknown command `%s'\n", words[0]);
     }
 }
 
-- 
2.1.0

[Qemu-devel] [PATCH v2 2/4] qtest: Add base64 encoded read/write

[John Snow]

For larger pieces of data that won't need to be debugged and
viewing the hex nibbles is unlikely to be useful, we can encode
data using base64 instead of encoding each byte as %02x, which
leads to some space savings and faster reads/writes.

For now, the default is left as hex nibbles in memwrite() and memread().
For the purposes of making qtest io easier to read and debug, some
callers may want to specify using the old encoding format for small
patches of data where the savings from base64 wouldn't be that profound.

memwrite/memread use a data encoding that takes 2x the size of the original
buffer, but base64 uses "only" (4/3)x, so for larger buffers we can save a
decent amount of time and space.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 qtest.c          | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 tests/libqtest.c | 31 +++++++++++++++++++++++++
 tests/libqtest.h | 49 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 149 insertions(+)

diff --git a/qtest.c b/qtest.c
index f635a48..fd85e04 100644
--- a/qtest.c
+++ b/qtest.c
@@ -119,12 +119,21 @@ static bool qtest_opened;
  *  > write ADDR SIZE DATA
  *  < OK
  *
+ *  > b64read ADDR SIZE
+ *  < OK B64_DATA
+ *
+ *  > b64write ADDR SIZE B64_DATA
+ *  < OK
+ *
  * ADDR, SIZE, VALUE are all integers parsed with strtoul() with a base of 0.
  *
  * DATA is an arbitrarily long hex number prefixed with '0x'.  If it's smaller
  * than the expected size, the value will be zero filled at the end of the data
  * sequence.
  *
+ * B64_DATA is an arbitrarily long base64 encoded string.
+ * If the sizes do not match, the data will be truncated.
+ *
  * IRQ management:
  *
  *  > irq_intercept_in QOM-PATH
@@ -182,6 +191,21 @@ static void qtest_send_prefix(CharDriverState *chr)
             (long) tv.tv_sec, (long) tv.tv_usec);
 }
 
+static void GCC_FMT_ATTR(1, 2) qtest_log_send(const char *fmt, ...)
+{
+    va_list ap;
+
+    if (!qtest_log_fp || !qtest_opened) {
+        return;
+    }
+
+    qtest_send_prefix(NULL);
+
+    va_start(ap, fmt);
+    vfprintf(qtest_log_fp, fmt, ap);
+    va_end(ap);
+}
+
 static void do_qtest_send(CharDriverState *chr, const char *str, size_t len)
 {
     qemu_chr_fe_write_all(chr, (uint8_t *)str, len);
@@ -403,6 +427,23 @@ static void qtest_process_command(CharDriverState *chr, gchar **words)
         qtest_send(chr, "\n");
 
         g_free(data);
+    } else if (strcmp(words[0], "b64read") == 0) {
+        uint64_t addr, len;
+        uint8_t *data;
+        gchar *b64_data;
+
+        g_assert(words[1] && words[2]);
+        addr = strtoull(words[1], NULL, 0);
+        len = strtoull(words[2], NULL, 0);
+
+        data = g_malloc(len);
+        cpu_physical_memory_read(addr, data, len);
+        b64_data = g_base64_encode(data, len);
+        qtest_send_prefix(chr);
+        qtest_sendf(chr, "OK %s\n", b64_data);
+
+        g_free(data);
+        g_free(b64_data);
     } else if (strcmp(words[0], "write") == 0) {
         uint64_t addr, len, i;
         uint8_t *data;
@@ -432,6 +473,34 @@ static void qtest_process_command(CharDriverState *chr, gchar **words)
 
         qtest_send_prefix(chr);
         qtest_send(chr, "OK\n");
+    }  else if (strcmp(words[0], "b64write") == 0) {
+        uint64_t addr, len;
+        uint8_t *data;
+        size_t data_len;
+        gsize out_len;
+
+        g_assert(words[1] && words[2] && words[3]);
+        addr = strtoull(words[1], NULL, 0);
+        len = strtoull(words[2], NULL, 0);
+
+        data_len = strlen(words[3]);
+        if (data_len < 3) {
+            qtest_send(chr, "ERR invalid argument size\n");
+            return;
+        }
+
+        data = g_base64_decode_inplace(words[3], &out_len);
+        if (out_len != len) {
+            qtest_log_send("b64write: data length mismatch (told %zu, "
+                           "found %zu)\n",
+                           len, out_len);
+            out_len = MIN(out_len, len);
+        }
+
+        cpu_physical_memory_write(addr, data, out_len);
+
+        qtest_send_prefix(chr);
+        qtest_send(chr, "OK\n");
     } else if (qtest_enabled() && strcmp(words[0], "clock_step") == 0) {
         int64_t ns;
 
diff --git a/tests/libqtest.c b/tests/libqtest.c
index a525dc5..5f57005 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -695,6 +695,37 @@ void qtest_add_data_func(const char *str, const void *data, void (*fn))
     g_free(path);
 }
 
+void qtest_bufwrite(QTestState *s, uint64_t addr, const void *data, size_t size)
+{
+    gchar *bdata;
+
+    bdata = g_base64_encode(data, size);
+    qtest_sendf(s, "b64write 0x%" PRIx64 " 0x%zx ", addr, size);
+    socket_send(s->fd, bdata, strlen(bdata));
+    socket_send(s->fd, "\n", 1);
+    qtest_rsp(s, 0);
+    g_free(bdata);
+}
+
+void qtest_bufread(QTestState *s, uint64_t addr, void *data, size_t size)
+{
+    gchar **args;
+    size_t len;
+
+    qtest_sendf(s, "b64read 0x%" PRIx64 " 0x%zx\n", addr, size);
+    args = qtest_rsp(s, 2);
+
+    g_base64_decode_inplace(args[1], &len);
+    if (size != len) {
+        fprintf(stderr, "bufread: asked for %zu bytes but decoded %zu\n",
+                size, len);
+        len = MIN(len, size);
+    }
+
+    memcpy(data, args[1], len);
+    g_strfreev(args);
+}
+
 void qtest_memwrite(QTestState *s, uint64_t addr, const void *data, size_t size)
 {
     const uint8_t *ptr = data;
diff --git a/tests/libqtest.h b/tests/libqtest.h
index 4b54b5d..ec42031 100644
--- a/tests/libqtest.h
+++ b/tests/libqtest.h
@@ -301,6 +301,17 @@ uint64_t qtest_readq(QTestState *s, uint64_t addr);
 void qtest_memread(QTestState *s, uint64_t addr, void *data, size_t size);
 
 /**
+ * qtest_bufread:
+ * @s: #QTestState instance to operate on.
+ * @addr: Guest address to read from.
+ * @data: Pointer to where memory contents will be stored.
+ * @size: Number of bytes to read.
+ *
+ * Read guest memory into a buffer and receive using a base64 encoding.
+ */
+void qtest_bufread(QTestState *s, uint64_t addr, void *data, size_t size);
+
+/**
  * qtest_memwrite:
  * @s: #QTestState instance to operate on.
  * @addr: Guest address to write to.
@@ -312,6 +323,18 @@ void qtest_memread(QTestState *s, uint64_t addr, void *data, size_t size);
 void qtest_memwrite(QTestState *s, uint64_t addr, const void *data, size_t size);
 
 /**
+ * qtest_bufwrite:
+ * @s: #QTestState instance to operate on.
+ * @addr: Guest address to write to.
+ * @data: Pointer to the bytes that will be written to guest memory.
+ * @size: Number of bytes to write.
+ *
+ * Write a buffer to guest memory and transmit using a base64 encoding.
+ */
+void qtest_bufwrite(QTestState *s, uint64_t addr,
+                    const void *data, size_t size);
+
+/**
  * qtest_memset:
  * @s: #QTestState instance to operate on.
  * @addr: Guest address to write to.
@@ -699,6 +722,19 @@ static inline void memread(uint64_t addr, void *data, size_t size)
 }
 
 /**
+ * bufread:
+ * @addr: Guest address to read from.
+ * @data: Pointer to where memory contents will be stored.
+ * @size: Number of bytes to read.
+ *
+ * Read guest memory into a buffer, receive using a base64 encoding.
+ */
+static inline void bufread(uint64_t addr, void *data, size_t size)
+{
+    qtest_bufread(global_qtest, addr, data, size);
+}
+
+/**
  * memwrite:
  * @addr: Guest address to write to.
  * @data: Pointer to the bytes that will be written to guest memory.
@@ -712,6 +748,19 @@ static inline void memwrite(uint64_t addr, const void *data, size_t size)
 }
 
 /**
+ * bufwrite:
+ * @addr: Guest address to write to.
+ * @data: Pointer to the bytes that will be written to guest memory.
+ * @size: Number of bytes to write.
+ *
+ * Write a buffer to guest memory, transmit using a base64 encoding.
+ */
+static inline void bufwrite(uint64_t addr, const void *data, size_t size)
+{
+    qtest_bufwrite(global_qtest, addr, data, size);
+}
+
+/**
  * qmemset:
  * @addr: Guest address to write to.
  * @patt: Byte pattern to fill the guest memory region with.
-- 
2.1.0

[Qemu-devel] [PATCH v2 3/4] qtest: add memset to qtest protocol

[John Snow]

Previously, memset was just a frontend to write() and only
stupidly sent the pattern many times across the wire.

Let's not discuss who stupidly wrote it like that in the first place.
(Hint: It was me.)

Signed-off-by: John Snow <jsnow@redhat.com>
---
 qtest.c          | 20 ++++++++++++++++++++
 tests/libqtest.c |  8 +-------
 2 files changed, 21 insertions(+), 7 deletions(-)

diff --git a/qtest.c b/qtest.c
index fd85e04..c1a0493 100644
--- a/qtest.c
+++ b/qtest.c
@@ -125,6 +125,9 @@ static bool qtest_opened;
  *  > b64write ADDR SIZE B64_DATA
  *  < OK
  *
+ *  > memset ADDR SIZE VALUE
+ *  < OK
+ *
  * ADDR, SIZE, VALUE are all integers parsed with strtoul() with a base of 0.
  *
  * DATA is an arbitrarily long hex number prefixed with '0x'.  If it's smaller
@@ -473,6 +476,23 @@ static void qtest_process_command(CharDriverState *chr, gchar **words)
 
         qtest_send_prefix(chr);
         qtest_send(chr, "OK\n");
+    } else if (strcmp(words[0], "memset") == 0) {
+        uint64_t addr, len;
+        uint8_t *data;
+        uint8_t pattern;
+
+        g_assert(words[1] && words[2] && words[3]);
+        addr = strtoull(words[1], NULL, 0);
+        len = strtoull(words[2], NULL, 0);
+        pattern = strtoull(words[3], NULL, 0);
+
+        data = g_malloc(len);
+        memset(data, pattern, len);
+        cpu_physical_memory_write(addr, data, len);
+        g_free(data);
+
+        qtest_send_prefix(chr);
+        qtest_send(chr, "OK\n");
     }  else if (strcmp(words[0], "b64write") == 0) {
         uint64_t addr, len;
         uint8_t *data;
diff --git a/tests/libqtest.c b/tests/libqtest.c
index 5f57005..055aad6 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -741,13 +741,7 @@ void qtest_memwrite(QTestState *s, uint64_t addr, const void *data, size_t size)
 
 void qtest_memset(QTestState *s, uint64_t addr, uint8_t pattern, size_t size)
 {
-    size_t i;
-
-    qtest_sendf(s, "write 0x%" PRIx64 " 0x%zx 0x", addr, size);
-    for (i = 0; i < size; i++) {
-        qtest_sendf(s, "%02x", pattern);
-    }
-    qtest_sendf(s, "\n");
+    qtest_sendf(s, "memset 0x%" PRIx64 " 0x%zx 0x%02x\n", addr, size, pattern);
     qtest_rsp(s, 0);
 }
 
-- 
2.1.0

[Qemu-devel] [PATCH v2 4/4] libqos/ahci: Swap memread/write with bufread/write

[John Snow]

Where it makes sense, use the new faster primitives.
For generally small reads/writes such as for the PRDT
and FIS packets, stick with the more wasteful but
easier to debug memread/memwrite.

For ahci-test;
With this patch:
real	0m4.802s
user	0m3.506s
sys	0m2.393s

Without this series:
real	0m14.171s
user	0m12.072s
sys	0m12.527s

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/ahci-test.c   | 8 ++++----
 tests/libqos/ahci.c | 4 ++--
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/tests/ahci-test.c b/tests/ahci-test.c
index 7c23bb2..bbcb52a 100644
--- a/tests/ahci-test.c
+++ b/tests/ahci-test.c
@@ -806,7 +806,7 @@ static void ahci_test_io_rw_simple(AHCIQState *ahci, unsigned bufsize,
 
     /* Write some indicative pattern to our buffer. */
     generate_pattern(tx, bufsize, AHCI_SECTOR_SIZE);
-    memwrite(ptr, tx, bufsize);
+    bufwrite(ptr, tx, bufsize);
 
     /* Write this buffer to disk, then read it back to the DMA buffer. */
     ahci_guest_io(ahci, port, write_cmd, ptr, bufsize, sector);
@@ -814,7 +814,7 @@ static void ahci_test_io_rw_simple(AHCIQState *ahci, unsigned bufsize,
     ahci_guest_io(ahci, port, read_cmd, ptr, bufsize, sector);
 
     /*** Read back the Data ***/
-    memread(ptr, rx, bufsize);
+    bufread(ptr, rx, bufsize);
     g_assert_cmphex(memcmp(tx, rx, bufsize), ==, 0);
 
     ahci_free(ahci, ptr);
@@ -950,7 +950,7 @@ static void test_dma_fragmented(void)
     /* Create a DMA buffer in guest memory, and write our pattern to it. */
     ptr = guest_alloc(ahci->parent->alloc, bufsize);
     g_assert(ptr);
-    memwrite(ptr, tx, bufsize);
+    bufwrite(ptr, tx, bufsize);
 
     cmd = ahci_command_create(CMD_WRITE_DMA);
     ahci_command_adjust(cmd, 0, ptr, bufsize, 32);
@@ -967,7 +967,7 @@ static void test_dma_fragmented(void)
     g_free(cmd);
 
     /* Read back the guest's receive buffer into local memory */
-    memread(ptr, rx, bufsize);
+    bufread(ptr, rx, bufsize);
     guest_free(ahci->parent->alloc, ptr);
 
     g_assert_cmphex(memcmp(tx, rx, bufsize), ==, 0);
diff --git a/tests/libqos/ahci.c b/tests/libqos/ahci.c
index 843cf72..e9b8bde 100644
--- a/tests/libqos/ahci.c
+++ b/tests/libqos/ahci.c
@@ -625,13 +625,13 @@ void ahci_io(AHCIQState *ahci, uint8_t port, uint8_t ide_cmd,
     g_assert(ptr);
 
     if (props->write) {
-        memwrite(ptr, buffer, bufsize);
+        bufwrite(ptr, buffer, bufsize);
     }
 
     ahci_guest_io(ahci, port, ide_cmd, ptr, bufsize, sector);
 
     if (props->read) {
-        memread(ptr, buffer, bufsize);
+        bufread(ptr, buffer, bufsize);
     }
 
     ahci_free(ahci, ptr);
-- 
2.1.0

Re: [Qemu-devel] [PATCH v2 4/4] libqos/ahci: Swap memread/write with bufread/write

[Paolo Bonzini]

On 01/05/2015 21:55, John Snow wrote:
> Where it makes sense, use the new faster primitives.
> For generally small reads/writes such as for the PRDT
> and FIS packets, stick with the more wasteful but
> easier to debug memread/memwrite.
> 
> For ahci-test;
> With this patch:
> real	0m4.802s
> user	0m3.506s
> sys	0m2.393s
> 
> Without this series:
> real	0m14.171s
> user	0m12.072s
> sys	0m12.527s

The overhead of memread is 2, the overhead of base64 is 1.33, also
base64 should have a larger cost of computing each byte.  It doesn't add up.

Could it be simply that calling qtest_send (and hence
vsnprintf+qemu_chr_fe_write_all, neither of which are speed demons) once
per byte is hideously inefficient? :)

Paolo

Re: [Qemu-devel] [PATCH v2 4/4] libqos/ahci: Swap memread/write with bufread/write

[John Snow]

On 05/01/2015 04:48 PM, Paolo Bonzini wrote:
>
>
> On 01/05/2015 21:55, John Snow wrote:
>> Where it makes sense, use the new faster primitives.
>> For generally small reads/writes such as for the PRDT
>> and FIS packets, stick with the more wasteful but
>> easier to debug memread/memwrite.
>>
>> For ahci-test;
>> With this patch:
>> real	0m4.802s
>> user	0m3.506s
>> sys	0m2.393s
>>
>> Without this series:
>> real	0m14.171s
>> user	0m12.072s
>> sys	0m12.527s
>
> The overhead of memread is 2, the overhead of base64 is 1.33, also
> base64 should have a larger cost of computing each byte.  It doesn't add up.
>

Right, there's more at play here than just the wire length.

> Could it be simply that calling qtest_send (and hence
> vsnprintf+qemu_chr_fe_write_all, neither of which are speed demons) once
> per byte is hideously inefficient? :)
>

I can batch those too in another patch,
or should I consider this a NACK?

> Paolo
>

--js

Re: [Qemu-devel] [PATCH v2 4/4] libqos/ahci: Swap memread/write with bufread/write

[John Snow]

On 05/01/2015 04:48 PM, Paolo Bonzini wrote:
>
>
> On 01/05/2015 21:55, John Snow wrote:
>> Where it makes sense, use the new faster primitives.
>> For generally small reads/writes such as for the PRDT
>> and FIS packets, stick with the more wasteful but
>> easier to debug memread/memwrite.
>>
>> For ahci-test;
>> With this patch:
>> real	0m4.802s
>> user	0m3.506s
>> sys	0m2.393s
>>
>> Without this series:
>> real	0m14.171s
>> user	0m12.072s
>> sys	0m12.527s
>
> The overhead of memread is 2, the overhead of base64 is 1.33, also
> base64 should have a larger cost of computing each byte.  It doesn't add up.
>
> Could it be simply that calling qtest_send (and hence
> vsnprintf+qemu_chr_fe_write_all, neither of which are speed demons) once
> per byte is hideously inefficient? :)
>
> Paolo
>

I wrote a loop to batch the ascii-hex conversion instead of letting 
printf do it; then ran some more very, very scientific tests:

memset alone:
real	0m10.888s
user	0m9.303s
sys	0m9.146s

send-batching:
real	0m6.541s
user	0m5.027s
sys	0m4.941s

memset+batching+b64:
real	0m3.675s
user	0m2.582s
sys	0m1.718s

So it still seems as if the b64 batching is a strict improvement 
speed-wise. I'll send the non-b64 batching patch separately later, 
unless you have thoughts otherwise.

--js

Re: [Qemu-devel] [PATCH v2 4/4] libqos/ahci: Swap memread/write with bufread/write

[Paolo Bonzini]

On 02/05/2015 02:13, John Snow wrote:
>>
> 
> I wrote a loop to batch the ascii-hex conversion instead of letting
> printf do it; then ran some more very, very scientific tests:
> 
> memset alone:
> real    0m10.888s
> user    0m9.303s
> sys    0m9.146s
> 
> send-batching:
> real    0m6.541s
> user    0m5.027s
> sys    0m4.941s
> 
> memset+batching+b64:
> real    0m3.675s
> user    0m2.582s
> sys    0m1.718s
> 
> So it still seems as if the b64 batching is a strict improvement
> speed-wise. I'll send the non-b64 batching patch separately later,
> unless you have thoughts otherwise.

Ok, this is more similar to what I'd expect (3.6 * 6 / 4 = 5.4, I'm not
sure if you have the memset optimization in the send-batching test).

Hex is obviously more debuggable compared to Base64 (unless you starred
in the Matrix movies), so I'm a bit undecided about this one.  Anyone
can break the tie?

Paolo

Re: [Qemu-devel] [PATCH v2 4/4] libqos/ahci: Swap memread/write with bufread/write

[John Snow]

On 05/05/2015 06:35 AM, Paolo Bonzini wrote:
>
>
> On 02/05/2015 02:13, John Snow wrote:
>>>
>>
>> I wrote a loop to batch the ascii-hex conversion instead of letting
>> printf do it; then ran some more very, very scientific tests:
>>
>> memset alone:
>> real    0m10.888s
>> user    0m9.303s
>> sys    0m9.146s
>>
>> send-batching:
>> real    0m6.541s
>> user    0m5.027s
>> sys    0m4.941s
>>
>> memset+batching+b64:
>> real    0m3.675s
>> user    0m2.582s
>> sys    0m1.718s
>>
>> So it still seems as if the b64 batching is a strict improvement
>> speed-wise. I'll send the non-b64 batching patch separately later,
>> unless you have thoughts otherwise.
>
> Ok, this is more similar to what I'd expect (3.6 * 6 / 4 = 5.4, I'm not
> sure if you have the memset optimization in the send-batching test).
>

I did, yes. Hence the "very, very scientific" warning. I just pushed 
patches down my stack and tested with each new optimization.

Unoptimized is still ~14s.

> Hex is obviously more debuggable compared to Base64 (unless you starred
> in the Matrix movies), so I'm a bit undecided about this one.  Anyone
> can break the tie?
>
> Paolo
>

I specifically left things that alter control flow using hex nibbles -- 
such as the FIS packets, PRD tables, and all other existing tests. I 
only use the b64 encoding for raw data patterns, which don't really need 
to be debugged. Either they match or they don't: any particular values 
are uninteresting.

Any future test can be switched to/from hex/b64 by just altering 
"mem{read,write}()" to "buf{read,write}()" as desired. I specifically 
opted not to alter *all* qtest IO for this very reason.

Does that help? :)

If you're not opposed to the rest of this series, I will send a v2 
including the hex batching optimization.

--js

Re: [Qemu-devel] [PATCH v2 4/4] libqos/ahci: Swap memread/write with bufread/write

[Paolo Bonzini]

On 05/05/2015 17:48, John Snow wrote:
> I specifically left things that alter control flow using hex nibbles --
> such as the FIS packets, PRD tables, and all other existing tests. I
> only use the b64 encoding for raw data patterns, which don't really need
> to be debugged. Either they match or they don't: any particular values
> are uninteresting.

To some extent: they may still be interesting for stuff like IDENTIFY.

> Any future test can be switched to/from hex/b64 by just altering
> "mem{read,write}()" to "buf{read,write}()" as desired. I specifically
> opted not to alter *all* qtest IO for this very reason.
> 
> Does that help? :)

Yes.  We could also switch unconditionally to b64, but keep hex in the
qtest-log.

> If you're not opposed to the rest of this series, I will send a v2
> including the hex batching optimization.

This series is okay.  I just wanted some clarifications and ideas for
future work.

Paolo

Re: [Qemu-devel] [PATCH v2 4/4] libqos/ahci: Swap memread/write with bufread/write

[John Snow]

On 05/05/2015 12:19 PM, Paolo Bonzini wrote:
> 
> 
> On 05/05/2015 17:48, John Snow wrote:
>> I specifically left things that alter control flow using hex nibbles --
>> such as the FIS packets, PRD tables, and all other existing tests. I
>> only use the b64 encoding for raw data patterns, which don't really need
>> to be debugged. Either they match or they don't: any particular values
>> are uninteresting.
> 
> To some extent: they may still be interesting for stuff like IDENTIFY.
> 

You got me there.

>> Any future test can be switched to/from hex/b64 by just altering
>> "mem{read,write}()" to "buf{read,write}()" as desired. I specifically
>> opted not to alter *all* qtest IO for this very reason.
>>
>> Does that help? :)
> 
> Yes.  We could also switch unconditionally to b64, but keep hex in the
> qtest-log.
> 

Hmm, there's a thought. Though b64 for a handful of bytes seems
definitely likely to be slower. I am comfortable having two
implementations at the moment.

(Mostly because I don't feel like testing what the threshold for which
strings should be sent with what method is. Too much effort for qtest.)

hex-always in the log is a good idea, though. I may do that.

>> If you're not opposed to the rest of this series, I will send a v2
>> including the hex batching optimization.
> 
> This series is okay.  I just wanted some clarifications and ideas for
> future work.
> 
> Paolo
> 

Just trying to make you happy ;)

--js