From: Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com>
To: Markus Armbruster <armbru@redhat.com>,
Pradeep Jagadeesh <pradeepkiruvale@gmail.com>
Cc: eric blake <eblake@redhat.com>, greg kurz <groug@kaod.org>,
jani kokkonen <jani.kokkonen@huawei.com>,
alberto garcia <berto@igalia.com>,
qemu-devel@nongnu.org
Subject: Re: [Qemu-devel] [PATCH v5 1/5] throttle: factor out duplicate code
Date: Thu, 29 Jun 2017 11:50:05 +0200 [thread overview]
Message-ID: <a636b34e-3e06-8db3-7ffe-834fcce6805f@huawei.com> (raw)
In-Reply-To: <87shisgk96.fsf@dusky.pond.sub.org>
On 6/22/2017 4:38 PM, Markus Armbruster wrote:
> Pradeep Jagadeesh <pradeepkiruvale@gmail.com> writes:
>
>> This patch factor out the duplicate throttle code that was present in
>> block and fsdev devices.
>>
>> Signed-off-by: Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com>
>> ---
>> blockdev.c | 44 +-----------------------------------
>> fsdev/qemu-fsdev-throttle.c | 43 +----------------------------------
>> fsdev/qemu-fsdev-throttle.h | 1 +
>> include/qemu/throttle-options.h | 4 ++++
>> util/throttle.c | 50 +++++++++++++++++++++++++++++++++++++++++
>> 5 files changed, 57 insertions(+), 85 deletions(-)
>>
>> diff --git a/blockdev.c b/blockdev.c
>> index 6472548..5db9e5c 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -386,49 +386,7 @@ static void extract_common_blockdev_options(QemuOpts *opts, int *bdrv_flags,
>> }
>>
>> if (throttle_cfg) {
>> - throttle_config_init(throttle_cfg);
>> - throttle_cfg->buckets[THROTTLE_BPS_TOTAL].avg =
>> - qemu_opt_get_number(opts, "throttling.bps-total", 0);
>> - throttle_cfg->buckets[THROTTLE_BPS_READ].avg =
>> - qemu_opt_get_number(opts, "throttling.bps-read", 0);
>> - throttle_cfg->buckets[THROTTLE_BPS_WRITE].avg =
>> - qemu_opt_get_number(opts, "throttling.bps-write", 0);
>> - throttle_cfg->buckets[THROTTLE_OPS_TOTAL].avg =
>> - qemu_opt_get_number(opts, "throttling.iops-total", 0);
>> - throttle_cfg->buckets[THROTTLE_OPS_READ].avg =
>> - qemu_opt_get_number(opts, "throttling.iops-read", 0);
>> - throttle_cfg->buckets[THROTTLE_OPS_WRITE].avg =
>> - qemu_opt_get_number(opts, "throttling.iops-write", 0);
>> -
>> - throttle_cfg->buckets[THROTTLE_BPS_TOTAL].max =
>> - qemu_opt_get_number(opts, "throttling.bps-total-max", 0);
>> - throttle_cfg->buckets[THROTTLE_BPS_READ].max =
>> - qemu_opt_get_number(opts, "throttling.bps-read-max", 0);
>> - throttle_cfg->buckets[THROTTLE_BPS_WRITE].max =
>> - qemu_opt_get_number(opts, "throttling.bps-write-max", 0);
>> - throttle_cfg->buckets[THROTTLE_OPS_TOTAL].max =
>> - qemu_opt_get_number(opts, "throttling.iops-total-max", 0);
>> - throttle_cfg->buckets[THROTTLE_OPS_READ].max =
>> - qemu_opt_get_number(opts, "throttling.iops-read-max", 0);
>> - throttle_cfg->buckets[THROTTLE_OPS_WRITE].max =
>> - qemu_opt_get_number(opts, "throttling.iops-write-max", 0);
>> -
>> - throttle_cfg->buckets[THROTTLE_BPS_TOTAL].burst_length =
>> - qemu_opt_get_number(opts, "throttling.bps-total-max-length", 1);
>> - throttle_cfg->buckets[THROTTLE_BPS_READ].burst_length =
>> - qemu_opt_get_number(opts, "throttling.bps-read-max-length", 1);
>> - throttle_cfg->buckets[THROTTLE_BPS_WRITE].burst_length =
>> - qemu_opt_get_number(opts, "throttling.bps-write-max-length", 1);
>> - throttle_cfg->buckets[THROTTLE_OPS_TOTAL].burst_length =
>> - qemu_opt_get_number(opts, "throttling.iops-total-max-length", 1);
>> - throttle_cfg->buckets[THROTTLE_OPS_READ].burst_length =
>> - qemu_opt_get_number(opts, "throttling.iops-read-max-length", 1);
>> - throttle_cfg->buckets[THROTTLE_OPS_WRITE].burst_length =
>> - qemu_opt_get_number(opts, "throttling.iops-write-max-length", 1);
>> -
>> - throttle_cfg->op_size =
>> - qemu_opt_get_number(opts, "throttling.iops-size", 0);
>> -
>> + throttle_parse_options(throttle_cfg, opts);
>> if (!throttle_is_valid(throttle_cfg, errp)) {
>> return;
>> }
>> diff --git a/fsdev/qemu-fsdev-throttle.c b/fsdev/qemu-fsdev-throttle.c
>> index 7ae4e86..da9c225 100644
>> --- a/fsdev/qemu-fsdev-throttle.c
>> +++ b/fsdev/qemu-fsdev-throttle.c
>> @@ -31,48 +31,7 @@ static void fsdev_throttle_write_timer_cb(void *opaque)
>>
>> void fsdev_throttle_parse_opts(QemuOpts *opts, FsThrottle *fst, Error **errp)
>> {
>> - throttle_config_init(&fst->cfg);
>> - fst->cfg.buckets[THROTTLE_BPS_TOTAL].avg =
>> - qemu_opt_get_number(opts, "throttling.bps-total", 0);
>> - fst->cfg.buckets[THROTTLE_BPS_READ].avg =
>> - qemu_opt_get_number(opts, "throttling.bps-read", 0);
>> - fst->cfg.buckets[THROTTLE_BPS_WRITE].avg =
>> - qemu_opt_get_number(opts, "throttling.bps-write", 0);
>> - fst->cfg.buckets[THROTTLE_OPS_TOTAL].avg =
>> - qemu_opt_get_number(opts, "throttling.iops-total", 0);
>> - fst->cfg.buckets[THROTTLE_OPS_READ].avg =
>> - qemu_opt_get_number(opts, "throttling.iops-read", 0);
>> - fst->cfg.buckets[THROTTLE_OPS_WRITE].avg =
>> - qemu_opt_get_number(opts, "throttling.iops-write", 0);
>> -
>> - fst->cfg.buckets[THROTTLE_BPS_TOTAL].max =
>> - qemu_opt_get_number(opts, "throttling.bps-total-max", 0);
>> - fst->cfg.buckets[THROTTLE_BPS_READ].max =
>> - qemu_opt_get_number(opts, "throttling.bps-read-max", 0);
>> - fst->cfg.buckets[THROTTLE_BPS_WRITE].max =
>> - qemu_opt_get_number(opts, "throttling.bps-write-max", 0);
>> - fst->cfg.buckets[THROTTLE_OPS_TOTAL].max =
>> - qemu_opt_get_number(opts, "throttling.iops-total-max", 0);
>> - fst->cfg.buckets[THROTTLE_OPS_READ].max =
>> - qemu_opt_get_number(opts, "throttling.iops-read-max", 0);
>> - fst->cfg.buckets[THROTTLE_OPS_WRITE].max =
>> - qemu_opt_get_number(opts, "throttling.iops-write-max", 0);
>> -
>> - fst->cfg.buckets[THROTTLE_BPS_TOTAL].burst_length =
>> - qemu_opt_get_number(opts, "throttling.bps-total-max-length", 1);
>> - fst->cfg.buckets[THROTTLE_BPS_READ].burst_length =
>> - qemu_opt_get_number(opts, "throttling.bps-read-max-length", 1);
>> - fst->cfg.buckets[THROTTLE_BPS_WRITE].burst_length =
>> - qemu_opt_get_number(opts, "throttling.bps-write-max-length", 1);
>> - fst->cfg.buckets[THROTTLE_OPS_TOTAL].burst_length =
>> - qemu_opt_get_number(opts, "throttling.iops-total-max-length", 1);
>> - fst->cfg.buckets[THROTTLE_OPS_READ].burst_length =
>> - qemu_opt_get_number(opts, "throttling.iops-read-max-length", 1);
>> - fst->cfg.buckets[THROTTLE_OPS_WRITE].burst_length =
>> - qemu_opt_get_number(opts, "throttling.iops-write-max-length", 1);
>> - fst->cfg.op_size =
>> - qemu_opt_get_number(opts, "throttling.iops-size", 0);
>> -
>> + throttle_parse_options(&fst->cfg, opts);
>> throttle_is_valid(&fst->cfg, errp);
>> }
>>
>> diff --git a/fsdev/qemu-fsdev-throttle.h b/fsdev/qemu-fsdev-throttle.h
>> index e418643..c493e83 100644
>> --- a/fsdev/qemu-fsdev-throttle.h
>> +++ b/fsdev/qemu-fsdev-throttle.h
>> @@ -20,6 +20,7 @@
>> #include "qemu/coroutine.h"
>> #include "qapi/error.h"
>> #include "qemu/throttle.h"
>> +#include "qemu/throttle-options.h"
>
> Why?
>
Moved to qemu-fsdev-throttle.c
>>
>> typedef struct FsThrottle {
>> ThrottleState ts;
>> diff --git a/include/qemu/throttle-options.h b/include/qemu/throttle-options.h
>> index 3133d1c..565553a 100644
>> --- a/include/qemu/throttle-options.h
>> +++ b/include/qemu/throttle-options.h
>> @@ -10,6 +10,8 @@
>> #ifndef THROTTLE_OPTIONS_H
>> #define THROTTLE_OPTIONS_H
>>
>> +#include "qemu/throttle.h"
>> +
>
> Let's avoid this include: move the ThrottleConfig typedef to
> qemu/typedefs.h.
OK
>
>> #define THROTTLE_OPTS \
>> { \
>> .name = "throttling.iops-total",\
>> @@ -89,4 +91,6 @@
>> .help = "when limiting by iops max size of an I/O in bytes",\
>> }
>>
>> +void throttle_parse_options(ThrottleConfig *, QemuOpts *);
>> +
>> #endif
>> diff --git a/util/throttle.c b/util/throttle.c
>> index 3570ed2..ebe9dd0 100644
>> --- a/util/throttle.c
>> +++ b/util/throttle.c
>> @@ -514,3 +514,53 @@ void throttle_account(ThrottleState *ts, bool is_write, uint64_t size)
>> }
>> }
>>
>> +/* parse the throttle options
>> + *
>> + * @opts: qemu options
>> + * @throttle_cfg: throttle configuration
>> + */
>> +void throttle_parse_options(ThrottleConfig *throttle_cfg, QemuOpts *opts)
>> +{
>> + throttle_config_init(throttle_cfg);
>> + throttle_cfg->buckets[THROTTLE_BPS_TOTAL].avg =
>> + qemu_opt_get_number(opts, "throttling.bps-total", 0);
>> + throttle_cfg->buckets[THROTTLE_BPS_READ].avg =
>> + qemu_opt_get_number(opts, "throttling.bps-read", 0);
>> + throttle_cfg->buckets[THROTTLE_BPS_WRITE].avg =
>> + qemu_opt_get_number(opts, "throttling.bps-write", 0);
>> + throttle_cfg->buckets[THROTTLE_OPS_TOTAL].avg =
>> + qemu_opt_get_number(opts, "throttling.iops-total", 0);
>> + throttle_cfg->buckets[THROTTLE_OPS_READ].avg =
>> + qemu_opt_get_number(opts, "throttling.iops-read", 0);
>> + throttle_cfg->buckets[THROTTLE_OPS_WRITE].avg =
>> + qemu_opt_get_number(opts, "throttling.iops-write", 0);
>> +
>> + throttle_cfg->buckets[THROTTLE_BPS_TOTAL].max =
>> + qemu_opt_get_number(opts, "throttling.bps-total-max", 0);
>> + throttle_cfg->buckets[THROTTLE_BPS_READ].max =
>> + qemu_opt_get_number(opts, "throttling.bps-read-max", 0);
>> + throttle_cfg->buckets[THROTTLE_BPS_WRITE].max =
>> + qemu_opt_get_number(opts, "throttling.bps-write-max", 0);
>> + throttle_cfg->buckets[THROTTLE_OPS_TOTAL].max =
>> + qemu_opt_get_number(opts, "throttling.iops-total-max", 0);
>> + throttle_cfg->buckets[THROTTLE_OPS_READ].max =
>> + qemu_opt_get_number(opts, "throttling.iops-read-max", 0);
>> + throttle_cfg->buckets[THROTTLE_OPS_WRITE].max =
>> + qemu_opt_get_number(opts, "throttling.iops-write-max", 0);
>> +
>> + throttle_cfg->buckets[THROTTLE_BPS_TOTAL].burst_length =
>> + qemu_opt_get_number(opts, "throttling.bps-total-max-length", 1);
>> + throttle_cfg->buckets[THROTTLE_BPS_READ].burst_length =
>> + qemu_opt_get_number(opts, "throttling.bps-read-max-length", 1);
>> + throttle_cfg->buckets[THROTTLE_BPS_WRITE].burst_length =
>> + qemu_opt_get_number(opts, "throttling.bps-write-max-length", 1);
>> + throttle_cfg->buckets[THROTTLE_OPS_TOTAL].burst_length =
>> + qemu_opt_get_number(opts, "throttling.iops-total-max-length", 1);
>> + throttle_cfg->buckets[THROTTLE_OPS_READ].burst_length =
>> + qemu_opt_get_number(opts, "throttling.iops-read-max-length", 1);
>> + throttle_cfg->buckets[THROTTLE_OPS_WRITE].burst_length =
>> + qemu_opt_get_number(opts, "throttling.iops-write-max-length", 1);
>> +
>> + throttle_cfg->op_size =
>> + qemu_opt_get_number(opts, "throttling.iops-size", 0);
>> +}
>
> Looks good otherwise.
>
next prev parent reply other threads:[~2017-06-29 9:51 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-06-19 13:11 [Qemu-devel] [PATCH v5 0/5] fsdev: qmp interface for io throttling Pradeep Jagadeesh
2017-06-19 13:11 ` [Qemu-devel] [PATCH v5 1/5] throttle: factor out duplicate code Pradeep Jagadeesh
2017-06-20 15:46 ` Greg Kurz
2017-06-21 12:55 ` Alberto Garcia
2017-06-21 13:43 ` Greg Kurz
2017-06-21 14:21 ` Alberto Garcia
2017-06-22 14:38 ` Markus Armbruster
2017-06-22 16:23 ` Greg Kurz
2017-06-29 9:50 ` Pradeep Jagadeesh [this message]
2017-06-19 13:11 ` [Qemu-devel] [PATCH v5 2/5] qmp: Create IOThrottle structure Pradeep Jagadeesh
2017-06-20 15:51 ` Greg Kurz
2017-06-19 13:11 ` [Qemu-devel] [PATCH v5 3/5] qmp: refactor duplicate code Pradeep Jagadeesh
2017-06-20 16:05 ` Greg Kurz
2017-06-21 8:34 ` Pradeep Jagadeesh
2017-06-21 10:00 ` Greg Kurz
2017-06-29 13:35 ` Pradeep Jagadeesh
2017-06-19 13:11 ` [Qemu-devel] [PATCH v5 4/5] fsdev: hmp interface for throttling Pradeep Jagadeesh
2017-06-19 13:11 ` [Qemu-devel] [PATCH v5 5/5] fsdev: QMP " Pradeep Jagadeesh
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=a636b34e-3e06-8db3-7ffe-834fcce6805f@huawei.com \
--to=pradeep.jagadeesh@huawei.com \
--cc=armbru@redhat.com \
--cc=berto@igalia.com \
--cc=eblake@redhat.com \
--cc=groug@kaod.org \
--cc=jani.kokkonen@huawei.com \
--cc=pradeepkiruvale@gmail.com \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).