From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52451) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1dQZcB-0005Bh-Gs for qemu-devel@nongnu.org; Thu, 29 Jun 2017 09:36:16 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1dQZc4-0006XG-QY for qemu-devel@nongnu.org; Thu, 29 Jun 2017 09:36:11 -0400 Received: from lhrrgout.huawei.com ([194.213.3.17]:16153) by eggs.gnu.org with esmtps (TLS1.0:RSA_ARCFOUR_SHA1:16) (Exim 4.71) (envelope-from ) id 1dQZc4-0006Vq-3U for qemu-devel@nongnu.org; Thu, 29 Jun 2017 09:36:04 -0400 References: <1497877896-35700-1-git-send-email-pradeep.jagadeesh@huawei.com> <1497877896-35700-4-git-send-email-pradeep.jagadeesh@huawei.com> <20170620180516.143c5359@bahia.lab.toulouse-stg.fr.ibm.com> <20170621120014.77c2c03d@bahia.lab.toulouse-stg.fr.ibm.com> From: Pradeep Jagadeesh Message-ID: <85bc81d4-e247-393e-eef3-865ec2cbec14@huawei.com> Date: Thu, 29 Jun 2017 15:35:43 +0200 MIME-Version: 1.0 In-Reply-To: <20170621120014.77c2c03d@bahia.lab.toulouse-stg.fr.ibm.com> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v5 3/5] qmp: refactor duplicate code List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Greg Kurz Cc: Pradeep Jagadeesh , eric blake , alberto garcia , jani kokkonen , qemu-devel@nongnu.org, Markus Armbruster , "Dr. David Alan Gilbert" On 6/21/2017 12:00 PM, Greg Kurz wrote: > On Wed, 21 Jun 2017 10:34:42 +0200 > Pradeep Jagadeesh wrote: > >> On 6/20/2017 6:05 PM, Greg Kurz wrote: >>> On Mon, 19 Jun 2017 09:11:34 -0400 >>> Pradeep Jagadeesh wrote: >>> >>>> This patch factor out the duplicate qmp throttle interface code >>>> that was present in both block and fsdev device files. >>>> >>> >>> The text is obviously wrong. I don't see any duplicate code below. >> OK, I will fix this. >>> >>> It is more something like: let's factor out the code that will be used >>> by the existing QMP throttling API for block devices and the future >>> QMP throttling API for fs devices. >>> >>> Please move the HMP part to another patch, as asked during v4 review. >> I have moved the hmp patches for the fsdev into a separate patch. Do you >> want me to push this also into a separate patch? > > Yes. These changes aren't related and theoretically belong to separate > sub-maintainer trees. OK, I will split the commit and make separate patches. -Pradeep > >>> >>> Also, blockdev.c and hmp.c do have maintainers. You should Cc: them each >>> time because they know better than me and even if these patches are carried >>> through my tree, I won't do it without an ack from them. >> >> OK, I will add them next time on. >> >> -Pradeep >> >>> Cc'ing Markus for blockdev.c and David for hmp.c. >>> >>>> Signed-off-by: Pradeep Jagadeesh >>>> --- >>>> blockdev.c | 53 +++--------------------------------- >>>> hmp.c | 21 ++++++++++----- >>>> include/qemu/throttle-options.h | 3 +++ >>>> util/throttle.c | 60 +++++++++++++++++++++++++++++++++++++++++ >>>> 4 files changed, 81 insertions(+), 56 deletions(-) >>>> >>>> diff --git a/blockdev.c b/blockdev.c >>>> index 5db9e5c..3d06e9e 100644 >>>> --- a/blockdev.c >>>> +++ b/blockdev.c >>>> @@ -2593,6 +2593,7 @@ void qmp_block_set_io_throttle(BlockIOThrottle *arg, Error **errp) >>>> BlockDriverState *bs; >>>> BlockBackend *blk; >>>> AioContext *aio_context; >>>> + IOThrottle *iothrottle; >>>> >>>> blk = qmp_get_blk(arg->has_device ? arg->device : NULL, >>>> arg->has_id ? arg->id : NULL, >>>> @@ -2610,56 +2611,8 @@ void qmp_block_set_io_throttle(BlockIOThrottle *arg, Error **errp) >>>> goto out; >>>> } >>>> >>>> - 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; >>>> - >>>> - cfg.buckets[THROTTLE_OPS_TOTAL].avg = arg->iops; >>>> - cfg.buckets[THROTTLE_OPS_READ].avg = arg->iops_rd; >>>> - cfg.buckets[THROTTLE_OPS_WRITE].avg = arg->iops_wr; >>>> - >>>> - if (arg->has_bps_max) { >>>> - cfg.buckets[THROTTLE_BPS_TOTAL].max = arg->bps_max; >>>> - } >>>> - if (arg->has_bps_rd_max) { >>>> - cfg.buckets[THROTTLE_BPS_READ].max = arg->bps_rd_max; >>>> - } >>>> - if (arg->has_bps_wr_max) { >>>> - cfg.buckets[THROTTLE_BPS_WRITE].max = arg->bps_wr_max; >>>> - } >>>> - if (arg->has_iops_max) { >>>> - cfg.buckets[THROTTLE_OPS_TOTAL].max = arg->iops_max; >>>> - } >>>> - if (arg->has_iops_rd_max) { >>>> - cfg.buckets[THROTTLE_OPS_READ].max = arg->iops_rd_max; >>>> - } >>>> - if (arg->has_iops_wr_max) { >>>> - cfg.buckets[THROTTLE_OPS_WRITE].max = arg->iops_wr_max; >>>> - } >>>> - >>>> - if (arg->has_bps_max_length) { >>>> - cfg.buckets[THROTTLE_BPS_TOTAL].burst_length = arg->bps_max_length; >>>> - } >>>> - if (arg->has_bps_rd_max_length) { >>>> - cfg.buckets[THROTTLE_BPS_READ].burst_length = arg->bps_rd_max_length; >>>> - } >>>> - if (arg->has_bps_wr_max_length) { >>>> - cfg.buckets[THROTTLE_BPS_WRITE].burst_length = arg->bps_wr_max_length; >>>> - } >>>> - if (arg->has_iops_max_length) { >>>> - cfg.buckets[THROTTLE_OPS_TOTAL].burst_length = arg->iops_max_length; >>>> - } >>>> - if (arg->has_iops_rd_max_length) { >>>> - cfg.buckets[THROTTLE_OPS_READ].burst_length = arg->iops_rd_max_length; >>>> - } >>>> - if (arg->has_iops_wr_max_length) { >>>> - cfg.buckets[THROTTLE_OPS_WRITE].burst_length = arg->iops_wr_max_length; >>>> - } >>>> - >>>> - if (arg->has_iops_size) { >>>> - cfg.op_size = arg->iops_size; >>>> - } >>>> + iothrottle = qapi_BlockIOThrottle_base(arg); >>>> + throttle_set_io_limits(&cfg, iothrottle); >>>> >>>> if (!throttle_is_valid(&cfg, errp)) { >>>> goto out; >>>> diff --git a/hmp.c b/hmp.c >>>> index 8c72c58..220d301 100644 >>>> --- a/hmp.c >>>> +++ b/hmp.c >>>> @@ -1749,20 +1749,29 @@ 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_id = true; >>>> + iot->id = (char *) qdict_get_str(qdict, "id"); >>>> + 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; >>>> + IOThrottle *iothrottle; >>>> BlockIOThrottle throttle = { >>>> .has_device = true, >>>> .device = (char *) qdict_get_str(qdict, "device"), >>>> - .bps = qdict_get_int(qdict, "bps"), >>>> - .bps_rd = qdict_get_int(qdict, "bps_rd"), >>>> - .bps_wr = qdict_get_int(qdict, "bps_wr"), >>>> - .iops = qdict_get_int(qdict, "iops"), >>>> - .iops_rd = qdict_get_int(qdict, "iops_rd"), >>>> - .iops_wr = qdict_get_int(qdict, "iops_wr"), >>>> }; >>>> >>>> + iothrottle = qapi_BlockIOThrottle_base(&throttle); >>>> + hmp_initialize_io_throttle(iothrottle, qdict); >>>> qmp_block_set_io_throttle(&throttle, &err); >>>> hmp_handle_error(mon, &err); >>>> } >>>> diff --git a/include/qemu/throttle-options.h b/include/qemu/throttle-options.h >>>> index 565553a..e94ea39 100644 >>>> --- a/include/qemu/throttle-options.h >>>> +++ b/include/qemu/throttle-options.h >>>> @@ -11,6 +11,7 @@ >>>> #define THROTTLE_OPTIONS_H >>>> >>>> #include "qemu/throttle.h" >>>> +#include "qmp-commands.h" >>>> >>>> #define THROTTLE_OPTS \ >>>> { \ >>>> @@ -93,4 +94,6 @@ >>>> >>>> void throttle_parse_options(ThrottleConfig *, QemuOpts *); >>>> >>>> +void throttle_set_io_limits(ThrottleConfig *, IOThrottle *); >>>> + >>>> #endif >>>> diff --git a/util/throttle.c b/util/throttle.c >>>> index ebe9dd0..2cf9ec5 100644 >>>> --- a/util/throttle.c >>>> +++ b/util/throttle.c >>>> @@ -27,6 +27,7 @@ >>>> #include "qemu/throttle.h" >>>> #include "qemu/timer.h" >>>> #include "block/aio.h" >>>> +#include "qemu/throttle-options.h" >>>> >>>> /* This function make a bucket leak >>>> * >>>> @@ -564,3 +565,62 @@ void throttle_parse_options(ThrottleConfig *throttle_cfg, QemuOpts *opts) >>>> throttle_cfg->op_size = >>>> qemu_opt_get_number(opts, "throttling.iops-size", 0); >>>> } >>>> + >>>> +/* set the throttle limits >>>> + * >>>> + * @arg: iothrottle limits >>>> + * @cfg: throttle configuration >>>> + */ >>>> +void throttle_set_io_limits(ThrottleConfig *cfg, IOThrottle *arg) >>>> +{ >>>> + 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; >>>> + >>>> + cfg->buckets[THROTTLE_OPS_TOTAL].avg = arg->iops; >>>> + cfg->buckets[THROTTLE_OPS_READ].avg = arg->iops_rd; >>>> + cfg->buckets[THROTTLE_OPS_WRITE].avg = arg->iops_wr; >>>> + >>>> + if (arg->has_bps_max) { >>>> + cfg->buckets[THROTTLE_BPS_TOTAL].max = arg->bps_max; >>>> + } >>>> + if (arg->has_bps_rd_max) { >>>> + cfg->buckets[THROTTLE_BPS_READ].max = arg->bps_rd_max; >>>> + } >>>> + if (arg->has_bps_wr_max) { >>>> + cfg->buckets[THROTTLE_BPS_WRITE].max = arg->bps_wr_max; >>>> + } >>>> + if (arg->has_iops_max) { >>>> + cfg->buckets[THROTTLE_OPS_TOTAL].max = arg->iops_max; >>>> + } >>>> + if (arg->has_iops_rd_max) { >>>> + cfg->buckets[THROTTLE_OPS_READ].max = arg->iops_rd_max; >>>> + } >>>> + if (arg->has_iops_wr_max) { >>>> + cfg->buckets[THROTTLE_OPS_WRITE].max = arg->iops_wr_max; >>>> + } >>>> + >>>> + if (arg->has_bps_max_length) { >>>> + cfg->buckets[THROTTLE_BPS_TOTAL].burst_length = arg->bps_max_length; >>>> + } >>>> + if (arg->has_bps_rd_max_length) { >>>> + cfg->buckets[THROTTLE_BPS_READ].burst_length = arg->bps_rd_max_length; >>>> + } >>>> + if (arg->has_bps_wr_max_length) { >>>> + cfg->buckets[THROTTLE_BPS_WRITE].burst_length = arg->bps_wr_max_length; >>>> + } >>>> + if (arg->has_iops_max_length) { >>>> + cfg->buckets[THROTTLE_OPS_TOTAL].burst_length = arg->iops_max_length; >>>> + } >>>> + if (arg->has_iops_rd_max_length) { >>>> + cfg->buckets[THROTTLE_OPS_READ].burst_length = arg->iops_rd_max_length; >>>> + } >>>> + if (arg->has_iops_wr_max_length) { >>>> + cfg->buckets[THROTTLE_OPS_WRITE].burst_length = arg->iops_wr_max_length; >>>> + } >>>> + >>>> + if (arg->has_iops_size) { >>>> + cfg->op_size = arg->iops_size; >>>> + } >>>> +} >>> >> >