[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: |
Stefan Hajnoczi |
Subject: |
Re: [Qemu-devel] [RFC PATCH] block/qapi: Optimize the query function of the blockstats |
Date: |
Thu, 8 Dec 2016 10:07:44 +0000 |
User-agent: |
Mutt/1.7.1 (2016-10-04) |
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 <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.
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
>
>
>
signature.asc
Description: PGP signature