* [Qemu-devel] [RFC PATCH] block/qapi: Optimize the query function of the blockstats
@ 2016-12-08 7:56 Dou Liyang
2016-12-08 10:07 ` Stefan Hajnoczi
0 siblings, 1 reply; 4+ messages in thread
From: Dou Liyang @ 2016-12-08 7:56 UTC (permalink / raw)
To: qemu-devel, stefanha, famz, armbru, kwolf, danpb
Cc: izumi.taku, caoj.fnst, fanc.fnst, Dou Liyang
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 <douly.fnst@cn.fujitsu.com>
---
block/qapi.c | 106 +++++++++++++++++++++++++++++------------------------------
1 file changed, 53 insertions(+), 53 deletions(-)
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [RFC PATCH] block/qapi: Optimize the query function of the blockstats
2016-12-08 7:56 [Qemu-devel] [RFC PATCH] block/qapi: Optimize the query function of the blockstats Dou Liyang
@ 2016-12-08 10:07 ` Stefan Hajnoczi
2016-12-08 11:17 ` Dou Liyang
0 siblings, 1 reply; 4+ messages in thread
From: Stefan Hajnoczi @ 2016-12-08 10:07 UTC (permalink / raw)
To: Dou Liyang
Cc: qemu-devel, famz, armbru, kwolf, danpb, izumi.taku, caoj.fnst,
fanc.fnst
[-- Attachment #1: Type: text/plain, Size: 8935 bytes --]
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.
Usually when I see "optimize" I think improving performance. When I see
"refactor" I think improving the structure of the code.
> 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 <douly.fnst@cn.fujitsu.com>
> ---
> 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.
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
>
>
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [RFC PATCH] block/qapi: Optimize the query function of the blockstats
2016-12-08 10:07 ` Stefan Hajnoczi
@ 2016-12-08 11:17 ` Dou Liyang
2016-12-08 11:20 ` Dou Liyang
0 siblings, 1 reply; 4+ messages in thread
From: Dou Liyang @ 2016-12-08 11:17 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: qemu-devel, famz, armbru, kwolf, danpb, izumi.taku, caoj.fnst,
fanc.fnst
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 <douly.fnst@cn.fujitsu.com>
>> ---
>> 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
>>
>>
>>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [RFC PATCH] block/qapi: Optimize the query function of the blockstats
2016-12-08 11:17 ` Dou Liyang
@ 2016-12-08 11:20 ` Dou Liyang
0 siblings, 0 replies; 4+ messages in thread
From: Dou Liyang @ 2016-12-08 11:20 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: qemu-devel, famz, armbru, kwolf, danpb, izumi.taku, caoj.fnst,
fanc.fnst
At 12/08/2016 07:17 PM, Dou Liyang wrote:
>
>
> 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
before that patch: 1 disk : 3290ns | 500 disks : 610947ns
after that patch: 1 disk : 2567ns | 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 <douly.fnst@cn.fujitsu.com>
>>> ---
>>> 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
>>>
>>>
>>>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-12-08 11:20 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-12-08 7:56 [Qemu-devel] [RFC PATCH] block/qapi: Optimize the query function of the blockstats Dou Liyang
2016-12-08 10:07 ` Stefan Hajnoczi
2016-12-08 11:17 ` Dou Liyang
2016-12-08 11:20 ` Dou Liyang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).