[v11] fsdev: qmp interface for io throttling

[Qemu-devel] [PATCH v11 1/6] throttle: factor out duplicate code

Posted by Pradeep Jagadeesh 8 years, 4 months ago

This patch factors out the duplicate throttle code that was still
present in block and fsdev devices.

Signed-off-by: Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Greg Kurz <groug@kaod.org>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 blockdev.c                      | 44 +---------------------------------
 fsdev/qemu-fsdev-throttle.c     | 44 ++--------------------------------
 include/qemu/throttle-options.h |  3 +++
 include/qemu/throttle.h         |  4 ++--
 include/qemu/typedefs.h         |  1 +
 util/throttle.c                 | 52 +++++++++++++++++++++++++++++++++++++++++
 6 files changed, 61 insertions(+), 87 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 56a6b24..9d33c25 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -387,49 +387,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 49eebb5..0e6fb86 100644
--- a/fsdev/qemu-fsdev-throttle.c
+++ b/fsdev/qemu-fsdev-throttle.c
@@ -16,6 +16,7 @@
 #include "qemu/error-report.h"
 #include "qemu-fsdev-throttle.h"
 #include "qemu/iov.h"
+#include "qemu/throttle-options.h"
 
 static void fsdev_throttle_read_timer_cb(void *opaque)
 {
@@ -31,48 +32,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/include/qemu/throttle-options.h b/include/qemu/throttle-options.h
index 3528a8f..9709dcd 100644
--- a/include/qemu/throttle-options.h
+++ b/include/qemu/throttle-options.h
@@ -9,6 +9,7 @@
  */
 #ifndef THROTTLE_OPTIONS_H
 #define THROTTLE_OPTIONS_H
+#include "typedefs.h"
 
 #define QEMU_OPT_IOPS_TOTAL "iops-total"
 #define QEMU_OPT_IOPS_TOTAL_MAX "iops-total-max"
@@ -111,4 +112,6 @@
             .help = "when limiting by iops max size of an I/O in bytes",\
         }
 
+void throttle_parse_options(ThrottleConfig *, QemuOpts *);
+
 #endif
diff --git a/include/qemu/throttle.h b/include/qemu/throttle.h
index 8c93237..b6ebc6d 100644
--- a/include/qemu/throttle.h
+++ b/include/qemu/throttle.h
@@ -89,10 +89,10 @@ typedef struct LeakyBucket {
  * However it allows to keep the code clean and the bucket field is reset to
  * zero at the right time.
  */
-typedef struct ThrottleConfig {
+struct ThrottleConfig {
     LeakyBucket buckets[BUCKETS_COUNT]; /* leaky buckets */
     uint64_t op_size;         /* size of an operation in bytes */
-} ThrottleConfig;
+};
 
 typedef struct ThrottleState {
     ThrottleConfig cfg;       /* configuration */
diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
index 39bc835..90fe0f9 100644
--- a/include/qemu/typedefs.h
+++ b/include/qemu/typedefs.h
@@ -100,6 +100,7 @@ typedef struct uWireSlave uWireSlave;
 typedef struct VirtIODevice VirtIODevice;
 typedef struct Visitor Visitor;
 typedef struct node_info NodeInfo;
+typedef struct ThrottleConfig ThrottleConfig;
 typedef void SaveStateHandler(QEMUFile *f, void *opaque);
 typedef int LoadStateHandler(QEMUFile *f, void *opaque, int version_id);
 
diff --git a/util/throttle.c b/util/throttle.c
index 06bf916..9ef28c4 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
  *
@@ -635,3 +636,54 @@ void throttle_config_to_limits(ThrottleConfig *cfg, ThrottleLimits *var)
     var->has_iops_write_max_length = true;
     var->has_iops_size = true;
 }
+
+/* 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);
+}
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v11 1/6] throttle: factor out duplicate code

Posted by Greg Kurz 8 years, 4 months ago

On Thu, 14 Sep 2017 06:40:05 -0400
Pradeep Jagadeesh <pradeepkiruvale@gmail.com> wrote:

> This patch factors out the duplicate throttle code that was still
> present in block and fsdev devices.
> 
> Signed-off-by: Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com>
> Reviewed-by: Alberto Garcia <berto@igalia.com>
> Reviewed-by: Greg Kurz <groug@kaod.org>

I see you took my remarks into account, except for the patch title
which is still the very same as commit:

a2a7862ca9ab throttle: factor out duplicate code

:-\

> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  blockdev.c                      | 44 +---------------------------------
>  fsdev/qemu-fsdev-throttle.c     | 44 ++--------------------------------
>  include/qemu/throttle-options.h |  3 +++
>  include/qemu/throttle.h         |  4 ++--
>  include/qemu/typedefs.h         |  1 +
>  util/throttle.c                 | 52 +++++++++++++++++++++++++++++++++++++++++
>  6 files changed, 61 insertions(+), 87 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 56a6b24..9d33c25 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -387,49 +387,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 49eebb5..0e6fb86 100644
> --- a/fsdev/qemu-fsdev-throttle.c
> +++ b/fsdev/qemu-fsdev-throttle.c
> @@ -16,6 +16,7 @@
>  #include "qemu/error-report.h"
>  #include "qemu-fsdev-throttle.h"
>  #include "qemu/iov.h"
> +#include "qemu/throttle-options.h"
>  
>  static void fsdev_throttle_read_timer_cb(void *opaque)
>  {
> @@ -31,48 +32,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/include/qemu/throttle-options.h b/include/qemu/throttle-options.h
> index 3528a8f..9709dcd 100644
> --- a/include/qemu/throttle-options.h
> +++ b/include/qemu/throttle-options.h
> @@ -9,6 +9,7 @@
>   */
>  #ifndef THROTTLE_OPTIONS_H
>  #define THROTTLE_OPTIONS_H
> +#include "typedefs.h"
>  
>  #define QEMU_OPT_IOPS_TOTAL "iops-total"
>  #define QEMU_OPT_IOPS_TOTAL_MAX "iops-total-max"
> @@ -111,4 +112,6 @@
>              .help = "when limiting by iops max size of an I/O in bytes",\
>          }
>  
> +void throttle_parse_options(ThrottleConfig *, QemuOpts *);
> +
>  #endif
> diff --git a/include/qemu/throttle.h b/include/qemu/throttle.h
> index 8c93237..b6ebc6d 100644
> --- a/include/qemu/throttle.h
> +++ b/include/qemu/throttle.h
> @@ -89,10 +89,10 @@ typedef struct LeakyBucket {
>   * However it allows to keep the code clean and the bucket field is reset to
>   * zero at the right time.
>   */
> -typedef struct ThrottleConfig {
> +struct ThrottleConfig {
>      LeakyBucket buckets[BUCKETS_COUNT]; /* leaky buckets */
>      uint64_t op_size;         /* size of an operation in bytes */
> -} ThrottleConfig;
> +};
>  
>  typedef struct ThrottleState {
>      ThrottleConfig cfg;       /* configuration */
> diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
> index 39bc835..90fe0f9 100644
> --- a/include/qemu/typedefs.h
> +++ b/include/qemu/typedefs.h
> @@ -100,6 +100,7 @@ typedef struct uWireSlave uWireSlave;
>  typedef struct VirtIODevice VirtIODevice;
>  typedef struct Visitor Visitor;
>  typedef struct node_info NodeInfo;
> +typedef struct ThrottleConfig ThrottleConfig;
>  typedef void SaveStateHandler(QEMUFile *f, void *opaque);
>  typedef int LoadStateHandler(QEMUFile *f, void *opaque, int version_id);
>  
> diff --git a/util/throttle.c b/util/throttle.c
> index 06bf916..9ef28c4 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
>   *
> @@ -635,3 +636,54 @@ void throttle_config_to_limits(ThrottleConfig *cfg, ThrottleLimits *var)
>      var->has_iops_write_max_length = true;
>      var->has_iops_size = true;
>  }
> +
> +/* 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);
> +}

Re: [Qemu-devel] [PATCH v11 1/6] throttle: factor out duplicate code

Posted by Manos Pitsidianakis 8 years, 4 months ago

On Thu, Sep 14, 2017 at 06:40:05AM -0400, Pradeep Jagadeesh wrote:
>This patch factors out the duplicate throttle code that was still
>present in block and fsdev devices.
>
>Signed-off-by: Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com>
>Reviewed-by: Alberto Garcia <berto@igalia.com>
>Reviewed-by: Greg Kurz <groug@kaod.org>
>Reviewed-by: Eric Blake <eblake@redhat.com>
>---
> blockdev.c                      | 44 +---------------------------------
> fsdev/qemu-fsdev-throttle.c     | 44 ++--------------------------------
> include/qemu/throttle-options.h |  3 +++
> include/qemu/throttle.h         |  4 ++--
> include/qemu/typedefs.h         |  1 +
> util/throttle.c                 | 52 +++++++++++++++++++++++++++++++++++++++++
> 6 files changed, 61 insertions(+), 87 deletions(-)
>
>diff --git a/blockdev.c b/blockdev.c
>index 56a6b24..9d33c25 100644
>--- a/blockdev.c
>+++ b/blockdev.c
>@@ -387,49 +387,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 49eebb5..0e6fb86 100644
>--- a/fsdev/qemu-fsdev-throttle.c
>+++ b/fsdev/qemu-fsdev-throttle.c
>@@ -16,6 +16,7 @@
> #include "qemu/error-report.h"
> #include "qemu-fsdev-throttle.h"
> #include "qemu/iov.h"
>+#include "qemu/throttle-options.h"
>
> static void fsdev_throttle_read_timer_cb(void *opaque)
> {
>@@ -31,48 +32,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/include/qemu/throttle-options.h b/include/qemu/throttle-options.h
>index 3528a8f..9709dcd 100644
>--- a/include/qemu/throttle-options.h
>+++ b/include/qemu/throttle-options.h
>@@ -9,6 +9,7 @@
>  */
> #ifndef THROTTLE_OPTIONS_H
> #define THROTTLE_OPTIONS_H
>+#include "typedefs.h"
>
> #define QEMU_OPT_IOPS_TOTAL "iops-total"
> #define QEMU_OPT_IOPS_TOTAL_MAX "iops-total-max"
>@@ -111,4 +112,6 @@
>             .help = "when limiting by iops max size of an I/O in bytes",\
>         }
>
>+void throttle_parse_options(ThrottleConfig *, QemuOpts *);
>+
> #endif
>diff --git a/include/qemu/throttle.h b/include/qemu/throttle.h
>index 8c93237..b6ebc6d 100644
>--- a/include/qemu/throttle.h
>+++ b/include/qemu/throttle.h
>@@ -89,10 +89,10 @@ typedef struct LeakyBucket {
>  * However it allows to keep the code clean and the bucket field is reset to
>  * zero at the right time.
>  */
>-typedef struct ThrottleConfig {
>+struct ThrottleConfig {
>     LeakyBucket buckets[BUCKETS_COUNT]; /* leaky buckets */
>     uint64_t op_size;         /* size of an operation in bytes */
>-} ThrottleConfig;
>+};
>
> typedef struct ThrottleState {
>     ThrottleConfig cfg;       /* configuration */
>diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
>index 39bc835..90fe0f9 100644
>--- a/include/qemu/typedefs.h
>+++ b/include/qemu/typedefs.h
>@@ -100,6 +100,7 @@ typedef struct uWireSlave uWireSlave;
> typedef struct VirtIODevice VirtIODevice;
> typedef struct Visitor Visitor;
> typedef struct node_info NodeInfo;
>+typedef struct ThrottleConfig ThrottleConfig;

Is this really needed? Just include include/qemu/throttle.h wherever you 
need.

> typedef void SaveStateHandler(QEMUFile *f, void *opaque);
> typedef int LoadStateHandler(QEMUFile *f, void *opaque, int version_id);
>
>diff --git a/util/throttle.c b/util/throttle.c
>index 06bf916..9ef28c4 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
>  *
>@@ -635,3 +636,54 @@ void throttle_config_to_limits(ThrottleConfig *cfg, ThrottleLimits *var)
>     var->has_iops_write_max_length = true;
>     var->has_iops_size = true;
> }
>+
>+/* 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);
>+}

You should reuse the #define's in include/qemu/throttle-options.h
See throttle_extract_options() in this patch: 
http://patchwork.ozlabs.org/patch/803919/ Disregard the UINT_MAX checks, 
unsigned ints in LeakyBucket were replaced by uint64_t.

Don't forget to drop the reviews when you change a patch! The original 
reviews will probably not be valid anymore.

Re: [Qemu-devel] [PATCH v11 1/6] throttle: factor out duplicate code

Posted by Pradeep Jagadeesh 8 years, 4 months ago

On 9/18/2017 6:20 PM, Manos Pitsidianakis wrote:
> On Thu, Sep 14, 2017 at 06:40:05AM -0400, Pradeep Jagadeesh wrote:
>> This patch factors out the duplicate throttle code that was still
>> present in block and fsdev devices.
>>
>> Signed-off-by: Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com>
>> Reviewed-by: Alberto Garcia <berto@igalia.com>
>> Reviewed-by: Greg Kurz <groug@kaod.org>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> ---
>> blockdev.c                      | 44 +---------------------------------
>> fsdev/qemu-fsdev-throttle.c     | 44 ++--------------------------------
>> include/qemu/throttle-options.h |  3 +++
>> include/qemu/throttle.h         |  4 ++--
>> include/qemu/typedefs.h         |  1 +
>> util/throttle.c                 | 52
>> +++++++++++++++++++++++++++++++++++++++++
>> 6 files changed, 61 insertions(+), 87 deletions(-)
>>
>> diff --git a/blockdev.c b/blockdev.c
>> index 56a6b24..9d33c25 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -387,49 +387,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 49eebb5..0e6fb86 100644
>> --- a/fsdev/qemu-fsdev-throttle.c
>> +++ b/fsdev/qemu-fsdev-throttle.c
>> @@ -16,6 +16,7 @@
>> #include "qemu/error-report.h"
>> #include "qemu-fsdev-throttle.h"
>> #include "qemu/iov.h"
>> +#include "qemu/throttle-options.h"
>>
>> static void fsdev_throttle_read_timer_cb(void *opaque)
>> {
>> @@ -31,48 +32,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/include/qemu/throttle-options.h
>> b/include/qemu/throttle-options.h
>> index 3528a8f..9709dcd 100644
>> --- a/include/qemu/throttle-options.h
>> +++ b/include/qemu/throttle-options.h
>> @@ -9,6 +9,7 @@
>>  */
>> #ifndef THROTTLE_OPTIONS_H
>> #define THROTTLE_OPTIONS_H
>> +#include "typedefs.h"
>>
>> #define QEMU_OPT_IOPS_TOTAL "iops-total"
>> #define QEMU_OPT_IOPS_TOTAL_MAX "iops-total-max"
>> @@ -111,4 +112,6 @@
>>             .help = "when limiting by iops max size of an I/O in bytes",\
>>         }
>>
>> +void throttle_parse_options(ThrottleConfig *, QemuOpts *);
>> +
>> #endif
>> diff --git a/include/qemu/throttle.h b/include/qemu/throttle.h
>> index 8c93237..b6ebc6d 100644
>> --- a/include/qemu/throttle.h
>> +++ b/include/qemu/throttle.h
>> @@ -89,10 +89,10 @@ typedef struct LeakyBucket {
>>  * However it allows to keep the code clean and the bucket field is
>> reset to
>>  * zero at the right time.
>>  */
>> -typedef struct ThrottleConfig {
>> +struct ThrottleConfig {
>>     LeakyBucket buckets[BUCKETS_COUNT]; /* leaky buckets */
>>     uint64_t op_size;         /* size of an operation in bytes */
>> -} ThrottleConfig;
>> +};
>>
>> typedef struct ThrottleState {
>>     ThrottleConfig cfg;       /* configuration */
>> diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
>> index 39bc835..90fe0f9 100644
>> --- a/include/qemu/typedefs.h
>> +++ b/include/qemu/typedefs.h
>> @@ -100,6 +100,7 @@ typedef struct uWireSlave uWireSlave;
>> typedef struct VirtIODevice VirtIODevice;
>> typedef struct Visitor Visitor;
>> typedef struct node_info NodeInfo;
>> +typedef struct ThrottleConfig ThrottleConfig;
>
> Is this really needed? Just include include/qemu/throttle.h wherever you
> need.
>
>> typedef void SaveStateHandler(QEMUFile *f, void *opaque);
>> typedef int LoadStateHandler(QEMUFile *f, void *opaque, int version_id);
>>
>> diff --git a/util/throttle.c b/util/throttle.c
>> index 06bf916..9ef28c4 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
>>  *
>> @@ -635,3 +636,54 @@ void throttle_config_to_limits(ThrottleConfig
>> *cfg, ThrottleLimits *var)
>>     var->has_iops_write_max_length = true;
>>     var->has_iops_size = true;
>> }
>> +
>> +/* 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);
>> +}
>
> You should reuse the #define's in include/qemu/throttle-options.h
> See throttle_extract_options() in this patch:
> http://patchwork.ozlabs.org/patch/803919/ Disregard the UINT_MAX checks,
> unsigned ints in LeakyBucket were replaced by uint64_t.
You mean something like below?
  qemu_opt_get_number(opts, QEMU_OPT_IOPS_READ, 0);
instead
qemu_opt_get_number(opts, "throttling.bps-total", 0);

Regards,
Pradeep


> Don't forget to drop the reviews when you change a patch! The original
> reviews will probably not be valid anymore.

Re: [Qemu-devel] [PATCH v11 1/6] throttle: factor out duplicate code

Posted by Pradeep Jagadeesh 8 years, 4 months ago

On 9/18/2017 6:20 PM, Manos Pitsidianakis wrote:
> On Thu, Sep 14, 2017 at 06:40:05AM -0400, Pradeep Jagadeesh wrote:
>> This patch factors out the duplicate throttle code that was still
>> present in block and fsdev devices.
>>
>> Signed-off-by: Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com>
>> Reviewed-by: Alberto Garcia <berto@igalia.com>
>> Reviewed-by: Greg Kurz <groug@kaod.org>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> ---
>> blockdev.c                      | 44 +---------------------------------
>> fsdev/qemu-fsdev-throttle.c     | 44 ++--------------------------------
>> include/qemu/throttle-options.h |  3 +++
>> include/qemu/throttle.h         |  4 ++--
>> include/qemu/typedefs.h         |  1 +
>> util/throttle.c                 | 52
>> +++++++++++++++++++++++++++++++++++++++++
>> 6 files changed, 61 insertions(+), 87 deletions(-)
>>
>> diff --git a/blockdev.c b/blockdev.c
>> index 56a6b24..9d33c25 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -387,49 +387,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 49eebb5..0e6fb86 100644
>> --- a/fsdev/qemu-fsdev-throttle.c
>> +++ b/fsdev/qemu-fsdev-throttle.c
>> @@ -16,6 +16,7 @@
>> #include "qemu/error-report.h"
>> #include "qemu-fsdev-throttle.h"
>> #include "qemu/iov.h"
>> +#include "qemu/throttle-options.h"
>>
>> static void fsdev_throttle_read_timer_cb(void *opaque)
>> {
>> @@ -31,48 +32,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/include/qemu/throttle-options.h
>> b/include/qemu/throttle-options.h
>> index 3528a8f..9709dcd 100644
>> --- a/include/qemu/throttle-options.h
>> +++ b/include/qemu/throttle-options.h
>> @@ -9,6 +9,7 @@
>>  */
>> #ifndef THROTTLE_OPTIONS_H
>> #define THROTTLE_OPTIONS_H
>> +#include "typedefs.h"
>>
>> #define QEMU_OPT_IOPS_TOTAL "iops-total"
>> #define QEMU_OPT_IOPS_TOTAL_MAX "iops-total-max"
>> @@ -111,4 +112,6 @@
>>             .help = "when limiting by iops max size of an I/O in bytes",\
>>         }
>>
>> +void throttle_parse_options(ThrottleConfig *, QemuOpts *);
>> +
>> #endif
>> diff --git a/include/qemu/throttle.h b/include/qemu/throttle.h
>> index 8c93237..b6ebc6d 100644
>> --- a/include/qemu/throttle.h
>> +++ b/include/qemu/throttle.h
>> @@ -89,10 +89,10 @@ typedef struct LeakyBucket {
>>  * However it allows to keep the code clean and the bucket field is
>> reset to
>>  * zero at the right time.
>>  */
>> -typedef struct ThrottleConfig {
>> +struct ThrottleConfig {
>>     LeakyBucket buckets[BUCKETS_COUNT]; /* leaky buckets */
>>     uint64_t op_size;         /* size of an operation in bytes */
>> -} ThrottleConfig;
>> +};
>>
>> typedef struct ThrottleState {
>>     ThrottleConfig cfg;       /* configuration */
>> diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
>> index 39bc835..90fe0f9 100644
>> --- a/include/qemu/typedefs.h
>> +++ b/include/qemu/typedefs.h
>> @@ -100,6 +100,7 @@ typedef struct uWireSlave uWireSlave;
>> typedef struct VirtIODevice VirtIODevice;
>> typedef struct Visitor Visitor;
>> typedef struct node_info NodeInfo;
>> +typedef struct ThrottleConfig ThrottleConfig;
>
> Is this really needed? Just include include/qemu/throttle.h wherever you
> need.
>
>> typedef void SaveStateHandler(QEMUFile *f, void *opaque);
>> typedef int LoadStateHandler(QEMUFile *f, void *opaque, int version_id);
>>
>> diff --git a/util/throttle.c b/util/throttle.c
>> index 06bf916..9ef28c4 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
>>  *
>> @@ -635,3 +636,54 @@ void throttle_config_to_limits(ThrottleConfig
>> *cfg, ThrottleLimits *var)
>>     var->has_iops_write_max_length = true;
>>     var->has_iops_size = true;
>> }
>> +
>> +/* 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);
>> +}
>
> You should reuse the #define's in include/qemu/throttle-options.h
> See throttle_extract_options() in this patch:
> http://patchwork.ozlabs.org/patch/803919/ Disregard the UINT_MAX checks,
> unsigned ints in LeakyBucket were replaced by uint64_t.
>
I can not use the #define's because I use throttling.*.
In my last patch also we wanted to have it like that to align with the 
block device throttle options.
If you see in blockdev.c, still there exists throttling.*

There may be change required every where, so would like to do it as part 
of another patch.

-Pradeep
> Don't forget to drop the reviews when you change a patch! The original
> reviews will probably not be valid anymore.

Re: [Qemu-devel] [PATCH v11 1/6] throttle: factor out duplicate code

Posted by Manos Pitsidianakis 8 years, 4 months ago

On Fri, Sep 22, 2017 at 01:31:58PM +0200, Pradeep Jagadeesh wrote:
>On 9/18/2017 6:20 PM, Manos Pitsidianakis wrote:
>>On Thu, Sep 14, 2017 at 06:40:05AM -0400, Pradeep Jagadeesh wrote:
>>>This patch factors out the duplicate throttle code that was still
>>>present in block and fsdev devices.
>>>
>>>Signed-off-by: Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com>
>>>Reviewed-by: Alberto Garcia <berto@igalia.com>
>>>Reviewed-by: Greg Kurz <groug@kaod.org>
>>>Reviewed-by: Eric Blake <eblake@redhat.com>
>>>---
>>>blockdev.c                      | 44 +---------------------------------
>>>fsdev/qemu-fsdev-throttle.c     | 44 ++--------------------------------
>>>include/qemu/throttle-options.h |  3 +++
>>>include/qemu/throttle.h         |  4 ++--
>>>include/qemu/typedefs.h         |  1 +
>>>util/throttle.c                 | 52
>>>+++++++++++++++++++++++++++++++++++++++++
>>>6 files changed, 61 insertions(+), 87 deletions(-)
>>>
>>>diff --git a/blockdev.c b/blockdev.c
>>>index 56a6b24..9d33c25 100644
>>>--- a/blockdev.c
>>>+++ b/blockdev.c
>>>@@ -387,49 +387,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 49eebb5..0e6fb86 100644
>>>--- a/fsdev/qemu-fsdev-throttle.c
>>>+++ b/fsdev/qemu-fsdev-throttle.c
>>>@@ -16,6 +16,7 @@
>>>#include "qemu/error-report.h"
>>>#include "qemu-fsdev-throttle.h"
>>>#include "qemu/iov.h"
>>>+#include "qemu/throttle-options.h"
>>>
>>>static void fsdev_throttle_read_timer_cb(void *opaque)
>>>{
>>>@@ -31,48 +32,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/include/qemu/throttle-options.h
>>>b/include/qemu/throttle-options.h
>>>index 3528a8f..9709dcd 100644
>>>--- a/include/qemu/throttle-options.h
>>>+++ b/include/qemu/throttle-options.h
>>>@@ -9,6 +9,7 @@
>>> */
>>>#ifndef THROTTLE_OPTIONS_H
>>>#define THROTTLE_OPTIONS_H
>>>+#include "typedefs.h"
>>>
>>>#define QEMU_OPT_IOPS_TOTAL "iops-total"
>>>#define QEMU_OPT_IOPS_TOTAL_MAX "iops-total-max"
>>>@@ -111,4 +112,6 @@
>>>            .help = "when limiting by iops max size of an I/O in bytes",\
>>>        }
>>>
>>>+void throttle_parse_options(ThrottleConfig *, QemuOpts *);
>>>+
>>>#endif
>>>diff --git a/include/qemu/throttle.h b/include/qemu/throttle.h
>>>index 8c93237..b6ebc6d 100644
>>>--- a/include/qemu/throttle.h
>>>+++ b/include/qemu/throttle.h
>>>@@ -89,10 +89,10 @@ typedef struct LeakyBucket {
>>> * However it allows to keep the code clean and the bucket field is
>>>reset to
>>> * zero at the right time.
>>> */
>>>-typedef struct ThrottleConfig {
>>>+struct ThrottleConfig {
>>>    LeakyBucket buckets[BUCKETS_COUNT]; /* leaky buckets */
>>>    uint64_t op_size;         /* size of an operation in bytes */
>>>-} ThrottleConfig;
>>>+};
>>>
>>>typedef struct ThrottleState {
>>>    ThrottleConfig cfg;       /* configuration */
>>>diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
>>>index 39bc835..90fe0f9 100644
>>>--- a/include/qemu/typedefs.h
>>>+++ b/include/qemu/typedefs.h
>>>@@ -100,6 +100,7 @@ typedef struct uWireSlave uWireSlave;
>>>typedef struct VirtIODevice VirtIODevice;
>>>typedef struct Visitor Visitor;
>>>typedef struct node_info NodeInfo;
>>>+typedef struct ThrottleConfig ThrottleConfig;
>>
>>Is this really needed? Just include include/qemu/throttle.h wherever you
>>need.
>>
>>>typedef void SaveStateHandler(QEMUFile *f, void *opaque);
>>>typedef int LoadStateHandler(QEMUFile *f, void *opaque, int version_id);
>>>
>>>diff --git a/util/throttle.c b/util/throttle.c
>>>index 06bf916..9ef28c4 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
>>> *
>>>@@ -635,3 +636,54 @@ void throttle_config_to_limits(ThrottleConfig
>>>*cfg, ThrottleLimits *var)
>>>    var->has_iops_write_max_length = true;
>>>    var->has_iops_size = true;
>>>}
>>>+
>>>+/* 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);
>>>+}
>>
>>You should reuse the #define's in include/qemu/throttle-options.h
>>See throttle_extract_options() in this patch:
>>http://patchwork.ozlabs.org/patch/803919/ Disregard the UINT_MAX checks,
>>unsigned ints in LeakyBucket were replaced by uint64_t.
>>
>I can not use the #define's because I use throttling.*.
>In my last patch also we wanted to have it like that to align with the 
>block device throttle options.
>If you see in blockdev.c, still there exists throttling.*
>

You can use them.

qemu_opt_get_number(opts, "throttling.iops-size", 0) becomes:
qemu_opt_get_number(opts, THROTTLE_OPT_PREFIX QEMU_OPT_IOPS_SIZE, 0) 

This is what I did in the patch I linked, except for redefining 
THROTTLE_OPT_PREFIX to "limits.". I plan to send some patches for 
blockdev.c code when the old legacy interface is replaced.

Re: [Qemu-devel] [PATCH v11 1/6] throttle: factor out duplicate code

Posted by Pradeep Jagadeesh 8 years, 4 months ago

On 9/23/2017 12:33 PM, Manos Pitsidianakis wrote:
> On Fri, Sep 22, 2017 at 01:31:58PM +0200, Pradeep Jagadeesh wrote:
>> On 9/18/2017 6:20 PM, Manos Pitsidianakis wrote:
>>> On Thu, Sep 14, 2017 at 06:40:05AM -0400, Pradeep Jagadeesh wrote:
>>>> This patch factors out the duplicate throttle code that was still
>>>> present in block and fsdev devices.
>>>>
>>>> Signed-off-by: Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com>
>>>> Reviewed-by: Alberto Garcia <berto@igalia.com>
>>>> Reviewed-by: Greg Kurz <groug@kaod.org>
>>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>>> ---
>>>> blockdev.c                      | 44 +---------------------------------
>>>> fsdev/qemu-fsdev-throttle.c     | 44 ++--------------------------------
>>>> include/qemu/throttle-options.h |  3 +++
>>>> include/qemu/throttle.h         |  4 ++--
>>>> include/qemu/typedefs.h         |  1 +
>>>> util/throttle.c                 | 52
>>>> +++++++++++++++++++++++++++++++++++++++++
>>>> 6 files changed, 61 insertions(+), 87 deletions(-)
>>>>
>>>> diff --git a/blockdev.c b/blockdev.c
>>>> index 56a6b24..9d33c25 100644
>>>> --- a/blockdev.c
>>>> +++ b/blockdev.c
>>>> @@ -387,49 +387,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 49eebb5..0e6fb86 100644
>>>> --- a/fsdev/qemu-fsdev-throttle.c
>>>> +++ b/fsdev/qemu-fsdev-throttle.c
>>>> @@ -16,6 +16,7 @@
>>>> #include "qemu/error-report.h"
>>>> #include "qemu-fsdev-throttle.h"
>>>> #include "qemu/iov.h"
>>>> +#include "qemu/throttle-options.h"
>>>>
>>>> static void fsdev_throttle_read_timer_cb(void *opaque)
>>>> {
>>>> @@ -31,48 +32,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/include/qemu/throttle-options.h
>>>> b/include/qemu/throttle-options.h
>>>> index 3528a8f..9709dcd 100644
>>>> --- a/include/qemu/throttle-options.h
>>>> +++ b/include/qemu/throttle-options.h
>>>> @@ -9,6 +9,7 @@
>>>> */
>>>> #ifndef THROTTLE_OPTIONS_H
>>>> #define THROTTLE_OPTIONS_H
>>>> +#include "typedefs.h"
>>>>
>>>> #define QEMU_OPT_IOPS_TOTAL "iops-total"
>>>> #define QEMU_OPT_IOPS_TOTAL_MAX "iops-total-max"
>>>> @@ -111,4 +112,6 @@
>>>>            .help = "when limiting by iops max size of an I/O in
>>>> bytes",\
>>>>        }
>>>>
>>>> +void throttle_parse_options(ThrottleConfig *, QemuOpts *);
>>>> +
>>>> #endif
>>>> diff --git a/include/qemu/throttle.h b/include/qemu/throttle.h
>>>> index 8c93237..b6ebc6d 100644
>>>> --- a/include/qemu/throttle.h
>>>> +++ b/include/qemu/throttle.h
>>>> @@ -89,10 +89,10 @@ typedef struct LeakyBucket {
>>>> * However it allows to keep the code clean and the bucket field is
>>>> reset to
>>>> * zero at the right time.
>>>> */
>>>> -typedef struct ThrottleConfig {
>>>> +struct ThrottleConfig {
>>>>    LeakyBucket buckets[BUCKETS_COUNT]; /* leaky buckets */
>>>>    uint64_t op_size;         /* size of an operation in bytes */
>>>> -} ThrottleConfig;
>>>> +};
>>>>
>>>> typedef struct ThrottleState {
>>>>    ThrottleConfig cfg;       /* configuration */
>>>> diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
>>>> index 39bc835..90fe0f9 100644
>>>> --- a/include/qemu/typedefs.h
>>>> +++ b/include/qemu/typedefs.h
>>>> @@ -100,6 +100,7 @@ typedef struct uWireSlave uWireSlave;
>>>> typedef struct VirtIODevice VirtIODevice;
>>>> typedef struct Visitor Visitor;
>>>> typedef struct node_info NodeInfo;
>>>> +typedef struct ThrottleConfig ThrottleConfig;
>>>
>>> Is this really needed? Just include include/qemu/throttle.h wherever you
>>> need.
>>>
>>>> typedef void SaveStateHandler(QEMUFile *f, void *opaque);
>>>> typedef int LoadStateHandler(QEMUFile *f, void *opaque, int
>>>> version_id);
>>>>
>>>> diff --git a/util/throttle.c b/util/throttle.c
>>>> index 06bf916..9ef28c4 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
>>>> *
>>>> @@ -635,3 +636,54 @@ void throttle_config_to_limits(ThrottleConfig
>>>> *cfg, ThrottleLimits *var)
>>>>    var->has_iops_write_max_length = true;
>>>>    var->has_iops_size = true;
>>>> }
>>>> +
>>>> +/* 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);
>>>> +}
>>>
>>> You should reuse the #define's in include/qemu/throttle-options.h
>>> See throttle_extract_options() in this patch:
>>> http://patchwork.ozlabs.org/patch/803919/ Disregard the UINT_MAX checks,
>>> unsigned ints in LeakyBucket were replaced by uint64_t.
>>>
>> I can not use the #define's because I use throttling.*.
>> In my last patch also we wanted to have it like that to align with the
>> block device throttle options.
>> If you see in blockdev.c, still there exists throttling.*
>>
>
> You can use them.
>
> qemu_opt_get_number(opts, "throttling.iops-size", 0) becomes:
> qemu_opt_get_number(opts, THROTTLE_OPT_PREFIX QEMU_OPT_IOPS_SIZE, 0)
> This is what I did in the patch I linked, except for redefining
> THROTTLE_OPT_PREFIX to "limits.". I plan to send some patches for
> blockdev.c code when the old legacy interface is replaced.
OK, I can do sendout a patch for blockdev.c also.

Regards,
Pradeep

[Qemu-devel] [PATCH v11 1/6] throttle: factor out duplicate code
[Qemu-devel] [PATCH v11 2/6] qmp: Use ThrottleLimits structure
[Qemu-devel] [PATCH v11 3/6] qmp: factor out throttle code to reuse code
[Qemu-devel] [PATCH v11 4/6] hmp: create a throttle initialization function for code reuse
[Qemu-devel] [PATCH v11 5/6] fsdev: QMP interface for throttling
[Qemu-devel] [PATCH v11 6/6] fsdev: hmp interface for throttling