[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH RFC v3 2/2] block/qapi: reduce the execution tim
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH RFC v3 2/2] block/qapi: reduce the execution time of qmp_query_blockstats |
Date: |
Fri, 13 Jan 2017 11:42:03 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/25.1 (gnu/linux) |
Dou Liyang <address@hidden> writes:
> In order to reduce the execution time, this patch optimize
> the qmp_query_blockstats():
> Remove the next_query_bds function.
> Remove the bdrv_query_stats function.
> Remove some judgement sentence.
>
> The original qmp_query_blockstats calls next_query_bds to get
> the next objects in each loops. In the next_query_bds, it checks
> the query_nodes and blk. It also call bdrv_query_stats to get
> the stats, In the bdrv_query_stats, it checks blk and bs each
> times. This waste more times, which may stall the main loop a
> bit. And if the disk is too many and donot use the dataplane
> feature, this may affect the performance in main loop thread.
>
> This patch removes that two functions, and makes the structure
> clearly.
>
> Signed-off-by: Dou Liyang <address@hidden>
> ---
> block/qapi.c | 72
> +++++++++++++++++++++++-------------------------------------
> 1 file changed, 28 insertions(+), 44 deletions(-)
>
> diff --git a/block/qapi.c b/block/qapi.c
> index bc622cd..6e1e8bd 100644
> --- a/block/qapi.c
> +++ b/block/qapi.c
> @@ -456,23 +456,6 @@ static BlockStats *bdrv_query_bds_stats(const
> BlockDriverState *bs,
> return s;
> }
>
> -static BlockStats *bdrv_query_stats(BlockBackend *blk,
> - const BlockDriverState *bs,
> - bool query_backing)
> -{
> - BlockStats *s;
> -
> - s = bdrv_query_bds_stats(bs, query_backing);
> -
> - if (blk) {
> - s->has_device = true;
> - s->device = g_strdup(blk_name(blk));
> - bdrv_query_blk_stats(s->stats, blk);
> - }
> -
> - return s;
> -}
> -
> BlockInfoList *qmp_query_block(Error **errp)
> {
> BlockInfoList *head = NULL, **p_next = &head;
> @@ -496,42 +479,43 @@ 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;
> - BlockDriverState *bs = NULL;
> + BlockBackend *blk;
> + BlockDriverState *bs;
>
> /* Just to be safe if query_nodes is not always initialized */
> - query_nodes = has_query_nodes && query_nodes;
> -
> - 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);
> + if (has_query_nodes && query_nodes) {
> + for (bs = bdrv_next_node(NULL); bs; bs = bdrv_next_node(bs)) {
> + BlockStatsList *info = g_malloc0(sizeof(*info));
> + AioContext *ctx = bdrv_get_aio_context(bs);
>
> - 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(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);
> +
> + aio_context_acquire(ctx);
> + BlockStats *s = bdrv_query_bds_stats(blk_bs(blk), true);
Please don't put declarations after statements. Suggest:
BlockStats *s;
aio_context_acquire(ctx);
info->value = s = bdrv_query_bds_stats(blk_bs(blk), true);
> + s->has_device = true;
> + s->device = g_strdup(blk_name(blk));
> + bdrv_query_blk_stats(s->stats, blk);
> + aio_context_release(ctx);
> +
> + info->value = s;
> + *p_next = info;
> + p_next = &info->next;
> + }
> }
>
> return head;