[PATCH RFC v3 05/16] qemu: monitor: Add support for ThrottleGroup operations

wucf@linux.ibm.com posted 16 patches 3 months ago
There is a newer version of this series
[PATCH RFC v3 05/16] qemu: monitor: Add support for ThrottleGroup operations
Posted by wucf@linux.ibm.com 3 months ago
From: Chun Feng Wu <wucf@linux.ibm.com>

* ThrottleGroup is updated through "qemuMonitorJSONUpdateThrottleGroup"
* ThrottleGroup is retrieved through "qemuMonitorJSONGetThrottleGroup"
* ThrottleGroup is deleted by reusing "qemuMonitorDelObject"
* ThrottleGroup is added by reusing "qemuMonitorAddObject"
* "qemuMonitorMakeThrottleGroupLimits" will be used by building qemu cmd as well

Signed-off-by: Chun Feng Wu <wucf@linux.ibm.com>
---
 src/qemu/qemu_monitor.c      |  34 ++++++++
 src/qemu/qemu_monitor.h      |  14 ++++
 src/qemu/qemu_monitor_json.c | 150 +++++++++++++++++++++++++++++++++++
 src/qemu/qemu_monitor_json.h |  14 ++++
 4 files changed, 212 insertions(+)

diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index 34e2ccab97..2f067ab5d6 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -2996,6 +2996,40 @@ qemuMonitorGetBlockIoThrottle(qemuMonitor *mon,
 }
 
 
+int
+qemuMonitorThrottleGroupLimits(virJSONValue *limits,
+                               const virDomainThrottleGroupDef *group)
+{
+    return qemuMonitorMakeThrottleGroupLimits(limits, group);
+}
+
+
+int
+qemuMonitorUpdateThrottleGroup(qemuMonitor *mon,
+                               const char *qomid,
+                               virDomainBlockIoTuneInfo *info)
+{
+    VIR_DEBUG("qomid=%s, info=%p", qomid, info);
+
+    QEMU_CHECK_MONITOR(mon);
+
+    return qemuMonitorJSONUpdateThrottleGroup(mon, qomid, info);
+}
+
+
+int
+qemuMonitorGetThrottleGroup(qemuMonitor *mon,
+                            const char *groupname,
+                            virDomainBlockIoTuneInfo *reply)
+{
+    VIR_DEBUG("throttle-group=%s, reply=%p", groupname, reply);
+
+    QEMU_CHECK_MONITOR(mon);
+
+    return qemuMonitorJSONGetThrottleGroup(mon, groupname, reply);
+}
+
+
 int
 qemuMonitorVMStatusToPausedReason(const char *status)
 {
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
index b78f539c85..6474ce124c 100644
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -1062,6 +1062,20 @@ int qemuMonitorGetBlockIoThrottle(qemuMonitor *mon,
                                   const char *qdevid,
                                   virDomainBlockIoTuneInfo *reply);
 
+int
+qemuMonitorThrottleGroupLimits(virJSONValue *limits,
+                               const virDomainThrottleGroupDef *group);
+
+int
+qemuMonitorUpdateThrottleGroup(qemuMonitor *mon,
+                               const char *qomid,
+                               virDomainBlockIoTuneInfo *info);
+
+int
+qemuMonitorGetThrottleGroup(qemuMonitor *mon,
+                            const char *groupname,
+                            virDomainBlockIoTuneInfo *reply);
+
 int qemuMonitorSystemWakeup(qemuMonitor *mon);
 
 int qemuMonitorGetVersion(qemuMonitor *mon,
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index c5e758e7f8..462b40cb6b 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -4633,6 +4633,156 @@ int qemuMonitorJSONGetBlockIoThrottle(qemuMonitor *mon,
     return qemuMonitorJSONBlockIoThrottleInfo(devices, qdevid, reply);
 }
 
+
+int
+qemuMonitorMakeThrottleGroupLimits(virJSONValue *limits,
+                                   const virDomainThrottleGroupDef *group)
+{
+    if (virJSONValueObjectAdd(&limits,
+                              "P:bps-total",
+                              group->total_bytes_sec,
+                              "P:bps-read",
+                              group->read_bytes_sec,
+                              "P:bps-write",
+                              group->write_bytes_sec,
+                              "P:iops-total",
+                              group->total_iops_sec,
+                              "P:iops-read",
+                              group->read_iops_sec,
+                              "P:iops-write",
+                              group->write_iops_sec,
+                              "P:bps-total-max",
+                              group->total_bytes_sec_max,
+                              "P:bps-read-max",
+                              group->read_bytes_sec_max,
+                              "P:bps-write-max",
+                              group->write_bytes_sec_max,
+                              "P:iops-total-max",
+                              group->total_iops_sec_max,
+                              "P:iops-read-max",
+                              group->read_iops_sec_max,
+                              "P:iops-write-max",
+                              group->write_iops_sec_max,
+                              "P:iops-size",
+                              group->size_iops_sec,
+                              /* avoid error from QEMU: "the burst length cannot be 0" for throttlelimits
+                              * when setting max-length
+                              */
+                              "P:bps-total-max-length",
+                              group->total_bytes_sec_max_length,
+                              "P:bps-read-max-length",
+                              group->read_bytes_sec_max_length,
+                              "P:bps-write-max-length",
+                              group->write_bytes_sec_max_length,
+                              "P:iops-total-max-length",
+                              group->total_iops_sec_max_length,
+                              "P:iops-read-max-length",
+                              group->read_iops_sec_max_length,
+                              "P:iops-write-max-length",
+                              group->write_iops_sec_max_length,
+                              NULL) < 0)
+        return -1;
+
+    return 0;
+}
+
+
+int
+qemuMonitorJSONUpdateThrottleGroup(qemuMonitor *mon,
+                                   const char *qomid,
+                                   virDomainBlockIoTuneInfo *info)
+{
+    g_autoptr(virJSONValue) cmd = NULL;
+    g_autoptr(virJSONValue) result = NULL;
+    g_autoptr(virJSONValue) limits = virJSONValueNewObject();
+
+    if (qemuMonitorMakeThrottleGroupLimits(limits, info)<0)
+        return -1;
+
+    if (!(cmd = qemuMonitorJSONMakeCommand("qom-set",
+                                           "s:property", "limits",
+                                           "s:path", qomid,
+                                           "a:value", &limits,
+                                           NULL)))
+        return -1;
+
+    if (qemuMonitorJSONCommand(mon, cmd, &result) < 0)
+        return -1;
+
+    if (qemuMonitorJSONCheckError(cmd, result) < 0)
+        return -1;
+
+    return 0;
+}
+
+
+#define GET_THROTTLE_GROUP_VALUE(FIELD, STORE) \
+    if (virJSONValueObjectHasKey(ret, FIELD)) { \
+        if (virJSONValueObjectGetNumberUlong(ret, FIELD, &reply->STORE) < 0) { \
+            virReportError(VIR_ERR_OPERATION_UNSUPPORTED, \
+                       _("throttle group field '%1$s' missing in qemu's output"), \
+                       #STORE); \
+            return -1; \
+        } \
+    }
+
+
+int
+qemuMonitorJSONGetThrottleGroup(qemuMonitor *mon,
+                                const char *gname,
+                                virDomainBlockIoTuneInfo *reply)
+{
+    g_autoptr(virJSONValue) cmd = NULL;
+    g_autoptr(virJSONValue) result = NULL;
+    g_autofree char *groupCopy = NULL;
+    virJSONValue *ret;
+
+    g_autofree char *path = g_strdup_printf("/objects/%s", gname);
+    if (!(cmd = qemuMonitorJSONMakeCommand("qom-get",
+                                           "s:path", path,
+                                           "s:property", "limits",
+                                           NULL)))
+        return -1;
+
+    if (qemuMonitorJSONCommand(mon, cmd, &result) < 0)
+        return -1;
+
+    if (qemuMonitorJSONCheckError(cmd, result) < 0)
+        return -1;
+
+    if (!(ret = qemuMonitorJSONGetReply(cmd, result, VIR_JSON_TYPE_OBJECT)))
+        return -1;
+
+    GET_THROTTLE_GROUP_VALUE("bps-total", total_bytes_sec);
+    GET_THROTTLE_GROUP_VALUE("bps-read", read_bytes_sec);
+    GET_THROTTLE_GROUP_VALUE("bps-write", write_bytes_sec);
+    GET_THROTTLE_GROUP_VALUE("iops-total", total_iops_sec);
+    GET_THROTTLE_GROUP_VALUE("iops-read", read_iops_sec);
+    GET_THROTTLE_GROUP_VALUE("iops-write", write_iops_sec);
+
+    GET_THROTTLE_GROUP_VALUE("bps-total-max", total_bytes_sec_max);
+    GET_THROTTLE_GROUP_VALUE("bps-read-max", read_bytes_sec_max);
+    GET_THROTTLE_GROUP_VALUE("bps-write-max", write_bytes_sec_max);
+    GET_THROTTLE_GROUP_VALUE("iops-total-max", total_iops_sec_max);
+    GET_THROTTLE_GROUP_VALUE("iops-read-max", read_iops_sec_max);
+    GET_THROTTLE_GROUP_VALUE("iops-write-max", write_iops_sec_max);
+    GET_THROTTLE_GROUP_VALUE("iops-size", size_iops_sec);
+
+    GET_THROTTLE_GROUP_VALUE("bps-total-max-length", total_bytes_sec_max_length);
+    GET_THROTTLE_GROUP_VALUE("bps-read-max-length", read_bytes_sec_max_length);
+    GET_THROTTLE_GROUP_VALUE("bps-write-max-length", write_bytes_sec_max_length);
+    GET_THROTTLE_GROUP_VALUE("iops-total-max-length", total_iops_sec_max_length);
+    GET_THROTTLE_GROUP_VALUE("iops-read-max-length", read_iops_sec_max_length);
+    GET_THROTTLE_GROUP_VALUE("iops-write-max-length", write_iops_sec_max_length);
+
+    groupCopy = g_strdup(gname);
+    reply->group_name = g_steal_pointer(&groupCopy);
+
+    return 0;
+}
+#undef GET_THROTTLE_GROUP_VALUE
+
+
 int qemuMonitorJSONSystemWakeup(qemuMonitor *mon)
 {
     g_autoptr(virJSONValue) cmd = NULL;
diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h
index 9684660d86..fa3138bbf9 100644
--- a/src/qemu/qemu_monitor_json.h
+++ b/src/qemu/qemu_monitor_json.h
@@ -404,6 +404,20 @@ qemuMonitorJSONSetBlockIoThrottle(qemuMonitor *mon,
                                   const char *qomid,
                                   virDomainBlockIoTuneInfo *info);
 
+int
+qemuMonitorMakeThrottleGroupLimits(virJSONValue *limits,
+                                   const virDomainThrottleGroupDef *group);
+
+int
+qemuMonitorJSONUpdateThrottleGroup(qemuMonitor *mon,
+                                   const char *qomid,
+                                   virDomainBlockIoTuneInfo *info);
+
+int
+qemuMonitorJSONGetThrottleGroup(qemuMonitor *mon,
+                                const char *gname,
+                                virDomainBlockIoTuneInfo *reply);
+
 int
 qemuMonitorJSONGetBlockIoThrottle(qemuMonitor *mon,
                                   const char *qdevid,
-- 
2.34.1
Re: [PATCH RFC v3 05/16] qemu: monitor: Add support for ThrottleGroup operations
Posted by Peter Krempa 1 month, 3 weeks ago
On Wed, Jun 12, 2024 at 03:02:13 -0700, wucf@linux.ibm.com wrote:
> From: Chun Feng Wu <wucf@linux.ibm.com>
> 
> * ThrottleGroup is updated through "qemuMonitorJSONUpdateThrottleGroup"
> * ThrottleGroup is retrieved through "qemuMonitorJSONGetThrottleGroup"
> * ThrottleGroup is deleted by reusing "qemuMonitorDelObject"
> * ThrottleGroup is added by reusing "qemuMonitorAddObject"
> * "qemuMonitorMakeThrottleGroupLimits" will be used by building qemu cmd as well
> 
> Signed-off-by: Chun Feng Wu <wucf@linux.ibm.com>
> ---
>  src/qemu/qemu_monitor.c      |  34 ++++++++
>  src/qemu/qemu_monitor.h      |  14 ++++
>  src/qemu/qemu_monitor_json.c | 150 +++++++++++++++++++++++++++++++++++
>  src/qemu/qemu_monitor_json.h |  14 ++++
>  4 files changed, 212 insertions(+)
> 
> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
> index 34e2ccab97..2f067ab5d6 100644
> --- a/src/qemu/qemu_monitor.c
> +++ b/src/qemu/qemu_monitor.c
> @@ -2996,6 +2996,40 @@ qemuMonitorGetBlockIoThrottle(qemuMonitor *mon,
>  }
>  
>  
> +int
> +qemuMonitorThrottleGroupLimits(virJSONValue *limits,
> +                               const virDomainThrottleGroupDef *group)
> +{
> +    return qemuMonitorMakeThrottleGroupLimits(limits, group);
> +}
> +
> +
> +int
> +qemuMonitorUpdateThrottleGroup(qemuMonitor *mon,
> +                               const char *qomid,
> +                               virDomainBlockIoTuneInfo *info)
> +{
> +    VIR_DEBUG("qomid=%s, info=%p", qomid, info);
> +
> +    QEMU_CHECK_MONITOR(mon);
> +
> +    return qemuMonitorJSONUpdateThrottleGroup(mon, qomid, info);
> +}
> +
> +
> +int
> +qemuMonitorGetThrottleGroup(qemuMonitor *mon,
> +                            const char *groupname,
> +                            virDomainBlockIoTuneInfo *reply)
> +{
> +    VIR_DEBUG("throttle-group=%s, reply=%p", groupname, reply);
> +
> +    QEMU_CHECK_MONITOR(mon);
> +
> +    return qemuMonitorJSONGetThrottleGroup(mon, groupname, reply);
> +}
> +
> +
>  int
>  qemuMonitorVMStatusToPausedReason(const char *status)
>  {
> diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
> index b78f539c85..6474ce124c 100644
> --- a/src/qemu/qemu_monitor.h
> +++ b/src/qemu/qemu_monitor.h
> @@ -1062,6 +1062,20 @@ int qemuMonitorGetBlockIoThrottle(qemuMonitor *mon,
>                                    const char *qdevid,
>                                    virDomainBlockIoTuneInfo *reply);
>  
> +int
> +qemuMonitorThrottleGroupLimits(virJSONValue *limits,
> +                               const virDomainThrottleGroupDef *group);
> +
> +int
> +qemuMonitorUpdateThrottleGroup(qemuMonitor *mon,
> +                               const char *qomid,
> +                               virDomainBlockIoTuneInfo *info);
> +
> +int
> +qemuMonitorGetThrottleGroup(qemuMonitor *mon,
> +                            const char *groupname,
> +                            virDomainBlockIoTuneInfo *reply);
> +
>  int qemuMonitorSystemWakeup(qemuMonitor *mon);
>  
>  int qemuMonitorGetVersion(qemuMonitor *mon,
> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> index c5e758e7f8..462b40cb6b 100644
> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c
> @@ -4633,6 +4633,156 @@ int qemuMonitorJSONGetBlockIoThrottle(qemuMonitor *mon,
>      return qemuMonitorJSONBlockIoThrottleInfo(devices, qdevid, reply);
>  }
>  
> +
> +int
> +qemuMonitorMakeThrottleGroupLimits(virJSONValue *limits,
> +                                   const virDomainThrottleGroupDef *group)
> +{
> +    if (virJSONValueObjectAdd(&limits,
> +                              "P:bps-total",
> +                              group->total_bytes_sec,
> +                              "P:bps-read",
> +                              group->read_bytes_sec,
> +                              "P:bps-write",
> +                              group->write_bytes_sec,
> +                              "P:iops-total",
> +                              group->total_iops_sec,
> +                              "P:iops-read",
> +                              group->read_iops_sec,
> +                              "P:iops-write",
> +                              group->write_iops_sec,
> +                              "P:bps-total-max",
> +                              group->total_bytes_sec_max,
> +                              "P:bps-read-max",
> +                              group->read_bytes_sec_max,
> +                              "P:bps-write-max",
> +                              group->write_bytes_sec_max,
> +                              "P:iops-total-max",
> +                              group->total_iops_sec_max,
> +                              "P:iops-read-max",
> +                              group->read_iops_sec_max,
> +                              "P:iops-write-max",
> +                              group->write_iops_sec_max,
> +                              "P:iops-size",
> +                              group->size_iops_sec,
> +                              /* avoid error from QEMU: "the burst length cannot be 0" for throttlelimits
> +                              * when setting max-length
> +                              */
> +                              "P:bps-total-max-length",
> +                              group->total_bytes_sec_max_length,
> +                              "P:bps-read-max-length",
> +                              group->read_bytes_sec_max_length,
> +                              "P:bps-write-max-length",
> +                              group->write_bytes_sec_max_length,
> +                              "P:iops-total-max-length",
> +                              group->total_iops_sec_max_length,
> +                              "P:iops-read-max-length",
> +                              group->read_iops_sec_max_length,
> +                              "P:iops-write-max-length",
> +                              group->write_iops_sec_max_length,
> +                              NULL) < 0)
> +        return -1;
> +
> +    return 0;
> +}
> +
> +
> +int
> +qemuMonitorJSONUpdateThrottleGroup(qemuMonitor *mon,
> +                                   const char *qomid,
> +                                   virDomainBlockIoTuneInfo *info)
> +{
> +    g_autoptr(virJSONValue) cmd = NULL;
> +    g_autoptr(virJSONValue) result = NULL;
> +    g_autoptr(virJSONValue) limits = virJSONValueNewObject();
> +
> +    if (qemuMonitorMakeThrottleGroupLimits(limits, info)<0)

Formatting.

> +        return -1;
> +
> +    if (!(cmd = qemuMonitorJSONMakeCommand("qom-set",
> +                                           "s:property", "limits",
> +                                           "s:path", qomid,
> +                                           "a:value", &limits,
> +                                           NULL)))
> +        return -1;
> +
> +    if (qemuMonitorJSONCommand(mon, cmd, &result) < 0)
> +        return -1;
> +
> +    if (qemuMonitorJSONCheckError(cmd, result) < 0)
> +        return -1;
> +
> +    return 0;
> +}
> +
> +
> +#define GET_THROTTLE_GROUP_VALUE(FIELD, STORE) \
> +    if (virJSONValueObjectHasKey(ret, FIELD)) { \
> +        if (virJSONValueObjectGetNumberUlong(ret, FIELD, &reply->STORE) < 0) { \
> +            virReportError(VIR_ERR_OPERATION_UNSUPPORTED, \
> +                       _("throttle group field '%1$s' missing in qemu's output"), \

The error string is incorrect, because you check first whether the field
exists, thus at this point it did exist but is malformed.

> +                       #STORE); \
> +            return -1; \
> +        } \
> +    }
> +
> +
> +int
> +qemuMonitorJSONGetThrottleGroup(qemuMonitor *mon,
> +                                const char *gname,
> +                                virDomainBlockIoTuneInfo *reply)
> +{
> +    g_autoptr(virJSONValue) cmd = NULL;
> +    g_autoptr(virJSONValue) result = NULL;
> +    g_autofree char *groupCopy = NULL;
> +    virJSONValue *ret;
> +
> +    g_autofree char *path = g_strdup_printf("/objects/%s", gname);

Note that since all objects live in one namespace in qemu you'll have to
add a prefix to the group name so that the user will not be able to
accidentaly pick a group name (which would be equivalent with the object
'id') which we might either be using or introduce in the future.

Add a 'throttle-' prefix so that we clearly separate the throttle group
objects into their own namespace.

This will need to be done everywhere where you pass the throttle group
name as object name to qemu.


> +    if (!(cmd = qemuMonitorJSONMakeCommand("qom-get",
> +                                           "s:path", path,
> +                                           "s:property", "limits",
> +                                           NULL)))
> +        return -1;
> +
> +    if (qemuMonitorJSONCommand(mon, cmd, &result) < 0)
> +        return -1;
> +
> +    if (qemuMonitorJSONCheckError(cmd, result) < 0)
> +        return -1;
> +
> +    if (!(ret = qemuMonitorJSONGetReply(cmd, result, VIR_JSON_TYPE_OBJECT)))
> +        return -1;
> +
> +    GET_THROTTLE_GROUP_VALUE("bps-total", total_bytes_sec);
> +    GET_THROTTLE_GROUP_VALUE("bps-read", read_bytes_sec);
> +    GET_THROTTLE_GROUP_VALUE("bps-write", write_bytes_sec);
> +    GET_THROTTLE_GROUP_VALUE("iops-total", total_iops_sec);
> +    GET_THROTTLE_GROUP_VALUE("iops-read", read_iops_sec);
> +    GET_THROTTLE_GROUP_VALUE("iops-write", write_iops_sec);
> +
> +    GET_THROTTLE_GROUP_VALUE("bps-total-max", total_bytes_sec_max);
> +    GET_THROTTLE_GROUP_VALUE("bps-read-max", read_bytes_sec_max);
> +    GET_THROTTLE_GROUP_VALUE("bps-write-max", write_bytes_sec_max);
> +    GET_THROTTLE_GROUP_VALUE("iops-total-max", total_iops_sec_max);
> +    GET_THROTTLE_GROUP_VALUE("iops-read-max", read_iops_sec_max);
> +    GET_THROTTLE_GROUP_VALUE("iops-write-max", write_iops_sec_max);
> +    GET_THROTTLE_GROUP_VALUE("iops-size", size_iops_sec);
> +
> +    GET_THROTTLE_GROUP_VALUE("bps-total-max-length", total_bytes_sec_max_length);
> +    GET_THROTTLE_GROUP_VALUE("bps-read-max-length", read_bytes_sec_max_length);
> +    GET_THROTTLE_GROUP_VALUE("bps-write-max-length", write_bytes_sec_max_length);
> +    GET_THROTTLE_GROUP_VALUE("iops-total-max-length", total_iops_sec_max_length);
> +    GET_THROTTLE_GROUP_VALUE("iops-read-max-length", read_iops_sec_max_length);
> +    GET_THROTTLE_GROUP_VALUE("iops-write-max-length", write_iops_sec_max_length);
> +
> +    groupCopy = g_strdup(gname);
> +    reply->group_name = g_steal_pointer(&groupCopy);
> +
> +    return 0;
> +}
> +#undef GET_THROTTLE_GROUP_VALUE
> +
> +
>  int qemuMonitorJSONSystemWakeup(qemuMonitor *mon)
>  {
>      g_autoptr(virJSONValue) cmd = NULL;
Re: [PATCH RFC v3 05/16] qemu: monitor: Add support for ThrottleGroup operations
Posted by Chun Feng Wu 1 month, 1 week ago
let me confirm my understanding, do you mean there should be group name 
mapping between DOM($group_name_in_DOM) and 
QOM(throttle-$group_name_in_DOM) for both throttle group and throttle 
filter? if so, there seems two ways to achieve that:

- mapping group_name in callers like qemu_driver.c, qemu_hotplug.c, 
qemu_command.c

- or put all mappings only into qemu_monitor.c/qemu_monitor_json.c, in 
this way, I may need to expose more methods within monitor to prepare 
virJSONValueObject for ThrottleGroup and ThrottleFilter creation, it 
seems this way can centralize mapping logic within monitor only

On 2024/7/26 21:58, Peter Krempa wrote:
> Note that since all objects live in one namespace in qemu you'll have to
> add a prefix to the group name so that the user will not be able to
> accidentaly pick a group name (which would be equivalent with the object
> 'id') which we might either be using or introduce in the future.
>
> Add a 'throttle-' prefix so that we clearly separate the throttle group
> objects into their own namespace.
>
> This will need to be done everywhere where you pass the throttle group
> name as object name to qemu.

-- 
Thanks and Regards,

Wu
Re: [PATCH RFC v3 05/16] qemu: monitor: Add support for ThrottleGroup operations
Posted by Peter Krempa 1 month, 1 week ago
On Wed, Aug 07, 2024 at 12:07:26 +0800, Chun Feng Wu wrote:

Please do not top-post on technical lists. Put your reply inline with
the relevant context.

> let me confirm my understanding, do you mean there should be group name
> mapping between DOM($group_name_in_DOM) and QOM(throttle-$group_name_in_DOM)
> for both throttle group and throttle filter? if so, there seems two ways to
> achieve that:

The main point is that the throttle group names are arbitrary, we must
ensure that the user won't pick a name that collides with any other QOM
name or for that matter for any thing else.

There are multiple ways how to achieve that, but the easiest seems to
add a prefix and sanitize the name to e.g contain only characters and
numbers. Others like numbering them arbitrarily or so seem to be too
much hassle.

For the -blockdev throttle naming they should use existing node name
generators and not be based on anything user provided. Node-names are
length-limited so users might pick a name that'd exceed the limit. Thus
not allowing any control is the best there.

> - mapping group_name in callers like qemu_driver.c, qemu_hotplug.c,
> qemu_command.c
> 
> - or put all mappings only into qemu_monitor.c/qemu_monitor_json.c, in this
> way, I may need to expose more methods within monitor to prepare
> virJSONValueObject for ThrottleGroup and ThrottleFilter creation, it seems
> this way can centralize mapping logic within monitor only

It can't be limited to monitor, because you need to construct the
trhottling '-object' also when starting up qemu. So it needs to apply
wherever appropriate.

As the prefix is constant you don't need any helpers or extra infra to
decorate it. Just add it where appropriate.

> 
> On 2024/7/26 21:58, Peter Krempa wrote:
> > Note that since all objects live in one namespace in qemu you'll have to
> > add a prefix to the group name so that the user will not be able to
> > accidentaly pick a group name (which would be equivalent with the object
> > 'id') which we might either be using or introduce in the future.
> > 
> > Add a 'throttle-' prefix so that we clearly separate the throttle group
> > objects into their own namespace.
> > 
> > This will need to be done everywhere where you pass the throttle group
> > name as object name to qemu.
> 
> -- 
> Thanks and Regards,
> 
> Wu
Re: [PATCH RFC v3 05/16] qemu: monitor: Add support for ThrottleGroup operations
Posted by Peter Krempa 2 months, 2 weeks ago
On Wed, Jun 12, 2024 at 03:02:13 -0700, wucf@linux.ibm.com wrote:
> From: Chun Feng Wu <wucf@linux.ibm.com>
> 
> * ThrottleGroup is updated through "qemuMonitorJSONUpdateThrottleGroup"
> * ThrottleGroup is retrieved through "qemuMonitorJSONGetThrottleGroup"
> * ThrottleGroup is deleted by reusing "qemuMonitorDelObject"
> * ThrottleGroup is added by reusing "qemuMonitorAddObject"
> * "qemuMonitorMakeThrottleGroupLimits" will be used by building qemu cmd as well
> 
> Signed-off-by: Chun Feng Wu <wucf@linux.ibm.com>
> ---
>  src/qemu/qemu_monitor.c      |  34 ++++++++
>  src/qemu/qemu_monitor.h      |  14 ++++
>  src/qemu/qemu_monitor_json.c | 150 +++++++++++++++++++++++++++++++++++
>  src/qemu/qemu_monitor_json.h |  14 ++++
>  4 files changed, 212 insertions(+)
> 

[...]

> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> index c5e758e7f8..462b40cb6b 100644
> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c
> @@ -4633,6 +4633,156 @@ int qemuMonitorJSONGetBlockIoThrottle(qemuMonitor *mon,
>      return qemuMonitorJSONBlockIoThrottleInfo(devices, qdevid, reply);
>  }
>  
> +
> +int
> +qemuMonitorMakeThrottleGroupLimits(virJSONValue *limits,
> +                                   const virDomainThrottleGroupDef *group)
> +{
> +    if (virJSONValueObjectAdd(&limits,
> +                              "P:bps-total",
> +                              group->total_bytes_sec,

Please format both the string and the value on a single line
(disregarding any line length "suggestions"):

> +                              "P:bps-read", group->read_bytes_sec,
> +                              "P:bps-write", group->write_bytes_sec,

like that

> +                              "P:iops-total",
> +                              group->total_iops_sec,
> +                              "P:iops-read",
> +                              group->read_iops_sec,
> +                              "P:iops-write",
> +                              group->write_iops_sec,
> +                              "P:bps-total-max",
> +                              group->total_bytes_sec_max,
> +                              "P:bps-read-max",
> +                              group->read_bytes_sec_max,

Please note that I'll be on hollidays, so the rest of the review will be
delayed.
Re: [PATCH RFC v3 05/16] qemu: monitor: Add support for ThrottleGroup operations
Posted by Chun Feng Wu 2 months, 2 weeks ago
Sure, enjoy your holidays!