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

[Stefan Hajnoczi <stefanha@gmail.com>]

[-- 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 --]