[PATCH] vsh: Don't init history in cmdComplete()
Recent rework of virshtest uncovered a subtle bug that was
dormant in now vsh but before that even in monolithic virsh.
In vsh.c there's this vshReadlineInit() function that's supposed
to initialize readline library, i.e. set those global rl_*
pointers. But it also initializes history library. Then, when
virsh/virt-admin quits, vshReadlineDeinit() is called which
writes history into a file (ensuring the parent directory
exists). So far no problem.
Problem arises when cmdComplete() is called (from a bash
completer, for instance). It does not guard call to
vshReadlineInit() with check for interactive shell (and it should
not), but it sets ctl->historyfile which signals to
vshReadlineDeinit() the history should be written.
Now, no real history is written, because nothing was entered on
the stdin, but the parent directory is created nevertheless. With
recent movement in virshtest.c this means some test cases might
create virsh history file which breaks our promise of not
touching user's data in test suite.
Resolves: https://bugs.gentoo.org/931109
Signed-off-by: Michal Privoznik
---
tools/vsh.c | 11 ---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/tools/vsh.c b/tools/vsh.c
index 58855f63ba..e74045c24e 100644
--- a/tools/vsh.c
+++ b/tools/vsh.c
@@ -2973,7 +2973,7 @@ vshReadlineInit(vshControl *ctl)
const char *quote_characters = "\"'";
/* initialize readline stuff only once */
-if (ctl->historydir)
+if (autoCompleteOpaque)
return 0;
/* Opaque data for autocomplete callbacks. */
@@ -2989,6 +2989,11 @@ vshReadlineInit(vshControl *ctl)
rl_completer_quote_characters = quote_characters;
rl_char_is_quoted_p = vshReadlineCharIsQuoted;
+/* Stuff below is needed only for interactive mode. */
+if (!ctl->imode) {
+return 0;
+}
+
histsize_env = g_strdup_printf("%s_HISTSIZE", ctl->env_prefix);
/* Limit the total size of the history buffer */
@@ -3149,7 +3154,7 @@ vshInit(vshControl *ctl, const vshCmdGrp *groups)
cmdGroups = groups;
if (vshInitDebug(ctl) < 0 ||
-(ctl->imode && vshReadlineInit(ctl) < 0))
+vshReadlineInit(ctl) < 0)
return false;
return true;
@@ -3168,7 +3173,7 @@ vshInitReload(vshControl *ctl)
if (ctl->imode)
vshReadlineDeinit(ctl);
-if (ctl->imode && vshReadlineInit(ctl) < 0)
+if (vshReadlineInit(ctl) < 0)
return false;
return true;
--
2.43.2
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org
The Raptors defeated the Warriors
If The watch solutions feel mind-boggling, it may help to narrow your focus. Probably you merely want to know you’re finding something from a revered brand you could have faith in. https://www.newzealandescortspage.com ___ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-le...@lists.libvirt.org
Re: [PATCH RFC v2 06/12] qemu: command: Support throttle groups and filters during qemuProcessLaunch
On Thu, Apr 11, 2024 at 19:01:54 -0700, w...@linux.ibm.com wrote:
> From: Chun Feng Wu
>
> * Add qemuBuildThrottleGroupCommandLine in qemuBuildCommandLine
> * Add qemuBuildThrottleFiltersCommandLine in qemuBuildDiskCommandLine
> * Make sure referenced throttle group exists
>
> Signed-off-by: Chun Feng Wu
> ---
Similarly to previous patches this should be after the patch adding XML
schema so that it's obvious what the design is first.
Additionally you mix the thottle group definition code with the disk
filter code again. It makes this series very unpleasant to review as
it's mixing contexts.
>
> diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c
> index 395e036e8f..fffe274afc 100644
> --- a/src/conf/domain_validate.c
> +++ b/src/conf/domain_validate.c
> @@ -663,6 +663,7 @@ virDomainDiskDefValidate(const virDomainDef *def,
> const virDomainDiskDef *disk)
> {
> virStorageSource *next;
> +size_t i;
>
> /* disk target is used widely in other code so it must be validated
> first */
> if (!disk->dst) {
> @@ -942,6 +943,19 @@ virDomainDiskDefValidate(const virDomainDef *def,
> return -1;
> }
>
> +/* referenced group should be defined */
> +for (i = 0; i < disk->nthrottlefilters; i++) {
> +virDomainThrottleFilterDef *filter = disk->throttlefilters[i];
> +if (filter) {
Can this even be NULL?
> +if (!virDomainThrottleGroupFind(def, filter->group_name)) {
> +virReportError(VIR_ERR_XML_ERROR,
> +_("throttle group '%1$s' not found"),
> +filter->group_name);
> +return -1;
> +}
> +}
> +}
Additionally this validation is insufficient.
'qemuDiskConfigThrottleFilterEnabled' below skips the formatting of the
throttle group object if it has no config, but present name.
This code will accept it but qemu will most likely reject it as the
group was not defined.
I've also seen the statement that old-style throttling should not be
combined with this approach. I'm missing a check that forbids such
configs.
> +
> return 0;
> }
>
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 2d8036c3ae..34164c098b 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -1584,6 +1584,14 @@ qemuDiskConfigThrottleFilterChainEnabled(const
> virDomainDiskDef *disk)
> }
>
>
> +bool
> +qemuDiskConfigThrottleFilterEnabled(const virDomainThrottleGroupDef *group)
> +{
> +return !!group->group_name &&
> + virDomainBlockIoTuneInfoHasAny(group);
> +}
> +
> +
> /**
> * qemuDiskBusIsSD:
> * @bus: disk bus
> @@ -2221,6 +2229,45 @@
> qemuBuildBlockStorageSourceAttachDataCommandline(virCommand *cmd,
> }
>
>
> +static int
> +qemuBuildBlockThrottleFilterCommandline(virCommand *cmd,
> +qemuBlockThrottleFilterAttachData
> *data)
> +{
> +char *tmp;
> +
> +if (data->filterProps) {
> +if (!(tmp = virJSONValueToString(data->filterProps, false)))
> +return -1;
> +
> +virCommandAddArgList(cmd, "-blockdev", tmp, NULL);
> +VIR_FREE(tmp);
Declare 'tmp' inside the loop with g_autofree instead of this manual
thing.
> +}
> +
> +return 0;
> +}
> +
> +
> +static int
> +qemuBuildThrottleFiltersCommandLine(virCommand *cmd,
> +virDomainDiskDef *disk)
> +{
> +g_autoptr(qemuBlockThrottleFilterChainData) data = NULL;
> +size_t i;
> +
> +if (!(data = qemuBuildThrottleFilterChainAttachPrepareBlockdev(disk))) {
> +return -1;
> +} else {
Don't do these compound conditions. Always directly return on error and
leave the success code paths in the main control flow. It's interrupting
the understanding of the code.
> +for (i = 0; i < data->nfilterdata; i++) {
> +if (qemuBuildBlockThrottleFilterCommandline(cmd,
> +data->filterdata[i])
> < 0)
> +return -1;
> +}
> +}
> +
> +return 0;
> +}
> +
> +
> static int
> qemuBuildDiskSourceCommandLine(virCommand *cmd,
> virDomainDiskDef *disk,
> @@ -2278,6 +2325,9 @@ qemuBuildDiskCommandLine(virCommand *cmd,
> if (qemuBuildDiskSourceCommandLine(cmd, disk, qemuCaps) < 0)
> return -1;
>
> +if (qemuBuildThrottleFiltersCommandLine(cmd, disk) < 0)
> +return -1;
The name should include 'Disk'.
> +
> /* SD cards are currently instantiated via -drive if=sd, so the -device
> * part must be skipped */
> if (qemuDiskBusIsSD(disk->bus))
> @@ -7451,6 +7501,47 @@ qemuBuildIOThreadCommandLine(virCommand *cmd,
> }
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org
Re: [PATCH RFC v2 05/12] qemu: hotplug: Support hot attach block disk along with throttle filters
On Thu, Apr 11, 2024 at 19:01:53 -0700, w...@linux.ibm.com wrote:
> From: Chun Feng Wu
>
> When attaching disk along with specified throttle groups, those groups will
> be chained up by parent node name, this change includes service side codes:
> * Each filter references one throttle group by group name
> * Each filter has a nodename, and those filters are chained up in sequence
> * Filter nodename index is persistented in
> virDomainObj->privateData(qemuDomainObjPrivate)
> * During hotplug, filter is created through QMP request("blockdev-add" with
> "driver":"throttle") to QEMU
> * Finally, "device_add"(attach) QMP request is adjusted to set "drive" to be
> top filter nodename in chain.
>
> Signed-off-by: Chun Feng Wu
> ---a
As noted before, the proper approach is to add the XML bits first so
some asumptions may be off.
Before next version please add implementation only after the XML and
docs is added.
> src/qemu/qemu_block.c | 131 ++
> src/qemu/qemu_block.h | 53 +++
> src/qemu/qemu_command.c | 64 +
> src/qemu/qemu_command.h | 6 +
> src/qemu/qemu_domain.c| 71 ++
> src/qemu/qemu_domain.h| 19 ++-
> src/qemu/qemu_hotplug.c | 24
> .../qemustatusxml2xmldata/backup-pull-in.xml | 1 +
> .../blockjob-blockdev-in.xml | 1 +
> .../blockjob-mirror-in.xml| 1 +
> .../migration-in-params-in.xml| 1 +
> .../migration-out-nbd-bitmaps-in.xml | 1 +
> .../migration-out-nbd-out.xml | 1 +
> .../migration-out-nbd-tls-out.xml | 1 +
> .../migration-out-params-in.xml | 1 +
> tests/qemustatusxml2xmldata/modern-in.xml | 1 +
> tests/qemustatusxml2xmldata/upgrade-out.xml | 1 +
> .../qemustatusxml2xmldata/vcpus-multi-in.xml | 1 +
> 18 files changed, 375 insertions(+), 4 deletions(-)
>
> diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c
> index 738b72d7ea..c53de2465e 100644
> --- a/src/qemu/qemu_block.c
> +++ b/src/qemu/qemu_block.c
> @@ -2739,6 +2739,137 @@ qemuBlockStorageSourceCreateDetectSize(GHashTable
> *blockNamedNodeData,
> }
>
>
> +void
> +qemuBlockThrottleFilterSetNodename(virDomainThrottleFilterDef *filter,
> + char *nodename)
> +{
> +g_free(filter->nodename);
> +filter->nodename = nodename;
> +}
> +
> +
> +const char *
> +qemuBlockThrottleFilterGetNodename(virDomainThrottleFilterDef *filter)
> +{
> +return filter->nodename;
> +}
> +
> +
> +/**
> + * qemuBlockThrottleFilterGetProps:
> + * @filter: throttle filter
> + * @parentNodeName: parent nodename of @filter
> + *
> + * Build "arguments" part of "blockdev-add" QMP cmd.
> + * e.g. {"execute":"blockdev-add", "arguments":{"driver":"throttle",
> + * "node-name":"libvirt-2-filter", "throttle-group":"limits0",
> + * "file": "libvirt-1-format"}}
> + */
> +virJSONValue *
> +qemuBlockThrottleFilterGetProps(virDomainThrottleFilterDef *filter,
> +const char *parentNodeName)
> +{
> +g_autoptr(virJSONValue) props = NULL;
> +
> +if (virJSONValueObjectAdd(&props,
> + "s:driver", "throttle",
> + "s:node-name",
> qemuBlockThrottleFilterGetNodename(filter),
> + "s:throttle-group", filter->group_name,
> + "s:file", parentNodeName,
> + NULL) < 0)
> +return 0;
> +
> +return g_steal_pointer(&props);
> +}
> +
> +
> +void
> +qemuBlockThrottleFilterAttachDataFree(qemuBlockThrottleFilterAttachData
> *data)
> +{
> +if (!data)
> +return;
> +
> +virJSONValueFree(data->filterProps);
> +g_free(data);
> +}
> +
> +
> +qemuBlockThrottleFilterAttachData *
> +qemuBlockThrottleFilterAttachPrepareBlockdev(virDomainThrottleFilterDef
> *filter,
> + const char *parentNodeName)
> +{
> +g_autoptr(qemuBlockThrottleFilterAttachData) data = NULL;
> +
> +data = g_new0(qemuBlockThrottleFilterAttachData, 1);
> +
> +if (!(data->filterProps = qemuBlockThrottleFilterGetProps(filter,
> parentNodeName)))
> +return NULL;
> +
> +data->filterNodeName = qemuBlockThrottleFilterGetNodename(filter);
> +data->filterAttached = true;
You can't claim that the filter node was attached at this point. The
rollback code won't work properly.
> +
> +return g_steal_pointer(&data);
> +}
> +
> +
> +void
> +qemuBlockThrottleFilterAttachRollback(qemuMonitor *mon,
> + qemuBlockThrottleFilterAttachData
> *data)
> +{
> +virErrorPtr orig_err;
> +
> +virErrorPreserveLast(&orig_err);
> +
> +if (data->filterAttached)
> +ignore_value(qemuMonitorBlockdevDel(mon, data->filterN
Re: [PATCH-for-9.1 v2 2/3] migration: Remove RDMA protocol handling
On Fri, May 03, 2024 at 08:40:03AM +0200, Jinpu Wang wrote: > I had a brief check in the rsocket changelog, there seems some > improvement over time, > might be worth revisiting this. due to socket abstraction, we can't > use some feature like > ODP, it won't be a small and easy task. It'll be good to know whether Dan's suggestion would work first, without rewritting everything yet so far. Not sure whether some perf test could help with the rsocket APIs even without QEMU's involvements (or looking for test data supporting / invalidate such conversions). Thanks, -- Peter Xu ___ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-le...@lists.libvirt.org
Re: [PATCH] hyperv: prevent potential NULL dereference
On Fri, May 3, 2024 at 11:43 AM Oleg Sviridov wrote: > Return value of a function 'virDomainChrDefNew' is dereferenced > at hyperv_driver.c without checking for NULL, which can lead to > NULL dereference immediatly after. > > Found by Linux Verification Center (linuxtesting.org) with SVACE. > > Signed-off-by: Oleg Sviridov > --- > src/hyperv/hyperv_driver.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > Reviewed-by: Kristína Hanicová ___ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-le...@lists.libvirt.org
Re: [PATCH RFC v2 04/12] qemu: Implement qemu driver for throttle API
On Thu, Apr 11, 2024 at 19:01:52 -0700, w...@linux.ibm.com wrote:
> From: Chun Feng Wu
>
> Implement the following methods:
> * virDomainSetThrottleGroup
> * virDomainGetThrottleGroup
> * virDomainDelThrottleGroup
Similarly to previous patch, note how you've done this rather than what
you've done, which is visible from the diff.
>
> Signed-off-by: Chun Feng Wu
> ---
As noted in previous patch this really needs to go after the XML and
qemu commandline bits.
> src/qemu/qemu_domain.c | 14 ++
> src/qemu/qemu_domain.h | 4 +
> src/qemu/qemu_driver.c | 523 +
> 3 files changed, 541 insertions(+)
>
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 6b869797a8..b676f59c3a 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -10246,6 +10246,20 @@ qemuDomainDiskByName(virDomainDef *def,
> }
>
>
> +virDomainThrottleGroupDef *
> +qemuDomainThrottleGroupByName(virDomainDef *def,
> + const char *name)
> +{
> +virDomainThrottleGroupDef *ret;
> +
> +if (!(ret = virDomainThrottleGroupByName(def, name))) {
> +return NULL;
This wrapper is really pointless. Use virDomainThrottleGroupByName
directly.
> +}
> +
> +return ret;
> +}
> +
> +
> int
> qemuDomainPrepareChannel(virDomainChrDef *channel,
> const char *domainChannelTargetDir)
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index d01f788aea..da0f9590db 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -19995,6 +19995,526 @@ qemuDomainGraphicsReload(virDomainPtr domain,
> return ret;
> }
>
> +
> +/* wrapper of qemuDomainSetBlockIoTuneDefaults for throttle group since they
> use the same data structure */
> +static int
> +qemuDomainSetThrottleGroupDefaults(virDomainBlockIoTuneInfo *newinfo,
> + virDomainBlockIoTuneInfo *oldinfo,
> + qemuBlockIoTuneSetFlags set_fields)
> +{
> +return qemuDomainSetBlockIoTuneDefaults(newinfo, oldinfo, set_fields);
> +}
Same here, this is a pointless wrapper.
> +
> +
> +static int
> +qemuDomainCheckThrottleGroupReset(const char *groupname,
> + virDomainBlockIoTuneInfo *newiotune)
> +{
> +if (virDomainBlockIoTuneInfoHasAny(newiotune))
> +return 0;
> +
> +if (newiotune->group_name &&
> +STRNEQ_NULLABLE(newiotune->group_name, groupname)) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("creating a new group/updating existing with all
> parameters zero is not supported"));
> +return -1;
> +}
> +
> +/* all zero means remove any throttling and remove from group for qemu */
> +VIR_FREE(newiotune->group_name);
It's not quite obvious what this function is supposed to do and on the
first glance it looks like it's a set of pre-checks. As of such you'll
be better of doing this set of checks
> +
> +return 0;
> +}
> +
> +
> +static int
> +qemuDomainSetThrottleGroup(virDomainPtr dom,
> + const char *groupname,
> + virTypedParameterPtr params,
> + int nparams,
> + unsigned int flags)
> +{
> +virQEMUDriver *driver = dom->conn->privateData;
> +virDomainObj *vm = NULL;
> +virDomainDef *def = NULL;
> +virDomainDef *persistentDef = NULL;
> +virDomainThrottleGroupDef info = { 0 };
> +virDomainThrottleGroupDef conf_info = { 0 };
> +int ret = -1;
> +size_t i;
> +qemuBlockIoTuneSetFlags set_fields = 0;
> +g_autoptr(virQEMUDriverConfig) cfg = NULL;
> +virObjectEvent *event = NULL;
> +virTypedParameterPtr eventParams = NULL;
> +int eventNparams = 0;
> +int eventMaxparams = 0;
> +virDomainThrottleGroupDef *cur_info;
> +virDomainThrottleGroupDef *conf_cur_info;
> +int rc = 0;
> +g_autoptr(virJSONValue) props = NULL;
> +g_autoptr(virJSONValue) limits = virJSONValueNewObject();
> +
> +
> +virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
> + VIR_DOMAIN_AFFECT_CONFIG, -1);
> +if (virTypedParamsValidate(params, nparams,
> + VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC,
> + VIR_TYPED_PARAM_ULLONG,
> + VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC,
> + VIR_TYPED_PARAM_ULLONG,
> + VIR_DOMAIN_BLOCK_IOTUNE_WRITE_BYTES_SEC,
> + VIR_TYPED_PARAM_ULLONG,
> + VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_IOPS_SEC,
> + VIR_TYPED_PARAM_ULLONG,
> + VIR_DOMAIN_BLOCK_IOTUNE_READ_IOPS_SEC,
> + VIR_TYPED_PARAM_ULLONG,
> + VIR_DOMAIN_BLOCK_IOTUNE_WRITE_IOPS_SEC,
> +
[PATCH] NEWS: document qemu: ras as a new feature
Signed-off-by: Kristina Hanicova --- NEWS.rst | 5 + 1 file changed, 5 insertions(+) diff --git a/NEWS.rst b/NEWS.rst index 5a771b4b2f..d72c15bf10 100644 --- a/NEWS.rst +++ b/NEWS.rst @@ -17,6 +17,11 @@ v10.4.0 (unreleased) * **New features** + * qemu: Support for ras feature for virt machine type + +It is now possible to set on/off ``ras`` feature in the domain XML for virt +(Arm) machine type as . + * **Improvements** * **Bug fixes** -- 2.42.0 ___ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-le...@lists.libvirt.org
[PATCH] hyperv: prevent potential NULL dereference
Return value of a function 'virDomainChrDefNew' is dereferenced at hyperv_driver.c without checking for NULL, which can lead to NULL dereference immediatly after. Found by Linux Verification Center (linuxtesting.org) with SVACE. Signed-off-by: Oleg Sviridov --- src/hyperv/hyperv_driver.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c index 414274fdfd..7580c6a06c 100644 --- a/src/hyperv/hyperv_driver.c +++ b/src/hyperv/hyperv_driver.c @@ -1534,7 +1534,8 @@ hypervDomainDefParseSerial(virDomainDef *def, Msvm_ResourceAllocationSettingData continue; } -serial = virDomainChrDefNew(NULL); +if (!(serial = virDomainChrDefNew(NULL))) +return -1; serial->deviceType = VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL; serial->source->type = VIR_DOMAIN_CHR_TYPE_PIPE; -- 2.44.0 ___ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-le...@lists.libvirt.org
Re: [PATCH-for-9.1 v2 2/3] migration: Remove RDMA protocol handling
Hi Daniel, On Wed, May 1, 2024 at 6:00 PM Daniel P. Berrangé wrote: > > On Wed, May 01, 2024 at 11:31:13AM -0400, Peter Xu wrote: > > What I worry more is whether this is really what we want to keep rdma in > > qemu, and that's also why I was trying to request for some serious > > performance measurements comparing rdma v.s. nics. And here when I said > > "we" I mean both QEMU community and any company that will support keeping > > rdma around. > > > > The problem is if NICs now are fast enough to perform at least equally > > against rdma, and if it has a lower cost of overall maintenance, does it > > mean that rdma migration will only be used by whoever wants to keep them in > > the products and existed already? In that case we should simply ask new > > users to stick with tcp, and rdma users should only drop but not increase. > > > > It seems also destined that most new migration features will not support > > rdma: see how much we drop old features in migration now (which rdma > > _might_ still leverage, but maybe not), and how much we add mostly multifd > > relevant which will probably not apply to rdma at all. So in general what > > I am worrying is a both-loss condition, if the company might be easier to > > either stick with an old qemu (depending on whether other new features are > > requested to be used besides RDMA alone), or do periodic rebase with RDMA > > downstream only. > > I don't know much about the originals of RDMA support in QEMU and why > this particular design was taken. It is indeed a huge maint burden to > have a completely different code flow for RDMA with 4000+ lines of > custom protocol signalling which is barely understandable. > > I would note that /usr/include/rdma/rsocket.h provides a higher level > API that is a 1-1 match of the normal kernel 'sockets' API. If we had > leveraged that, then QIOChannelSocket class and the QAPI SocketAddress > type could almost[1] trivially have supported RDMA. There would have > been almost no RDMA code required in the migration subsystem, and all > the modern features like compression, multifd, post-copy, etc would > "just work". I guess at the time rsocket is less mature, and less performant compared to using uverbs directly. > > I guess the 'rsocket.h' shim may well limit some of the possible > performance gains, but it might still have been a better tradeoff > to have not quite so good peak performance, but with massively > less maint burden. I had a brief check in the rsocket changelog, there seems some improvement over time, might be worth revisiting this. due to socket abstraction, we can't use some feature like ODP, it won't be a small and easy task. > With regards, > Daniel Thanks for the suggestion. > > [1] "almost" trivially, because the poll() integration for rsockets > requires a bit more magic sauce since rsockets FDs are not > really FDs from the kernel's POV. Still, QIOCHannel likely can > abstract that probme. > -- > |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| > |: https://libvirt.org -o-https://fstop138.berrange.com :| > |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| > ___ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-le...@lists.libvirt.org
Re: [PATCH-for-9.1 v2 2/3] migration: Remove RDMA protocol handling
Hi Peter On Thu, May 2, 2024 at 6:20 PM Peter Xu wrote: > > On Thu, May 02, 2024 at 03:30:58PM +0200, Jinpu Wang wrote: > > Hi Michael, Hi Peter, > > > > > > On Thu, May 2, 2024 at 3:23 PM Michael Galaxy wrote: > > > > > > Yu Zhang / Jinpu, > > > > > > Any possibility (at your lesiure, and within the disclosure rules of > > > your company, IONOS) if you could share any of your performance > > > information to educate the group? > > > > > > NICs have indeed changed, but not everybody has 100ge mellanox cards at > > > their disposal. Some people don't. > > Our staging env is with 100 Gb/s IB environment. > > We will have a new setup in the coming months with Ethernet (RoCE), we > > will run some performance > > comparison when we have the environment ready. > > Thanks both. Please keep us posted. > > Just to double check, we're comparing "tcp:" v.s. "rdma:", RoCE is not > involved, am I right? kinds of. Our new hardware is RDMA capable, we can configure it to run in "rdma" transport or "tcp" it is more straight comparison, When run "rdma" transport, RoCE is involved, eg the rdma-core/ibverbs/rdmacm/vendor verbs driver are used. > > The other note is that the comparison needs to be with multifd enabled for > the "tcp:" case. I'd suggest we start with 8 threads if it's 100Gbps. > > I think I can still fetch some 100Gbps or even 200Gbps nics around our labs > without even waiting for months. If you want I can try to see how we can > test together. And btw I don't think we need a cluster, IIUC we simply > need two hosts, 100G nic on both sides? IOW, it seems to me we only need > two cards just for experiments, systems that can drive the cards, and a > wire supporting 100G? Yes, the simple setup can be just two hosts directly connected. This remind me, I may also able to find a test setup with 100 G nic in lab, will keep you posted. Regards! > > > > > > > > > - Michael > > > > Thx! > > Jinpu > > > > > > On 5/1/24 11:16, Peter Xu wrote: > > > > On Wed, May 01, 2024 at 04:59:38PM +0100, Daniel P. Berrangé wrote: > > > >> On Wed, May 01, 2024 at 11:31:13AM -0400, Peter Xu wrote: > > > >>> What I worry more is whether this is really what we want to keep rdma > > > >>> in > > > >>> qemu, and that's also why I was trying to request for some serious > > > >>> performance measurements comparing rdma v.s. nics. And here when I > > > >>> said > > > >>> "we" I mean both QEMU community and any company that will support > > > >>> keeping > > > >>> rdma around. > > > >>> > > > >>> The problem is if NICs now are fast enough to perform at least equally > > > >>> against rdma, and if it has a lower cost of overall maintenance, does > > > >>> it > > > >>> mean that rdma migration will only be used by whoever wants to keep > > > >>> them in > > > >>> the products and existed already? In that case we should simply ask > > > >>> new > > > >>> users to stick with tcp, and rdma users should only drop but not > > > >>> increase. > > > >>> > > > >>> It seems also destined that most new migration features will not > > > >>> support > > > >>> rdma: see how much we drop old features in migration now (which rdma > > > >>> _might_ still leverage, but maybe not), and how much we add mostly > > > >>> multifd > > > >>> relevant which will probably not apply to rdma at all. So in general > > > >>> what > > > >>> I am worrying is a both-loss condition, if the company might be > > > >>> easier to > > > >>> either stick with an old qemu (depending on whether other new > > > >>> features are > > > >>> requested to be used besides RDMA alone), or do periodic rebase with > > > >>> RDMA > > > >>> downstream only. > > > >> I don't know much about the originals of RDMA support in QEMU and why > > > >> this particular design was taken. It is indeed a huge maint burden to > > > >> have a completely different code flow for RDMA with 4000+ lines of > > > >> custom protocol signalling which is barely understandable. > > > >> > > > >> I would note that /usr/include/rdma/rsocket.h provides a higher level > > > >> API that is a 1-1 match of the normal kernel 'sockets' API. If we had > > > >> leveraged that, then QIOChannelSocket class and the QAPI SocketAddress > > > >> type could almost[1] trivially have supported RDMA. There would have > > > >> been almost no RDMA code required in the migration subsystem, and all > > > >> the modern features like compression, multifd, post-copy, etc would > > > >> "just work". > > > >> > > > >> I guess the 'rsocket.h' shim may well limit some of the possible > > > >> performance gains, but it might still have been a better tradeoff > > > >> to have not quite so good peak performance, but with massively > > > >> less maint burden. > > > > My understanding so far is RDMA is sololy for performance but nothing > > > > else, > > > > then it's a question on whether rdma existing users would like to do so > > > > if > > > > it will run slower. > > > > > > > > Jinpu mentioned on the explicit usages of ib ver
Re: [PATCH v3 3/6] migration: Remove 'blk/-b' option from migrate commands
* Markus Armbruster (arm...@redhat.com) wrote:
> Fabiano Rosas writes:
>
> > The block migration is considered obsolete and has been deprecated in
> > 8.2. Remove the migrate command option that enables it. This only
> > affects the QMP and HMP commands, the feature can still be accessed by
> > setting the migration 'block' capability. The whole feature will be
> > removed in a future patch.
> >
> > Deprecation commit 8846b5bfca ("migration: migrate 'blk' command
> > option is deprecated.").
> >
> > Reviewed-by: Markus Armbruster
> > Signed-off-by: Fabiano Rosas
>
> [...]
>
> > diff --git a/hmp-commands.hx b/hmp-commands.hx
> > index 7978302949..ebca2cdced 100644
> > --- a/hmp-commands.hx
> > +++ b/hmp-commands.hx
> > @@ -909,21 +909,17 @@ ERST
> >
> > {
> > .name = "migrate",
> > -.args_type = "detach:-d,blk:-b,resume:-r,uri:s",
> > -.params = "[-d] [-b] [-r] uri",
> > +.args_type = "detach:-d,resume:-r,uri:s",
> > +.params = "[-d] [-r] uri",
> > .help = "migrate to URI (using -d to not wait for
> > completion)"
> > - "\n\t\t\t -b for migration without shared storage with"
> > - " full copy of disk\n\t\t\t -r to resume a paused
> > migration",
> > + "\n\t\t\t -r to resume a paused migration",
> > .cmd= hmp_migrate,
> > },
> >
> >
> > SRST
> > -``migrate [-d] [-b]`` *uri*
> > +``migrate [-d]`` *uri*
> >Migrate to *uri* (using -d to not wait for completion).
> > -
> > - ``-b``
> > -for migration with full copy of disk
> > ERST
>
> Not this patch's fault, but here goes anyway: -r is undocumented here.
Probably one for Peter I guess.
Dave
> >
> > {
>
> [...]
>
--
-Open up your eyes, open up your mind, open up your code ---
/ Dr. David Alan Gilbert| Running GNU/Linux | Happy \
\dave @ treblig.org | | In Hex /
\ _|_ http://www.treblig.org |___/
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org
Re: [PATCH-for-9.1 v2 2/3] migration: Remove RDMA protocol handling
Hi Michael, Hi Peter, On Thu, May 2, 2024 at 3:23 PM Michael Galaxy wrote: > > Yu Zhang / Jinpu, > > Any possibility (at your lesiure, and within the disclosure rules of > your company, IONOS) if you could share any of your performance > information to educate the group? > > NICs have indeed changed, but not everybody has 100ge mellanox cards at > their disposal. Some people don't. Our staging env is with 100 Gb/s IB environment. We will have a new setup in the coming months with Ethernet (RoCE), we will run some performance comparison when we have the environment ready. > > - Michael Thx! Jinpu > > On 5/1/24 11:16, Peter Xu wrote: > > On Wed, May 01, 2024 at 04:59:38PM +0100, Daniel P. Berrangé wrote: > >> On Wed, May 01, 2024 at 11:31:13AM -0400, Peter Xu wrote: > >>> What I worry more is whether this is really what we want to keep rdma in > >>> qemu, and that's also why I was trying to request for some serious > >>> performance measurements comparing rdma v.s. nics. And here when I said > >>> "we" I mean both QEMU community and any company that will support keeping > >>> rdma around. > >>> > >>> The problem is if NICs now are fast enough to perform at least equally > >>> against rdma, and if it has a lower cost of overall maintenance, does it > >>> mean that rdma migration will only be used by whoever wants to keep them > >>> in > >>> the products and existed already? In that case we should simply ask new > >>> users to stick with tcp, and rdma users should only drop but not increase. > >>> > >>> It seems also destined that most new migration features will not support > >>> rdma: see how much we drop old features in migration now (which rdma > >>> _might_ still leverage, but maybe not), and how much we add mostly multifd > >>> relevant which will probably not apply to rdma at all. So in general what > >>> I am worrying is a both-loss condition, if the company might be easier to > >>> either stick with an old qemu (depending on whether other new features are > >>> requested to be used besides RDMA alone), or do periodic rebase with RDMA > >>> downstream only. > >> I don't know much about the originals of RDMA support in QEMU and why > >> this particular design was taken. It is indeed a huge maint burden to > >> have a completely different code flow for RDMA with 4000+ lines of > >> custom protocol signalling which is barely understandable. > >> > >> I would note that /usr/include/rdma/rsocket.h provides a higher level > >> API that is a 1-1 match of the normal kernel 'sockets' API. If we had > >> leveraged that, then QIOChannelSocket class and the QAPI SocketAddress > >> type could almost[1] trivially have supported RDMA. There would have > >> been almost no RDMA code required in the migration subsystem, and all > >> the modern features like compression, multifd, post-copy, etc would > >> "just work". > >> > >> I guess the 'rsocket.h' shim may well limit some of the possible > >> performance gains, but it might still have been a better tradeoff > >> to have not quite so good peak performance, but with massively > >> less maint burden. > > My understanding so far is RDMA is sololy for performance but nothing else, > > then it's a question on whether rdma existing users would like to do so if > > it will run slower. > > > > Jinpu mentioned on the explicit usages of ib verbs but I am just mostly > > quotting that word as I don't really know such details: > > > > https://urldefense.com/v3/__https://lore.kernel.org/qemu-devel/camgffem2twjxopcnqtq1sjytf5395dbztcmyikrqfxdzjws...@mail.gmail.com/__;!!GjvTz_vk!W6-HGWM-XkF_52am249DrLIDQeZctVOHg72LvOHGUcwxqQM5mY0GNYYl-yNJslN7A5GfLOew9oW_kg$ > > > > So not sure whether that applies here too, in that having qiochannel > > wrapper may not allow direct access to those ib verbs. > > > > Thanks, > > > >> With regards, > >> Daniel > >> > >> [1] "almost" trivially, because the poll() integration for rsockets > >> requires a bit more magic sauce since rsockets FDs are not > >> really FDs from the kernel's POV. Still, QIOCHannel likely can > >> abstract that probme. > >> -- > >> |: > >> https://urldefense.com/v3/__https://berrange.com__;!!GjvTz_vk!W6-HGWM-XkF_52am249DrLIDQeZctVOHg72LvOHGUcwxqQM5mY0GNYYl-yNJslN7A5GfLOfyTmFFUQ$ > >>-o- > >> https://urldefense.com/v3/__https://www.flickr.com/photos/dberrange__;!!GjvTz_vk!W6-HGWM-XkF_52am249DrLIDQeZctVOHg72LvOHGUcwxqQM5mY0GNYYl-yNJslN7A5GfLOf8A5OC0Q$ > >> :| > >> |: > >> https://urldefense.com/v3/__https://libvirt.org__;!!GjvTz_vk!W6-HGWM-XkF_52am249DrLIDQeZctVOHg72LvOHGUcwxqQM5mY0GNYYl-yNJslN7A5GfLOf3gffAdg$ > >> -o- > >> https://urldefense.com/v3/__https://fstop138.berrange.com__;!!GjvTz_vk!W6-HGWM-XkF_52am249DrLIDQeZctVOHg72LvOHGUcwxqQM5mY0GNYYl-yNJslN7A5GfLOfPMofYqw$ > >> :| > >> |: > >> https://urldefense.com/v3/__https://entangle-photo.org__;!!GjvTz_vk!W6-HGWM-XkF_52am249DrLIDQeZctVOHg72LvOHGUcwxqQM5mY0GNYYl-yNJslN7A5GfLOeQ5jjAeQ$ > >> -o- > >> https:
Re: [PATCH RFC v2 01/12] config: Introduce ThrottleGroup and ThrottleFilter
On Thu, Apr 11, 2024 at 19:01:49 -0700, w...@linux.ibm.com wrote: > From: Chun Feng Wu > > * Define new structs 'virDomainThrottleGroupDef' and > 'virDomainThrottleFilterDef' > * Update _virDomainDef to include virDomainThrottleGroupDef > * Update _virDomainDiskDef to include virDomainThrottleFilterDef > * Support new resource operations for DOM XML and structs, corresponding > "Parse" and "Format" methods are provided > > Signed-off-by: Chun Feng Wu > --- [...] > diff --git a/src/conf/virconftypes.h b/src/conf/virconftypes.h > index 0779bc224b..1cf68b2814 100644 > --- a/src/conf/virconftypes.h > +++ b/src/conf/virconftypes.h > @@ -80,6 +80,10 @@ typedef struct _virDomainBlkiotune virDomainBlkiotune; > > typedef struct _virDomainBlockIoTuneInfo virDomainBlockIoTuneInfo; > > +typedef struct _virDomainBlockIoTuneInfo virDomainThrottleGroupDef; > + NACK, don't do this. The name should match. If it is the same, treat it the same as it will allow refactors. ___ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-le...@lists.libvirt.org
Re: [PATCH RFC v2 03/12] remote: New APIs for ThrottleGroup lifecycle management
On Thu, Apr 11, 2024 at 19:01:51 -0700, w...@linux.ibm.com wrote:
> From: Chun Feng Wu
>
> Support throttlegroup lifecycle management by the following implementation:
> * New methods defined in "include/libvirt/libvirt-domain.h"
> * And they're exported in "src/libvirt_public.syms"
> * Corresponding internal API is defined in "src/driver-hypervisor.h"
> * Public API calls are implemented in "src/libvirt-domain.c"
> * Wire protocol is defined in "src/remote/remote_protocol.x"
> * RPC client implementation is in "src/remote/remote_driver.c"
> * Server side dispatch is implemented in "src/remote/remote_daemon_dispatch.c"
> * Also "src/remote_protocol-structs" is updated
> * Yu Jie Gu helped add
> remote_domain_set_throttle_group_args in src/remote_protocol-structs
> to fix issue reported by check-remote_protocol
Please, rather than describing where you've put the code, use this space
to describe the API itself. Which parameters it takes how it's supposed
to be used.
>
> Signed-off-by: Chun Feng Wu
> ---
This patch with new APIs really needs to go in after all of the other
code which allows to configure it via XML. I'll for now add some top
level comments and then get back once I see how the XML is designed
first.
> include/libvirt/libvirt-domain.h| 29 +
> src/driver-hypervisor.h | 22
> src/libvirt-domain.c| 190
> src/libvirt_private.syms| 9 ++
> src/libvirt_public.syms | 7 +
> src/remote/remote_daemon_dispatch.c | 61 +
> src/remote/remote_driver.c | 49 +++
> src/remote/remote_protocol.x| 50 +++-
> src/remote_protocol-structs | 30 +
> 9 files changed, 446 insertions(+), 1 deletion(-)
>
> diff --git a/include/libvirt/libvirt-domain.h
> b/include/libvirt/libvirt-domain.h
> index 2f5b01bbfe..5435ab7fcb 100644
> --- a/include/libvirt/libvirt-domain.h
> +++ b/include/libvirt/libvirt-domain.h
> @@ -5593,6 +5593,16 @@ typedef void
> (*virConnectDomainEventJobCompletedCallback)(virConnectPtr conn,
> */
> # define VIR_DOMAIN_TUNABLE_BLKDEV_WRITE_IOPS_SEC_MAX_LENGTH
> "blkdeviotune.write_iops_sec_max_length"
>
> +/**
> + * VIR_DOMAIN_THROTTLE_GROUP:
> + *
> + * Macro represents the name of throttle group for which the values are
> updated,
> + * as VIR_TYPED_PARAM_STRING.
> + *
> + * Since: 10.3.0
> + */
> +# define VIR_DOMAIN_THROTTLE_GROUP "throttlegroup.name"
So all of the APIs below take the group name as argument, why do we need
it a s typed parameter?
> +
[...]
> @@ -1726,4 +1745,7 @@ struct _virHypervisorDriver {
> virDrvDomainStartDirtyRateCalc domainStartDirtyRateCalc;
> virDrvDomainFDAssociate domainFDAssociate;
> virDrvDomainGraphicsReload domainGraphicsReload;
> +virDrvDomainSetThrottleGroup domainSetThrottleGroup;
> +virDrvDomainGetThrottleGroup domainGetThrottleGroup;
> +virDrvDomainDelThrottleGroup domainDelThrottleGroup;
> };
> diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
> index 7c6b93963c..da88457601 100644
> --- a/src/libvirt-domain.c
> +++ b/src/libvirt-domain.c
> @@ -14162,3 +14162,193 @@ virDomainGraphicsReload(virDomainPtr domain,
> virDispatchError(domain->conn);
> return -1;
> }
> +
> +
> +/**
> + * virDomainSetThrottleGroup:
> + * @dom: pointer to domain object
> + * @group: throttle group name
> + * @params: Pointer to blkio parameter objects
> + * @nparams: Number of blkio parameters (this value can be the same or
> + * less than the number of parameters supported)
> + * @flags: bitwise-OR of virDomainModificationImpact
> + *
> + * add or change throttle group.
Please bee more specific by includingwhich names of parameters are
accepted and/or reference other APIs which do this.
> + *
> + *
> + * Returns -1 in case of error, 0 in case of success.
> + *
> + * Since: 10.3.0
Don't forget to update all of these.
> + */
> +int
> +virDomainSetThrottleGroup(virDomainPtr dom,
> + const char *group,
> + virTypedParameterPtr params,
> + int nparams,
> + unsigned int flags)
> +{
> +virConnectPtr conn;
> +
> +VIR_DOMAIN_DEBUG(dom, "params=%p, nparams=%d, flags=0x%x",
> + params, nparams, flags);
> +VIR_TYPED_PARAMS_DEBUG(params, nparams);
> +
> +virResetLastError();
> +
> +virCheckDomainReturn(dom, -1);
> +conn = dom->conn;
> +
> +virCheckReadOnlyGoto(conn->flags, error);
> +virCheckPositiveArgGoto(nparams, error);
> +virCheckNonNullArgGoto(params, error);
> +
> +if (virTypedParameterValidateSet(dom->conn, params, nparams) < 0)
> +goto error;
> +
> +if (conn->driver->domainSetThrottleGroup) {
> +int ret;
> +ret = conn->driver->domainSetThrottleGroup(dom, group, params,
> nparams, flags);
> +if (ret < 0)
> +goto error;
> +return ret;
> +}
> +
>
Re: [PATCH RFC v2 09/12] tests: Test qemuMonitorJSONGetThrottleGroup and qemuMonitorJSONUpdateThrottleGroup
On Thu, Apr 11, 2024 at 19:01:57 -0700, w...@linux.ibm.com wrote:
> From: Chun Feng Wu
>
> Within "testQemuMonitorJSONqemuMonitorJSONUpdateThrottleGroup"
> * Test qemuMonitorJSONGetThrottleGroup
> * Test qemuMonitorJSONUpdateThrottleGroup, which updates limits through
> "qom-set"
>
> Signed-off-by: Chun Feng Wu
> ---
> tests/qemumonitorjsontest.c | 88 +
> 1 file changed, 88 insertions(+)
Ah okay so this is indeed testing both the getter and setter.
Move this patch either directly after the code implementing the monitor
or squash them together.
>
> diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c
> index 45cee23798..b4c243 100644
> --- a/tests/qemumonitorjsontest.c
> +++ b/tests/qemumonitorjsontest.c
> @@ -147,6 +147,32 @@ const char *queryBlockReply =
> "\"id\": \"libvirt-10\""
> "}";
>
> +const char *qomGetReply =
> +"{"
> +"\"return\": {"
> +" \"bps-total\": 1,"
> +" \"bps-read\": 2,"
> +" \"bps-write\": 3,"
> +" \"iops-total\": 4,"
> +" \"iops-read\": 5,"
> +" \"iops-write\": 6,"
> +" \"bps-total-max\": 7,"
> +" \"bps-read-max\": 8,"
> +" \"bps-write-max\": 9,"
> +" \"iops-total-max\": 10,"
> +" \"iops-read-max\": 11,"
> +" \"iops-write-max\": 12,"
> +" \"iops-size\": 13,"
> +" \"bps-total-max-length\": 15,"
> +" \"bps-read-max-length\": 16,"
> +" \"bps-write-max-length\": 17,"
> +" \"iops-total-max-length\": 18,"
> +" \"iops-read-max-length\": 19,"
> +" \"iops-write-max-length\": 20"
> +"},"
> +"\"id\": \"libvirt-12\""
> +"}";
This is in an extra variable ...
> +
> static int
> testQemuMonitorJSONGetStatus(const void *opaque)
> {
> @@ -1853,6 +1879,67 @@
> testQemuMonitorJSONqemuMonitorJSONSetBlockIoThrottle(const void *opaque)
> return ret;
> }
>
> +
> +static int
> +testQemuMonitorJSONqemuMonitorJSONUpdateThrottleGroup(const void *opaque)
> +{
> +const testGenericData *data = opaque;
> +virDomainXMLOption *xmlopt = data->xmlopt;
> +virDomainBlockIoTuneInfo info, expectedInfo;
> +g_autoptr(qemuMonitorTest) test = NULL;
> +
> +if (!(test = qemuMonitorTestNewSchema(xmlopt, data->schema)))
> +return -1;
> +
> +expectedInfo = (virDomainBlockIoTuneInfo) {1, 2, 3, 4, 5, 6, 7, 8, 9,
> 10, 11, 12, 13, NULL, 15, 16, 17, 18, 19, 20};
> +expectedInfo.group_name = g_strdup("limit0");
> +
> +if (qemuMonitorTestAddItem(test, "qom-get", qomGetReply) < 0)
> +return -1;
> +
> +if (qemuMonitorTestAddItemVerbatim(test,
> + "{\"execute\":\"qom-set\","
> + " \"arguments\":{\"property\":
> \"limits\","
> + "\"path\":
> \"limit1\","
> + "
> \"value\":{\"bps-total\": 1,"
> + "
> \"bps-read\": 2,"
> + "
> \"bps-write\": 3,"
> + "
> \"iops-total\": 4,"
> + "
> \"iops-read\": 5,"
> + "
> \"iops-write\": 6,"
> + "
> \"bps-total-max\": 7,"
> + "
> \"bps-read-max\": 8,"
> + "
> \"bps-write-max\": 9,"
> + "
> \"iops-total-max\": 10,"
> + "
> \"iops-read-max\": 11,"
> + "
> \"iops-write-max\": 12,"
> + "
> \"iops-size\": 13,"
> + "
> \"bps-total-max-length\": 15,"
> + "
> \"bps-read-max-length\": 16,"
> + "
> \"bps-write-max-length\": 17,"
> + "
> \"iops-total-max-length\": 18,"
> + "
> \"iops-read-max-length\": 19,"
> + "
> \"iops-write-max-length\": 20}},"
> + " \"id\":\"libvirt-2\"}",
> + NULL,
> + "{ \"return\" : {}}") < 0)
... while this
Re: [PATCH RFC v2 02/12] qemu: monitor: Add support for ThrottleGroup operations
On Thu, Apr 11, 2024 at 19:01:50 -0700, w...@linux.ibm.com wrote: > From: Chun Feng Wu > > * 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 > --- > src/qemu/qemu_monitor.c | 34 > src/qemu/qemu_monitor.h | 14 > src/qemu/qemu_monitor_json.c | 151 +++ > src/qemu/qemu_monitor_json.h | 14 > 4 files changed, 213 insertions(+) Also note that this should really be tested in qemumonitorjsontest. Especially the setter function as it will validate the object definitions against qemu's definitions. ___ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-le...@lists.libvirt.org
Re: [PATCH RFC v2 02/12] qemu: monitor: Add support for ThrottleGroup operations
On Thu, Apr 11, 2024 at 19:01:50 -0700, w...@linux.ibm.com wrote:
> From: Chun Feng Wu
>
> * 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
> ---
> src/qemu/qemu_monitor.c | 34
> src/qemu/qemu_monitor.h | 14
> src/qemu/qemu_monitor_json.c | 151 +++
> src/qemu/qemu_monitor_json.h | 14
> 4 files changed, 213 insertions(+)
>
> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
> index 34e2ccab97..2cd122ea9e 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", NULLSTR(qomid), info);
See below.
> +
> +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", NULLSTR(groupname), reply);
'qemuMonitorJSONGetThrottleGroup' requires that the group name is
non-NULL thus NULLSTR is not needed.
> +
> +QEMU_CHECK_MONITOR(mon);
> +
> +return qemuMonitorJSONGetThrottleGroup(mon, groupname, reply);
> +}
> +
> +
> int
> qemuMonitorVMStatusToPausedReason(const char *status)
[...]
> int qemuMonitorGetVersion(qemuMonitor *mon,
> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> index eb84a3d938..9abcb40df2 100644
> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c
> @@ -4633,6 +4633,157 @@ int qemuMonitorJSONGetBlockIoThrottle(qemuMonitor
> *mon,
> return qemuMonitorJSONBlockIoThrottleInfo(devices, qdevid, reply);
> }
>
> +
> +int
> +qemuMonitorMakeThrottleGroupLimits(virJSONValue *limits,
> + const virDomainThrottleGroupDef *group)
> +{
> +if (virJSONValueObjectAppendNumberLong(limits, "bps-total",
> group->total_bytes_sec)<0)
> +return -1;
> +if (virJSONValueObjectAppendNumberLong(limits, "bps-read",
> group->read_bytes_sec)<0)
> +return -1;
> +if (virJSONValueObjectAppendNumberLong(limits, "bps-write",
> group->write_bytes_sec)<0)
> +return -1;
> +if (virJSONValueObjectAppendNumberLong(limits, "iops-total",
> group->total_iops_sec)<0)
> +return -1;
> +if (virJSONValueObjectAppendNumberLong(limits, "iops-read",
> group->read_iops_sec)<0)
> +return -1;
> +if (virJSONValueObjectAppendNumberLong(limits, "iops-write",
> group->write_iops_sec)<0)
> +return -1;
> +if (virJSONValueObjectAppendNumberLong(limits, "bps-total-max",
> group->total_bytes_sec_max)<0)
> +return -1;
> +if (virJSONValueObjectAppendNumberLong(limits, "bps-read-max",
> group->read_bytes_sec_max)<0)
> +return -1;
> +if (virJSONValueObjectAppendNumberLong(limits, "bps-write-max",
> group->write_bytes_sec_max)<0)
> +return -1;
> +if (virJSONValueObjectAppendNumberLong(limits, "iops-total-max",
> group->total_iops_sec_max)<0)
> +return -1;
> +if (virJSONValueObjectAppendNumberLong(limits, "iops-read-max",
> group->read_iops_sec_max)<0)
> +return -1;
> +if (virJSONValueObjectAppendNumberLong(limits, "iops-write-max",
> group->write_iops_sec_max)<0)
> +return -1;
> +if (virJSONValueObjectAppendNumberLong(limits, "iops-size",
> group->size_iops_sec)<0)
> +return -1;
> +/* avoid error from QEMU: "the burst length cannot be 0" for
> throttlelimits
> + * when setting max-length
> + */
> +if (virJSONValueObjectAdd(&limits, "P:bps-total-max-length",
> group->total_bytes_sec_max_length, NULL)<0)
> +return -1;
> +if (virJSONValueObjectAdd(&limits, "P:bps-read-max-length",
> group->read_bytes_sec_max_length, NULL)<0)
> +return -1;
> +if (virJSONValueObjectAdd(&limits, "P:bps-write-max-length",
> group->write_bytes_sec_max_length, NULL)<0)
> +return -1;
> +if (virJSONValueObjectAdd(&limits, "P:iops-total-max-length",
> group->total_iops_sec_max_length, NULL)<0)
> +return -1;
> +if (virJSONValueObjectAdd(&limi
Re: [PATCH RFC v2 01/12] config: Introduce ThrottleGroup and ThrottleFilter
On Thu, Apr 11, 2024 at 19:01:49 -0700, w...@linux.ibm.com wrote:
> From: Chun Feng Wu
>
> * Define new structs 'virDomainThrottleGroupDef' and
> 'virDomainThrottleFilterDef'
> * Update _virDomainDef to include virDomainThrottleGroupDef
> * Update _virDomainDiskDef to include virDomainThrottleFilterDef
> * Support new resource operations for DOM XML and structs, corresponding
> "Parse" and "Format" methods are provided
>
> Signed-off-by: Chun Feng Wu
> ---
> src/conf/domain_conf.c | 386
> src/conf/domain_conf.h | 54 ++
> src/conf/virconftypes.h | 4 +
> 3 files changed, 444 insertions(+)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 48c5d546da..134a787dda 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -3747,6 +3747,54 @@ virDomainIOThreadIDDefArrayInit(virDomainDef *def,
> }
>
>
> +static virDomainThrottleFilterDef *
> +virDomainThrottleFilterDefNew(void)
Having an constructor can be handy if you need to set non zero values in
it.
> +{
> +virDomainThrottleFilterDef *def = g_new0(virDomainThrottleFilterDef, 1);
'g_new0' explicitly zeroes out the allocated memory.
Also virDomainThrottleFilterDef is declared with a 'virObject' member,
but this is not the correct way to instantiate 'virObjects' there's
plenty more boilerplate code needed to do that.
Do you even need reference counting for the struct? Otherwise you don't
probably need to use a virObject
> +
> +def->group_name = NULL;
So this is not needed and we avoid it.
> +
> +return def;
> +}
> +
> +
> +void
> +virDomainThrottleFilterDefFree(virDomainThrottleFilterDef *def)
> +{
> +if (!def)
> +return;
> +VIR_FREE(def->group_name);
> +VIR_FREE(def->nodename);
Preferrably new code should not use VIR_FREE bug g_free instead in
destructors (if the variable won't be around any more thus the value
doesn't matter.
> +virObjectUnref(def);
See note about proper virObject usage.
> +}
> +
> +
> +void
> +virDomainThrottleGroupDefFree(virDomainThrottleGroupDef *def)
> +{
> +if (!def)
> +return;
> +g_free(def->group_name);
> +g_free(def);
See, you do it here, please be consistent.
> +}
> +
> +
> +static void
> +virDomainThrottleGroupDefArrayFree(virDomainThrottleGroupDef **def,
> + int nthrottlegroups)
> +{
> +size_t i;
> +
> +if (!def)
> +return;
> +
> +for (i = 0; i < nthrottlegroups; i++)
> +virDomainThrottleGroupDefFree(def[i]);
> +
> +g_free(def);
> +}
> +
> +
> void
> virDomainResourceDefFree(virDomainResourceDef *resource)
> {
> @@ -4026,6 +4074,8 @@ void virDomainDefFree(virDomainDef *def)
>
> virDomainIOThreadIDDefArrayFree(def->iothreadids, def->niothreadids);
>
> +virDomainThrottleGroupDefArrayFree(def->throttlegroups,
> def->nthrottlegroups);
> +
> g_free(def->defaultIOThread);
>
> virBitmapFree(def->cputune.emulatorpin);
> @@ -7694,6 +7744,165 @@ virDomainDiskDefIotuneParse(virDomainDiskDef *def,
> #undef PARSE_IOTUNE
>
>
> +static virDomainThrottleFilterDef *
> +virDomainDiskThrottleFilterDefParse(xmlNodePtr node,
> +xmlXPathContextPtr ctxt)
Okay so as long as you are touching the XML parser this patch needs to
be also adding the XML docs and schema, or the schema and docs need to
be added *before* this patch. And the summary (first line) of the commit
message should then also state that it's adding XML parsing stuff as
that's way more improtant information
Also this patch doesn't really need to add both the '*Filter*' and
'*Group*' stuff at once, which would make the patch easier to go
through.
> +{
> +g_autoptr(virDomainThrottleFilterDef) filter =
> virDomainThrottleFilterDefNew();
> +
> +VIR_XPATH_NODE_AUTORESTORE(ctxt)
> +ctxt->node = node;
> +
> +filter->group_name = virXMLPropString(ctxt->node, "group");
Umm, this is not actually using the XPath context, you don't even need
to touch it and can use 'node' here directly.
> +
> +return g_steal_pointer(&filter);
> +}
> +
> +
> +static int
> +virDomainDiskDefThrottleFilterChainParse(virDomainDiskDef *def,
> + xmlXPathContextPtr ctxt)
> +{
> +size_t i;
> +int n = 0;
> +g_autofree xmlNodePtr *nodes = NULL;
> +
> +if ((n = virXPathNodeSet("./throttlefilters/throttlefilter", ctxt,
> &nodes)) < 0)
> +return -1;
> +
> +if (n)
> +def->throttlefilters = g_new0(virDomainThrottleFilterDef *, n);
> +
> +for (i = 0; i < n; i++) {
> +g_autoptr(virDomainThrottleFilterDef) filter = NULL;
> +
> +if (!(filter = virDomainDiskThrottleFilterDefParse(nodes[i], ctxt)))
> {
> +return -1;
This function currently can't fail so this error reporting makes no
sense.
> +}
> +
> +if (virDomainThrottleFilterFind(def, filter->group_name)) {
Note that 'virDomainDiskT
