qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [RFC PATCH] block/qapi: Optimize the query function of


From: Dou Liyang
Subject: Re: [Qemu-devel] [RFC PATCH] block/qapi: Optimize the query function of the blockstats
Date: Thu, 8 Dec 2016 19:17:42 +0800
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0



At 12/08/2016 06:07 PM, Stefan Hajnoczi wrote:
On Thu, Dec 08, 2016 at 03:56:42PM +0800, Dou Liyang wrote:

The commit message says "optimize" but I think the purpose of this patch
is to refactor qmp_query_blockstats() to make it easier to read?  Just
checking if you expect a change in performance.

Yes, I expect.
It not just for readability. also for enhancing the performance.

And I did a simple test:

+    int64_t start_time = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
     stats_list = qmp_query_blockstats(false, false, NULL);
+    int64_t end_time = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
+    monitor_printf(mon, " start_time=%" PRId64
+                        " end_time=%" PRId64
+                        " takes_time=%" PRId64
+                        "\n",
+                        start_time,
+                        end_time,
+                        (end_time -start_time));

Then, use "info blockstats" 10 times and get the averages and
distributions of the values:

--default: use blk--
before that patch: 1 disk : 3695ns  | 500 disks : 658156ns

after that patch:  1 disk : 3290ns  | 500 disks : 651000ns

--query_node: use BDS--

before that patch: 1 disk : 2567ns  | 500 disks : 610947ns

after that patch:  1 disk : 3290ns  | 500 disks : 597085ns

And actually, For the qmp_query_blockstats(), I hope it could be O(1) by adding a parameter or something else. However, as you said, it was a
reasonable short-term workaround.
I am also confused whether it is necessary to do in the longer term ?

whatever it is, I guess this patch may be helpful.

Usually when I see "optimize" I think improving performance.  When I see
"refactor" I think improving the structure of the code.


I see.

In the query function named qmp_query_blockstats, for each block/node,
the process is as shown below:
the 0 func determines which the lookup queue will be used by the 1 func,
then gets the next object by the 1 func and calls the 2. The 2 func calls
the 3 and 4 func. There are some judgement and work that is useless and
repetitive.

+--------------+      +---------------------+
 | 1            |      | 4.                  |
 |next_query_bds|      |bdrv_query_bds_stats +---+
 |              |      |                     |   |
 +--------^-----+      +-------------^-------+   |
          |                          |           |
+---------+----------+      +--------+-------+   |
| 0.                 |      | 2.             |   |
|qmp_query_blockstats+------>bdrv_query_stats<----
|                    |      |                |
+--------------------+      +--------+-------+
                                     |
                       +-------------v-------+
                       | 3.                  |
                       |bdrv_query_blk_stats |
                       |                     |
                       +---------------------+

So, we think its logic is complex, and its readability is low.
There is no need to do so. Just let the function do its own thing.

Now, we optimize them make sure that:
1. the func named bdrv_query_bds_stats do the work for the BDS.
2. the func named bdrv_query_blk_stats do the work for the Blocks.
3. the query func just call what the want actually in need.
                                    +--------------+
                                    |              |
                           +--------v-----------+  |
                       +--->  3.                |  |
+-------------------+  |   |bdrv_query_bds_stats+--+
| 1.                +--+   |                    |
|                   +      +--------------------+
|qmp_query_blockstats--+
|                   |  |
+-------------------+  |   +--------------------+
                       |   | 2.                 |
                       +--->                    |
                           |bdrv_query_blk_stats|
                           |                    |
                           +--------------------+

Signed-off-by: Dou Liyang <address@hidden>
---
 block/qapi.c | 106 +++++++++++++++++++++++++++++------------------------------
 1 file changed, 53 insertions(+), 53 deletions(-)

It's not immediately obvious that this change improves code readability.
53 insertions vs 53 deletions does not suggest a simplification.  I
guess it depends whether you prefer helper functions or open coding.


oops, I have not found that is the same! :)
I will try to improve.

Thanks,

 Liyang


I'll leave the review of this patch to the maintainers of block/qapi.c.


diff --git a/block/qapi.c b/block/qapi.c
index a62e862..090452b 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -357,14 +357,14 @@ static void bdrv_query_info(BlockBackend *blk, BlockInfo 
**p_info,
     qapi_free_BlockInfo(info);
 }

-static BlockStats *bdrv_query_stats(BlockBackend *blk,
-                                    const BlockDriverState *bs,
-                                    bool query_backing);
-
-static void bdrv_query_blk_stats(BlockDeviceStats *ds, BlockBackend *blk)
+static BlockStats *bdrv_query_blk_stats(BlockStats *s, BlockBackend *blk)
 {
     BlockAcctStats *stats = blk_get_stats(blk);
     BlockAcctTimedStats *ts = NULL;
+    BlockDeviceStats *ds = s->stats;
+
+    s->has_device = true;
+    s->device = g_strdup(blk_name(blk));

     ds->rd_bytes = stats->nr_bytes[BLOCK_ACCT_READ];
     ds->wr_bytes = stats->nr_bytes[BLOCK_ACCT_WRITE];
@@ -426,11 +426,24 @@ static void bdrv_query_blk_stats(BlockDeviceStats *ds, 
BlockBackend *blk)
         dev_stats->avg_wr_queue_depth =
             block_acct_queue_depth(ts, BLOCK_ACCT_WRITE);
     }
+
+    return s;
 }

-static void bdrv_query_bds_stats(BlockStats *s, const BlockDriverState *bs,
+static BlockStats *bdrv_query_bds_stats(BlockStats *s,
+                                 const BlockDriverState *bs,
                                  bool query_backing)
 {
+
+    if (!s) {
+        s = g_malloc0(sizeof(*s));
+        s->stats = g_malloc0(sizeof(*s->stats));
+    }
+
+    if (!bs) {
+        return s;
+    }
+
     if (bdrv_get_node_name(bs)[0]) {
         s->has_node_name = true;
         s->node_name = g_strdup(bdrv_get_node_name(bs));
@@ -440,32 +453,12 @@ static void bdrv_query_bds_stats(BlockStats *s, const 
BlockDriverState *bs,

     if (bs->file) {
         s->has_parent = true;
-        s->parent = bdrv_query_stats(NULL, bs->file->bs, query_backing);
+        s->parent = bdrv_query_bds_stats(NULL, bs->file->bs, query_backing);
     }

     if (query_backing && bs->backing) {
         s->has_backing = true;
-        s->backing = bdrv_query_stats(NULL, bs->backing->bs, query_backing);
-    }
-
-}
-
-static BlockStats *bdrv_query_stats(BlockBackend *blk,
-                                    const BlockDriverState *bs,
-                                    bool query_backing)
-{
-    BlockStats *s;
-
-    s = g_malloc0(sizeof(*s));
-    s->stats = g_malloc0(sizeof(*s->stats));
-
-    if (blk) {
-        s->has_device = true;
-        s->device = g_strdup(blk_name(blk));
-        bdrv_query_blk_stats(s->stats, blk);
-    }
-    if (bs) {
-        bdrv_query_bds_stats(s, bs, query_backing);
+        s->backing = bdrv_query_bds_stats(NULL, bs->backing->bs, 
query_backing);
     }

     return s;
@@ -494,42 +487,49 @@ BlockInfoList *qmp_query_block(Error **errp)
     return head;
 }

-static bool next_query_bds(BlockBackend **blk, BlockDriverState **bs,
-                           bool query_nodes)
-{
-    if (query_nodes) {
-        *bs = bdrv_next_node(*bs);
-        return !!*bs;
-    }
-
-    *blk = blk_next(*blk);
-    *bs = *blk ? blk_bs(*blk) : NULL;
-
-    return !!*blk;
-}
-
 BlockStatsList *qmp_query_blockstats(bool has_query_nodes,
                                      bool query_nodes,
                                      Error **errp)
 {
     BlockStatsList *head = NULL, **p_next = &head;
-    BlockBackend *blk = NULL;
+    BlockBackend *blk;
     BlockDriverState *bs = NULL;

     /* Just to be safe if query_nodes is not always initialized */
-    query_nodes = has_query_nodes && query_nodes;
+    if (has_query_nodes && query_nodes) {
+        while ((bs = bdrv_next_node(bs))) {
+            BlockStatsList *info = g_malloc0(sizeof(*info));
+            AioContext *ctx = bdrv_get_aio_context(bs);
+            BlockStats *s;

-    while (next_query_bds(&blk, &bs, query_nodes)) {
-        BlockStatsList *info = g_malloc0(sizeof(*info));
-        AioContext *ctx = blk ? blk_get_aio_context(blk)
-                              : bdrv_get_aio_context(bs);
+            s = g_malloc0(sizeof(*s));
+            s->stats = g_malloc0(sizeof(*s->stats));

-        aio_context_acquire(ctx);
-        info->value = bdrv_query_stats(blk, bs, !query_nodes);
-        aio_context_release(ctx);
+            aio_context_acquire(ctx);
+            info->value = bdrv_query_bds_stats(s, bs, false);
+            aio_context_release(ctx);

-        *p_next = info;
-        p_next = &info->next;
+            *p_next = info;
+             p_next = &info->next;
+        }
+    } else {
+        for (blk = blk_next(NULL); blk; blk = blk_next(blk)) {
+            BlockStatsList *info = g_malloc0(sizeof(*info));
+            AioContext *ctx = blk_get_aio_context(blk);
+            BlockStats *s;
+
+            s = g_malloc0(sizeof(*s));
+            s->stats = g_malloc0(sizeof(*s->stats));
+            bs = blk_bs(blk);
+
+            aio_context_acquire(ctx);
+            s = bdrv_query_blk_stats(s, blk);
+            info->value = bdrv_query_bds_stats(s, bs, true);
+            aio_context_release(ctx);
+
+            *p_next = info;
+            p_next = &info->next;
+        }
     }

     return head;
--
2.5.5








reply via email to

[Prev in Thread] Current Thread [Next in Thread]