[Top][All Lists]

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

Re: [Qemu-devel] [PATCH 2/2 v1] throttle: factor out the redundant code

From: Pradeep Jagadeesh
Subject: Re: [Qemu-devel] [PATCH 2/2 v1] throttle: factor out the redundant code
Date: Tue, 28 Mar 2017 13:11:58 +0200
User-agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.7.1

On 3/23/2017 3:46 PM, Eric Blake wrote:
On 03/23/2017 07:20 AM, Pradeep Jagadeesh wrote:
This patchset removes the throttle redundant code that was present
in block and fsdev files.

Signed-off-by: Pradeep Jagadeesh <address@hidden>

I think you want portions of this patch to come first; you want to
refactor out the IOThrottle portion of existing types, and then extend
IOThrottle to be used in more places.
OK, I will do that.

 blockdev.c                      | 106 ++++-----------------------------------
 fsdev/Makefile.objs             |   1 +
 fsdev/qemu-fsdev-throttle.c     |  94 +---------------------------------
 fsdev/qemu-fsdev-throttle.h     |   3 +-
 hmp.c                           |  39 +++++++--------
 include/qemu/throttle-options.h |   5 ++
 qapi/block-core.json            |  76 +---------------------------
 qapi/iothrottle.json            |   4 +-
 util/Makefile.objs              |   1 +
 util/throttle-options.c         | 108 ++++++++++++++++++++++++++++++++++++++++
 10 files changed, 151 insertions(+), 286 deletions(-)
 create mode 100644 util/throttle-options.c

+++ b/fsdev/qemu-fsdev-throttle.c
@@ -33,56 +33,7 @@ void fsdev_set_io_throttle(IOThrottle *arg, FsThrottle *fst, 
Error **errp)
     ThrottleConfig cfg;

-    throttle_config_init(&cfg);
-    cfg.buckets[THROTTLE_BPS_TOTAL].avg = arg->bps;
-    cfg.buckets[THROTTLE_BPS_READ].avg  = arg->bps_rd;
-    cfg.buckets[THROTTLE_BPS_WRITE].avg = arg->bps_wr;

This code was added in the last patch - which makes for a lot of
unnecessary churn.  Again, the best series does refactoring of existing
code first, then adds the new code that takes advantage of the
refactored entry points, so that the new code isn't churning.


+++ b/hmp.c
@@ -1554,20 +1554,25 @@ void hmp_change(Monitor *mon, const QDict *qdict)
     hmp_handle_error(mon, &err);

+static void hmp_initialize_io_throttle(IOThrottle *iot, const QDict *qdict)
+    iot->has_device = true;
+    iot->device = (char *) qdict_get_str(qdict, "device");

Is casting away const going to result in a double free of memory later
on?  Or put another way, should this be a g_strdup'd copy (which you
then have to make sure is not leaked)?
This code was there in block_set_io_throttle(). I have just moved to new function. But I will think about it.

+    iot->bps = qdict_get_int(qdict, "bps");
+    iot->bps_rd = qdict_get_int(qdict, "bps_rd");
+    iot->bps_wr = qdict_get_int(qdict, "bps_wr");
+    iot->iops = qdict_get_int(qdict, "iops");
+    iot->iops_rd = qdict_get_int(qdict, "iops_rd");
+    iot->iops_wr = qdict_get_int(qdict, "iops_wr");
 void hmp_block_set_io_throttle(Monitor *mon, const QDict *qdict)
     Error *err = NULL;
-    BlockIOThrottle throttle = {
-        .has_device = true,
-        .device = (char *) qdict_get_str(qdict, "device"),

At any rate, I see it is just code motion.  You are refactoring too much
in one patch here; I'd consider splitting it along the lines of one
patch that adds hmp_initialize_io_throttle; and another patch that adds
parse_io_throttle_options(), so that it's easier to see just one piece
of code motion at a time, rather than trying to track multiple motions
in the same large patch.
I will split the code into different patches.

+++ b/qapi/block-core.json
@@ -1758,86 +1758,14 @@
 # A set of parameters describing block throttling.

-# @iops_size: an I/O size in bytes (Since 1.7)
+# @iothrottle: throttle configuration (Since 1.7)

NACK. This is an incompatible change; you are breaking the QMP wire
structure (that is, where I used to pass "arguments":{"device":"foo",
"bps_rd":1, "group":"bar" }, your change would now require me to pass
"arguments":{"iothrottle":{"device":"foo", "bps_rd":1}, "group":"bar"};
the extra nesting is what breaks things).

 # @group: throttle group name (Since 2.4)
 # Since: 1.1
 { 'struct': 'BlockIOThrottle',
-  'data': { '*device': 'str', '*id': 'str', 'bps': 'int', 'bps_rd': 'int',
-            'bps_wr': 'int', 'iops': 'int', 'iops_rd': 'int', 'iops_wr': 'int',
-            '*bps_max': 'int', '*bps_rd_max': 'int',
-            '*bps_wr_max': 'int', '*iops_max': 'int',
-            '*iops_rd_max': 'int', '*iops_wr_max': 'int',
-            '*bps_max_length': 'int', '*bps_rd_max_length': 'int',
-            '*bps_wr_max_length': 'int', '*iops_max_length': 'int',
-            '*iops_rd_max_length': 'int', '*iops_wr_max_length': 'int',
-            '*iops_size': 'int', '*group': 'str' } }
+  'data': { '*iothrottle': 'IOThrottle', '*group': 'str' } }

What you REALLY want is a compatible change, namely:

{ 'struct': 'BlockIOThrottle', 'base': 'IOThrottle',
  'data': { '*group': 'str' } }

which says that all fields of IOThrottle are inherited at the same level
as the additional field 'group'.

For that matter, are you sure that 'group' is the only field that should
not be directly in IOThrottle, or should 'device' also be specific to
BlockIOThrottle (I'm not sure whether you were actually using 'device'
in the fsdev case, or if 'id' was sufficient).

Yes, I just need id in case of fsdev. I will fix that. But if I do this way, some of the code refactoring is not possible.Because they will have different structures for fsdev and block.

 # @block-stream:
diff --git a/qapi/iothrottle.json b/qapi/iothrottle.json
index f7b615d..58f4520 100644
--- a/qapi/iothrottle.json
+++ b/qapi/iothrottle.json
@@ -14,6 +14,8 @@
 # @device: Block device name
+# @id: The name or QOM path of the guest device (since: 2.8)

You've missed 2.8 by a long shot.  Oh - this is an argument that it is
'id' (and not 'device') that does not belong here, but should be placed
alongside 'group' in the BlockIOThrottle subtype.
id is required for the fsdev/block both. This was part of the BlockIOThrottle. So I just took it and kept the description as it is.
What should I changed to? 2.10?

 # @bps: total throughput limit in bytes per second
 # @bps_rd: read throughput limit in bytes per second
@@ -80,7 +82,7 @@
 # Since: 2.10
 { 'struct': 'IOThrottle',
-  'data': { '*device': 'str', 'bps': 'int', 'bps_rd': 'int',
+  'data': { '*device': 'str', '*id': 'str', 'bps': 'int', 'bps_rd': 'int',
             'bps_wr': 'int', 'iops': 'int', 'iops_rd': 'int', 'iops_wr': 'int',
             '*bps_max': 'int', '*bps_rd_max': 'int',
             '*bps_wr_max': 'int', '*iops_max': 'int',

reply via email to

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