Re: [libvirt] [RFC PATCH] Add new migration flag VIR_MIGRATE_DRY_RUN
On 11/12/18 4:26 AM, Daniel P. Berrangé wrote:
On Fri, Nov 02, 2018 at 04:34:02PM -0600, Jim Fehlig wrote:
A dry run can be used as a best-effort check that a migration command
will succeed. The destination host will be checked to see if it can
accommodate the resources required by the domain. DRY_RUN will fail if
the destination host is not capable of running the domain. Although a
subsequent migration will likely succeed, the success of DRY_RUN does not
ensure a future migration will succeed. Resources on the destination host
could become unavailable between a DRY_RUN and actual migration.
I'm not really convinced this is a particularly useful concept,
as it is only going to catch a very small number of the reasons
why migration can fail. So you still have to expect the real
migration invokation to have a strong chance of failing.
I agree it is difficult to reliably check that a migration will succeed. TBH, I
was expecting opposition due to libvirt already providing info for applications
to do the check themselves. E.g. as nova has done with
check_can_live_migrate_{source,destination} APIs.
Do you think libvirt provides enough information for an app to determine if a VM
can be migrated between two hosts? Or maybe better asked: What info is currently
missing for an app to reliably check if a VM can be migrated between two hosts?
Signed-off-by: Jim Fehlig
---
If it is agreed this is useful, my thought was to use the begin and
prepare phases of migration to implement it. qemuMigrationDstPrepareAny()
already does a lot of the heavy lifting wrt checking the host can
accommodate the domain. Some of it, and the remaining migration phases,
can be short-circuited in the case of dry run.
One interesting wrinkle I've observed is the check for cpu compatibility.
AFAICT qemu is actually invoked on the dst, "filtered-features" of the cpu
are requested via qmp, and results are checked against cpu in domain config.
If cpu on dst is insufficient, migration fails in the prepare phase with
something like "guest CPU doesn't match specification: missing features: z y z".
I was hoping to avoid launching qemu in the case of dry run, but that may
be unavoidable if we'd like a dependable dry run result.
Even launching QEMU isn't good enough - it has to actually process the
migration data stream for devices to get a good indication of success,
at which point you're basically doing a real migration.
Bummer. I guess that answers my question above: no. It also implies apps cannot
reliably check if a migration will succeed and should instead put effort into
handling errors from an actual migration :-).
Regards,
Jim
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3] lxc: Include support to lxc version 3.0 or higher.
On 11/11/18 12:46 PM, Julio Faracco wrote:
> Em sáb, 10 de nov de 2018 às 11:17, John Ferlan escreveu:
>>
>>
>>
>> On 11/9/18 12:30 PM, Julio Faracco wrote:
>>> This patch introduce the new settings for LXC 3.0 or higher. The older
>>> versions keep the compatibility to deprecated settings for LXC, but
>>> after release 3.0, the compatibility was removed. This commit adds the
>>> support to the refactored settings.
>>>
>>> v1-v2: Michal's suggestions to handle differences between versions.
>>> v2-v3: Adding suggestions from Pino and John too.
>>
>> These type of comments would go below the --- below so that they're not
>> part of commit history...
>>
>>>
>>> Signed-off-by: Julio Faracco
>>> ---
>>> src/lxc/lxc_native.c | 45 +++-
>>> 1 file changed, 32 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/src/lxc/lxc_native.c b/src/lxc/lxc_native.c
>>> index cbdec723dd..d3ba12bb0e 100644
>>> --- a/src/lxc/lxc_native.c
>>> +++ b/src/lxc/lxc_native.c
>>
>> [...]
>>
>>> @@ -724,12 +734,13 @@ lxcIdmapWalkCallback(const char *name,
>>> virConfValuePtr value, void *data)
>>> char type;
>>> unsigned long start, target, count;
>>>
>>> -if (STRNEQ(name, "lxc.id_map") || !value->str)
>>> +/* LXC 3.0 uses "lxc.idmap", while legacy used "lxc.id_map" */
>>> +if (STRNEQ(name, "lxc.idmap") || STRNEQ(name, "lxc.id_map") ||
>>> !value->str)
>>> return 0;
>>
>> This one caused lxcconf2xmltest to fail and needs to change to:
>>
>> /* LXC 3.0 uses "lxc.idmap", while legacy used "lxc.id_map" */
>> if (STRNEQ(name, "lxc.idmap") || !value->str) {
>> if (!value->str || STRNEQ(name, "lxc.id_map"))
>> return 0;
>> }
>>
>> The failure occurred because of the STRNEQ OR not being true (silly me
>> on first pass not running the tests too ;-))
>>
>>>
>>> if (sscanf(value->str, "%c %lu %lu %lu", ,
>>> , , ) != 4) {
>>> -virReportError(VIR_ERR_INTERNAL_ERROR, _("invalid lxc.id_map:
>>> '%s'"),
>>> +virReportError(VIR_ERR_INTERNAL_ERROR, _("invalid: '%s'"),
>>
>> Do you mind if I alter this to:
>>
>> virReportError(VIR_ERR_INTERNAL_ERROR, _("invalid %s: '%s'"),
>>name, value->str);
>>
>> That way the conf name string is provided like it was before
>>
>>
>>> value->str);
>>> return -1;
>>> }
>>> @@ -1041,19 +1052,27 @@ lxcParseConfigString(const char *config,
>>> if (VIR_STRDUP(vmdef->os.init, "/sbin/init") < 0)
>>> goto error;
>>>
>>> -if (virConfGetValueString(properties, "lxc.utsname", ) <= 0 ||
>>> -VIR_STRDUP(vmdef->name, value) < 0)
>>> -goto error;
>>> +if (virConfGetValueString(properties, "lxc.uts.name", ) <= 0) {
>>> +virResetLastError();
>>> +
>>> +/* Check for pre LXC 3.0 legacy key */
>>> +if (virConfGetValueString(properties, "lxc.utsname", ) <= 0)
>>> +goto error;
>>> +}
>>> +
>>
>> I think in this case the @value needs to be restored... Previous if the
>> GetValueString OR the VIR_STRDUP of the value was < 0, we go to error.
>> Although I'm not quite sure how @value would be NULL so as to cause the
>> subsequent line to be executed... In any case, copying @value needs to
>> be done, so add:
>>
>> if (VIR_STRDUP(vmdef->name, value) < 0)
>> goto error;
>>
>> Which I can add if you agree.
>
> No problems, John. You can go ahead with the changes.
> I forgot too add VIR_STRDUP after checking the property.
> It was causing the test error.
>
>>
>> With those changes,
>>
>> Reviewed-by: John Ferlan
>>
>> John
>>
>> As a follow-up maybe adding/altering/updating the tests/lxcconf2xmldata
>> to include both pre and post 3.0 type data would be a good thing.
>
> Yes, I agree too. But only config files that don't have netowork settings.
> Version 3.X and higher have another syntax to configure network too.
> And it was not implemented yet. I'm planning to propose this feature
> in the future.
>
>>
>> [...]
Since you have access to the V3.0 environment, perhaps it's best that
you update the patch based on my comments and also add the test *.config
files using the v3 syntax.
John
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/2] conf: Move VFIO AP validation from post parse to QEMU validation code
On 11/12/18 1:14 PM, Erik Skultety wrote:
Even though commit 11708641 claims that a domain is allowed have a
single VFIO AP hostdev only, this is a limitation posed by the platform
vendor on purely virtual devices. Generally, post parse should only be
I am little confused by the term "purely virtual devices".
If you are talking about the mediated device itself "purely virtual"
sounds okay but if you also consider what it represents within a guest
than that is no longer "purely virtual" since a vfio-ap hostdev
represents a bunch of "real crypto hardware" that is passed through to
the guest.
used to populate some default values if missing or at least fail
gracefully with VIR_DOMAIN_DEF_PARSE_ALLOW_POST_PARSE_FAIL).
This patch more of an attempt to follow the guidelines for adding new
features rather than a precaution measure, since even if vfio-ap
supported multiple devices, one would have to downgrade libvirt for a
machine to vanish from the list or in terms of future device migration
to slightly older libvirt, there would be most probably a driver mismatch
that would render the migration impossible anyway.
I agree that this is the correct place for the checking. Thanks for
catching and fixing it. I successfully ran some tests with these changes
with regard to vfio-ap. Looks good to me so far.
Signed-off-by: Erik Skultety
---
src/conf/domain_conf.c | 28
src/qemu/qemu_domain.c | 28 +++-
2 files changed, 27 insertions(+), 29 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 237540bccc..e8e0adc819 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -4275,31 +4275,6 @@ virDomainDefPostParseGraphics(virDomainDef *def)
}
-static int
-virDomainDefPostParseHostdev(virDomainDefPtr def)
-{
-size_t i;
-bool vfioap_found = false;
-
-/* verify settings of hostdevs vfio-ap */
-for (i = 0; i < def->nhostdevs; i++) {
-virDomainHostdevDefPtr hostdev = def->hostdevs[i];
-
-if (virHostdevIsMdevDevice(hostdev) &&
-hostdev->source.subsys.u.mdev.model ==
VIR_MDEV_MODEL_TYPE_VFIO_AP) {
-if (vfioap_found) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("Only one hostdev of model vfio-ap is "
- "supported"));
-return -1;
-}
-vfioap_found = true;
-}
-}
-return 0;
-}
-
-
/**
* virDomainDriveAddressIsUsedByDisk:
* @def: domain definition containing the disks to check
@@ -5210,9 +5185,6 @@ virDomainDefPostParseCommon(virDomainDefPtr def,
virDomainDefPostParseGraphics(def);
-if (virDomainDefPostParseHostdev(def) < 0)
-return -1;
-
if (virDomainDefPostParseCPU(def) < 0)
return -1;
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 17d207513d..90253ae867 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -4599,6 +4599,32 @@ qemuDomainMdevDefVFIOPCIValidate(const
virDomainHostdevSubsysMediatedDev *dev,
}
+static int
+qemuDomainMdevDefVFIOAPValidate(const virDomainDef *def)
+{
+size_t i;
+bool vfioap_found = false;
+
+/* currently, VFIO-AP is restricted to a single device only */
Well, even so it is just on mdev device it defines the complete set of
crypto devices consisting of adapters, domains and controldomains on the
AP bus of the guest. The ap architecture allows only one such
configuration.
So I suggest to remove "currently," and instead of "single device" to
write "single mediated device".
Besides that
Reviewed-by: Boris Fiuczynski
+for (i = 0; i < def->nhostdevs; i++) {
+virDomainHostdevDefPtr hostdev = def->hostdevs[i];
+
+if (virHostdevIsMdevDevice(hostdev) &&
+hostdev->source.subsys.u.mdev.model ==
VIR_MDEV_MODEL_TYPE_VFIO_AP) {
+if (vfioap_found) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("Only one hostdev of model vfio-ap is "
+ "supported"));
+return -1;
+}
+vfioap_found = true;
+}
+}
+
+return 0;
+}
+
+
static int
qemuDomainMdevDefValidate(const virDomainHostdevSubsysMediatedDev *mdevsrc,
const virDomainDef *def,
@@ -4609,7 +4635,7 @@ qemuDomainMdevDefValidate(const
virDomainHostdevSubsysMediatedDev *mdevsrc,
case VIR_MDEV_MODEL_TYPE_VFIO_PCI:
return qemuDomainMdevDefVFIOPCIValidate(mdevsrc, def, qemuCaps);
case VIR_MDEV_MODEL_TYPE_VFIO_AP:
-break;
+return qemuDomainMdevDefVFIOAPValidate(def);
case VIR_MDEV_MODEL_TYPE_VFIO_CCW:
break;
case VIR_MDEV_MODEL_TYPE_LAST:
--
Mit freundlichen Grüßen/Kind regards
Boris Fiuczynski
IBM Deutschland Research & Development GmbH
Vorsitzender des
Re: [libvirt] [Qemu-devel] [PULL 0/4] Fixes 31 20181112 patches
On 12 November 2018 at 15:15, Gerd Hoffmann wrote: > The following changes since commit 460f0236c12a86a38692c12d9bf8e2391dc10a77: > > Merge remote-tracking branch 'remotes/thibault/tags/samuel-thibault' into > staging (2018-11-12 10:12:07 +) > > are available in the git repository at: > > git://git.kraxel.org/qemu tags/fixes-31-20181112-pull-request > > for you to fetch changes up to f1aba960cc40ab65fa88c8678883bd2201708c55: > > ui/gtk: fix cursor in egl mode (2018-11-12 14:15:54 +0100) > > > fixes for 3.1: mark bt as deprecated, bugfixes for pulse, gtk and edid. > > > > Gerd Hoffmann (2): > pulseaudio: process audio data in smaller chunks > ui/gtk: fix cursor in egl mode > > Marc-André Lureau (1): > edid: silence a stringop-overflow warning > > Thomas Huth (1): > bt: Mark the bluetooth subsystem as deprecated Applied, thanks. -- PMM -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH RFC 08/22] qemu_process: Persist stderr in qemuProcess struct
On 11/11/2018 08:59 PM, Chris Venteicher wrote:
> A qemuProcess struct tracks the entire lifespan of a single QEMU Process
> including storing error output when the process terminates or activation
> fails.
>
> Error output remains available until qemuProcessFree is called.
>
> The qmperr buffer no longer needs to be maintained outside of the
> qemuProcess structure because the structure is used for a single QEMU
> process and the structures can be maintained as long as required
> to retrieve the process error info.
>
> Capabilities init code is refactored but continues to report QEMU
> process error data only when the initial (non KVM) QEMU process activation
> fails to result in a usable monitor connection for retrieving
> capabilities.
>
> The VIR_ERR_INTERNAL_ERROR "Failed to probe QEMU binary" reporting is
> moved into virQEMUCapsInitQMP function and the stderr string is
> extracted from the qemuProcess struct using a macro to retrieve the
> string.
>
> The same error and log message should be generated, in the same
> conditions, after this patch as before.
>
> Signed-off-by: Chris Venteicher
> ---
> src/qemu/qemu_capabilities.c | 27 ---
> src/qemu/qemu_process.c | 12
> src/qemu/qemu_process.h | 6 --
> 3 files changed, 20 insertions(+), 25 deletions(-)
>
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index a957c3bdbd..f5e327097e 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -4216,8 +4216,7 @@ static int
> virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps,
> const char *libDir,
> uid_t runUid,
> - gid_t runGid,
> - char **qmperr)
> + gid_t runGid)
> {
> qemuProcessPtr proc = NULL;
> qemuProcessPtr proc_kvm = NULL;
> @@ -4226,7 +4225,7 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps,
> bool forceTCG = false;
>
> if (!(proc = qemuProcessNew(qemuCaps->binary, libDir,
> -runUid, runGid, qmperr, forceTCG)))
> +runUid, runGid, forceTCG)))
> goto cleanup;
>
> if ((rc = qemuProcessRun(proc)) != 0) {
> @@ -4242,7 +4241,7 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps,
> qemuProcessStopQmp(proc);
>
> forceTCG = true;
> -proc_kvm = qemuProcessNew(qemuCaps->binary, libDir, runUid, runGid,
> NULL, forceTCG);
> +proc_kvm = qemuProcessNew(qemuCaps->binary, libDir, runUid, runGid,
> forceTCG);
>
> if ((rc = qemuProcessRun(proc_kvm)) != 0) {
> if (rc == 1)
> @@ -4257,6 +4256,13 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps,
> ret = 0;
>
> cleanup:
> +if (proc && !proc->mon) {
> +char *err = QEMU_PROCESS_ERROR(proc) ? QEMU_PROCESS_ERROR(proc) :
> _("unknown error");
or just:
char *err = proc->qmperr;
if (!err)
err = _("uknown error");
> +
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Failed to probe QEMU binary with QMP: %s"), err);
> +}
> +
> qemuProcessStopQmp(proc);
> qemuProcessStopQmp(proc_kvm);
> qemuProcessFree(proc);
> @@ -4296,7 +4302,6 @@ virQEMUCapsNewForBinaryInternal(virArch hostArch,
> {
> virQEMUCapsPtr qemuCaps;
> struct stat sb;
> -char *qmperr = NULL;
>
> if (!(qemuCaps = virQEMUCapsNew()))
> goto error;
> @@ -4323,15 +4328,8 @@ virQEMUCapsNewForBinaryInternal(virArch hostArch,
> goto error;
> }
>
> -if (virQEMUCapsInitQMP(qemuCaps, libDir, runUid, runGid, ) < 0) {
> -virQEMUCapsLogProbeFailure(binary);
> -goto error;
> -}
> -
> -if (!qemuCaps->usedQMP) {
> -virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("Failed to probe QEMU binary with QMP: %s"),
> - qmperr ? qmperr : _("unknown error"));
> +if (virQEMUCapsInitQMP(qemuCaps, libDir, runUid, runGid) < 0 ||
> +!qemuCaps->usedQMP) {
> virQEMUCapsLogProbeFailure(binary);
Oh, this won't fly. So virReportError() sets the error object and
virQEMUCapsLogProbeFailure() uses it (by calling
virGetLastErrorMessage()). But since you're removing the
virReportError() call then there's no error object to get the error
message from. IOW this will probably log: "Failed to probe capabilities
for /usr/bin/qemu: no error" even though later the actual qemu error
message is logged.
Up until now, patches 01-07 look reasonable.
> goto error;
> }
> @@ -4350,7 +4348,6 @@ virQEMUCapsNewForBinaryInternal(virArch hostArch,
> }
>
> cleanup:
> -VIR_FREE(qmperr);
> return qemuCaps;
>
> error:
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index dda74d5b7a..a741d1cf91 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -8091,6 +8091,7 @@ qemuProcessFree(qemuProcessPtr proc)
>
Re: [libvirt] [PATCH RFC 07/22] qemu_process: Use qemuProcess struct for a single process
On 11/11/2018 08:59 PM, Chris Venteicher wrote:
> In new process code, move from model where qemuProcess struct can be
> used to activate a series of Qemu processes to model where one
> qemuProcess struct is used for one and only one Qemu process.
>
> The forceTCG parameter (use / don't use KVM) will be passed when the
> qemuProcess struct is initialized since the qemuProcess struct won't be
> reused.
>
> Signed-off-by: Chris Venteicher
> ---
> src/qemu/qemu_capabilities.c | 16
> src/qemu/qemu_process.c | 11 +++
> src/qemu/qemu_process.h | 6 --
> 3 files changed, 23 insertions(+), 10 deletions(-)
>
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index 082874082b..a957c3bdbd 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -4220,14 +4220,16 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps,
> char **qmperr)
> {
> qemuProcessPtr proc = NULL;
> +qemuProcessPtr proc_kvm = NULL;
> int ret = -1;
> int rc;
> +bool forceTCG = false;
>
> if (!(proc = qemuProcessNew(qemuCaps->binary, libDir,
> -runUid, runGid, qmperr)))
> +runUid, runGid, qmperr, forceTCG)))
> goto cleanup;
>
> -if ((rc = qemuProcessRun(proc, false)) != 0) {
> +if ((rc = qemuProcessRun(proc)) != 0) {
> if (rc == 1)
> ret = 0;
> goto cleanup;
> @@ -4238,13 +4240,17 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps,
>
> if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM)) {
> qemuProcessStopQmp(proc);
> -if ((rc = qemuProcessRun(proc, true)) != 0) {
> +
> +forceTCG = true;
> +proc_kvm = qemuProcessNew(qemuCaps->binary, libDir, runUid, runGid,
> NULL, forceTCG);
This is a very long line. There's this limit of 80 characters per line
(although it's a soft limit).
> +
> +if ((rc = qemuProcessRun(proc_kvm)) != 0) {
> if (rc == 1)
> ret = 0;
> goto cleanup;
> }
>
> -if (virQEMUCapsInitQMPMonitorTCG(qemuCaps, proc->mon) < 0)
> +if (virQEMUCapsInitQMPMonitorTCG(qemuCaps, proc_kvm->mon) < 0)
> goto cleanup;
> }
>
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH for-3.2 v2] vhost-user: define conventions for vhost-user backends
On Wed, Nov 07, 2018 at 07:13:11PM +0400, Marc-André Lureau wrote:
> As discussed during "[PATCH v4 00/29] vhost-user for input & GPU"
> review, let's define a common set of backend conventions to help with
> management layer implementation, and interoperability.
>
> v2:
> - use a vhost-user.json schema to discover backends and describe
>capability format
> - drop --pidfile
> - add some notes about daemonizing & stdin/out/err
>
> Cc: libvir-list@redhat.com
> Cc: Gerd Hoffmann
> Cc: Daniel P. Berrangé
> Cc: Changpeng Liu
> Cc: Dr. David Alan Gilbert
> Cc: Felipe Franciosi
> Cc: Gonglei
> Cc: Maxime Coquelin
> Cc: Michael S. Tsirkin
> Cc: Victor Kaplansky
> Signed-off-by: Marc-André Lureau
> ---
> MAINTAINERS | 1 +
> docs/interop/vhost-user.json | 219 +++
> docs/interop/vhost-user.txt | 101 +++-
> 3 files changed, 319 insertions(+), 2 deletions(-)
> create mode 100644 docs/interop/vhost-user.json
> diff --git a/docs/interop/vhost-user.json b/docs/interop/vhost-user.json
> new file mode 100644
> index 00..91b5bf499e
> --- /dev/null
> +++ b/docs/interop/vhost-user.json
> @@ -0,0 +1,219 @@
> +# -*- Mode: Python -*-
> +#
> +# Copyright (C) 2018 Red Hat, Inc.
> +#
> +# Authors:
> +# Marc-André Lureau
> +#
> +# This work is licensed under the terms of the GNU GPL, version 2 or
> +# later. See the COPYING file in the top-level directory.
> +
> +##
> +# = vhost user backend discovery & capabilities
> +##
> +
> +##
> +# @VHostUserBackendType:
> +#
> +# List the various vhost user backend types.
> +#
> +# @net: virtio net
> +# @block: virtio block
> +# @console: virtio console
> +# @rng: virtio rng
> +# @balloon: virtio balloon
> +# @rpmsg: virtio remote processor messaging
> +# @scsi: virtio scsi
> +# @9p: 9p virtio console
> +# @rproc-serial: virtio remoteproc serial link
> +# @caif: virtio caif
> +# @gpu: virtio gpu
> +# @input: virtio input
> +# @vsock: virtio vsock transport
> +# @crypto: virtio crypto
Is it possible to actually use an external backend process with
all these yet ? If not, perhaps we should only start with the
backends that will be usable immediately ?
> +#
> +# Since: 3.2
> +##
> +{
> + 'enum': 'VHostUserBackendType',
> + 'data': [ 'net', 'block', 'console', 'rng', 'balloon', 'rpmsg',
> +'scsi', '9p', 'rproc-serial', 'caif', 'gpu', 'input', 'vsock',
> +'crypto' ]
> +}
Regardless of the answer to the above question,
Reviewed-by: Daniel P. Berrangé
Regards,
Daniel
--
|: 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 :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH RFC 04/22] qemu_process: Refer to proc not cmd in process code
On 11/11/2018 08:59 PM, Chris Venteicher wrote:
> s/cmd/proc/ in process code imported from qemu_capabilities.
>
> No functionality is changed. Just variable renaming.
>
> Process code imported from qemu_capabilities was oriented around
> starting a process to issue a single QMP command.
>
> Future usecases (ex. baseline, compare) expect to use a single process
> to issue multiple different QMP commands.
>
> This patch changes the variable naming from cmd to proc to put focus
> on the process being maintained to issue commands.
>
> Signed-off-by: Chris Venteicher
> ---
> src/qemu/qemu_capabilities.c | 14 ++--
> src/qemu/qemu_process.c | 140 +--
> src/qemu/qemu_process.h | 6 +-
> 3 files changed, 80 insertions(+), 80 deletions(-)
>
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index f6d97648ce..1ea63000e2 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -4219,7 +4219,7 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps,
> gid_t runGid,
> char **qmperr)
> {
> -qemuProcessPtr cmd = NULL;
> +qemuProcessPtr proc = NULL;
> int ret = -1;
> int rc;
This is actually the place where the problem I've raised in 02/22 should
be addressed. s/cmd/proc/ should happen here.
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH RFC 02/22] qemu_process: Use qemuProcess prefix
On 11/11/2018 08:59 PM, Chris Venteicher wrote:
> s/virQEMUCapsInitQMPCommand/qemuProcess/
>
> No functionality change.
>
> Use appropriate prefix in moved code.
>
> Signed-off-by: Chris Venteicher
> ---
> src/qemu/qemu_capabilities.c | 14 +++---
> src/qemu/qemu_process.c | 28 ++--
> src/qemu/qemu_process.h | 22 +++---
> 3 files changed, 32 insertions(+), 32 deletions(-)
>
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index 0f70fdf46d..f6d97648ce 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -4219,15 +4219,15 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps,
> gid_t runGid,
> char **qmperr)
> {
> -virQEMUCapsInitQMPCommandPtr cmd = NULL;
> +qemuProcessPtr cmd = NULL;
> int ret = -1;
> int rc;
>
> -if (!(cmd = virQEMUCapsInitQMPCommandNew(qemuCaps->binary, libDir,
> - runUid, runGid, qmperr)))
> +if (!(proc = qemuProcessNew(qemuCaps->binary, libDir,
Ooops, this needs to stay @cmd. The idea is that after every single
commit one has to be able to 'make all syntax-check check'.
> +runUid, runGid, qmperr)))
> goto cleanup;
>
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] conf: Add new module node_device_util
On Mon, Nov 12, 2018 at 03:25:02PM +0100, Michal Privoznik wrote: > On 11/12/2018 12:56 PM, Erik Skultety wrote: > > There's a lot of stuff going on in src/conf/nodedev_conf which is > > sometimes not directly related to config and we're not really consistent > > with putting only parser/formatter related stuff here, e.g. like we do > > for domains. So, let's start simply by adding a new module > > node_device_util containing some of the helpers. Unfortunately, even > > though these helpers tend to open a secondary driver connection and would > > be much therefore better suited as a nodedev driver module, we can't do > > that without pulling headers from the driver into conf/ and that's wrong > > because we want conf/ to stay driver-agnostic. > > > > Signed-off-by: Erik Skultety > > --- > > src/conf/Makefile.inc.am | 2 + > > src/conf/node_device_conf.c | 199 --- > > src/conf/node_device_conf.h | 11 -- > > src/conf/node_device_util.c | 229 +++ > > src/conf/node_device_util.h | 35 > > src/conf/virstorageobj.c | 1 + > > src/libvirt_private.syms | 7 +- > > src/node_device/node_device_driver.c | 1 + > > src/storage/storage_backend_scsi.c | 1 + > > 9 files changed, 273 insertions(+), 213 deletions(-) > > create mode 100644 src/conf/node_device_util.c > > create mode 100644 src/conf/node_device_util.h > > ACK Pushed, thanks. Erik -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] qemu: Extract MDEV VFIO PCI validation code into a separate helper
On 11/12/18 1:14 PM, Erik Skultety wrote:
Since we'll need to validate other models apart from VFIO PCI too,
having a helper for each model should keep the code base cleaner.
Signed-off-by: Erik Skultety
---
src/qemu/qemu_domain.c | 35 +--
1 file changed, 29 insertions(+), 6 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index e25afdad6b..17d207513d 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -4564,11 +4564,11 @@ qemuDomainDeviceDefValidateNetwork(const
virDomainNetDef *net)
static int
-qemuDomainMdevDefValidate(const virDomainHostdevSubsysMediatedDev *mdevsrc,
- const virDomainDef *def,
- virQEMUCapsPtr qemuCaps)
+qemuDomainMdevDefVFIOPCIValidate(const virDomainHostdevSubsysMediatedDev *dev,
+ const virDomainDef *def,
+ virQEMUCapsPtr qemuCaps)
{
-if (mdevsrc->display == VIR_TRISTATE_SWITCH_ABSENT)
+if (dev->display == VIR_TRISTATE_SWITCH_ABSENT)
return 0;
if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VFIO_PCI_DISPLAY)) {
@@ -4578,7 +4578,7 @@ qemuDomainMdevDefValidate(const
virDomainHostdevSubsysMediatedDev *mdevsrc,
return -1;
}
-if (mdevsrc->model != VIR_MDEV_MODEL_TYPE_VFIO_PCI) {
+if (dev->model != VIR_MDEV_MODEL_TYPE_VFIO_PCI) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_(" attribute 'display' is only supported"
" with model='vfio-pci'"));
@@ -4586,7 +4586,7 @@ qemuDomainMdevDefValidate(const
virDomainHostdevSubsysMediatedDev *mdevsrc,
return -1;
}
-if (mdevsrc->display == VIR_TRISTATE_SWITCH_ON) {
+if (dev->display == VIR_TRISTATE_SWITCH_ON) {
if (def->ngraphics == 0) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("graphics device is needed for attribute value "
@@ -4599,6 +4599,29 @@ qemuDomainMdevDefValidate(const
virDomainHostdevSubsysMediatedDev *mdevsrc,
}
+static int
+qemuDomainMdevDefValidate(const virDomainHostdevSubsysMediatedDev *mdevsrc,
+ const virDomainDef *def,
+ virQEMUCapsPtr qemuCaps)
+{
+
Syntax-check does not like the above blank line... :-)
+switch ((virMediatedDeviceModelType) mdevsrc->model) {
+case VIR_MDEV_MODEL_TYPE_VFIO_PCI:
+return qemuDomainMdevDefVFIOPCIValidate(mdevsrc, def, qemuCaps);
+case VIR_MDEV_MODEL_TYPE_VFIO_AP:
+break;
+case VIR_MDEV_MODEL_TYPE_VFIO_CCW:
+break;
+case VIR_MDEV_MODEL_TYPE_LAST:
+virReportEnumRangeError(virMediatedDeviceModelType,
+mdevsrc->model);
+return -1;
+}
+
+return 0;
+}
+
+
static int
qemuDomainDeviceDefValidateHostdev(const virDomainHostdevDef *hostdev,
const virDomainDef *def,
Besides Marc question regard the default switch case
Reviewed-by: Boris Fiuczynski
--
Mit freundlichen Grüßen/Kind regards
Boris Fiuczynski
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Martina Köderitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PULL 4/4] ui/gtk: fix cursor in egl mode
In egl mode the scale_x and scale_y variables are not set, so the
scaling logic in the mouse motion event handler does not work.
Fix that. Also scale the cursor position in gd_egl_cursor_position().
Reported-by: Chen Zhang
Signed-off-by: Gerd Hoffmann
Tested-by: Chen Zhang
Message-id: 20181107074949.13805-1-kra...@redhat.com
---
ui/gtk-egl.c | 17 -
1 file changed, 12 insertions(+), 5 deletions(-)
diff --git a/ui/gtk-egl.c b/ui/gtk-egl.c
index a77c25b490..5420c2362b 100644
--- a/ui/gtk-egl.c
+++ b/ui/gtk-egl.c
@@ -68,8 +68,15 @@ void gd_egl_draw(VirtualConsole *vc)
return;
}
+window = gtk_widget_get_window(vc->gfx.drawing_area);
+ww = gdk_window_get_width(window);
+wh = gdk_window_get_height(window);
+
if (vc->gfx.scanout_mode) {
gd_egl_scanout_flush(>gfx.dcl, 0, 0, vc->gfx.w, vc->gfx.h);
+
+vc->gfx.scale_x = (double)ww / vc->gfx.w;
+vc->gfx.scale_y = (double)wh / vc->gfx.h;
} else {
if (!vc->gfx.ds) {
return;
@@ -77,13 +84,13 @@ void gd_egl_draw(VirtualConsole *vc)
eglMakeCurrent(qemu_egl_display, vc->gfx.esurface,
vc->gfx.esurface, vc->gfx.ectx);
-window = gtk_widget_get_window(vc->gfx.drawing_area);
-ww = gdk_window_get_width(window);
-wh = gdk_window_get_height(window);
surface_gl_setup_viewport(vc->gfx.gls, vc->gfx.ds, ww, wh);
surface_gl_render_texture(vc->gfx.gls, vc->gfx.ds);
eglSwapBuffers(qemu_egl_display, vc->gfx.esurface);
+
+vc->gfx.scale_x = (double)ww / surface_width(vc->gfx.ds);
+vc->gfx.scale_y = (double)wh / surface_height(vc->gfx.ds);
}
}
@@ -232,8 +239,8 @@ void gd_egl_cursor_position(DisplayChangeListener *dcl,
{
VirtualConsole *vc = container_of(dcl, VirtualConsole, gfx.dcl);
-vc->gfx.cursor_x = pos_x;
-vc->gfx.cursor_y = pos_y;
+vc->gfx.cursor_x = pos_x * vc->gfx.scale_x;
+vc->gfx.cursor_y = pos_y * vc->gfx.scale_y;
}
void gd_egl_release_dmabuf(DisplayChangeListener *dcl,
--
2.9.3
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PULL 0/4] Fixes 31 20181112 patches
The following changes since commit 460f0236c12a86a38692c12d9bf8e2391dc10a77: Merge remote-tracking branch 'remotes/thibault/tags/samuel-thibault' into staging (2018-11-12 10:12:07 +) are available in the git repository at: git://git.kraxel.org/qemu tags/fixes-31-20181112-pull-request for you to fetch changes up to f1aba960cc40ab65fa88c8678883bd2201708c55: ui/gtk: fix cursor in egl mode (2018-11-12 14:15:54 +0100) fixes for 3.1: mark bt as deprecated, bugfixes for pulse, gtk and edid. Gerd Hoffmann (2): pulseaudio: process audio data in smaller chunks ui/gtk: fix cursor in egl mode Marc-André Lureau (1): edid: silence a stringop-overflow warning Thomas Huth (1): bt: Mark the bluetooth subsystem as deprecated audio/paaudio.c| 4 ++-- hw/display/edid-generate.c | 2 +- ui/gtk-egl.c | 17 - vl.c | 4 qemu-deprecated.texi | 7 +++ qemu-options.hx| 4 6 files changed, 30 insertions(+), 8 deletions(-) -- 2.9.3 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PULL 1/4] bt: Mark the bluetooth subsystem as deprecated
From: Thomas Huth
It has been unmaintained since years, and there were only trivial or
tree-wide changes to the related files since many years, so the
code is likely very bitrotten and broken. For example the following
segfaults as soon as as you press a key:
qemu-system-x86_64 -usb -device usb-bt-dongle -bt hci -bt device:keyboard
Since we are not aware of anybody using bluetooth with the current
version of QEMU, let's mark the subsystem as deprecated, with a special
request for the users to write to the qemu-devel mailing list in case
they still use it (so we could revert the deprecation status in that
case).
Signed-off-by: Thomas Huth
Message-id: 1542016830-19189-1-git-send-email-th...@redhat.com
Signed-off-by: Gerd Hoffmann
---
vl.c | 4
qemu-deprecated.texi | 7 +++
qemu-options.hx | 4
3 files changed, 15 insertions(+)
diff --git a/vl.c b/vl.c
index 55bab005b6..fa25d1ae2d 100644
--- a/vl.c
+++ b/vl.c
@@ -3269,6 +3269,10 @@ int main(int argc, char **argv, char **envp)
break;
#endif
case QEMU_OPTION_bt:
+warn_report("The bluetooth subsystem is deprecated and will "
+"be removed soon. If the bluetooth subsystem is "
+"still useful for you, please send a mail to "
+"qemu-de...@nongnu.org with your usecase.");
add_device_config(DEV_BT, optarg);
break;
case QEMU_OPTION_audio_help:
diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
index 5d2d7a3588..cb4291f1e5 100644
--- a/qemu-deprecated.texi
+++ b/qemu-deprecated.texi
@@ -128,6 +128,13 @@ The @option{[hub_id name]} parameter tuple of the
'hostfwd_add' and
The ``ivshmem'' device type is replaced by either the ``ivshmem-plain''
or ``ivshmem-doorbell`` device types.
+@subsection bluetooth (since 3.1)
+
+The bluetooth subsystem is unmaintained since many years and likely bitrotten
+quite a bit. It will be removed without replacement unless some users speaks
+up at the @email{qemu-devel@@nongnu.org} mailing list with information about
+their usecases.
+
@section System emulator machines
@subsection pc-0.10 and pc-0.11 (since 3.0)
diff --git a/qemu-options.hx b/qemu-options.hx
index 38c7a978c1..ee379b32e3 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -2772,6 +2772,10 @@ logic. The Transport Layer is decided by the machine
type. Currently
the machines @code{n800} and @code{n810} have one HCI and all other
machines have none.
+Note: This option and the whole bluetooth subsystem is considered as
deprecated.
+If you still use it, please send a mail to @email{qemu-devel@@nongnu.org} where
+you describe your usecase.
+
@anchor{bt-hcis}
The following three types are recognized:
--
2.9.3
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PULL 3/4] pulseaudio: process audio data in smaller chunks
The rate of pulseaudio absorbing the audio stream is used to control the
the rate of the guests audio stream. When the emulated hardware uses
small chunks (like intel-hda does) we need small chunks on the audio
backend side too, otherwise that feedback loop doesn't work very well.
Cc: Max Ehrlich
Cc: Martin Schrodt
Buglink: https://bugs.launchpad.net/bugs/1795527
Signed-off-by: Gerd Hoffmann
Reviewed-by: Philippe Mathieu-Daudé
Message-id: 20181109142032.1628-1-kra...@redhat.com
---
audio/paaudio.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/audio/paaudio.c b/audio/paaudio.c
index 949769774d..4c100bc318 100644
--- a/audio/paaudio.c
+++ b/audio/paaudio.c
@@ -227,7 +227,7 @@ static void *qpa_thread_out (void *arg)
}
}
-decr = to_mix = audio_MIN (pa->live, pa->g->conf.samples >> 2);
+decr = to_mix = audio_MIN(pa->live, pa->g->conf.samples >> 5);
rpos = pa->rpos;
if (audio_pt_unlock(>pt, __func__)) {
@@ -319,7 +319,7 @@ static void *qpa_thread_in (void *arg)
}
}
-incr = to_grab = audio_MIN (pa->dead, pa->g->conf.samples >> 2);
+incr = to_grab = audio_MIN(pa->dead, pa->g->conf.samples >> 5);
wpos = pa->wpos;
if (audio_pt_unlock(>pt, __func__)) {
--
2.9.3
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PULL 2/4] edid: silence a stringop-overflow warning
From: Marc-André Lureau
Simplify the code that doesn't need strncpy() since length of string
is already computed.
/home/elmarco/src/qemu/hw/display/edid-generate.c: In function 'edid_desc_text':
/home/elmarco/src/qemu/hw/display/edid-generate.c:168:5: error: 'strncpy'
specified bound depends on the length of the source argument
[-Werror=stringop-overflow=]
strncpy((char *)(desc + 5), text, len);
^~
/home/elmarco/src/qemu/hw/display/edid-generate.c:164:11: note: length computed
here
len = strlen(text);
^~~~
cc1: all warnings being treated as errors
Signed-off-by: Marc-André Lureau
Reviewed-by: Markus Armbruster
Message-id: 20181110111623.31356-1-marcandre.lur...@redhat.com
Signed-off-by: Gerd Hoffmann
---
hw/display/edid-generate.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/display/edid-generate.c b/hw/display/edid-generate.c
index bdf5e1d4d4..77d9127344 100644
--- a/hw/display/edid-generate.c
+++ b/hw/display/edid-generate.c
@@ -165,7 +165,7 @@ static void edid_desc_text(uint8_t *desc, uint8_t type,
if (len > 12) {
len = 12;
}
-strncpy((char *)(desc + 5), text, len);
+memcpy(desc + 5, text, len);
desc[5 + len] = '\n';
}
--
2.9.3
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] qemu: Extract MDEV VFIO PCI validation code into a separate helper
On Mon, Nov 12, 2018 at 03:03:34PM +0100, Marc Hartmayer wrote:
> On Mon, Nov 12, 2018 at 01:14 PM +0100, Erik Skultety
> wrote:
> > Since we'll need to validate other models apart from VFIO PCI too,
> > having a helper for each model should keep the code base cleaner.
> >
> > Signed-off-by: Erik Skultety
> > ---
> > src/qemu/qemu_domain.c | 35 +--
> > 1 file changed, 29 insertions(+), 6 deletions(-)
> >
> > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> > index e25afdad6b..17d207513d 100644
> > --- a/src/qemu/qemu_domain.c
> > +++ b/src/qemu/qemu_domain.c
> > @@ -4564,11 +4564,11 @@ qemuDomainDeviceDefValidateNetwork(const
> > virDomainNetDef *net)
> >
> >
> > static int
> > -qemuDomainMdevDefValidate(const virDomainHostdevSubsysMediatedDev *mdevsrc,
> > - const virDomainDef *def,
> > - virQEMUCapsPtr qemuCaps)
> > +qemuDomainMdevDefVFIOPCIValidate(const virDomainHostdevSubsysMediatedDev
> > *dev,
> > + const virDomainDef *def,
> > + virQEMUCapsPtr qemuCaps)
> > {
> > -if (mdevsrc->display == VIR_TRISTATE_SWITCH_ABSENT)
> > +if (dev->display == VIR_TRISTATE_SWITCH_ABSENT)
> > return 0;
> >
> > if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VFIO_PCI_DISPLAY)) {
> > @@ -4578,7 +4578,7 @@ qemuDomainMdevDefValidate(const
> > virDomainHostdevSubsysMediatedDev *mdevsrc,
> > return -1;
> > }
> >
> > -if (mdevsrc->model != VIR_MDEV_MODEL_TYPE_VFIO_PCI) {
> > +if (dev->model != VIR_MDEV_MODEL_TYPE_VFIO_PCI) {
> > virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> > _(" attribute 'display' is only supported"
> > " with model='vfio-pci'"));
> > @@ -4586,7 +4586,7 @@ qemuDomainMdevDefValidate(const
> > virDomainHostdevSubsysMediatedDev *mdevsrc,
> > return -1;
> > }
> >
> > -if (mdevsrc->display == VIR_TRISTATE_SWITCH_ON) {
> > +if (dev->display == VIR_TRISTATE_SWITCH_ON) {
> > if (def->ngraphics == 0) {
> > virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> > _("graphics device is needed for attribute
> > value "
> > @@ -4599,6 +4599,29 @@ qemuDomainMdevDefValidate(const
> > virDomainHostdevSubsysMediatedDev *mdevsrc,
> > }
> >
> >
> > +static int
> > +qemuDomainMdevDefValidate(const virDomainHostdevSubsysMediatedDev *mdevsrc,
> > + const virDomainDef *def,
> > + virQEMUCapsPtr qemuCaps)
> > +{
> > +
> > +switch ((virMediatedDeviceModelType) mdevsrc->model) {
> > +case VIR_MDEV_MODEL_TYPE_VFIO_PCI:
> > +return qemuDomainMdevDefVFIOPCIValidate(mdevsrc, def, qemuCaps);
> > +case VIR_MDEV_MODEL_TYPE_VFIO_AP:
> > +break;
> > +case VIR_MDEV_MODEL_TYPE_VFIO_CCW:
> > +break;
> > +case VIR_MDEV_MODEL_TYPE_LAST:
> > +virReportEnumRangeError(virMediatedDeviceModelType,
> > +mdevsrc->model);
> > +return -1;
> > +}
>
> default case is missing? Otherwise looking good.
Yeah, it could only happen due to memory corruption, but you're right we should
stay both consistent and safe, consider it added.
Thanks,
Erik
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [REPOST PATCH v2 00/12] Allow modification of IOThread polling values (redux)
ping?
Tks,
John
On 11/5/18 7:58 AM, John Ferlan wrote:
> v2: https://www.redhat.com/archives/libvir-list/2018-October/msg01065.html
>
> NB: Minor mods for this are change using 4.10.0 instead of 4.9.0, merge of
> qemu_capabilities conflict, and updated news.xml
>
> .. v2 Cover Letter:
>
> v1: https://www.redhat.com/archives/libvir-list/2018-October/msg00456.html
>
> Changes since v1 (from code review)
>
> Patch 2: Move the job back into qemuDomainGetIOThreadsLive
>
> Patch 3: Add check for ActiveJob before allowing, use true for
> *StatsWorker, and print 'iothread' in output not 'block'
>
> Patch 5: Use virCheckPositiveArgGoto(nparams, error) instead of using
> virCheckNonNegativeArgGoto(nparams, error). And then remove
> the if (nparams) before the virCheckNonNullArgGoto(params, error);
>
> Patch 6: Add ability to determine which parameter was set via bool
> set_poll_{max_ns|grow|shrink} values. Then use those in
> the macro that sets the value to determine whether or not
> the value will be set based on whether it was changed.
>
> Patch 10: Use bool's to set_ when the value is found in the incoming
> params list. Remove the check that says poll_max_ns needs
> to be set. Testing proves that if it's set to 0, then the
> grow and shrink values can be changed (although they do
> nothing)
>
> Patch 12: (NEW) - News article
>
>
> .. v1 Cover Letter:
>
> This series attempts to resurrect the concept of being able to modify
> the IOThread polling parameters; however, in a slightly different
> manner than the previous series posted by posted by Pavel Hrdina
> :
>
> https://www.redhat.com/archives/libvir-list/2017-February/msg01047.html
>
> The work is prompted by continued pleas found in the bz:
>
> https://bugzilla.redhat.com/show_bug.cgi?id=1545732
>
> to provide some way to modify the paremters without needing to supply
> QEMU command line pass through values.
>
> It's accepted that the values being changed are fairly or extremely
> low level type knobs; however, it's also shown that by being able to
> turn the knob it is possible for certain, specific appliances to be
> able to gain a performance benefit for the thread at the expense of
> other competing threads.
>
> Unlike the previous series, this series does not attempt to save the
> polling values in the guest XML. Rather, it only modifies the live
> guest's IOThread with new values. It also doesn't provide the polling
> values in a virsh iothread* command, rather it uses the domstats
> in order to fetch and display the values. The theory being this
> leaves the onus on the higher level appliance/application to provide
> the "proper guidance" and "concerns" related to changing the values
> to the consumer. Not saving the values means whatever values that
> are chosen do not "live" in perpetuity. Once the guest is shut down
> or the IOThread removed from guest, the hypervisor default values
> take over again. Perhaps not a perfect situation in terms of what
> the bz requests; however, storage of default values that could
> cause performance issues is not an optimal situation. So this I
> figured is a "comprimise" of sorts.
>
> If it's still felt that no we don't want to do this, then fine,
> but please in doing so own the bz, state your case, and close it.
> I'm 50/50 on it, but figured at least I'd present this option and
> see what the concensus was.
>
>
> John Ferlan (12):
> qemu: Check for and return IOThread polling values if available
> qemu: Split qemuDomainGetIOThreadsLive
> qemu: Implement the ability to return IOThread stats
> virsh: Add ability to display IOThread stats
> lib: Introduce virDomainSetIOThreadParams
> qemu: Add monitor functions to set IOThread params
> qemu: Alter qemuDomainChgIOThread to take enum instead of bool
> qemu: Alter qemuDomainChgIOThread to take qemuMonitorIOThreadInfo
> qemu: Detect whether iothread polling is supported
> qemu: Introduce qemuDomainSetIOThreadParams
> tools: Add virsh iothreadset command
> docs: Add news article for IOThread polling
>
> docs/news.xml | 13 +
> include/libvirt/libvirt-domain.h | 45 ++
> src/driver-hypervisor.h | 8 +
> src/libvirt-domain.c | 108 +
> src/libvirt_public.syms | 5 +
> src/qemu/qemu_capabilities.c | 2 +
> src/qemu/qemu_capabilities.h | 1 +
> src/qemu/qemu_driver.c| 384 --
> src/qemu/qemu_monitor.c | 19 +
> src/qemu/qemu_monitor.h | 9 +
> src/qemu/qemu_monitor_json.c | 50 +++
> src/qemu/qemu_monitor_json.h | 4 +
> src/remote/remote_driver.c| 1 +
> src/remote/remote_protocol.x | 21
Re: [libvirt] [PATCH v2] qemu: Introduce caching whether /dev/kvm is accessible
On Mon, Nov 12, 2018 at 03:13 PM +0100, "Daniel P. Berrangé" wrote: > On Mon, Nov 12, 2018 at 02:48:09PM +0100, Marc Hartmayer wrote: >> On Mon, Nov 12, 2018 at 01:30 PM +0100, Pavel Hrdina >> wrote: >> > On Mon, Nov 12, 2018 at 12:50:41PM +0100, Marc Hartmayer wrote: >> >> On Thu, Nov 01, 2018 at 09:31 AM +0100, Martin Kletzander >> >> wrote: >> > >> > [...] >> > >> >> How can you run a machine/QEMU VM under a different user:group other >> >> than changing the user:group in qemu.conf and restart/reload libvirtd? >> >> >> >> As soon as a VM is running we have not to verify /dev/kvm access, no? >> >> (so there should be no problem when libvirtd tries to “reconnect” to >> >> already running VMs). >> > >> > You can add this into your domain XML: >> > >> > >> > phrdina:phrdina >> > >> > >> > And it will run the qemu process under that user. >> >> Interesting :) Actually, if we consider this then the QEMU caps caching >> is broken anyway since 'virQEMUCapsNewData' is calling >> 'virQEMUCapsNewForBinaryInternal(…, priv->runUid, priv->runGid, …)'. >> >> And 'priv->runUid/runGid' is only set once in virQEMUCapsCacheNew. >> >> Maybe I missed something. > > I dont think it is really broken - it merely impacts error reporting. Yep, you’re right. The use of “broken” was clearly exaggerated by me. > > When running 'virsh capabilities' we are trying to figure out if > KVM is usable on the host. This is always using the default uid/gid > so gives a fairly coarse answer, but the answer is correct for most > common usage. > > When building command line ARGV for spawning a specific QEMU > instance, the KVM capability merely affect whether libvirt > reports an error about the guest config. In the case where > the capability works with default uid/gid, but breaks with the > custom per-VM uid/gid, libvirt will mistakenly thing KVM is > usable and so launch the guest. QEMU will then be unable to > access it and quit with some suitable error message. > > So the "brokenness" about not using the per-VM uid/gid merely > delays the error reporting. I don't think this is important > enough to worry about, using the default uid/gid is sufficient. For this patch as well? > > Regards, > Daniel > -- > |: 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 :| > -- Kind regards / Beste Grüße Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzende des Aufsichtsrats: Martina Koederitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3] lxc: Include support to lxc version 3.0 or higher.
On 11/12/18 6:13 AM, Daniel P. Berrangé wrote: > On Fri, Nov 09, 2018 at 03:30:59PM -0200, Julio Faracco wrote: >> This patch introduce the new settings for LXC 3.0 or higher. The older >> versions keep the compatibility to deprecated settings for LXC, but >> after release 3.0, the compatibility was removed. This commit adds the >> support to the refactored settings. >> >> v1-v2: Michal's suggestions to handle differences between versions. >> v2-v3: Adding suggestions from Pino and John too. >> >> Signed-off-by: Julio Faracco >> --- >> src/lxc/lxc_native.c | 45 +++- >> 1 file changed, 32 insertions(+), 13 deletions(-) > I'd expect to additions to the test suite to cover these changes > eg lxcconf2xmltest data files Actually, I was just going to send mail saying that this patch breaks the *existing* lxcfonc2xml tests. (run "make check" and you'll see the failure, then run "VIR_TEST_DEBUG=1 tests/lxcconf2xml" to see more details about the failure - it's getting the name of the new domain wrong) pEpkey.asc Description: application/pgp-keys -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] conf: Add new module node_device_util
On 11/12/2018 12:56 PM, Erik Skultety wrote: > There's a lot of stuff going on in src/conf/nodedev_conf which is > sometimes not directly related to config and we're not really consistent > with putting only parser/formatter related stuff here, e.g. like we do > for domains. So, let's start simply by adding a new module > node_device_util containing some of the helpers. Unfortunately, even > though these helpers tend to open a secondary driver connection and would > be much therefore better suited as a nodedev driver module, we can't do > that without pulling headers from the driver into conf/ and that's wrong > because we want conf/ to stay driver-agnostic. > > Signed-off-by: Erik Skultety > --- > src/conf/Makefile.inc.am | 2 + > src/conf/node_device_conf.c | 199 --- > src/conf/node_device_conf.h | 11 -- > src/conf/node_device_util.c | 229 +++ > src/conf/node_device_util.h | 35 > src/conf/virstorageobj.c | 1 + > src/libvirt_private.syms | 7 +- > src/node_device/node_device_driver.c | 1 + > src/storage/storage_backend_scsi.c | 1 + > 9 files changed, 273 insertions(+), 213 deletions(-) > create mode 100644 src/conf/node_device_util.c > create mode 100644 src/conf/node_device_util.h ACK Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 03/13] security: Always spawn process for transactions
On 11/09/2018 03:02 PM, John Ferlan wrote:
>
>
> On 11/9/18 7:42 AM, Michal Privoznik wrote:
>> On 11/08/2018 11:45 PM, John Ferlan wrote:
>>>
>>>
>>> On 10/17/18 5:06 AM, Michal Privoznik wrote:
In the next commit the virSecurityManagerMetadataLock() is going
to be turned thread unsafe. Therefore, we have to spawn a
separate process for it. Always.
Signed-off-by: Michal Privoznik
---
src/security/security_dac.c | 2 +-
src/security/security_selinux.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
>>>
>>> Based slightly upon the KVM Forum presentation detailing some
>>> performance issues related to the usage of virFork for
>>> virQEMUCapsIsValid and calls to virFileAccessibleAs:
>>>
>>> https://www.redhat.com/archives/libvir-list/2018-October/msg01333.html
>>>
>>> Given that this patch would thus virFork "always", what kind of impact
>>> could that have? Have you been able to do any performance profiling?
>>> What would cause a single round of SELinux & DAC settings?
>>
>> This is what I was discussing with Daniel. Although I can't recall
>> where. Anyway, fork() is expensive sure, but my argumentation in
>> previous versions was that we are doing it already anyway. Namespaces
>> are enabled by default and unless users turned them off this adds no new
>> fork(). Only if namespaces are turned off then there is new fork(). At
>> any rate, this is one fork per one secdriver call. It's not one fork per
>> path (which would be horrible).
>>
>
> I too recall seeing the convo w/ Daniel, but couldn't find it in the
> recent patches... I wonder if it was on internal IRC.
>
> I did do a small amount of profiling before I asked, but my config
> doesn't have some of the qemu_command qemuSecuritySet* calls for sockets
> and tapfd's that are outside the batched qemuSecuritySetAllLabel. As an
> aside, qemuSecurityDomainSetPathLabel strays from the normal
> qemuSecurity{Set|Restore}* nomenclature (/me being grumpy about searches
> for call patterns and would gladly R-by a qemuSecuritySetDomain* patch
> as well as one that describes the functionality including the well
> there's no need to run the Restore because these are domain transient
> things that get removed anyway ;-)).
>
> Anyway, currently it's just a "handful" of calls, but some in areas that
> we seem to get flack on for. If we can get ahead of that flack because
> we know what we're about to do w/r/t virFork that'd be good.
>
>>>
>>> I know in an earlier patch I wasn't including performance of virFork
>>> because I believed that the only time it would be used would be for
>>> metadata locking when (re)labeling would be batched and for that case
>>> the "one off" virFork would seem reasonable.
>>
>> This is still true. There is no extra fork() if you have namespaces
>> enabled. Unfortunately, POSIX file locking is fubared and doing fork is
>> the least painful option.
>>
>>>
>>> Since it is possible to turn off the NS code and thus go through the
>>> direct call, I think there's "room for it" here too for those singular
>>> cases we could use "pid == -1" to indicate direct calls without virFork
>>> and "pid == 0" to batch together calls using virProcessRunInFork.
>>>
>>> That way when you *know* you are batching together more than singular
>>> changes, then the caller can choose to run in a fork knowing the
>>> possible performance penalty, but with the benefits. For those that are
>>> batched we're stuck, but my memory on all the metadata locking paths is
>>> fuzzy already.
>>>
>>> What's here does work, but after that KVM Forum preso I guess we
>>> definitely need to pay attention to areas that can be perf bottlenecks
>>> for paths that may be really common.
>>>
>>> Having a way to disable or avoid virFork is something we may just need
>>> to have. I'm willing to be convinced otherwise - I'm just "wary" now.
>>
>> The metadata locking needs to be there so that the setting seclabels is
>> atomic. I mean, so far chown() and setfcon() are atomic. However, there
>> will be some xattr related calls around those. Therefore to make the
>> whole critical section atomic there has to be a lock:
>>
>> lock(file);
>> xattr(file);
>> chown(file);
>> xattr(file);
>> unlock(file);
>>
>> The xattr() calls set/recall the original owner of the file. I can make
>> this configurable, but if there is no locking the only thing libvirt can
>> do is chown(), not the xattr() because that wouldn't be atomic. (I'm
>> saying only chown(), but it is the same story for setfcon().) Therefore,
>> if users disable metadata locking the original owner of the file can't
>> be preserved. On the other hand, we can enable it by default and have an
>> opt out for cases where it doesn't work - just like we have for
>> namespaces. And I believe people did disable them in their early days
>> (even though I don't understand why, they were bugfree :-P)
>>
>
> I understand (generically) why we need the lock. I'm OK with it being
>
Re: [libvirt] [PATCH v3 04/13] security_manager: Rework metadata locking
On 11/09/2018 03:47 PM, John Ferlan wrote: > So, then I assume the disks are shared because they've been allowed in > two domains that are allowed to be running at the same time. If they're > shared and domainA has/uses a different label than domainB, then who > "wins" that war in the long run? Seems to me shared disks are > "dangerous" in this labeling game (and that's beyond the locking game). The fact that a disk changes DAC label does not necessarily mean that qemu is losing access. Firstly, the it might be just UID that changes on the file and GID staying the same. Secondly, there might be an ACL entry that lets qemu in no matter what. I agree that the disk should be marked as shareable and readonly, but it's not for this code to decide. Actually, it is not for libvirt to tell at all. Parsing UIDs is complex and there's pretty good implementation in kernel so might as well rely on that instead of duplicating the code into libvirt. IOW, if users misconfigure disks to their domains and get EPERM or cut off access to an already running domain, it's their problem and we shouldn't mitigate it. On the other hand, we shouldn't just deadlock. > > Should a mechanism be created to disallow multiple threads from running > the SetAllLabel "simultaneously" to avoid/solve the problem? Since it's > really the only one with the ordering problem... This would hurt performance IMO. If there are two domains being started at once and they don't share any common path then there is no reason for relabel operation to run serialized. > > Seems we each have our doom and gloom scenarios in mind ;-) > > Thanks for the details - > > John > Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] qemu: Extract MDEV VFIO PCI validation code into a separate helper
On Mon, Nov 12, 2018 at 01:14 PM +0100, Erik Skultety
wrote:
> Since we'll need to validate other models apart from VFIO PCI too,
> having a helper for each model should keep the code base cleaner.
>
> Signed-off-by: Erik Skultety
> ---
> src/qemu/qemu_domain.c | 35 +--
> 1 file changed, 29 insertions(+), 6 deletions(-)
>
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index e25afdad6b..17d207513d 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -4564,11 +4564,11 @@ qemuDomainDeviceDefValidateNetwork(const
> virDomainNetDef *net)
>
>
> static int
> -qemuDomainMdevDefValidate(const virDomainHostdevSubsysMediatedDev *mdevsrc,
> - const virDomainDef *def,
> - virQEMUCapsPtr qemuCaps)
> +qemuDomainMdevDefVFIOPCIValidate(const virDomainHostdevSubsysMediatedDev
> *dev,
> + const virDomainDef *def,
> + virQEMUCapsPtr qemuCaps)
> {
> -if (mdevsrc->display == VIR_TRISTATE_SWITCH_ABSENT)
> +if (dev->display == VIR_TRISTATE_SWITCH_ABSENT)
> return 0;
>
> if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VFIO_PCI_DISPLAY)) {
> @@ -4578,7 +4578,7 @@ qemuDomainMdevDefValidate(const
> virDomainHostdevSubsysMediatedDev *mdevsrc,
> return -1;
> }
>
> -if (mdevsrc->model != VIR_MDEV_MODEL_TYPE_VFIO_PCI) {
> +if (dev->model != VIR_MDEV_MODEL_TYPE_VFIO_PCI) {
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> _(" attribute 'display' is only supported"
> " with model='vfio-pci'"));
> @@ -4586,7 +4586,7 @@ qemuDomainMdevDefValidate(const
> virDomainHostdevSubsysMediatedDev *mdevsrc,
> return -1;
> }
>
> -if (mdevsrc->display == VIR_TRISTATE_SWITCH_ON) {
> +if (dev->display == VIR_TRISTATE_SWITCH_ON) {
> if (def->ngraphics == 0) {
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> _("graphics device is needed for attribute value "
> @@ -4599,6 +4599,29 @@ qemuDomainMdevDefValidate(const
> virDomainHostdevSubsysMediatedDev *mdevsrc,
> }
>
>
> +static int
> +qemuDomainMdevDefValidate(const virDomainHostdevSubsysMediatedDev *mdevsrc,
> + const virDomainDef *def,
> + virQEMUCapsPtr qemuCaps)
> +{
> +
> +switch ((virMediatedDeviceModelType) mdevsrc->model) {
> +case VIR_MDEV_MODEL_TYPE_VFIO_PCI:
> +return qemuDomainMdevDefVFIOPCIValidate(mdevsrc, def, qemuCaps);
> +case VIR_MDEV_MODEL_TYPE_VFIO_AP:
> +break;
> +case VIR_MDEV_MODEL_TYPE_VFIO_CCW:
> +break;
> +case VIR_MDEV_MODEL_TYPE_LAST:
> +virReportEnumRangeError(virMediatedDeviceModelType,
> +mdevsrc->model);
> +return -1;
> +}
default case is missing? Otherwise looking good.
> +
> +return 0;
> +}
> +
> +
> static int
> qemuDomainDeviceDefValidateHostdev(const virDomainHostdevDef *hostdev,
> const virDomainDef *def,
> --
> 2.19.1
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>
--
Kind regards / Beste Grüße
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] qemu: Introduce caching whether /dev/kvm is accessible
On Mon, Nov 12, 2018 at 02:48:09PM +0100, Marc Hartmayer wrote: > On Mon, Nov 12, 2018 at 01:30 PM +0100, Pavel Hrdina > wrote: > > On Mon, Nov 12, 2018 at 12:50:41PM +0100, Marc Hartmayer wrote: > >> On Thu, Nov 01, 2018 at 09:31 AM +0100, Martin Kletzander > >> wrote: > > > > [...] > > > >> How can you run a machine/QEMU VM under a different user:group other > >> than changing the user:group in qemu.conf and restart/reload libvirtd? > >> > >> As soon as a VM is running we have not to verify /dev/kvm access, no? > >> (so there should be no problem when libvirtd tries to “reconnect” to > >> already running VMs). > > > > You can add this into your domain XML: > > > > > > phrdina:phrdina > > > > > > And it will run the qemu process under that user. > > Interesting :) Actually, if we consider this then the QEMU caps caching > is broken anyway since 'virQEMUCapsNewData' is calling > 'virQEMUCapsNewForBinaryInternal(…, priv->runUid, priv->runGid, …)'. > > And 'priv->runUid/runGid' is only set once in virQEMUCapsCacheNew. > > Maybe I missed something. I dont think it is really broken - it merely impacts error reporting. When running 'virsh capabilities' we are trying to figure out if KVM is usable on the host. This is always using the default uid/gid so gives a fairly coarse answer, but the answer is correct for most common usage. When building command line ARGV for spawning a specific QEMU instance, the KVM capability merely affect whether libvirt reports an error about the guest config. In the case where the capability works with default uid/gid, but breaks with the custom per-VM uid/gid, libvirt will mistakenly thing KVM is usable and so launch the guest. QEMU will then be unable to access it and quit with some suitable error message. So the "brokenness" about not using the per-VM uid/gid merely delays the error reporting. I don't think this is important enough to worry about, using the default uid/gid is sufficient. Regards, Daniel -- |: 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 :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 3/3] qemu: Set identity for the reconnect all thread
https://bugzilla.redhat.com/show_bug.cgi?id=1631622
If polkit authentication is enabled, an attempt to open
the connection failed during virAccessDriverPolkitGetCaller
when the call to virIdentityGetCurrent returned NULL resulting
in the errors:
virAccessDriverPolkitGetCaller:87 : access denied:
Policy kit denied action org.libvirt.api.connect.getattr from
Because qemuProcessReconnect runs in a thread during
daemonRunStateInit processing it doesn't have the thread
local identity. Thus when the virGetConnectNWFilter is
called as part of the qemuProcessFiltersInstantiate when
virDomainConfNWFilterInstantiate is run the attempt to get
the idenity fails and results in the anonymous error above.
To fix this, let's grab/use the virIdenityPtr of the process
that will be creating the thread, e.g. what daemonRunStateInit
has set and use that for our thread. That way any other similar
processing that uses/requires an identity for any other call
that would have previously been successfully run won't fail in
a similar manner.
Signed-off-by: John Ferlan
---
src/qemu/qemu_process.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 1850923914..df7f0bfafb 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -81,6 +81,7 @@
#include "netdev_bandwidth_conf.h"
#include "virresctrl.h"
#include "virvsock.h"
+#include "viridentity.h"
#define VIR_FROM_THIS VIR_FROM_QEMU
@@ -7705,6 +7706,7 @@ qemuProcessRefreshCPU(virQEMUDriverPtr driver,
struct qemuProcessReconnectData {
virQEMUDriverPtr driver;
virDomainObjPtr obj;
+virIdentityPtr identity;
};
/*
* Open an existing VM's monitor, re-detect VCPU threads
@@ -7742,6 +7744,8 @@ qemuProcessReconnect(void *opaque)
bool retry = true;
bool tryMonReconn = false;
+virIdentitySetCurrent(data->identity);
+virObjectUnref(data->identity);
VIR_FREE(data);
qemuDomainObjRestoreJob(obj, );
@@ -7968,6 +7972,7 @@ qemuProcessReconnect(void *opaque)
virObjectUnref(cfg);
virObjectUnref(caps);
virNWFilterUnlockFilterUpdates();
+virIdentitySetCurrent(NULL);
return;
error:
@@ -8011,6 +8016,7 @@ qemuProcessReconnectHelper(virDomainObjPtr obj,
memcpy(data, src, sizeof(*data));
data->obj = obj;
+data->identity = virIdentityGetCurrent();
virNWFilterReadLockFilterUpdates();
@@ -8034,6 +8040,7 @@ qemuProcessReconnectHelper(virDomainObjPtr obj,
virDomainObjEndAPI();
virNWFilterUnlockFilterUpdates();
+virObjectUnref(data->identity);
VIR_FREE(data);
return -1;
}
--
2.17.2
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 2/3] access: Modify the VIR_ERR_ACCESS_DENIED to include driverName
https://bugzilla.redhat.com/show_bug.cgi?id=1631606
Changes made to manage and utilize a secondary connection
driver to APIs outside the scope of the primary connection
driver have resulted in some confusion processing polkit rules
since the simple "access denied" error message doesn't provide
enough of a clue when combined with the "authentication failed:
access denied by policy" as to which connection driver refused
or failed the ACL check.
In order to provide some context, let's modify the existing
"access denied" error returned from the various vir*EnsureACL
API's to provide the connection driver name that is causing
the failure. This should provide the context for writing the
polkit rules that would allow access via the driver, but yet
still adhere to the virAccessManagerSanitizeError commentary
regarding not telling the user why access was denied.
Signed-off-by: John Ferlan
---
src/access/viraccessmanager.c | 26 ++
src/rpc/gendispatch.pl| 3 ++-
2 files changed, 16 insertions(+), 13 deletions(-)
diff --git a/src/access/viraccessmanager.c b/src/access/viraccessmanager.c
index e7b5bf38da..f5d62604cf 100644
--- a/src/access/viraccessmanager.c
+++ b/src/access/viraccessmanager.c
@@ -196,11 +196,13 @@ static void virAccessManagerDispose(void *object)
* should the admin need to debug things
*/
static int
-virAccessManagerSanitizeError(int ret)
+virAccessManagerSanitizeError(int ret,
+ const char *driverName)
{
if (ret < 0) {
virResetLastError();
-virAccessError(VIR_ERR_ACCESS_DENIED, NULL);
+virAccessError(VIR_ERR_ACCESS_DENIED,
+ _("'%s' denied access"), driverName);
}
return ret;
@@ -217,7 +219,7 @@ int virAccessManagerCheckConnect(virAccessManagerPtr
manager,
if (manager->drv->checkConnect)
ret = manager->drv->checkConnect(manager, driverName, perm);
-return virAccessManagerSanitizeError(ret);
+return virAccessManagerSanitizeError(ret, driverName);
}
@@ -233,7 +235,7 @@ int virAccessManagerCheckDomain(virAccessManagerPtr manager,
if (manager->drv->checkDomain)
ret = manager->drv->checkDomain(manager, driverName, domain, perm);
-return virAccessManagerSanitizeError(ret);
+return virAccessManagerSanitizeError(ret, driverName);
}
int virAccessManagerCheckInterface(virAccessManagerPtr manager,
@@ -248,7 +250,7 @@ int virAccessManagerCheckInterface(virAccessManagerPtr
manager,
if (manager->drv->checkInterface)
ret = manager->drv->checkInterface(manager, driverName, iface, perm);
-return virAccessManagerSanitizeError(ret);
+return virAccessManagerSanitizeError(ret, driverName);
}
int virAccessManagerCheckNetwork(virAccessManagerPtr manager,
@@ -263,7 +265,7 @@ int virAccessManagerCheckNetwork(virAccessManagerPtr
manager,
if (manager->drv->checkNetwork)
ret = manager->drv->checkNetwork(manager, driverName, network, perm);
-return virAccessManagerSanitizeError(ret);
+return virAccessManagerSanitizeError(ret, driverName);
}
int virAccessManagerCheckNodeDevice(virAccessManagerPtr manager,
@@ -278,7 +280,7 @@ int virAccessManagerCheckNodeDevice(virAccessManagerPtr
manager,
if (manager->drv->checkNodeDevice)
ret = manager->drv->checkNodeDevice(manager, driverName, nodedev,
perm);
-return virAccessManagerSanitizeError(ret);
+return virAccessManagerSanitizeError(ret, driverName);
}
int virAccessManagerCheckNWFilter(virAccessManagerPtr manager,
@@ -293,7 +295,7 @@ int virAccessManagerCheckNWFilter(virAccessManagerPtr
manager,
if (manager->drv->checkNWFilter)
ret = manager->drv->checkNWFilter(manager, driverName, nwfilter, perm);
-return virAccessManagerSanitizeError(ret);
+return virAccessManagerSanitizeError(ret, driverName);
}
int virAccessManagerCheckNWFilterBinding(virAccessManagerPtr manager,
@@ -308,7 +310,7 @@ int
virAccessManagerCheckNWFilterBinding(virAccessManagerPtr manager,
if (manager->drv->checkNWFilterBinding)
ret = manager->drv->checkNWFilterBinding(manager, driverName, binding,
perm);
-return virAccessManagerSanitizeError(ret);
+return virAccessManagerSanitizeError(ret, driverName);
}
int virAccessManagerCheckSecret(virAccessManagerPtr manager,
@@ -323,7 +325,7 @@ int virAccessManagerCheckSecret(virAccessManagerPtr manager,
if (manager->drv->checkSecret)
ret = manager->drv->checkSecret(manager, driverName, secret, perm);
-return virAccessManagerSanitizeError(ret);
+return virAccessManagerSanitizeError(ret, driverName);
}
int virAccessManagerCheckStoragePool(virAccessManagerPtr manager,
@@ -338,7 +340,7 @@ int virAccessManagerCheckStoragePool(virAccessManagerPtr
manager,
if (manager->drv->checkStoragePool)
ret = manager->drv->checkStoragePool(manager, driverName, pool, perm);
-return
[libvirt] [PATCH v2 1/3] Revert "access: Modify the VIR_ERR_ACCESS_DENIED to include driverName"
This reverts commit ccc72d5cbdd85f66cb737134b3be40aac1df03ef.
Based on upstream comment to a follow-up patch, this didn't take the
right approach and the right thing to do is revert and rework.
Signed-off-by: John Ferlan
---
src/access/viraccessmanager.c | 25 -
src/rpc/gendispatch.pl| 2 +-
src/util/virerror.c | 4 ++--
3 files changed, 15 insertions(+), 16 deletions(-)
diff --git a/src/access/viraccessmanager.c b/src/access/viraccessmanager.c
index 1dfff32b9d..e7b5bf38da 100644
--- a/src/access/viraccessmanager.c
+++ b/src/access/viraccessmanager.c
@@ -196,12 +196,11 @@ static void virAccessManagerDispose(void *object)
* should the admin need to debug things
*/
static int
-virAccessManagerSanitizeError(int ret,
- const char *driverName)
+virAccessManagerSanitizeError(int ret)
{
if (ret < 0) {
virResetLastError();
-virAccessError(VIR_ERR_ACCESS_DENIED, driverName, NULL);
+virAccessError(VIR_ERR_ACCESS_DENIED, NULL);
}
return ret;
@@ -218,7 +217,7 @@ int virAccessManagerCheckConnect(virAccessManagerPtr
manager,
if (manager->drv->checkConnect)
ret = manager->drv->checkConnect(manager, driverName, perm);
-return virAccessManagerSanitizeError(ret, driverName);
+return virAccessManagerSanitizeError(ret);
}
@@ -234,7 +233,7 @@ int virAccessManagerCheckDomain(virAccessManagerPtr manager,
if (manager->drv->checkDomain)
ret = manager->drv->checkDomain(manager, driverName, domain, perm);
-return virAccessManagerSanitizeError(ret, driverName);
+return virAccessManagerSanitizeError(ret);
}
int virAccessManagerCheckInterface(virAccessManagerPtr manager,
@@ -249,7 +248,7 @@ int virAccessManagerCheckInterface(virAccessManagerPtr
manager,
if (manager->drv->checkInterface)
ret = manager->drv->checkInterface(manager, driverName, iface, perm);
-return virAccessManagerSanitizeError(ret, driverName);
+return virAccessManagerSanitizeError(ret);
}
int virAccessManagerCheckNetwork(virAccessManagerPtr manager,
@@ -264,7 +263,7 @@ int virAccessManagerCheckNetwork(virAccessManagerPtr
manager,
if (manager->drv->checkNetwork)
ret = manager->drv->checkNetwork(manager, driverName, network, perm);
-return virAccessManagerSanitizeError(ret, driverName);
+return virAccessManagerSanitizeError(ret);
}
int virAccessManagerCheckNodeDevice(virAccessManagerPtr manager,
@@ -279,7 +278,7 @@ int virAccessManagerCheckNodeDevice(virAccessManagerPtr
manager,
if (manager->drv->checkNodeDevice)
ret = manager->drv->checkNodeDevice(manager, driverName, nodedev,
perm);
-return virAccessManagerSanitizeError(ret, driverName);
+return virAccessManagerSanitizeError(ret);
}
int virAccessManagerCheckNWFilter(virAccessManagerPtr manager,
@@ -294,7 +293,7 @@ int virAccessManagerCheckNWFilter(virAccessManagerPtr
manager,
if (manager->drv->checkNWFilter)
ret = manager->drv->checkNWFilter(manager, driverName, nwfilter, perm);
-return virAccessManagerSanitizeError(ret, driverName);
+return virAccessManagerSanitizeError(ret);
}
int virAccessManagerCheckNWFilterBinding(virAccessManagerPtr manager,
@@ -309,7 +308,7 @@ int
virAccessManagerCheckNWFilterBinding(virAccessManagerPtr manager,
if (manager->drv->checkNWFilterBinding)
ret = manager->drv->checkNWFilterBinding(manager, driverName, binding,
perm);
-return virAccessManagerSanitizeError(ret, driverName);
+return virAccessManagerSanitizeError(ret);
}
int virAccessManagerCheckSecret(virAccessManagerPtr manager,
@@ -324,7 +323,7 @@ int virAccessManagerCheckSecret(virAccessManagerPtr manager,
if (manager->drv->checkSecret)
ret = manager->drv->checkSecret(manager, driverName, secret, perm);
-return virAccessManagerSanitizeError(ret, driverName);
+return virAccessManagerSanitizeError(ret);
}
int virAccessManagerCheckStoragePool(virAccessManagerPtr manager,
@@ -339,7 +338,7 @@ int virAccessManagerCheckStoragePool(virAccessManagerPtr
manager,
if (manager->drv->checkStoragePool)
ret = manager->drv->checkStoragePool(manager, driverName, pool, perm);
-return virAccessManagerSanitizeError(ret, driverName);
+return virAccessManagerSanitizeError(ret);
}
int virAccessManagerCheckStorageVol(virAccessManagerPtr manager,
@@ -355,5 +354,5 @@ int virAccessManagerCheckStorageVol(virAccessManagerPtr
manager,
if (manager->drv->checkStorageVol)
ret = manager->drv->checkStorageVol(manager, driverName, pool, vol,
perm);
-return virAccessManagerSanitizeError(ret, driverName);
+return virAccessManagerSanitizeError(ret);
}
diff --git a/src/rpc/gendispatch.pl b/src/rpc/gendispatch.pl
index f599002056..0c4648c0fb 100755
--- a/src/rpc/gendispatch.pl
+++ b/src/rpc/gendispatch.pl
@@ -2199,7 +2199,7 @@ elsif ($mode eq "client") {
[libvirt] [PATCH v2 0/3] Fix some errors around VIR_ACCESS_DENIED
v1: https://www.redhat.com/archives/libvir-list/2018-November/msg00339.html Changes since v1: * Do the right thing, revert the bad patch and rework it. Thus patch1 is the revert and patch2 is the rework. If it's decided that patch2 is unnecessary or undesired, that's perfectly fine, but would then require a slight modification to the documentation from commit 4f1107614 to remove the reference about the access denied message. * From review - add the virObjectUnref for the data->identity for which a virIdentityGetCurrent reference was obtained. v1 cover: Patch 1 fixes my own error made in this release fortunately, but only seen because I was invesigating Patch 2 Patch 2 is perhaps a longer term issue, but perhaps coming more to light now that the nwfilter bindings have been separated and use a virConnectOpen for nwfilter:///system during reconnection processing; whereas, previously the filter bindings would have been "hidden" within various nwfilter, domain, and network driver callbacks and throughs. John Ferlan (3): Revert "access: Modify the VIR_ERR_ACCESS_DENIED to include driverName" access: Modify the VIR_ERR_ACCESS_DENIED to include driverName qemu: Set identity for the reconnect all thread src/access/viraccessmanager.c | 3 ++- src/qemu/qemu_process.c | 7 +++ src/rpc/gendispatch.pl| 3 ++- src/util/virerror.c | 4 ++-- 4 files changed, 13 insertions(+), 4 deletions(-) -- 2.17.2 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] qemu: Introduce caching whether /dev/kvm is accessible
On Mon, Nov 12, 2018 at 01:30 PM +0100, Pavel Hrdina wrote: > On Mon, Nov 12, 2018 at 12:50:41PM +0100, Marc Hartmayer wrote: >> On Thu, Nov 01, 2018 at 09:31 AM +0100, Martin Kletzander >> wrote: > > [...] > >> How can you run a machine/QEMU VM under a different user:group other >> than changing the user:group in qemu.conf and restart/reload libvirtd? >> >> As soon as a VM is running we have not to verify /dev/kvm access, no? >> (so there should be no problem when libvirtd tries to “reconnect” to >> already running VMs). > > You can add this into your domain XML: > > > phrdina:phrdina > > > And it will run the qemu process under that user. Interesting :) Actually, if we consider this then the QEMU caps caching is broken anyway since 'virQEMUCapsNewData' is calling 'virQEMUCapsNewForBinaryInternal(…, priv->runUid, priv->runGid, …)'. And 'priv->runUid/runGid' is only set once in virQEMUCapsCacheNew. Maybe I missed something. > > Pavel > -- > libvir-list mailing list > libvir-list@redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list -- Kind regards / Beste Grüße Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzende des Aufsichtsrats: Martina Koederitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCHv8 15/17] qemu: Refactor qemuDomainGetStatsCpu
Refactoring qemuDomainGetStatsCpu, make it possible to add
more CPU statistics.
Signed-off-by: Wang Huaqiang
---
src/qemu/qemu_driver.c | 21 -
1 file changed, 16 insertions(+), 5 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 09e04b8..89d46ee 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -19699,11 +19699,9 @@ typedef enum {
static int
-qemuDomainGetStatsCpu(virQEMUDriverPtr driver ATTRIBUTE_UNUSED,
- virDomainObjPtr dom,
- virDomainStatsRecordPtr record,
- int *maxparams,
- unsigned int privflags ATTRIBUTE_UNUSED)
+qemuDomainGetStatsCpuCgroup(virDomainObjPtr dom,
+virDomainStatsRecordPtr record,
+int *maxparams)
{
qemuDomainObjPrivatePtr priv = dom->privateData;
unsigned long long cpu_time = 0;
@@ -19739,6 +19737,19 @@ qemuDomainGetStatsCpu(virQEMUDriverPtr driver
ATTRIBUTE_UNUSED,
return 0;
}
+
+static int
+qemuDomainGetStatsCpu(virQEMUDriverPtr driver ATTRIBUTE_UNUSED,
+ virDomainObjPtr dom,
+ virDomainStatsRecordPtr record,
+ int *maxparams,
+ unsigned int privflags ATTRIBUTE_UNUSED)
+{
+if (qemuDomainGetStatsCpuCgroup(dom, record, maxparams) < 0)
+return -1;
+}
+
+
static int
qemuDomainGetStatsBalloon(virQEMUDriverPtr driver,
virDomainObjPtr dom,
--
2.7.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCHv8 14/17] qemu: enable resctrl monitor in qemu
Add functions for creating, destroying, reconnecting resctrl
monitor in qemu according to the configuration in domain XML.
Signed-off-by: Wang Huaqiang
---
src/qemu/qemu_process.c | 49 -
1 file changed, 48 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 1850923..096fea1 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -2597,10 +2597,20 @@ qemuProcessResctrlCreate(virQEMUDriverPtr driver,
return -1;
for (i = 0; i < vm->def->nresctrls; i++) {
+size_t j = 0;
if (virResctrlAllocCreate(caps->host.resctrl,
vm->def->resctrls[i]->alloc,
priv->machineName) < 0)
goto cleanup;
+
+for (j = 0; j < vm->def->resctrls[i]->nmonitors; j++) {
+virDomainResctrlMonDefPtr mon = NULL;
+
+mon = vm->def->resctrls[i]->monitors[j];
+if (virResctrlMonitorCreate(mon->instance,
+priv->machineName) < 0)
+goto cleanup;
+}
}
ret = 0;
@@ -5414,6 +5424,7 @@ qemuProcessSetupVcpu(virDomainObjPtr vm,
{
pid_t vcpupid = qemuDomainGetVcpuPid(vm, vcpuid);
virDomainVcpuDefPtr vcpu = virDomainDefGetVcpu(vm->def, vcpuid);
+virDomainResctrlMonDefPtr mon = NULL;
size_t i = 0;
if (qemuProcessSetupPid(vm, vcpupid, VIR_CGROUP_THREAD_VCPU,
@@ -5424,11 +5435,26 @@ qemuProcessSetupVcpu(virDomainObjPtr vm,
return -1;
for (i = 0; i < vm->def->nresctrls; i++) {
+size_t j = 0;
virDomainResctrlDefPtr ct = vm->def->resctrls[i];
if (virBitmapIsBitSet(ct->vcpus, vcpuid)) {
if (virResctrlAllocAddPID(ct->alloc, vcpupid) < 0)
return -1;
+
+for (j = 0; j < ct->nmonitors; j++) {
+mon = ct->monitors[j];
+
+if (virBitmapEqual(ct->vcpus, mon->vcpus))
+continue;
+
+if (virBitmapIsBitSet(mon->vcpus, vcpuid)) {
+if (virResctrlMonitorAddPID(mon->instance, vcpupid) < 0)
+return -1;
+break;
+}
+}
+
break;
}
}
@@ -7190,8 +7216,18 @@ void qemuProcessStop(virQEMUDriverPtr driver,
/* Remove resctrl allocation after cgroups are cleaned up which makes it
* kind of safer (although removing the allocation should work even with
* pids in tasks file */
-for (i = 0; i < vm->def->nresctrls; i++)
+for (i = 0; i < vm->def->nresctrls; i++) {
+size_t j = 0;
+
+for (j = 0; j < vm->def->resctrls[i]->nmonitors; j++) {
+virDomainResctrlMonDefPtr mon = NULL;
+
+mon = vm->def->resctrls[i]->monitors[j];
+virResctrlMonitorRemove(mon->instance);
+}
+
virResctrlAllocRemove(vm->def->resctrls[i]->alloc);
+}
qemuProcessRemoveDomainStatus(driver, vm);
@@ -7926,9 +7962,20 @@ qemuProcessReconnect(void *opaque)
goto error;
for (i = 0; i < obj->def->nresctrls; i++) {
+size_t j = 0;
+
if (virResctrlAllocDeterminePath(obj->def->resctrls[i]->alloc,
priv->machineName) < 0)
goto error;
+
+for (j = 0; j < obj->def->resctrls[i]->nmonitors; j++) {
+virDomainResctrlMonDefPtr mon = NULL;
+
+mon = obj->def->resctrls[i]->monitors[j];
+if (virResctrlMonitorDeterminePath(mon->instance,
+ priv->machineName) < 0)
+goto error;
+}
}
/* update domain state XML with possibly updated state in virDomainObj */
--
2.7.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCHv8 12/17] conf: Remove virDomainResctrlAppend and introduce virDomainResctrlNew
Introduced virDomainResctrlNew to do the most part of virDomainResctrlAppend
and move the operation of appending resctrl to @def->resctrls out of
function.
Rather than rely on virDomainResctrlAppend to perform the allocation, move
the onus to the caller and make use of virBitmapNewCopy for @vcpus and
virObjectRef for @alloc, thus removing the need to set each to NULL after the
call.
Signed-off-by: Wang Huaqiang
---
src/conf/domain_conf.c | 56 +-
1 file changed, 33 insertions(+), 23 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 237540b..8433214 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -18948,26 +18948,22 @@ virDomainCachetuneDefParseCache(xmlXPathContextPtr
ctxt,
}
-static int
-virDomainResctrlAppend(virDomainDefPtr def,
- xmlNodePtr node,
- virResctrlAllocPtr alloc,
- virBitmapPtr vcpus,
- unsigned int flags)
+static virDomainResctrlDefPtr
+virDomainResctrlNew(xmlNodePtr node,
+virResctrlAllocPtr alloc,
+virBitmapPtr vcpus,
+unsigned int flags)
{
char *vcpus_str = NULL;
char *alloc_id = NULL;
-virDomainResctrlDefPtr tmp_resctrl = NULL;
-int ret = -1;
-
-if (VIR_ALLOC(tmp_resctrl) < 0)
-goto cleanup;
+virDomainResctrlDefPtr resctrl = NULL;
+virDomainResctrlDefPtr ret = NULL;
/* We need to format it back because we need to be consistent in the naming
* even when users specify some "sub-optimal" string there. */
vcpus_str = virBitmapFormat(vcpus);
if (!vcpus_str)
-goto cleanup;
+return NULL;
if (!(flags & VIR_DOMAIN_DEF_PARSE_INACTIVE))
alloc_id = virXMLPropString(node, "id");
@@ -18985,15 +18981,20 @@ virDomainResctrlAppend(virDomainDefPtr def,
if (virResctrlAllocSetID(alloc, alloc_id) < 0)
goto cleanup;
-tmp_resctrl->vcpus = vcpus;
-tmp_resctrl->alloc = alloc;
+if (VIR_ALLOC(resctrl) < 0)
+goto cleanup;
-if (VIR_APPEND_ELEMENT(def->resctrls, def->nresctrls, tmp_resctrl) < 0)
+if (!(resctrl->vcpus = virBitmapNewCopy(vcpus))) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("failed to copy 'vcpus'"));
goto cleanup;
+}
-ret = 0;
+resctrl->alloc = virObjectRef(alloc);
+
+VIR_STEAL_PTR(ret, resctrl);
cleanup:
-virDomainResctrlDefFree(tmp_resctrl);
+virDomainResctrlDefFree(resctrl);
VIR_FREE(alloc_id);
VIR_FREE(vcpus_str);
return ret;
@@ -19010,6 +19011,7 @@ virDomainCachetuneDefParse(virDomainDefPtr def,
xmlNodePtr *nodes = NULL;
virBitmapPtr vcpus = NULL;
virResctrlAllocPtr alloc = NULL;
+virDomainResctrlDefPtr resctrl = NULL;
ssize_t i = 0;
int n;
int ret = -1;
@@ -19048,19 +19050,22 @@ virDomainCachetuneDefParse(virDomainDefPtr def,
goto cleanup;
}
+resctrl = virDomainResctrlNew(node, alloc, vcpus, flags);
+if (!resctrl)
+goto cleanup;
+
if (virResctrlAllocIsEmpty(alloc)) {
ret = 0;
goto cleanup;
}
-if (virDomainResctrlAppend(def, node, alloc, vcpus, flags) < 0)
+if (VIR_APPEND_ELEMENT(def->resctrls, def->nresctrls, resctrl) < 0)
goto cleanup;
-vcpus = NULL;
-alloc = NULL;
ret = 0;
cleanup:
ctxt->node = oldnode;
+virDomainResctrlDefFree(resctrl);
virObjectUnref(alloc);
virBitmapFree(vcpus);
VIR_FREE(nodes);
@@ -19218,6 +19223,8 @@ virDomainMemorytuneDefParse(virDomainDefPtr def,
xmlNodePtr *nodes = NULL;
virBitmapPtr vcpus = NULL;
virResctrlAllocPtr alloc = NULL;
+virDomainResctrlDefPtr resctrl = NULL;
+
ssize_t i = 0;
int n;
int ret = -1;
@@ -19262,15 +19269,18 @@ virDomainMemorytuneDefParse(virDomainDefPtr def,
* just update the existing alloc information, which is done in above
* virDomainMemorytuneDefParseMemory */
if (new_alloc) {
-if (virDomainResctrlAppend(def, node, alloc, vcpus, flags) < 0)
+resctrl = virDomainResctrlNew(node, alloc, vcpus, flags);
+if (!resctrl)
+goto cleanup;
+
+if (VIR_APPEND_ELEMENT(def->resctrls, def->nresctrls, resctrl) < 0)
goto cleanup;
-vcpus = NULL;
-alloc = NULL;
}
ret = 0;
cleanup:
ctxt->node = oldnode;
+virDomainResctrlDefFree(resctrl);
virObjectUnref(alloc);
virBitmapFree(vcpus);
VIR_FREE(nodes);
--
2.7.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCHv8 16/17] qemu: Report cache occupancy (CMT) with domstats
Adding the interface in qemu to report CMT statistic information
through command 'virsh domstats --cpu-total'.
Below is a typical output:
# virsh domstats 1 --cpu-total
Domain: 'ubuntu16.04-base'
...
cpu.cache.monitor.count=2
cpu.cache.monitor.0.name=vcpus_1
cpu.cache.monitor.0.vcpus=1
cpu.cache.monitor.0.bank.count=2
cpu.cache.monitor.0.bank.0.id=0
cpu.cache.monitor.0.bank.0.bytes=4505600
cpu.cache.monitor.0.bank.1.id=1
cpu.cache.monitor.0.bank.1.bytes=5586944
cpu.cache.monitor.1.name=vcpus_4-6
cpu.cache.monitor.1.vcpus=4,5,6
cpu.cache.monitor.1.bank.count=2
cpu.cache.monitor.1.bank.0.id=0
cpu.cache.monitor.1.bank.0.bytes=17571840
cpu.cache.monitor.1.bank.1.id=1
cpu.cache.monitor.1.bank.1.bytes=29106176
Signed-off-by: Wang Huaqiang
---
src/libvirt-domain.c | 9 +++
src/qemu/qemu_driver.c | 198 +
2 files changed, 207 insertions(+)
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
index 7690339..4895f9f 100644
--- a/src/libvirt-domain.c
+++ b/src/libvirt-domain.c
@@ -11345,6 +11345,15 @@ virConnectGetDomainCapabilities(virConnectPtr conn,
* "cpu.user" - user cpu time spent in nanoseconds as unsigned long long.
* "cpu.system" - system cpu time spent in nanoseconds as unsigned long
*long.
+ * "cpu.cache.monitor.count" - tocal cache monitoring groups
+ * "cpu.cache.monitor.M.name" - name of cache monitoring group 'M'
+ * "cpu.cache.monitor.M.vcpus" - vcpus for cache monitoring group 'M'
+ * "cpu.cache.monitor.M.bank.count" - total bank number of cache monitoring
+ *group 'M'
+ * "cpu.cache.monitor.M.bank.N.id" - OS assigned cache bank id for cache
+ *'N' in cache monitoring group 'M'
+ * "cpu.cache.monitor.M.bank.N.bytes" - monitor's cache occupancy of cache
+ *bank 'N' in cache monitoring group 'M'
*
* VIR_DOMAIN_STATS_BALLOON:
* Return memory balloon device information.
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 89d46ee..d41ae66 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -19698,6 +19698,199 @@ typedef enum {
#define HAVE_JOB(flags) ((flags) & QEMU_DOMAIN_STATS_HAVE_JOB)
+typedef struct _virQEMUCpuResMonitorData virQEMUCpuResMonitorData;
+typedef virQEMUCpuResMonitorData *virQEMUCpuResMonitorDataPtr;
+struct _virQEMUCpuResMonitorData{
+const char *name;
+char *vcpus;
+virResctrlMonitorType tag;
+virResctrlMonitorStatsPtr stats;
+size_t nstats;
+};
+
+
+static int
+qemuDomainGetCpuResMonitorData(virDomainObjPtr dom,
+ virQEMUCpuResMonitorDataPtr mondata)
+{
+virDomainResctrlDefPtr resctrl = NULL;
+size_t i = 0;
+size_t j = 0;
+size_t l = 0;
+
+for (i = 0; i < dom->def->nresctrls; i++) {
+resctrl = dom->def->resctrls[i];
+
+for (j = 0; j < resctrl->nmonitors; j++) {
+virDomainResctrlMonDefPtr domresmon = NULL;
+virResctrlMonitorPtr monitor = resctrl->monitors[j]->instance;
+
+domresmon = resctrl->monitors[j];
+mondata[l].tag = domresmon->tag;
+
+/* If virBitmapFormat successfully returns an vcpu string, then
+ * mondata[l].vcpus is assigned with an memory space holding it,
+ * let this newly allocated memory buffer to be freed along with
+ * the free of 'mondata' */
+if (!(mondata[l].vcpus = virBitmapFormat(domresmon->vcpus)))
+return -1;
+
+if (!(mondata[l].name = virResctrlMonitorGetID(monitor))) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Could not get monitor ID"));
+return -1;
+}
+
+if (domresmon->tag == VIR_RESCTRL_MONITOR_TYPE_CACHE) {
+if (virResctrlMonitorGetCacheOccupancy(monitor,
+ [l].stats,
+ [l].nstats) < 0)
+return -1;
+} else {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Invalid CPU resource type"));
+return -1;
+}
+
+l++;
+}
+}
+
+return 0;
+}
+
+
+static int
+qemuDomainGetStatsCpuResMonitorPerTag(virQEMUCpuResMonitorDataPtr mondata,
+ size_t nmondata,
+ virResctrlMonitorType tag,
+ virDomainStatsRecordPtr record,
+ int *maxparams)
+{
+char param_name[VIR_TYPED_PARAM_FIELD_LENGTH];
+unsigned int nmonitors = 0;
+const char *resname =
[libvirt] [PATCHv8 04/17] util: Add interface to determine monitor path
Add interface for resctrl monitor to determine the path.
Signed-off-by: Wang Huaqiang
Reviewed-by: John Ferlan
---
src/libvirt_private.syms | 1 +
src/util/virresctrl.c| 55
src/util/virresctrl.h| 5 -
3 files changed, 60 insertions(+), 1 deletion(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index d2573c5..5235046 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2680,6 +2680,7 @@ virResctrlInfoGetCache;
virResctrlInfoGetMonitorPrefix;
virResctrlInfoMonFree;
virResctrlInfoNew;
+virResctrlMonitorDeterminePath;
virResctrlMonitorNew;
diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
index 463555c..aa062c3 100644
--- a/src/util/virresctrl.c
+++ b/src/util/virresctrl.c
@@ -2462,3 +2462,58 @@ virResctrlMonitorNew(void)
return virObjectNew(virResctrlMonitorClass);
}
+
+
+/*
+ * virResctrlMonitorDeterminePath
+ *
+ * @monitor: Pointer to a resctrl monitor
+ * @machinename: Name string of the VM
+ *
+ * Determines the directory path that the underlying resctrl group will be
+ * created with.
+ *
+ * A monitor represents a directory under resource control file system,
+ * its directory path could be the same path as @monitor->alloc, could be a
+ * path of directory under 'mon_groups' of @monitor->alloc, or a path of
+ * directory under '/sys/fs/resctrl/mon_groups' if @monitor->alloc is NULL.
+ *
+ * Returns 0 on success, -1 on error.
+ */
+int
+virResctrlMonitorDeterminePath(virResctrlMonitorPtr monitor,
+ const char *machinename)
+{
+VIR_AUTOFREE(char *) parentpath = NULL;
+
+if (!monitor) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Invalid resctrl monitor"));
+return -1;
+}
+
+if (monitor->path) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Resctrl monitor path is already set to '%s'"),
+ monitor->path);
+return -1;
+}
+
+if (monitor->id && monitor->alloc && monitor->alloc->id) {
+if (STREQ(monitor->id, monitor->alloc->id)) {
+if (VIR_STRDUP(monitor->path, monitor->alloc->path) < 0)
+return -1;
+return 0;
+}
+}
+
+if (virAsprintf(, "%s/mon_groups", monitor->alloc->path) < 0)
+return -1;
+
+monitor->path = virResctrlDeterminePath(parentpath, machinename,
+monitor->id);
+if (!monitor->path)
+return -1;
+
+return 0;
+}
diff --git a/src/util/virresctrl.h b/src/util/virresctrl.h
index eaa077d..baae554 100644
--- a/src/util/virresctrl.h
+++ b/src/util/virresctrl.h
@@ -191,7 +191,10 @@ virResctrlInfoGetMonitorPrefix(virResctrlInfoPtr resctrl,
typedef struct _virResctrlMonitor virResctrlMonitor;
typedef virResctrlMonitor *virResctrlMonitorPtr;
-
virResctrlMonitorPtr
virResctrlMonitorNew(void);
+
+int
+virResctrlMonitorDeterminePath(virResctrlMonitorPtr monitor,
+ const char *machinename);
#endif /* __VIR_RESCTRL_H__ */
--
2.7.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCHv8 07/17] util: Refactor code for creating resctrl group
The code for creating resctrl allocation group could be reused
for monitoring group, refactor it for reuse in the later patch.
Signed-off-by: Wang Huaqiang
Reviewed-by: John Ferlan
---
src/util/virresctrl.c | 37 +++--
1 file changed, 23 insertions(+), 14 deletions(-)
diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
index 7bd52cd..94689dd 100644
--- a/src/util/virresctrl.c
+++ b/src/util/virresctrl.c
@@ -2316,6 +2316,26 @@ virResctrlAllocDeterminePath(virResctrlAllocPtr alloc,
}
+/* This function creates a resctrl directory in resource control file system,
+ * and the directory path is specified by @path. */
+static int
+virResctrlCreateGroupPath(const char *path)
+{
+/* Directory exists, return */
+if (virFileExists(path))
+return 0;
+
+if (virFileMakePath(path) < 0) {
+virReportSystemError(errno,
+ _("Cannot create resctrl directory '%s'"),
+ path);
+return -1;
+}
+
+return 0;
+}
+
+
/* This checks if the directory for the alloc exists. If not it tries to
create
* it and apply appropriate alloc settings. */
int
@@ -2344,13 +2364,6 @@ virResctrlAllocCreate(virResctrlInfoPtr resctrl,
if (STREQ(alloc->path, SYSFS_RESCTRL_PATH))
return 0;
-if (virFileExists(alloc->path)) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Path '%s' for resctrl allocation exists"),
- alloc->path);
-goto cleanup;
-}
-
lockfd = virResctrlLockWrite();
if (lockfd < 0)
goto cleanup;
@@ -2358,6 +2371,9 @@ virResctrlAllocCreate(virResctrlInfoPtr resctrl,
if (virResctrlAllocAssign(resctrl, alloc) < 0)
goto cleanup;
+if (virResctrlCreateGroupPath(alloc->path) < 0)
+goto cleanup;
+
alloc_str = virResctrlAllocFormat(alloc);
if (!alloc_str)
goto cleanup;
@@ -2365,13 +2381,6 @@ virResctrlAllocCreate(virResctrlInfoPtr resctrl,
if (virAsprintf(_path, "%s/schemata", alloc->path) < 0)
goto cleanup;
-if (virFileMakePath(alloc->path) < 0) {
-virReportSystemError(errno,
- _("Cannot create resctrl directory '%s'"),
- alloc->path);
-goto cleanup;
-}
-
VIR_DEBUG("Writing resctrl schemata '%s' into '%s'", alloc_str,
schemata_path);
if (virFileWriteStr(schemata_path, alloc_str, 0) < 0) {
rmdir(alloc->path);
--
2.7.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCHv8 01/17] docs, util: Refactor schemas and virresctrl to support optional cache
Refactor schemas and virresctrl to support optional element
in .
Later, the monitor entry will be introduced and to be placed
under . Either cache entry or monitor entry is
an optional element of .
An cachetune has no element is taking the default resource
allocating policy defined in '/sys/fs/resctrl/schemata'.
Signed-off-by: Wang Huaqiang
---
docs/formatdomain.html.in | 4 ++--
docs/schemas/domaincommon.rng | 4 ++--
src/util/virresctrl.c | 27 +++
3 files changed, 31 insertions(+), 4 deletions(-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 269741a..8850a71 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -943,8 +943,8 @@
cache
-This element controls the allocation of CPU cache and has the
-following attributes:
+This optional element controls the allocation of CPU cache and has
+the following attributes:
level
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index b9ac5df..2b465be 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -956,7 +956,7 @@
-
+
@@ -980,7 +980,7 @@
-
+
diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
index 5d811a2..7339e9b 100644
--- a/src/util/virresctrl.c
+++ b/src/util/virresctrl.c
@@ -234,6 +234,11 @@ virResctrlInfoMonFree(virResctrlInfoMonPtr mon)
* in case there is no allocation for that particular cache allocation (level,
* cache, ...) or memory allocation for particular node).
*
+ * Allocation corresponding to root directory, /sys/fs/sysctrl/, defines the
+ * default resource allocating policy, which is created immediately after
+ * mounting, and owns all the tasks and cpus in the system. Cache or memory
+ * bandwidth resource will be shared for tasks in this allocation.
+ *
* =Cache allocation technology (CAT)=
*
* Since one allocation can be made for caches on different levels, the first
@@ -2215,6 +2220,15 @@ virResctrlAllocDeterminePath(virResctrlAllocPtr alloc,
return -1;
}
+/* If the allocation is empty, then the path will be SYSFS_RESCTRL_PATH */
+if (virResctrlAllocIsEmpty(alloc)) {
+if (!alloc->path &&
+VIR_STRDUP(alloc->path, SYSFS_RESCTRL_PATH) < 0)
+return -1;
+
+return 0;
+}
+
if (!alloc->path &&
virAsprintf(>path, "%s/%s-%s",
SYSFS_RESCTRL_PATH, machinename, alloc->id) < 0)
@@ -2248,6 +2262,10 @@ virResctrlAllocCreate(virResctrlInfoPtr resctrl,
if (virResctrlAllocDeterminePath(alloc, machinename) < 0)
return -1;
+/* If using the system/default path for the allocation, then we're done */
+if (STREQ(alloc->path, SYSFS_RESCTRL_PATH))
+return 0;
+
if (virFileExists(alloc->path)) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Path '%s' for resctrl allocation exists"),
@@ -2302,6 +2320,11 @@ virResctrlAllocAddPID(virResctrlAllocPtr alloc,
char *pidstr = NULL;
int ret = 0;
+/* If allocation is empty, then no resctrl directory and the 'tasks' file
+ * exists, just return */
+if (virResctrlAllocIsEmpty(alloc))
+return 0;
+
if (!alloc->path) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("Cannot add pid to non-existing resctrl allocation"));
@@ -2334,6 +2357,10 @@ virResctrlAllocRemove(virResctrlAllocPtr alloc)
{
int ret = 0;
+/* Do not destroy if path is the system/default path for the allocation */
+if (STREQ(alloc->path, SYSFS_RESCTRL_PATH))
+return 0;
+
if (!alloc->path)
return 0;
--
2.7.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCHv8 10/17] util: Add interface for setting monitor ID.
Add virResctrlMonitorSetID by leveraging previous refactored patch.
Signed-off-by: Wang Huaqiang
---
src/util/virresctrl.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
index 4e4831c..ed682c9 100644
--- a/src/util/virresctrl.c
+++ b/src/util/virresctrl.c
@@ -2581,3 +2581,12 @@ virResctrlMonitorCreate(virResctrlMonitorPtr monitor,
virResctrlUnlock(lockfd);
return ret;
}
+
+
+int
+virResctrlMonitorSetID(virResctrlMonitorPtr monitor,
+ const char *id)
+
+{
+return virResctrlSetID(>id, id);
+}
--
2.7.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCHv8 05/17] util: Refactor code for adding PID to the resource group
The code of adding PID to the allocation could be reused, refactor it
for later reuse.
Signed-off-by: Wang Huaqiang
---
src/util/virresctrl.c | 30 +++---
1 file changed, 19 insertions(+), 11 deletions(-)
diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
index aa062c3..0bac032 100644
--- a/src/util/virresctrl.c
+++ b/src/util/virresctrl.c
@@ -2390,26 +2390,21 @@ virResctrlAllocCreate(virResctrlInfoPtr resctrl,
}
-int
-virResctrlAllocAddPID(virResctrlAllocPtr alloc,
- pid_t pid)
+static int
+virResctrlAddPID(const char *path,
+ pid_t pid)
{
char *tasks = NULL;
char *pidstr = NULL;
int ret = 0;
-/* If allocation is empty, then no resctrl directory and the 'tasks' file
- * exists, just return */
-if (virResctrlAllocIsEmpty(alloc))
-return 0;
-
-if (!alloc->path) {
+if (!path) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("Cannot add pid to non-existing resctrl allocation"));
+ _("Cannot add pid to non-existing resctrl group"));
return -1;
}
-if (virAsprintf(, "%s/tasks", alloc->path) < 0)
+if (virAsprintf(, "%s/tasks", path) < 0)
return -1;
if (virAsprintf(, "%lld", (long long int) pid) < 0)
@@ -2431,6 +2426,19 @@ virResctrlAllocAddPID(virResctrlAllocPtr alloc,
int
+virResctrlAllocAddPID(virResctrlAllocPtr alloc,
+ pid_t pid)
+{
+/* If allocation is empty, then no resctrl directory and the 'tasks' file
+ * exists, just return */
+if (virResctrlAllocIsEmpty(alloc))
+return 0;
+
+return virResctrlAddPID(alloc->path, pid);
+}
+
+
+int
virResctrlAllocRemove(virResctrlAllocPtr alloc)
{
int ret = 0;
--
2.7.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCHv8 13/17] conf: Introduce cache monitor element in cachetune
Introducing element under to represent
a cache monitor.
Signed-off-by: Wang Huaqiang
Reviewed-by: John Ferlan
---
docs/formatdomain.html.in | 26 +++
docs/schemas/domaincommon.rng | 10 +
src/conf/domain_conf.c | 234 -
src/conf/domain_conf.h | 11 +
tests/genericxml2xmlindata/cachetune-cdp.xml | 3 +
.../cachetune-colliding-monitor.xml| 30 +++
tests/genericxml2xmlindata/cachetune-small.xml | 7 +
tests/genericxml2xmltest.c | 2 +
8 files changed, 322 insertions(+), 1 deletion(-)
create mode 100644 tests/genericxml2xmlindata/cachetune-colliding-monitor.xml
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 8850a71..92ad4b2 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -759,6 +759,12 @@
cachetune vcpus='0-3'
cache id='0' level='3' type='both' size='3' unit='MiB'/
cache id='1' level='3' type='both' size='3' unit='MiB'/
+ monitor level='3' vcpus='1'/
+ monitor level='3' vcpus='0-3'/
+/cachetune
+cachetune vcpus='4-5'
+ monitor level='3' vcpus='4'/
+ monitor level='3' vcpus='5'/
/cachetune
memorytune vcpus='0-3'
node id='0' bandwidth='60'/
@@ -978,6 +984,26 @@
+ monitorSince 4.10.0
+
+The optional element monitor creates the cache
+monitor(s) for current cache allocation and has the following
+required attributes:
+
+ level
+
+Host cache level the monitor belongs to.
+
+ vcpus
+
+vCPU list the monitor applies to. A monitor's vCPU list
+can only be the member(s) of the vCPU list of the associated
+allocation. The default monitor has the same vCPU list as the
+associated allocation. For non-default monitors, overlapping
+vCPUs are not permitted.
+
+
+
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 2b465be..1296b7f 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -981,6 +981,16 @@
+
+
+
+
+
+
+
+
+
+
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 8433214..7696098 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -2955,13 +2955,31 @@ virDomainLoaderDefFree(virDomainLoaderDefPtr loader)
static void
+virDomainResctrlMonDefFree(virDomainResctrlMonDefPtr domresmon)
+{
+if (!domresmon)
+return;
+
+virBitmapFree(domresmon->vcpus);
+virObjectUnref(domresmon->instance);
+VIR_FREE(domresmon);
+}
+
+
+static void
virDomainResctrlDefFree(virDomainResctrlDefPtr resctrl)
{
+size_t i = 0;
+
if (!resctrl)
return;
+for (i = 0; i < resctrl->nmonitors; i++)
+virDomainResctrlMonDefFree(resctrl->monitors[i]);
+
virObjectUnref(resctrl->alloc);
virBitmapFree(resctrl->vcpus);
+VIR_FREE(resctrl->monitors);
VIR_FREE(resctrl);
}
@@ -18948,6 +18966,177 @@ virDomainCachetuneDefParseCache(xmlXPathContextPtr
ctxt,
}
+/* Checking if the monitor's vcpus is conflicted with existing allocation
+ * and monitors.
+ *
+ * Returns 1 if @vcpus equals to @resctrl->vcpus, then the monitor will
+ * share the underlying resctrl group with @resctrl->alloc. Returns - 1
+ * if any conflict found. Returns 0 if no conflict and @vcpus is not equal
+ * to @resctrl->vcpus.
+ */
+static int
+virDomainResctrlMonValidateVcpus(virDomainResctrlDefPtr resctrl,
+virBitmapPtr vcpus)
+{
+size_t i = 0;
+int vcpu = -1;
+size_t mons_same_alloc_vcpus = 0;
+
+if (virBitmapIsAllClear(vcpus)) {
+virReportError(VIR_ERR_INVALID_ARG, "%s",
+ _("vcpus is empty"));
+return -1;
+}
+
+while ((vcpu = virBitmapNextSetBit(vcpus, vcpu)) >= 0) {
+if (!virBitmapIsBitSet(resctrl->vcpus, vcpu)) {
+virReportError(VIR_ERR_INVALID_ARG, "%s",
+ _("Monitor vcpus conflicts with allocation"));
+return -1;
+}
+}
+
+if (virBitmapEqual(vcpus, resctrl->vcpus))
+return 1;
+
+for (i = 0; i < resctrl->nmonitors; i++) {
+if (virBitmapEqual(resctrl->vcpus, resctrl->monitors[i]->vcpus)) {
+mons_same_alloc_vcpus++;
+continue;
+}
+
+if (virBitmapOverlaps(vcpus, resctrl->monitors[i]->vcpus)) {
+virReportError(VIR_ERR_INVALID_ARG, "%s",
[libvirt] [PATCHv8 08/17] util: Add interface for creating monitor group
Add interface for creating the resource monitoring group according
to '@virResctrlMonitor->path'.
Signed-off-by: Wang Huaqiang
Reviewed-by: John Ferlan
---
src/libvirt_private.syms | 1 +
src/util/virresctrl.c| 24
src/util/virresctrl.h| 4
3 files changed, 29 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index e175c8b..a878083 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2681,6 +2681,7 @@ virResctrlInfoGetMonitorPrefix;
virResctrlInfoMonFree;
virResctrlInfoNew;
virResctrlMonitorAddPID;
+virResctrlMonitorCreate;
virResctrlMonitorDeterminePath;
virResctrlMonitorNew;
diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
index 94689dd..3216abe 100644
--- a/src/util/virresctrl.c
+++ b/src/util/virresctrl.c
@@ -2542,3 +2542,27 @@ virResctrlMonitorAddPID(virResctrlMonitorPtr monitor,
{
return virResctrlAddPID(monitor->path, pid);
}
+
+
+int
+virResctrlMonitorCreate(virResctrlMonitorPtr monitor,
+const char *machinename)
+{
+int lockfd = -1;
+int ret = -1;
+
+if (!monitor)
+return 0;
+
+if (virResctrlMonitorDeterminePath(monitor, machinename) < 0)
+return -1;
+
+lockfd = virResctrlLockWrite();
+if (lockfd < 0)
+return -1;
+
+ret = virResctrlCreateGroupPath(monitor->path);
+
+virResctrlUnlock(lockfd);
+return ret;
+}
diff --git a/src/util/virresctrl.h b/src/util/virresctrl.h
index 52d283a..76e40a2 100644
--- a/src/util/virresctrl.h
+++ b/src/util/virresctrl.h
@@ -201,4 +201,8 @@ virResctrlMonitorDeterminePath(virResctrlMonitorPtr monitor,
int
virResctrlMonitorAddPID(virResctrlMonitorPtr monitor,
pid_t pid);
+
+int
+virResctrlMonitorCreate(virResctrlMonitorPtr monitor,
+const char *machinename);
#endif /* __VIR_RESCTRL_H__ */
--
2.7.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCHv8 17/17] docs: Updated news.xml about the CMT support
Signed-off-by: Wang Huaqiang --- docs/news.xml | 11 +++ 1 file changed, 11 insertions(+) diff --git a/docs/news.xml b/docs/news.xml index 88774c5..3c84126 100644 --- a/docs/news.xml +++ b/docs/news.xml @@ -50,6 +50,17 @@ pvops-based HVM domains. + + + qemu: Added support for CMT (Cache Monitoring Technology) + + + Introduced cache monitor by monitor element + in cachetune for vCPU threads, and added interface + to get the cache utilization information through command + 'virsh domstats'. + + -- 2.7.4 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCHv8 03/17] util: Refactor code for determining allocation path
The code for determining resctrl allocation path could be reused
for monitor. Refactor it for reuse.
Signed-off-by: Wang Huaqiang
---
src/util/virresctrl.c | 38 ++
1 file changed, 30 insertions(+), 8 deletions(-)
diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
index e3c84a3..463555c 100644
--- a/src/util/virresctrl.c
+++ b/src/util/virresctrl.c
@@ -2266,28 +2266,50 @@ virResctrlAllocAssign(virResctrlInfoPtr resctrl,
}
+static char *
+virResctrlDeterminePath(const char *parentpath,
+const char *prefix,
+const char *id)
+{
+char *path = NULL;
+
+if (!id) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Resctrl ID must be set before determining resctrl "
+ "parentpath='%s' prefix='%s'"), parentpath, prefix);
+return NULL;
+}
+
+if (virAsprintf(, "%s/%s-%s", parentpath, prefix, id) < 0)
+return NULL;
+
+return path;
+}
+
+
int
virResctrlAllocDeterminePath(virResctrlAllocPtr alloc,
const char *machinename)
{
-if (!alloc->id) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("Resctrl Allocation ID must be set before creation"));
+if (alloc->path) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Resctrl allocation path is already set to '%s'"),
+ alloc->path);
return -1;
}
/* If the allocation is empty, then the path will be SYSFS_RESCTRL_PATH */
if (virResctrlAllocIsEmpty(alloc)) {
-if (!alloc->path &&
-VIR_STRDUP(alloc->path, SYSFS_RESCTRL_PATH) < 0)
+if (VIR_STRDUP(alloc->path, SYSFS_RESCTRL_PATH) < 0)
return -1;
return 0;
}
-if (!alloc->path &&
-virAsprintf(>path, "%s/%s-%s",
-SYSFS_RESCTRL_PATH, machinename, alloc->id) < 0)
+alloc->path = virResctrlDeterminePath(SYSFS_RESCTRL_PATH,
+ machinename, alloc->id);
+
+if (!alloc->path)
return -1;
return 0;
--
2.7.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCHv8 06/17] util: Add interface for adding PID to the monitor
Add interface for adding task PID to the monitor.
Signed-off-by: Wang Huaqiang
Reviewed-by: John Ferlan
---
src/libvirt_private.syms | 1 +
src/util/virresctrl.c| 8
src/util/virresctrl.h| 4
3 files changed, 13 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 5235046..e175c8b 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2680,6 +2680,7 @@ virResctrlInfoGetCache;
virResctrlInfoGetMonitorPrefix;
virResctrlInfoMonFree;
virResctrlInfoNew;
+virResctrlMonitorAddPID;
virResctrlMonitorDeterminePath;
virResctrlMonitorNew;
diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
index 0bac032..7bd52cd 100644
--- a/src/util/virresctrl.c
+++ b/src/util/virresctrl.c
@@ -2525,3 +2525,11 @@ virResctrlMonitorDeterminePath(virResctrlMonitorPtr
monitor,
return 0;
}
+
+
+int
+virResctrlMonitorAddPID(virResctrlMonitorPtr monitor,
+pid_t pid)
+{
+return virResctrlAddPID(monitor->path, pid);
+}
diff --git a/src/util/virresctrl.h b/src/util/virresctrl.h
index baae554..52d283a 100644
--- a/src/util/virresctrl.h
+++ b/src/util/virresctrl.h
@@ -197,4 +197,8 @@ virResctrlMonitorNew(void);
int
virResctrlMonitorDeterminePath(virResctrlMonitorPtr monitor,
const char *machinename);
+
+int
+virResctrlMonitorAddPID(virResctrlMonitorPtr monitor,
+pid_t pid);
#endif /* __VIR_RESCTRL_H__ */
--
2.7.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCHv8 11/17] util: Add more interfaces for resctrl monitor
Add interfaces monitor group to support operations such
as add PID, get ID, remove group ... etc.
Signed-off-by: Wang Huaqiang
---
src/libvirt_private.syms | 5 ++
src/util/virresctrl.c| 167 +++
src/util/virresctrl.h| 26
3 files changed, 198 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index a878083..2938833 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2683,7 +2683,12 @@ virResctrlInfoNew;
virResctrlMonitorAddPID;
virResctrlMonitorCreate;
virResctrlMonitorDeterminePath;
+virResctrlMonitorGetCacheOccupancy;
+virResctrlMonitorGetID;
virResctrlMonitorNew;
+virResctrlMonitorRemove;
+virResctrlMonitorSetAlloc;
+virResctrlMonitorSetID;
# util/virrotatingfile.h
diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
index ed682c9..9e7de62 100644
--- a/src/util/virresctrl.c
+++ b/src/util/virresctrl.c
@@ -2590,3 +2590,170 @@ virResctrlMonitorSetID(virResctrlMonitorPtr monitor,
{
return virResctrlSetID(>id, id);
}
+
+
+const char *
+virResctrlMonitorGetID(virResctrlMonitorPtr monitor)
+{
+return monitor->id;
+}
+
+
+void
+virResctrlMonitorSetAlloc(virResctrlMonitorPtr monitor,
+ virResctrlAllocPtr alloc)
+{
+monitor->alloc = virObjectRef(alloc);
+}
+
+
+int
+virResctrlMonitorRemove(virResctrlMonitorPtr monitor)
+{
+int ret = 0;
+
+if (!monitor->path)
+return 0;
+
+if (STREQ(monitor->path, monitor->alloc->path))
+return 0;
+
+VIR_DEBUG("Removing resctrl monitor path=%s", monitor->path);
+if (rmdir(monitor->path) != 0 && errno != ENOENT) {
+ret = -errno;
+VIR_ERROR(_("Unable to remove %s (%d)"), monitor->path, errno);
+}
+
+return ret;
+}
+
+
+static int
+virResctrlMonitorStatsSorter(const void *a,
+ const void *b)
+{
+return ((virResctrlMonitorStatsPtr)a)->id
+- ((virResctrlMonitorStatsPtr)b)->id;
+}
+
+
+/*
+ * virResctrlMonitorGetStats
+ *
+ * @monitor: The monitor that the statistic data will be retrieved from.
+ * @resource: The name for resource name. 'llc_occupancy' for cache resource.
+ * "mbm_total_bytes" and "mbm_local_bytes" for memory bandwidth resource.
+ * @stats: Array of virResctrlMonitorStatsPtr for holding cache or memory
+ * bandwidth usage data.
+ * @nstats: A size_t pointer to hold the returned array length of @stats
+ *
+ * Get cache or memory bandwidth utilization information.
+ *
+ * Returns 0 on success, -1 on error.
+ */
+static int
+virResctrlMonitorGetStats(virResctrlMonitorPtr monitor,
+ const char *resource,
+ virResctrlMonitorStatsPtr *stats,
+ size_t *nstats)
+{
+int rv = -1;
+int ret = -1;
+DIR *dirp = NULL;
+char *datapath = NULL;
+struct dirent *ent = NULL;
+virResctrlMonitorStatsPtr stat = NULL;
+
+if (!monitor) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Invalid resctrl monitor"));
+return -1;
+}
+
+if (virAsprintf(, "%s/mon_data", monitor->path) < 0)
+return -1;
+
+if (virDirOpen(, datapath) < 0)
+goto cleanup;
+
+*nstats = 0;
+while (virDirRead(dirp, , datapath) > 0) {
+char *node_id = NULL;
+
+if (VIR_ALLOC(stat) < 0)
+goto cleanup;
+
+/* Looking for directory that contains resource utilization
+ * information file. The directory name is arranged in format
+ * "mon__". For example, "mon_L3_00" and
+ * "mon_L3_01" are two target directories for a two nodes system
+ * with resource utilization data file for each node respectively.
+ */
+if (ent->d_type != DT_DIR)
+continue;
+
+/* Looking for directory has a prefix 'mon_L' */
+if (!(node_id = STRSKIP(ent->d_name, "mon_L")))
+continue;
+
+/* Looking for directory has another '_' */
+node_id = strchr(node_id, '_');
+if (!node_id)
+continue;
+
+/* Skip the character '_' */
+if (!(node_id = STRSKIP(node_id, "_")))
+continue;
+
+/* The node ID number should be here, parsing it. */
+if (virStrToLong_uip(node_id, NULL, 0, >id) < 0)
+goto cleanup;
+
+rv = virFileReadValueUint(>val, "%s/%s/%s", datapath,
+ ent->d_name, resource);
+if (rv == -2) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("File '%s/%s/%s' does not exist."),
+ datapath, ent->d_name, resource);
+}
+if (rv < 0)
+goto cleanup;
+
+if (VIR_APPEND_ELEMENT(*stats, *nstats, *stat) < 0)
+goto cleanup;
+}
+
+/* Sort in id's ascending order */
+if (*nstats)
+qsort(*stats, *nstats, sizeof(*stat), virResctrlMonitorStatsSorter);
+
+
[libvirt] [PATCHv8 09/17] util: Refactor virResctrlAllocSetID to set allocation ID
Refactor virResctrlAllocSetID.
In new code the error of id overwriting is reported promptly.
Signed-off-by: Wang Huaqiang
---
src/util/virresctrl.c | 25 -
1 file changed, 20 insertions(+), 5 deletions(-)
diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
index 3216abe..4e4831c 100644
--- a/src/util/virresctrl.c
+++ b/src/util/virresctrl.c
@@ -1361,17 +1361,32 @@ virResctrlAllocForeachMemory(virResctrlAllocPtr alloc,
}
-int
-virResctrlAllocSetID(virResctrlAllocPtr alloc,
- const char *id)
+static int
+virResctrlSetID(char **resctrlid,
+const char *id)
{
if (!id) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("Resctrl allocation 'id' cannot be NULL"));
+ _("New resctrl 'id' cannot be NULL"));
+return -1;
+}
+
+if (*resctrlid) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Attempt to overwrite resctrlid='%s' with id='%s'"),
+ *resctrlid, id);
return -1;
}
-return VIR_STRDUP(alloc->id, id);
+return VIR_STRDUP(*resctrlid, id);
+}
+
+
+int
+virResctrlAllocSetID(virResctrlAllocPtr alloc,
+ const char *id)
+{
+return virResctrlSetID(>id, id);
}
--
2.7.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCHv8 00/17] Introduce x86 Cache Monitoring Technology (CMT)
This series of patches and the series already been merged introduce the x86 Cache Monitoring Technology (CMT) to libvirt by interacting with kernel resource control (resctrl) interface. CMT is one of the Intel(R) x86 CPU feature which belongs to the Resource Director Technology (RDT). CMT reports the occupancy of the last level cache, which is shared by all CPU cores. In the v1 series, an original and complete feature for CMT was introduced The v2 and v3 patches address the feature for the host capability of CMT. v4 is addressing the feature for monitoring VM vcpu thread set cache occupancy and reporting it through a virsh command. We have serval discussion about the enabling of CMT, please refer to following links for the RFCs. RFCv3 https://www.redhat.com/archives/libvir-list/2018-August/msg01213.html RFCv2 https://www.redhat.com/archives/libvir-list/2018-July/msg00409.html https://www.redhat.com/archives/libvir-list/2018-July/msg01241.html RFCv1 https://www.redhat.com/archives/libvir-list/2018-June/msg00674.html And the merged commits are list as below, for host capability of CMT. 6af8417415508c31f8ce71234b573b4999f35980 8f6887998bf63594ae26e3db18d4d5896c5f2cb4 58fcee6f3a2b7e89c21c1fb4ec21429c31a0c5b8 12093f1feaf8f5023dcd9d65dff111022842183d a5d293c18831dcf69ec6195798387fbb70c9f461 1. About reason why CMT is necessary in libvirt? The perf events of 'CMT, MBML, MBMT' have been phased out since Linux kernel commit c39a0e2c8850f08249383f2425dbd8dbe4baad69, in libvirt the perf based cmt,mbm will not work with the latest linux kernel. These patches add CMT feature to libvirt through kernel resctrlfs interface. 2 Create cache monitoring group (cache monitor). The main interface for creating monitoring group is through XML file. The proposed configuration is like: + + In above XML, created 2 cache resctrl allocation groups and 2 resctrl monitoring groups. The changes of cache monitor will be effective in next booting of VM. 2 Show CMT result through command 'domstats' Adding the interface in qemu to report this information for resource monitor group through command 'virsh domstats --cpu-total'. Below is a typical output: # virsh domstats 1 --cpu-total Domain: 'ubuntu16.04-base' ... cpu.cache.monitor.count=2 cpu.cache.monitor.0.name=vcpus_1 cpu.cache.monitor.0.vcpus=1 cpu.cache.monitor.0.bank.count=2 cpu.cache.monitor.0.bank.0.id=0 cpu.cache.monitor.0.bank.0.bytes=4505600 cpu.cache.monitor.0.bank.1.id=1 cpu.cache.monitor.0.bank.1.bytes=5586944 cpu.cache.monitor.1.name=vcpus_4-6 cpu.cache.monitor.1.vcpus=4,5,6 cpu.cache.monitor.1.bank.count=2 cpu.cache.monitor.1.bank.0.id=0 cpu.cache.monitor.1.bank.0.bytes=17571840 cpu.cache.monitor.1.bank.1.id=1 cpu.cache.monitor.1.bank.1.bytes=29106176 Changes in v8: - Addressing John's review comments for v7. - Add patch for refactoring virRresctrlAllocSetID and a separate patch for virResctrlMonitorSetID. - Removed patch for 'resctrl->id'. - Removed patch for validating monitor through checking *tasks file. - Removed patch for setup vcpu in libvirt re-reconnection. - Re-designed the functions for showing the result for command 'virsh domstats'. - Move virResctrlMonitorGetCacheOccupancy and its local helper functions to virresctrl.c. Changes in v7: - Add several lines removed by mistake. Changes in v6: - Addressing John's review comments for v5. - Removed and cleaned the concepts of 'default allocation' and 'default monitor'. - qemu: virsh domstats --cpu-total output for CMT, add identifier 'monitor' for each itm. Changes in v5: - qemu: Setting up vcpu and adding pids to resctrl monitor groups during re-connection. - Add the document for domain configuration related to resctrl monitor. Changes in v4: v4 is addressing the feature for monitoring VM vcpu thread set cache occupancy and reporting it through a virsh command. - Introduced resctrl default allocation - Introduced resctrl monitor and default monitor Changes in v3: - Addressed John Ferlan's review. - Typo fixed. - Removed VIR_ENUM_DECL(virMonitor); Changes in v2: - Introduced MBM capability. - Capability layout changed * Moved from cahe to * Renamed to - Document for 'reuseThreshold' changed. - Introduced API virResctrlInfoGetMonitorPrefix - Added more tests, covering standalone CMT, fake new feature. - Creating CMT resource control group will be subsequent job. Wang Huaqiang (17): docs,util: Refactor schemas and virresctrl to support optional cache util: Introduce resctrl monitor for CMT util: Refactor code for determining allocation path util: Add interface to determine monitor path util: Refactor code for adding PID to the resource group util: Add interface for adding PID to the monitor util: Refactor code for creating resctrl group util: Add interface
[libvirt] [PATCHv8 02/17] util: Introduce resctrl monitor for CMT
Cache Monitoring Technology (aka CMT) provides the capability
to report cache utilization information of system task.
This patch introduces the concept of resctrl monitor through
data structure virResctrlMonitor.
Signed-off-by: Wang Huaqiang
---
src/libvirt_private.syms | 1 +
src/util/virresctrl.c| 80
src/util/virresctrl.h| 9 ++
3 files changed, 84 insertions(+), 6 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 335210c..d2573c5 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2680,6 +2680,7 @@ virResctrlInfoGetCache;
virResctrlInfoGetMonitorPrefix;
virResctrlInfoMonFree;
virResctrlInfoNew;
+virResctrlMonitorNew;
# util/virrotatingfile.h
diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
index 7339e9b..e3c84a3 100644
--- a/src/util/virresctrl.c
+++ b/src/util/virresctrl.c
@@ -36,9 +36,9 @@ VIR_LOG_INIT("util.virresctrl")
/* Resctrl is short for Resource Control. It might be implemented for various
- * resources. Currently this supports cache allocation technology (aka CAT) and
- * memory bandwidth allocation (aka MBA). More resources technologies may be
- * added in the future.
+ * resources. Currently this supports cache allocation technology (aka CAT),
+ * memory bandwidth allocation (aka MBA) and cache monitoring technology (aka
+ * CMT). More resources technologies may be added in the future.
*/
@@ -105,6 +105,7 @@ typedef virResctrlAllocMemBW *virResctrlAllocMemBWPtr;
/* Class definitions and initializations */
static virClassPtr virResctrlInfoClass;
static virClassPtr virResctrlAllocClass;
+static virClassPtr virResctrlMonitorClass;
/* virResctrlInfo */
@@ -224,11 +225,16 @@ virResctrlInfoMonFree(virResctrlInfoMonPtr mon)
}
-/* virResctrlAlloc */
+/* virResctrlAlloc and virResctrlMonitor*/
/*
- * virResctrlAlloc represents one allocation (in XML under cputune/cachetune
and
- * consequently a directory under /sys/fs/resctrl). Since it can have multiple
+ * virResctrlAlloc and virResctrlMonitor are representing a resource control
+ * group (in XML under cputune/cachetune and consequently a directory under
+ * /sys/fs/resctrl). virResctrlAlloc is the data structure for resource
+ * allocation, while the virResctrlMonitor represents the resource monitoring
+ * part.
+ *
+ * virResctrlAlloc represents one allocation. Since it can have multiple
* parts of multiple caches allocated it is represented as bunch of nested
* sparse arrays (by sparse I mean array of pointers so that each might be NULL
* in case there is no allocation for that particular cache allocation (level,
@@ -275,6 +281,18 @@ virResctrlInfoMonFree(virResctrlInfoMonPtr mon)
* a sparse array to represent whether a memory bandwidth allocation happens
* on corresponding node. The available memory controller number is collected
* in 'virResctrlInfo'.
+ *
+ * =Cache monitoring technology (CMT)=
+ *
+ * Cache monitoring technology is used to perceive how many cache the process
+ * is using actually. virResctrlMonitor represents the resource control
+ * monitoring group, it is supported to monitor resource utilization
+ * information on granularity of vcpu.
+ *
+ * From hardware perspective, cache monitoring technology (CMT), memory
+ * bandwidth technology (MBM), as well as the CAT and MBA, are all orthogonal
+ * features. The monitor will be created under the scope of default resctrl
+ * group if no specific CAT or MBA entries are provided for the guest."
*/
struct _virResctrlAllocPerType {
/* There could be bool saying whether this is set or not, but since
everything
@@ -320,6 +338,30 @@ struct _virResctrlAlloc {
char *path;
};
+/*
+ * virResctrlMonitor is the data structure for resctrl monitor. Resctrl
+ * monitor represents a resctrl monitoring group, which can be used to
+ * monitor the resource utilization information for either cache or
+ * memory bandwidth.
+ */
+struct _virResctrlMonitor {
+virObject parent;
+
+/* Each virResctrlMonitor is associated with one specific allocation,
+ * either the root directory allocation under /sys/fs/resctrl or a
+ * specific allocation defined under the root directory.
+ * This pointer points to the allocation this monitor is associated with.
+ */
+virResctrlAllocPtr alloc;
+/* The monitor identifier. For a monitor has the same @path name as its
+ * @alloc, the @id will be set to the same value as it is in @alloc->id.
+ */
+char *id;
+/* libvirt-generated path in /sys/fs/resctrl for this particular
+ * monitor */
+char *path;
+};
+
static void
virResctrlAllocDispose(void *obj)
@@ -369,6 +411,17 @@ virResctrlAllocDispose(void *obj)
}
+static void
+virResctrlMonitorDispose(void *obj)
+{
+virResctrlMonitorPtr monitor = obj;
+
+virObjectUnref(monitor->alloc);
+VIR_FREE(monitor->id);
+VIR_FREE(monitor->path);
+}
+
+
Re: [libvirt] [PATCH] bt: Mark the bluetooth subsystem as deprecated
On Mon, Nov 12, 2018 at 02:14:02PM +0100, Gerd Hoffmann wrote: > On Mon, Nov 12, 2018 at 11:00:30AM +0100, Thomas Huth wrote: > > It has been unmaintained since years, and there were only trivial or > > tree-wide changes to the related files since many years, so the > > code is likely very bitrotten and broken. For example the following > > segfaults as soon as as you press a key: > > > > qemu-system-x86_64 -usb -device usb-bt-dongle -bt hci -bt device:keyboard > > > > Since we are not aware of anybody using bluetooth with the current > > version of QEMU, let's mark the subsystem as deprecated, with a special > > request for the users to write to the qemu-devel mailing list in case > > they still use it (so we could revert the deprecation status in that > > case). > > I think that settles the bluetooth debate for now. We deprecate it, > wait for feedback, possibly un-deprecate (parts of) the code. Next > year we'll actually remove the unused code. > > Patch queued for 3.1. FWIW, libvirt has never exposed any of the bluetooth functionality, so no objections to removing it from our side. Regards, Daniel -- |: 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 :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] bt: Mark the bluetooth subsystem as deprecated
On Mon, Nov 12, 2018 at 11:00:30AM +0100, Thomas Huth wrote: > It has been unmaintained since years, and there were only trivial or > tree-wide changes to the related files since many years, so the > code is likely very bitrotten and broken. For example the following > segfaults as soon as as you press a key: > > qemu-system-x86_64 -usb -device usb-bt-dongle -bt hci -bt device:keyboard > > Since we are not aware of anybody using bluetooth with the current > version of QEMU, let's mark the subsystem as deprecated, with a special > request for the users to write to the qemu-devel mailing list in case > they still use it (so we could revert the deprecation status in that > case). I think that settles the bluetooth debate for now. We deprecate it, wait for feedback, possibly un-deprecate (parts of) the code. Next year we'll actually remove the unused code. Patch queued for 3.1. thanks, Gerd -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/2] qemu: don't log error for missing optional sources on stats
Every time we call all domain stats for inactive domain with
unavailable source we get error message in logs. It's a bit noisy.
While it's arguable whether we need such message or not for mandatory
disks we would like not to see messages for optional disks. Let's
filter at least for cases of local files. Fixing other cases would
require passing flag down the stack to .backendInit of storage
which is ugly.
Stats for active domain are fine because we either drop disks
with unavailable sources or clean source which is handled
by virStorageSourceIsEmpty in qemuDomainGetStatsOneBlockFallback.
We have these logs for successful stats since 25aa7035d which
in turn fixes 596a13713 which added substantial stats for offline
disks.
Signed-off-by: Nikolay Shirokovskiy
---
src/qemu/qemu_driver.c | 5 +
src/qemu/qemu_process.c | 9 +
src/qemu/qemu_process.h | 2 ++
3 files changed, 16 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index a52e249..72e4cfe 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -20290,6 +20290,11 @@ qemuDomainGetStatsBlockExportDisk(virDomainDiskDefPtr
disk,
const char *backendstoragealias;
int ret = -1;
+if (!virDomainObjIsActive(dom) &&
+qemuProcessMissingLocalOptionalDisk(disk))
+return qemuDomainGetStatsBlockExportHeader(disk, disk->src, *recordnr,
+ records, nrecords);
+
for (n = disk->src; virStorageSourceIsBacking(n); n = n->backingStore) {
if (blockdev) {
frontendalias = QEMU_DOMAIN_DISK_PRIVATE(disk)->qomName;
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 9cf9718..802274e 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -6070,6 +6070,15 @@ qemuProcessPrepareSEVGuestInput(virDomainObjPtr vm)
}
+bool
+qemuProcessMissingLocalOptionalDisk(virDomainDiskDefPtr disk)
+{
+return disk->startupPolicy == VIR_DOMAIN_STARTUP_POLICY_OPTIONAL &&
+ virStorageSourceIsLocalStorage(disk->src) && disk->src->path &&
+ !virFileExists(disk->src->path);
+}
+
+
static int
qemuProcessPrepareHostStorage(virQEMUDriverPtr driver,
virDomainObjPtr vm,
diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h
index 2037467..52d396d 100644
--- a/src/qemu/qemu_process.h
+++ b/src/qemu/qemu_process.h
@@ -214,4 +214,6 @@ int qemuProcessStartManagedPRDaemon(virDomainObjPtr vm);
void qemuProcessKillManagedPRDaemon(virDomainObjPtr vm);
+bool qemuProcessMissingLocalOptionalDisk(virDomainDiskDefPtr disk);
+
#endif /* __QEMU_PROCESS_H__ */
--
1.8.3.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/2] qemu: don't log error for missing optional sources on start
Because missing optional source is not error. The patch address only local files. Fixing other cases is a bit ugly. Signed-off-by: Nikolay Shirokovskiy --- src/qemu/qemu_process.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 802274e..5e04bf9 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -6100,7 +6100,8 @@ qemuProcessPrepareHostStorage(virQEMUDriverPtr driver, if (!blockdev) virStorageSourceBackingStoreClear(disk->src); -if (qemuDomainDetermineDiskChain(driver, vm, disk, true) >= 0) +if (!qemuProcessMissingLocalOptionalDisk(disk) && +qemuDomainDetermineDiskChain(driver, vm, disk, true) >= 0) continue; if (qemuDomainCheckDiskStartupPolicy(driver, vm, idx, cold_boot) >= 0) -- 1.8.3.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 0/2] qemu: fix misc noisy logs for optional sources
Nikolay Shirokovskiy (2): qemu: don't log error for missing optional sources on stats qemu: don't log error for missing optional sources on start src/qemu/qemu_driver.c | 5 + src/qemu/qemu_process.c | 12 +++- src/qemu/qemu_process.h | 2 ++ 3 files changed, 18 insertions(+), 1 deletion(-) -- 1.8.3.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] qemu: Introduce caching whether /dev/kvm is accessible
On Mon, Nov 12, 2018 at 12:43:11PM +0100, Marc Hartmayer wrote: > On Thu, Nov 01, 2018 at 10:18 AM +0100, "Daniel P. Berrangé" > wrote: > > On Thu, Nov 01, 2018 at 09:31:50AM +0100, Martin Kletzander wrote: > >> On Tue, Oct 30, 2018 at 03:07:59PM +, Daniel P. Berrangé wrote: > >> > On Tue, Oct 30, 2018 at 03:45:36PM +0100, Michal Privoznik wrote: > >> > > On 10/30/2018 02:46 PM, Michal Privoznik wrote: > >> > > > > >> > > > It is kernel problem. In my testing, the moment I call: > >> > > > > >> > > > ioctl(kvm, KVM_CREATE_VM, 0); > >> > > > >> > > Okay, I have to retract this claim. 'udevadm monitor' shows some > >> > > events: > >> > > > >> > > KERNEL[3631.129645] change /devices/virtual/misc/kvm (misc) > >> > > UDEV [3631.130816] change /devices/virtual/misc/kvm (misc) > >> > > > >> > > and stopping udevd leaves all three times untouched. So it is udev > >> > > after > >> > > all. I just don't know how to find the rule that is causing the issue. > >> > > Anyway, as for this patch, I think we can merge it in the end, can't > >> > > we? > >> > > >> > Not really. Udev is in use everywhere, so this behaviour makes the > >> > patch useless in practice, even though it is technically right in > >> > theory :-( > >> > > >> > >> Does it? With this behaviour we still do the "expensive" work after any > >> machine > >> has started. But for one machine starting it still has the effect of > >> running it > >> only once. And we *need* to run it for each machine unless we also cache > >> the > >> result per (at least) user:group of that machine as every machine can run > >> under > >> different user:group. > > > > Yes, with the current patch it still rechecks it for each VM startup > > But that happens only because of udev (see Michal’s answer). > > > , but > > I was going to suggest the caching of this is simply put in a global static > > variable as there's no reason to record this device permissions state in > > the per VM caps cache. > > I’m not sure I understand you correctly, but this patch doesn’t use the > VM/QEMU capabilities cache (that would be 'struct _virQEMUCaps') for the > caching. Instead it uses 'struct _virQEMUCapsCachePriv' and this struct > is initialized only once when initializing the QEMU driver. This should > be pretty similar to your proposal with the global static variable. Oh yes, I missed that. I think this is fine then, and I don't think we need to deal with checking against arbitrary user ids Regards, Daniel -- |: 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 :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 0/2] Don't perform the vfio-ap mdev validation in post parse
Move the respective bits to QEMU validation code instead. Erik Skultety (2): qemu: Extract MDEV VFIO PCI validation code into a separate helper conf: Move VFIO AP validation from post parse to QEMU validation code src/conf/domain_conf.c | 28 --- src/qemu/qemu_domain.c | 61 +- 2 files changed, 55 insertions(+), 34 deletions(-) -- 2.19.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/2] conf: Move VFIO AP validation from post parse to QEMU validation code
Even though commit 11708641 claims that a domain is allowed have a
single VFIO AP hostdev only, this is a limitation posed by the platform
vendor on purely virtual devices. Generally, post parse should only be
used to populate some default values if missing or at least fail
gracefully with VIR_DOMAIN_DEF_PARSE_ALLOW_POST_PARSE_FAIL).
This patch more of an attempt to follow the guidelines for adding new
features rather than a precaution measure, since even if vfio-ap
supported multiple devices, one would have to downgrade libvirt for a
machine to vanish from the list or in terms of future device migration
to slightly older libvirt, there would be most probably a driver mismatch
that would render the migration impossible anyway.
Signed-off-by: Erik Skultety
---
src/conf/domain_conf.c | 28
src/qemu/qemu_domain.c | 28 +++-
2 files changed, 27 insertions(+), 29 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 237540bccc..e8e0adc819 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -4275,31 +4275,6 @@ virDomainDefPostParseGraphics(virDomainDef *def)
}
-static int
-virDomainDefPostParseHostdev(virDomainDefPtr def)
-{
-size_t i;
-bool vfioap_found = false;
-
-/* verify settings of hostdevs vfio-ap */
-for (i = 0; i < def->nhostdevs; i++) {
-virDomainHostdevDefPtr hostdev = def->hostdevs[i];
-
-if (virHostdevIsMdevDevice(hostdev) &&
-hostdev->source.subsys.u.mdev.model ==
VIR_MDEV_MODEL_TYPE_VFIO_AP) {
-if (vfioap_found) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("Only one hostdev of model vfio-ap is "
- "supported"));
-return -1;
-}
-vfioap_found = true;
-}
-}
-return 0;
-}
-
-
/**
* virDomainDriveAddressIsUsedByDisk:
* @def: domain definition containing the disks to check
@@ -5210,9 +5185,6 @@ virDomainDefPostParseCommon(virDomainDefPtr def,
virDomainDefPostParseGraphics(def);
-if (virDomainDefPostParseHostdev(def) < 0)
-return -1;
-
if (virDomainDefPostParseCPU(def) < 0)
return -1;
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 17d207513d..90253ae867 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -4599,6 +4599,32 @@ qemuDomainMdevDefVFIOPCIValidate(const
virDomainHostdevSubsysMediatedDev *dev,
}
+static int
+qemuDomainMdevDefVFIOAPValidate(const virDomainDef *def)
+{
+size_t i;
+bool vfioap_found = false;
+
+/* currently, VFIO-AP is restricted to a single device only */
+for (i = 0; i < def->nhostdevs; i++) {
+virDomainHostdevDefPtr hostdev = def->hostdevs[i];
+
+if (virHostdevIsMdevDevice(hostdev) &&
+hostdev->source.subsys.u.mdev.model ==
VIR_MDEV_MODEL_TYPE_VFIO_AP) {
+if (vfioap_found) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("Only one hostdev of model vfio-ap is "
+ "supported"));
+return -1;
+}
+vfioap_found = true;
+}
+}
+
+return 0;
+}
+
+
static int
qemuDomainMdevDefValidate(const virDomainHostdevSubsysMediatedDev *mdevsrc,
const virDomainDef *def,
@@ -4609,7 +4635,7 @@ qemuDomainMdevDefValidate(const
virDomainHostdevSubsysMediatedDev *mdevsrc,
case VIR_MDEV_MODEL_TYPE_VFIO_PCI:
return qemuDomainMdevDefVFIOPCIValidate(mdevsrc, def, qemuCaps);
case VIR_MDEV_MODEL_TYPE_VFIO_AP:
-break;
+return qemuDomainMdevDefVFIOAPValidate(def);
case VIR_MDEV_MODEL_TYPE_VFIO_CCW:
break;
case VIR_MDEV_MODEL_TYPE_LAST:
--
2.19.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] qemu: Introduce caching whether /dev/kvm is accessible
On Mon, Nov 12, 2018 at 12:50:41PM +0100, Marc Hartmayer wrote: > On Thu, Nov 01, 2018 at 09:31 AM +0100, Martin Kletzander > wrote: [...] > How can you run a machine/QEMU VM under a different user:group other > than changing the user:group in qemu.conf and restart/reload libvirtd? > > As soon as a VM is running we have not to verify /dev/kvm access, no? > (so there should be no problem when libvirtd tries to “reconnect” to > already running VMs). You can add this into your domain XML: phrdina:phrdina And it will run the qemu process under that user. Pavel signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/2] qemu: Extract MDEV VFIO PCI validation code into a separate helper
Since we'll need to validate other models apart from VFIO PCI too,
having a helper for each model should keep the code base cleaner.
Signed-off-by: Erik Skultety
---
src/qemu/qemu_domain.c | 35 +--
1 file changed, 29 insertions(+), 6 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index e25afdad6b..17d207513d 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -4564,11 +4564,11 @@ qemuDomainDeviceDefValidateNetwork(const
virDomainNetDef *net)
static int
-qemuDomainMdevDefValidate(const virDomainHostdevSubsysMediatedDev *mdevsrc,
- const virDomainDef *def,
- virQEMUCapsPtr qemuCaps)
+qemuDomainMdevDefVFIOPCIValidate(const virDomainHostdevSubsysMediatedDev *dev,
+ const virDomainDef *def,
+ virQEMUCapsPtr qemuCaps)
{
-if (mdevsrc->display == VIR_TRISTATE_SWITCH_ABSENT)
+if (dev->display == VIR_TRISTATE_SWITCH_ABSENT)
return 0;
if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VFIO_PCI_DISPLAY)) {
@@ -4578,7 +4578,7 @@ qemuDomainMdevDefValidate(const
virDomainHostdevSubsysMediatedDev *mdevsrc,
return -1;
}
-if (mdevsrc->model != VIR_MDEV_MODEL_TYPE_VFIO_PCI) {
+if (dev->model != VIR_MDEV_MODEL_TYPE_VFIO_PCI) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_(" attribute 'display' is only supported"
" with model='vfio-pci'"));
@@ -4586,7 +4586,7 @@ qemuDomainMdevDefValidate(const
virDomainHostdevSubsysMediatedDev *mdevsrc,
return -1;
}
-if (mdevsrc->display == VIR_TRISTATE_SWITCH_ON) {
+if (dev->display == VIR_TRISTATE_SWITCH_ON) {
if (def->ngraphics == 0) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("graphics device is needed for attribute value "
@@ -4599,6 +4599,29 @@ qemuDomainMdevDefValidate(const
virDomainHostdevSubsysMediatedDev *mdevsrc,
}
+static int
+qemuDomainMdevDefValidate(const virDomainHostdevSubsysMediatedDev *mdevsrc,
+ const virDomainDef *def,
+ virQEMUCapsPtr qemuCaps)
+{
+
+switch ((virMediatedDeviceModelType) mdevsrc->model) {
+case VIR_MDEV_MODEL_TYPE_VFIO_PCI:
+return qemuDomainMdevDefVFIOPCIValidate(mdevsrc, def, qemuCaps);
+case VIR_MDEV_MODEL_TYPE_VFIO_AP:
+break;
+case VIR_MDEV_MODEL_TYPE_VFIO_CCW:
+break;
+case VIR_MDEV_MODEL_TYPE_LAST:
+virReportEnumRangeError(virMediatedDeviceModelType,
+mdevsrc->model);
+return -1;
+}
+
+return 0;
+}
+
+
static int
qemuDomainDeviceDefValidateHostdev(const virDomainHostdevDef *hostdev,
const virDomainDef *def,
--
2.19.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv7 13/18] qemu: enable resctrl monitor in qemu
On 11/6/2018 2:44 AM, John Ferlan wrote:
On 10/22/18 4:01 AM, Wang Huaqiang wrote:
Add functions for creating, destroying, reconnecting resctrl
monitor in qemu according to the configuration in domain XML.
Signed-off-by: Wang Huaqiang
---
src/qemu/qemu_process.c | 66 -
1 file changed, 65 insertions(+), 1 deletion(-)
[...]
@@ -5430,6 +5441,7 @@ qemuProcessSetupVcpu(virDomainObjPtr vm,
{
pid_t vcpupid = qemuDomainGetVcpuPid(vm, vcpuid);
virDomainVcpuDefPtr vcpu = virDomainDefGetVcpu(vm->def, vcpuid);
+virDomainResctrlMonDefPtr mon = NULL;
size_t i = 0;
if (qemuProcessSetupPid(vm, vcpupid, VIR_CGROUP_THREAD_VCPU,
@@ -5440,11 +5452,42 @@ qemuProcessSetupVcpu(virDomainObjPtr vm,
return -1;
for (i = 0; i < vm->def->nresctrls; i++) {
+size_t j = 0;
virDomainResctrlDefPtr ct = vm->def->resctrls[i];
if (virBitmapIsBitSet(ct->vcpus, vcpuid)) {
if (virResctrlAllocAddPID(ct->alloc, vcpupid) < 0)
return -1;
+
+/* The order of invoking virResctrlMonitorAddPID matters, it is
+ * required to invoke this function first for monitor that has
+ * the same vcpus setting as the allocation in same def->resctrl.
+ * Otherwise, some other monitor's pid may be removed from its
+ * resource group's 'tasks' file.*/
+for (j = 0; j < vm->def->resctrls[i]->nmonitors; j++) {
+mon = vm->def->resctrls[i]->monitors[j];
+
+if (!virBitmapEqual(ct->vcpus, mon->vcpus))
+continue;
+
+if (virBitmapIsBitSet(mon->vcpus, vcpuid)) {
+if (virResctrlMonitorAddPID(mon->instance, vcpupid) < 0)
+return -1;
+}
+break;
+}
+
+for (j = 0; j < vm->def->resctrls[i]->nmonitors; j++) {
+mon = vm->def->resctrls[i]->monitors[j];
+
+if (virBitmapEqual(ct->vcpus, mon->vcpus))
+continue;
+
+if (virBitmapIsBitSet(mon->vcpus, vcpuid)) {
+if (virResctrlMonitorAddPID(mon->instance, vcpupid) < 0)
+return -1;
+}
+}
break;
}
}
As I'm reading the subsequent patch, I'm wondering about the order of
the patches, what happens on the vcpu hotunplug case, and are we doing
the "correct thing" on the reconnect path.
1. with respect to order - it probably doesn't really matter, but I
guess adding another list to manage in the next patch got my attention
and thinking well shouldn't the above code for MonitorAddPID be "ready"
and not changed afterwards.
Then I need to merge the next patch, patch 14, with some previous patch,
at least
for the 'MonitorAddPID' function, to avoid a second changing of a same
function.
Got, thanks.
But, in my next series of patches, patch 14 is removed with all logic in
it. So the patch
order seems rational then.
2. Since qemuProcessSetupVcpu is called from qemuDomainHotplugAddVcpu
that means we probably need to handle qemuDomainHotplugDelVcpu
processing. Of course, when Martin added the resctrl support in commit
9a2fc2db8, the corollary wasn't handled.
So the *tasks file could have stale pids in it if vcpus were unplugged
since there's nothing (obvious to me) that removes the pid from the
file. Furthermore a stale pid in the grand scheme of pid reusage could
become not stale and what happens if a pid is found - do we end up in
some error path...
The *tasks file is not a file kept in a block device, its content, the
PIDS, could be
removed if the process/task is terminated or PIDs have been added to
some other
resctrl group.
It seems the *tasks file will not be stale in scenario of vcpu hot-plug.
For vcpu unplugging case, qemuDomainHotplugDelVcpu removes and terminates
the vcpu process, right? Then the *tasks file will automatically be
updated by removing
the process's PID. The removal of PID from *tasks is done by resctrl
file system.
So there is no hot-plug issue for resctrl in terms of tracking PID in
*tasks.
3. In the reconnect logic - it would seem that we would be starting from
scratch again, right?
In my understanding reconnect means the restart of libvirtd and the
information
re-building for VMs.
Originally I believed that vcpupid would be changed during a reconnect,
that is
the reason I add the function for validating monitor in patch14.
But the vcpupid has not been changed in process of reconnect. So I
decide to remove
patch14.
So would the *tasks file even be valid since vcpus
could be added/deleted and we didn't get notified...
Do you still believe "vcpus could be added/deleted and we didn't get
notified' in reconnect"?
If so, can you list more details?
And any operation for adding/deleting VM vcpus out of libvirt is not
Re: [libvirt] [PATCHv7 16/18] qemu: refactor qemuDomainGetStatsCpu
On 11/6/2018 3:27 AM, John Ferlan wrote:
On 10/22/18 4:01 AM, Wang Huaqiang wrote:
Refactoring qemuDomainGetStatsCpu, make it possible to add
more CPU statistics.
Signed-off-by: Wang Huaqiang
---
src/qemu/qemu_driver.c | 45 ++---
1 file changed, 22 insertions(+), 23 deletions(-)
Maybe instead of inverting the logic, let's create a cgroup helper...
qemuDomainGetStatsCpuCgroup(dom, record, *maxparams)
{
logic as is now
}
static int
qemuDomainGetStatsCpu(virQEMUDriverPtr driver ATTRIBUTE_UNUSED,
virDomainObjPtr dom,
virDomainStatsRecordPtr record,
int *maxparams,
unsigned int privflags ATTRIBUTE_UNUSED)
{
return qemuDomainGetStatsCpuCgroup(dom, record, maxparams);
}
Then the subsequent patch would alter the above check and add the new call.
John
Thanks for advice. Will be done in next series.
Huaqiang
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index a52e249..12a5f8f 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -19711,30 +19711,29 @@ qemuDomainGetStatsCpu(virQEMUDriverPtr driver
ATTRIBUTE_UNUSED,
unsigned long long sys_time = 0;
int err = 0;
-if (!priv->cgroup)
-return 0;
-
-err = virCgroupGetCpuacctUsage(priv->cgroup, _time);
-if (!err && virTypedParamsAddULLong(>params,
->nparams,
-maxparams,
-"cpu.time",
-cpu_time) < 0)
-return -1;
+if (priv->cgroup) {
+err = virCgroupGetCpuacctUsage(priv->cgroup, _time);
+if (!err && virTypedParamsAddULLong(>params,
+>nparams,
+maxparams,
+"cpu.time",
+cpu_time) < 0)
+return -1;
-err = virCgroupGetCpuacctStat(priv->cgroup, _time, _time);
-if (!err && virTypedParamsAddULLong(>params,
->nparams,
-maxparams,
-"cpu.user",
-user_time) < 0)
-return -1;
-if (!err && virTypedParamsAddULLong(>params,
->nparams,
-maxparams,
-"cpu.system",
-sys_time) < 0)
-return -1;
+err = virCgroupGetCpuacctStat(priv->cgroup, _time, _time);
+if (!err && virTypedParamsAddULLong(>params,
+>nparams,
+maxparams,
+"cpu.user",
+user_time) < 0)
+return -1;
+if (!err && virTypedParamsAddULLong(>params,
+>nparams,
+maxparams,
+"cpu.system",
+sys_time) < 0)
+return -1;
+}
return 0;
}
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv7 00/18] Introduce x86 Cache Monitoring Technology (CMT)
On 11/6/2018 4:19 AM, John Ferlan wrote: On 10/22/18 4:01 AM, Wang Huaqiang wrote: This series of patches and the series already been merged introduce the x86 Cache Monitoring Technology (CMT) to libvirt by interacting with kernel resource control (resctrl) interface. CMT is one of the Intel(R) x86 CPU feature which belongs to the Resource Director Technology (RDT). CMT reports the occupancy of the last level cache, which is shared by all CPU cores. Rather than top or bottom post ;-) - as noted during the reviewing - many of the patches are fine; however, once we get to the meat in patch 12 and beyond I think more adjustment is necessary. I wouldn't want to introduce the XML from patch 12, but have no teeth behind it from the remaining patches. Really appreciate your review and comments, your comments not only pointed out my faults but also provide very reasonable suggestions. For patch 1-11, I also addressed your review opinions in my next version code, please make a review at your convenient time. For patches after 11, I also addressed your review suggestions by removing the patches for adding 'resctrl->id' and for 'virResctrlMonitorIsRunning', and made some code split for patch17. Please make a review. Getting closer, but not quite there yet. If you're good with my proposed adjustments for patches 1-11, then I can get those pushed so that we can make some progress. On your next round, please let's try to get a news.xml to summarize the changes added at the end. Saves me from chasing later on. OK. A separate patch for change of news.xml is appended. I won't have a lot of cycles for the rest of the week to debate or provide feedback. I'm in class Wed -> Fri. I really don't think the comments I've provided in the later patches are that controversial. Agree. Your comments are very accurate. This time I have time to prepare the code along with the reply to your review emails. I do think we probably need a way to handle the Unplug with the existing usage of the *tasks file before we go too far and add monitor. By analyzing the code handling vcpu unplug , it seems there is no problem to keep the *tasks file aligned with PIDs. Because the removal of PID is automatically done by kernel if kernel find some vcpu process is disappeared. I need more time to test on vcpu unplug cases for my code. I have not achieved a successful live removal of vcpus, even with code without my new resctrl monitor patches. I will update when I have some conclusion. You also need to figure a way to not add the same pid twice. I realize now that a *Remove operation will remove the *tasks file from a $path so some concerns I had were at least reduced. To add PID twice is because I thought the *tasks file could be removed/added in process of re-connection, when performing 'systemctl restart libvirtd', but it seems is does not. So it is not necessary to add same pid twice. The libvirtd daemon restart process works fine after the remove of relevant patch. And *tasks file is not removed or changed in process of re-connection. So patch18 and patch14 are removed. John Thanks for all for your review. Huaqiang In the v1 series, an original and complete feature for CMT was introduced The v2 and v3 patches address the feature for the host capability of CMT. v4 is addressing the feature for monitoring VM vcpu thread set cache occupancy and reporting it through a virsh command. We have serval discussion about the enabling of CMT, please refer to following links for the RFCs. RFCv3 https://www.redhat.com/archives/libvir-list/2018-August/msg01213.html RFCv2 https://www.redhat.com/archives/libvir-list/2018-July/msg00409.html https://www.redhat.com/archives/libvir-list/2018-July/msg01241.html RFCv1 https://www.redhat.com/archives/libvir-list/2018-June/msg00674.html And the merged commits are list as below, for host capability of CMT. 6af8417415508c31f8ce71234b573b4999f35980 8f6887998bf63594ae26e3db18d4d5896c5f2cb4 58fcee6f3a2b7e89c21c1fb4ec21429c31a0c5b8 12093f1feaf8f5023dcd9d65dff111022842183d a5d293c18831dcf69ec6195798387fbb70c9f461 1. About reason why CMT is necessary in libvirt? The perf events of 'CMT, MBML, MBMT' have been phased out since Linux kernel commit c39a0e2c8850f08249383f2425dbd8dbe4baad69, in libvirt the perf based cmt,mbm will not work with the latest linux kernel. These patches add CMT feature to libvirt through kernel resctrlfs interface. 2 Create cache monitoring group (cache monitor). The main interface for creating monitoring group is through XML file. The proposed configuration is like: + + In above XML, created 2 cache resctrl allocation groups and 2 resctrl monitoring groups. The changes of cache monitor will be effective in next booting of VM. 2 Show CMT result through command 'domstats' Adding the interface in qemu to report this information
Re: [libvirt] [PATCHv7 18/18] qemu: Setting up vcpu and adding pids to resctrl monitor groups during reconnection
-Original Message- From: John Ferlan [mailto:jfer...@redhat.com] Sent: Tuesday, November 6, 2018 4:09 AM To: Wang, Huaqiang ; libvir-list@redhat.com Cc: Feng, Shaohe ; Niu, Bing ; Ding, Jian-feng ; Zang, Rui Subject: Re: [PATCHv7 18/18] qemu: Setting up vcpu and adding pids to resctrl monitor groups during reconnection On 10/22/18 4:01 AM, Wang Huaqiang wrote: > Invoking qemuProcessSetupVcpus in process of VM reconnection. > > The vcpu pid information need to be refilled to resctrl monitor after > a VM reconnection./ > > Signed-off-by: Wang Huaqiang > --- > src/qemu/qemu_process.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index > fba4fb4..ed0330b 100644 > --- a/src/qemu/qemu_process.c > +++ b/src/qemu/qemu_process.c > @@ -8008,6 +8008,9 @@ qemuProcessReconnect(void *opaque) > } > } > > +if (qemuProcessSetupVcpus(obj) < 0) > +goto error; > + IIRC the reason for not needing this is that the assumption is that the initial startup (ProcessLaunch) and any hotplug thereafter would end up calling qemuProcessSetupVcpu. Not sure this is necessary for everything except perhaps the resctrl monitor *pids list which I'm not even sure is necessary. This time I think this patch is not necessary. This patch is only required if *tasks file content is changed in process of re-connection. Now I don't think the *tasks file will be changed because the vcpu processes are still there in process of reconnection and no add no removal. Patch will be removed. John Thanks for review. Huaqiang > /* update domain state XML with possibly updated state in virDomainObj */ > if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, obj, driver->caps) < 0) > goto error; > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv7 14/18] util: Add function for checking if monitor is running
This patch is added previously because I though the vcpupid would be
changed during
libvirt re-connection, and if vcpupid is changed and libvirt is not
aware of this change
it will make monitor working on an stale *tasks file and monitor will
get wrong
cache information.
But the vcpuid will not change in process of libvirt re-connection, the
*tasks file is always
tacking on right PIDs. So this patch is not necessary now, will be
removed in next update
of patch series.
Thanks for review.
Huaqiang
On 11/6/2018 3:07 AM, John Ferlan wrote:
On 10/22/18 4:01 AM, Wang Huaqiang wrote:
Check whether monitor is running by checking the monitor's PIDs status.
Monitor is looked as running normally if the vcpu PID list matches with
the content of monitor's 'tasks' file.
Signed-off-by: Wang Huaqiang
---
src/libvirt_private.syms | 1 +
src/util/virresctrl.c| 102 ++-
src/util/virresctrl.h| 3 ++
3 files changed, 105 insertions(+), 1 deletion(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index d59ac86..91801ed 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2684,6 +2684,7 @@ virResctrlMonitorAddPID;
virResctrlMonitorCreate;
virResctrlMonitorDeterminePath;
virResctrlMonitorGetID;
+virResctrlMonitorIsRunning;
virResctrlMonitorNew;
virResctrlMonitorRemove;
virResctrlMonitorSetAlloc;
diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
index b0205bc..fa3e6e9 100644
--- a/src/util/virresctrl.c
+++ b/src/util/virresctrl.c
@@ -359,6 +359,9 @@ struct _virResctrlMonitor {
/* libvirt-generated path in /sys/fs/resctrl for this particular
* monitor */
char *path;
+/* Tracking the tasks' PID associated with this monitor */
+pid_t *pids;
+size_t npids;
};
@@ -418,6 +421,7 @@ virResctrlMonitorDispose(void *obj)
virObjectUnref(monitor->alloc);
VIR_FREE(monitor->id);
VIR_FREE(monitor->path);
+VIR_FREE(monitor->pids);
}
@@ -2540,7 +2544,20 @@ int
virResctrlMonitorAddPID(virResctrlMonitorPtr monitor,
pid_t pid)
{
-return virResctrlAddPID(monitor->path, pid);
+size_t i = 0;
+
+if (virResctrlAddPID(monitor->path, pid) < 0)
+return -1;
+
+for (i = 0; i < monitor->npids; i++) {
+if (pid == monitor->pids[i])
+return 0;
+}
+
+if (VIR_APPEND_ELEMENT(monitor->pids, monitor->npids, pid) < 0)
+return -1;
+
+return 0;
could just be a return VIR_APPEND_ELEMENT()
So we theoretically have our @pid in the task file (ResctrlAddPID) and
this internal list of pids which makes me wonder why other than
"quicker" lookup than opening the tasks file and looking for our pid, right?
But based on the next hunk for virResctrlMonitorIsRunning, I'm not so
sure. In fact I wonder why are we doing this?
}
@@ -2613,3 +2630,86 @@ virResctrlMonitorRemove(virResctrlMonitorPtr monitor)
return ret;
}
+
+
+static int
+virResctrlPIDSorter(const void *pida, const void *pidb)
+{
+return *(pid_t*)pida - *(pid_t*)pidb;
+}
+
+
+bool
+virResctrlMonitorIsRunning(virResctrlMonitorPtr monitor)
+{
+char *pidstr = NULL;
+char **spids = NULL;
+size_t nspids = 0;
+pid_t *pids = NULL;
+size_t npids = 0;
+size_t i = 0;
+int rv = -1;
+bool ret = false;
+
So this is a heavyweight type method and seems to me to do far more than
just determine if a monitor is running. In fact, it seems it's
validating that the internal list of monitor->pids matches whatever is
in the *tasks file. There's multiple failure scenarios that may or may
not "mean" a monitor is or isn't running.
+/* path is not determined yet, monitor is not running*/
+if (!monitor->path)
+return false;
+
+/* No vcpu PID filled, regard monitor as not running */
+if (monitor->npids == 0)
+return false;
+
+/* If no 'tasks' file found, underlying resctrl directory is not
+ * ready, regard monitor as not running */
+rv = virFileReadValueString(, "%s/tasks", monitor->path);
+if (rv < 0)
+goto cleanup;
So we read the file, but while we're reading someone could have added to
it... There's some locking concerns here.
The *tasks file can have a pid added by a and the same pid added
by a it seems at least from my reading of qemuProcessSetupVcpu
logic where virResctrlAllocAddPID would be called first followed by a
call to virResctrlMonitorAddPID. Neither checks if the pid exists before
writing (so yes more questions/concerns about patch 13 logic).
+
+/* no PID in task file, monitor is not running */
+if (!*pidstr)
+goto cleanup;
+
+/* The tracking monitor PID list is not identical to the
+ * list in current resctrl directory. monitor is corrupted,
+ * report error and un-running state */
+spids = virStringSplitCount(pidstr, "\n", 0, );
+if (nspids != monitor->npids) {
+
Re: [libvirt] [PATCHv7 13/18] qemu: enable resctrl monitor in qemu
On 11/6/2018 2:01 AM, John Ferlan wrote:
On 10/22/18 4:01 AM, Wang Huaqiang wrote:
Add functions for creating, destroying, reconnecting resctrl
monitor in qemu according to the configuration in domain XML.
Signed-off-by: Wang Huaqiang
---
src/qemu/qemu_process.c | 66 -
1 file changed, 65 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index e9c7618..fba4fb4 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
[...]
@@ -5440,11 +5452,42 @@ qemuProcessSetupVcpu(virDomainObjPtr vm,
return -1;
for (i = 0; i < vm->def->nresctrls; i++) {
+size_t j = 0;
virDomainResctrlDefPtr ct = vm->def->resctrls[i];
if (virBitmapIsBitSet(ct->vcpus, vcpuid)) {> if (virResctrlAllocAddPID(ct->alloc, vcpupid) < 0)
return -1;
+
+/* The order of invoking virResctrlMonitorAddPID matters, it is
+ * required to invoke this function first for monitor that has
+ * the same vcpus setting as the allocation in same def->resctrl.
+ * Otherwise, some other monitor's pid may be removed from its
+ * resource group's 'tasks' file.*/
+for (j = 0; j < vm->def->resctrls[i]->nmonitors; j++) {
s/vm->def->resctrls[i]/ct/ (above and below)
Yes. Will make such kind of substitution for all cases.
+mon = vm->def->resctrls[i]->monitors[j];
+
+if (!virBitmapEqual(ct->vcpus, mon->vcpus))
+continue;
+
+if (virBitmapIsBitSet(mon->vcpus, vcpuid)) {
+if (virResctrlMonitorAddPID(mon->instance, vcpupid) < 0)
+return -1;
+}
+break;
It seems this break should be inside the IsBitSet, right (as is for the
ct->alloc, vcpupid match)? Otherwise, we run the loop just once and not
run until we find our vcpuid in mon->vcpus
Yes. You are right.
+}
+
The next loop is duplicitous and can be removed, right?
In my original code logic, which is I need a @monitor->pids array to
track all pids belonging to
@monitor, these two loops are necessary. The first loop ignores all
non-default monitors
and adds the default monitor's PIDs to its 'tasks' file if default
monitor exists. The second
loop adds non-default monitors' PIDs. This order is intentionally
designed because file writing
for default monitor's 'tasks' file will remove PIDs that existing in
other monitor's 'tasks' file.
But I have taken your suggestion that the @monitor->pids is meaningless
and removed this
array, then, these two loops are not needed any more, only the second
loop is kept. Along
with the review comments you made the code would be:
@@ -5440,11 +5451,26 @@ qemuProcessSetupVcpu(virDomainObjPtr vm,
return -1;
for (i = 0; i < vm->def->nresctrls; i++) {
+ size_t j = 0;
virDomainResctrlDefPtr ct = vm->def->resctrls[i];
if (virBitmapIsBitSet(ct->vcpus, vcpuid)) {
if (virResctrlAllocAddPID(ct->alloc, vcpupid) < 0)
return -1;
+
+ for (j = 0; j < ct->nmonitors; j++) {
+ mon = ct->monitors[j];
+
+ if (virBitmapEqual(ct->vcpus, mon->vcpus))
+ continue;
+
+ if (virBitmapIsBitSet(mon->vcpus, vcpuid)) {
+ if (virResctrlMonitorAddPID(mon->instance, vcpupid)
< 0)
+ return -1;
+ break;
+ }
+ }
+
break;
}
}
with some adjustments (which I can make as described),
Reviewed-by: John Ferlan
John
Thanks for review.
Huaqiang
[...]
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv7 17/18] qemu: Report cache occupancy (CMT) with domstats
On 11/6/2018 4:04 AM, John Ferlan wrote:
On 10/22/18 4:01 AM, Wang Huaqiang wrote:
Adding the interface in qemu to report CMT statistic information
through command 'virsh domstats --cpu-total'.
Below is a typical output:
# virsh domstats 1 --cpu-total
Domain: 'ubuntu16.04-base'
...
cpu.cache.monitor.count=2
cpu.cache.monitor.0.name=vcpus_1
cpu.cache.monitor.0.vcpus=1
cpu.cache.monitor.0.bank.count=2
cpu.cache.monitor.0.bank.0.id=0
cpu.cache.monitor.0.bank.0.bytes=4505600
cpu.cache.monitor.0.bank.1.id=1
cpu.cache.monitor.0.bank.1.bytes=5586944
cpu.cache.monitor.1.name=vcpus_4-6
cpu.cache.monitor.1.vcpus=4,5,6
cpu.cache.monitor.1.bank.count=2
cpu.cache.monitor.1.bank.0.id=0
cpu.cache.monitor.1.bank.0.bytes=17571840
cpu.cache.monitor.1.bank.1.id=1
cpu.cache.monitor.1.bank.1.bytes=29106176
Signed-off-by: Wang Huaqiang
---
src/libvirt-domain.c | 9 ++
src/libvirt_private.syms | 1 +
src/qemu/qemu_driver.c | 229 +++
src/util/virresctrl.c| 130 +++
src/util/virresctrl.h| 12 +++
5 files changed, 381 insertions(+)
I have a feeling I'll be asking for this to be split up a bit, but let's
see... There's *util, *qemu, and *API code in the same patch.
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
index 7690339..4895f9f 100644
--- a/src/libvirt-domain.c
+++ b/src/libvirt-domain.c
@@ -11345,6 +11345,15 @@ virConnectGetDomainCapabilities(virConnectPtr conn,
* "cpu.user" - user cpu time spent in nanoseconds as unsigned long long.
* "cpu.system" - system cpu time spent in nanoseconds as unsigned long
*long.
+ * "cpu.cache.monitor.count" - tocal cache monitoring groups
+ * "cpu.cache.monitor.M.name" - name of cache monitoring group 'M'
+ * "cpu.cache.monitor.M.vcpus" - vcpus for cache monitoring group 'M'
+ * "cpu.cache.monitor.M.bank.count" - total bank number of cache monitoring
+ *group 'M'
+ * "cpu.cache.monitor.M.bank.N.id" - OS assigned cache bank id for cache
+ *'N' in cache monitoring group 'M'
+ * "cpu.cache.monitor.M.bank.N.bytes" - monitor's cache occupancy of cache
+ *bank 'N' in cache monitoring group 'M'
*
* VIR_DOMAIN_STATS_BALLOON:
* Return memory balloon device information.
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 91801ed..4551767 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2683,6 +2683,7 @@ virResctrlInfoNew;
virResctrlMonitorAddPID;
virResctrlMonitorCreate;
virResctrlMonitorDeterminePath;
+virResctrlMonitorGetCacheOccupancy;
virResctrlMonitorGetID;
virResctrlMonitorIsRunning;
virResctrlMonitorNew;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 12a5f8f..9828118 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -102,6 +102,7 @@
#include "virnuma.h"
#include "dirname.h"
#include "netdev_bandwidth_conf.h"
+#include "c-ctype.h"
Ahh.. this one's a red flag... Says to me that something should move to
a util function...
Got. Header file will be removed （actually I planned to remove all code
using it.)
#define VIR_FROM_THIS VIR_FROM_QEMU
@@ -19698,6 +19699,230 @@ typedef enum {
#define HAVE_JOB(flags) ((flags) & QEMU_DOMAIN_STATS_HAVE_JOB)
+/* In terms of the output of virBitmapFormat, both '1-3' and '1,3' are valid
+ * outputs and represent different vcpu set. But it is not easy to
+ * differentiate these two vcpu set formats at first glance.
+ *
+ * This function could be used to clear this ambiguity, it substitutes all '-'
+ * with ',' while generates semantically correct vcpu set.
+ * e.g. vcpu set string '1-3' will be replaced by string '1,2,3'. */
+static char *
+qemuDomainVcpuFormatHelper(const char *vcpus)
+{
+size_t i = 0;
+int last = 0;
+int start = 0;
+char * tmp = NULL;
+bool firstnum = true;
+const char *cur = vcpus;
+virBuffer buf = VIR_BUFFER_INITIALIZER;
+char *ret = NULL;
+
+if (virStringIsEmpty(cur))
+return NULL;
+
+while (*cur != '\0') {
+if (!c_isdigit(*cur))
Explains the #include, but in the long run, I don't think this method is
worth the effort since all you're doing is printing the format in the
output. Is there other libvirt generated output for cpu stats that does
a similar conversion? If so, share code, if not drop this.
No need to rearrange the range someone else entered and we've
succesfully parsed in some manner. I think the output should look like
the XML output unless there's some compelling reason to change it which
I don't see.
From above the:
cpu.cache.monitor.1.name=vcpus_4-6
Re: [libvirt] [PATCHv7 15/18] conf: Add 'id' to virDomainResctrlDef
On 11/6/2018 3:15 AM, John Ferlan wrote:
On 10/22/18 4:01 AM, Wang Huaqiang wrote:
Adding element 'id' to virDomainResctrlDef tracking resource group
id, it reflects the attribute 'id' of of element in XML.
virResctrlAlloc.id is a copy from virDomanResctrlDef.id.
Signed-off-by: Wang Huaqiang
---
src/conf/domain_conf.c | 20
src/conf/domain_conf.h | 1 +
2 files changed, 9 insertions(+), 12 deletions(-)
Not sure I see the need to duplicate the alloc->id value especially
since it's "invalid" have "resctrl->alloc == NULL", right?
Can this resctrl->id ever be different? Not that I can see. I see this
is a desire to reduce an error case in Format processing, but if
virResctrlAllocGetID returned NULL there's other problems...
John
This patch should be removed and will be removed.
It is introduced in series v4, in that series the 'resctrl->alloc' is
allowed
to be NULL representing a default monitor.
Now we changed our design and resctrl->alloc is always assigned with an
virResctrlAlloc data structure, then 'resctrl->id' is not necessary. We
can keep the
resctrl ID in alloc->id.
Thanks for review.
Huaqiang
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 01f795f..1aecd8c 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -2980,6 +2980,7 @@ virDomainResctrlDefFree(virDomainResctrlDefPtr resctrl)
virObjectUnref(resctrl->alloc);
virBitmapFree(resctrl->vcpus);
VIR_FREE(resctrl->monitors);
+VIR_FREE(resctrl->id);
VIR_FREE(resctrl);
}
@@ -19145,6 +19146,9 @@ virDomainResctrlNew(xmlNodePtr node,
if (VIR_ALLOC(resctrl) < 0)
goto cleanup;
+if (VIR_STRDUP(resctrl->id, alloc_id) < 0)
+goto cleanup;
+
if (!(resctrl->vcpus = virBitmapNewCopy(*vcpus))) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("failed to copy 'vcpus'"));
@@ -27323,13 +27327,9 @@ virDomainCachetuneDefFormat(virBufferPtr buf,
virBufferAsprintf(buf, "
-if (!(flags & VIR_DOMAIN_DEF_FORMAT_INACTIVE)) {
-const char *alloc_id = virResctrlAllocGetID(resctrl->alloc);
-if (!alloc_id)
-goto cleanup;
+if (!(flags & VIR_DOMAIN_DEF_FORMAT_INACTIVE))
+virBufferAsprintf(buf, " id='%s'", resctrl->id);
-virBufferAsprintf(buf, " id='%s'", alloc_id);
-}
virBufferAddLit(buf, ">\n");
virBufferAddBuffer(buf, );
@@ -27386,13 +27386,9 @@ virDomainMemorytuneDefFormat(virBufferPtr buf,
virBufferAsprintf(buf, "
-if (!(flags & VIR_DOMAIN_DEF_FORMAT_INACTIVE)) {
-const char *alloc_id = virResctrlAllocGetID(resctrl->alloc);
-if (!alloc_id)
-goto cleanup;
+if (!(flags & VIR_DOMAIN_DEF_FORMAT_INACTIVE))
+virBufferAsprintf(buf, " id='%s'", resctrl->id);
-virBufferAsprintf(buf, " id='%s'", alloc_id);
-}
virBufferAddLit(buf, ">\n");
virBufferAddBuffer(buf, );
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 60f6464..e190aa2 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2248,6 +2248,7 @@ typedef struct _virDomainResctrlDef virDomainResctrlDef;
typedef virDomainResctrlDef *virDomainResctrlDefPtr;
struct _virDomainResctrlDef {
+char *id;
virBitmapPtr vcpus;
virResctrlAllocPtr alloc;
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] nodedev: Add new module node_device_util
On Fri, Nov 02, 2018 at 03:30:56PM +0100, Michal Privoznik wrote: > On 11/01/2018 04:50 PM, Erik Skultety wrote: > > There's a lot of stuff going on in src/conf/nodedev_conf which not > > always has to do anything with config, so even though we're trying, > > we're not really consistent in putting only parser/formatter related > > stuff here like we do for domains. So, start the cleanup simply by adding > > a new module to the nodedev driver and put a few helper APIs which want > > to open a secondary driver connection in there (similar to storage_util > > module). > > > > Signed-off-by: Erik Skultety > > --- > > > > I verified the build with debian 9, centos 7, fedora 28, rawhide, and > > freebsd > > 11 > > > > src/conf/Makefile.inc.am | 1 + > > src/conf/node_device_conf.c | 199 --- > > src/conf/node_device_conf.h | 11 -- > > src/conf/virstorageobj.c | 1 + > > src/libvirt_private.syms | 8 +- > > src/node_device/Makefile.inc.am | 17 +- > > src/node_device/node_device_driver.c | 1 + > > src/node_device/node_device_util.c | 229 +++ > > src/node_device/node_device_util.h | 35 > > src/storage/Makefile.inc.am | 1 + > > src/storage/storage_backend_scsi.c | 1 + > > 11 files changed, 290 insertions(+), 214 deletions(-) > > create mode 100644 src/node_device/node_device_util.c > > create mode 100644 src/node_device/node_device_util.h > > > > diff --git a/src/conf/Makefile.inc.am b/src/conf/Makefile.inc.am > > index af23810640..7cb6c29d70 100644 > > --- a/src/conf/Makefile.inc.am > > +++ b/src/conf/Makefile.inc.am > > @@ -163,6 +163,7 @@ libvirt_la_BUILT_LIBADD += libvirt_conf.la > > libvirt_conf_la_SOURCES = $(CONF_SOURCES) > > libvirt_conf_la_CFLAGS = \ > > -I$(srcdir)/conf \ > > + -I$(srcdir)/node_device \ > > This doesn't feel right. The conf parser/formatter should be driver > agnostic. > > I see two options. If you want to clean up src/conf/node_device_conf.c > either you'll put node_device_util.c into src/conf/ right next to Yep, I pulled it completely apart again only to confirm that we have to go with ^this suggestion, I'll send a v2 in a minute. Erik > node_device_conf.c or you move it into src/util/ because conf module can > use util. > > The rest looks good. > > Michal > > -- > libvir-list mailing list > libvir-list@redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2] conf: Add new module node_device_util
There's a lot of stuff going on in src/conf/nodedev_conf which is
sometimes not directly related to config and we're not really consistent
with putting only parser/formatter related stuff here, e.g. like we do
for domains. So, let's start simply by adding a new module
node_device_util containing some of the helpers. Unfortunately, even
though these helpers tend to open a secondary driver connection and would
be much therefore better suited as a nodedev driver module, we can't do
that without pulling headers from the driver into conf/ and that's wrong
because we want conf/ to stay driver-agnostic.
Signed-off-by: Erik Skultety
---
src/conf/Makefile.inc.am | 2 +
src/conf/node_device_conf.c | 199 ---
src/conf/node_device_conf.h | 11 --
src/conf/node_device_util.c | 229 +++
src/conf/node_device_util.h | 35
src/conf/virstorageobj.c | 1 +
src/libvirt_private.syms | 7 +-
src/node_device/node_device_driver.c | 1 +
src/storage/storage_backend_scsi.c | 1 +
9 files changed, 273 insertions(+), 213 deletions(-)
create mode 100644 src/conf/node_device_util.c
create mode 100644 src/conf/node_device_util.h
diff --git a/src/conf/Makefile.inc.am b/src/conf/Makefile.inc.am
index af23810640..219ff350d7 100644
--- a/src/conf/Makefile.inc.am
+++ b/src/conf/Makefile.inc.am
@@ -119,6 +119,8 @@ SECRET_CONF_SOURCES = \
NODE_DEVICE_CONF_SOURCES = \
conf/node_device_conf.c \
conf/node_device_conf.h \
+ conf/node_device_util.c \
+ conf/node_device_util.h \
conf/virnodedeviceobj.c \
conf/virnodedeviceobj.h \
$(NULL)
diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c
index 03bd794dc0..74a7bc3933 100644
--- a/src/conf/node_device_conf.c
+++ b/src/conf/node_device_conf.c
@@ -2220,205 +2220,6 @@ virNodeDevCapsDefFree(virNodeDevCapsDefPtr caps)
}
-/* virNodeDeviceGetParentName
- * @conn: Connection pointer
- * @nodedev_name: Node device to lookup
- *
- * Lookup the node device by name and return the parent name
- *
- * Returns parent name on success, caller is responsible for freeing;
- * otherwise, returns NULL on failure
- */
-char *
-virNodeDeviceGetParentName(virConnectPtr conn,
- const char *nodedev_name)
-{
-virNodeDevicePtr device = NULL;
-char *parent;
-
-if (!(device = virNodeDeviceLookupByName(conn, nodedev_name))) {
-virReportError(VIR_ERR_XML_ERROR,
- _("Cannot find '%s' in node device database"),
- nodedev_name);
-return NULL;
-}
-
-ignore_value(VIR_STRDUP(parent, virNodeDeviceGetParent(device)));
-virObjectUnref(device);
-
-return parent;
-}
-
-
-/**
- * @fchost: Pointer to vHBA adapter
- *
- * Create a vHBA for Storage. This code accomplishes this via searching
- * through the sysfs for scsi_host/fc_host in order to first ensure some
- * vHBA doesn't already exist for the requested wwnn/wwpn (e.g. an unmanaged
- * vHBA) and to search for the parent vport capable scsi_host by name,
- * wwnn/wwpn, or fabric_wwn (if provided). If no parent is provided, then
- * a vport capable scsi_host will be selected.
- *
- * Returns vHBA name on success, NULL on failure with an error message set
- */
-char *
-virNodeDeviceCreateVport(virStorageAdapterFCHostPtr fchost)
-{
-unsigned int parent_host;
-char *name = NULL;
-char *parent_hoststr = NULL;
-bool skip_capable_check = false;
-
-VIR_DEBUG("parent='%s', wwnn='%s' wwpn='%s'",
- NULLSTR(fchost->parent), fchost->wwnn, fchost->wwpn);
-
-if (fchost->parent) {
-if (VIR_STRDUP(parent_hoststr, fchost->parent) < 0)
-goto cleanup;
-} else if (fchost->parent_wwnn && fchost->parent_wwpn) {
-if (!(parent_hoststr = virVHBAGetHostByWWN(NULL, fchost->parent_wwnn,
- fchost->parent_wwpn))) {
-virReportError(VIR_ERR_XML_ERROR, "%s",
- _("cannot find parent using provided wwnn/wwpn"));
-goto cleanup;
-}
-} else if (fchost->parent_fabric_wwn) {
-if (!(parent_hoststr =
- virVHBAGetHostByFabricWWN(NULL, fchost->parent_fabric_wwn))) {
-virReportError(VIR_ERR_XML_ERROR, "%s",
- _("cannot find parent using provided fabric_wwn"));
-goto cleanup;
-}
-} else {
-if (!(parent_hoststr = virVHBAFindVportHost(NULL))) {
-virReportError(VIR_ERR_XML_ERROR, "%s",
- _("'parent' for vHBA not specified, and "
- "cannot find one on this host"));
-goto cleanup;
-}
-skip_capable_check = true;
-}
-
-if (virSCSIHostGetNumber(parent_hoststr, _host) < 0)
-goto cleanup;
-
-/* NOTE:
- * We do not save the parent_hoststr
Re: [libvirt] [PATCH v2] qemu: Introduce caching whether /dev/kvm is accessible
On Thu, Nov 01, 2018 at 09:31 AM +0100, Martin Kletzander
wrote:
> On Tue, Oct 30, 2018 at 03:07:59PM +, Daniel P. Berrangé wrote:
>>On Tue, Oct 30, 2018 at 03:45:36PM +0100, Michal Privoznik wrote:
>>> On 10/30/2018 02:46 PM, Michal Privoznik wrote:
>>> > On 10/30/2018 01:55 PM, Daniel P. Berrangé wrote:
>>> >> On Tue, Oct 30, 2018 at 10:32:08AM +, Daniel P. Berrangé wrote:
>>> >>> On Tue, Oct 30, 2018 at 11:08:45AM +0100, Michal Privoznik wrote:
>>> On 10/30/2018 10:35 AM, Daniel P. Berrangé wrote:
>>> > On Tue, Oct 30, 2018 at 09:13:50AM +0100, Michal Privoznik wrote:
>>> >> On 10/29/2018 06:34 PM, Marc Hartmayer wrote:
>>> >>> Introduce caching whether /dev/kvm is usable as the QEMU user:QEMU
>>> >>> group. This reduces the overhead of the QEMU capabilities cache
>>> >>> lookup. Before this patch there were many fork() calls used for
>>> >>> checking whether /dev/kvm is accessible. Now we store the result
>>> >>> whether /dev/kvm is accessible or not and we only need to re-run the
>>> >>> virFileAccessibleAs check if the ctime of /dev/kvm has changed.
>>> >>>
>>> >>> Suggested-by: Daniel P. Berrangé
>>> >>> Signed-off-by: Marc Hartmayer
>>> >>> ---
>>> >>> src/qemu/qemu_capabilities.c | 54
>>> >>> ++--
>>> >>> 1 file changed, 52 insertions(+), 2 deletions(-)
>>> >>>
>>> >>> diff --git a/src/qemu/qemu_capabilities.c
>>> >>> b/src/qemu/qemu_capabilities.c
>>> >>> index e228f52ec0bb..85516954149b 100644
>>> >>> --- a/src/qemu/qemu_capabilities.c
>>> >>> +++ b/src/qemu/qemu_capabilities.c
>>> >>> @@ -3238,6 +3238,10 @@ struct _virQEMUCapsCachePriv {
>>> >>> virArch hostArch;
>>> >>> unsigned int microcodeVersion;
>>> >>> char *kernelVersion;
>>> >>> +
>>> >>> +/* cache whether /dev/kvm is usable as runUid:runGuid */
>>> >>> +virTristateBool kvmUsable;
>>> >>> +time_t kvmCtime;
>>> >>> };
>>> >>> typedef struct _virQEMUCapsCachePriv virQEMUCapsCachePriv;
>>> >>> typedef virQEMUCapsCachePriv *virQEMUCapsCachePrivPtr;
>>> >>> @@ -3824,6 +3828,52 @@ virQEMUCapsSaveFile(void *data,
>>> >>> }
>>> >>>
>>> >>>
>>> >>> +/* Determine whether '/dev/kvm' is usable as QEMU user:QEMU group.
>>> >>> */
>>> >>> +static bool
>>> >>> +virQEMUCapsKVMUsable(virQEMUCapsCachePrivPtr priv)
>>> >>> +{
>>> >>> +struct stat sb;
>>> >>> +static const char *kvm_device = "/dev/kvm";
>>> >>> +virTristateBool value;
>>> >>> +virTristateBool cached_value = priv->kvmUsable;
>>> >>> +time_t kvm_ctime;
>>> >>> +time_t cached_kvm_ctime = priv->kvmCtime;
>>> >>> +
>>> >>> +if (stat(kvm_device, ) < 0) {
>>> >>> +virReportSystemError(errno,
>>> >>> + _("Failed to stat %s"), kvm_device);
>>> >>> +return false;
>>> >>> +}
>>> >>> +kvm_ctime = sb.st_ctime;
>>> >>
>>> >> This doesn't feel right. /dev/kvm ctime is changed every time qemu is
>>> >> started or powered off (try running stat over it before and after a
>>> >> domain is started/shut off). So effectively we will fork more often
>>> >> than
>>> >> we would think. Should we cache inode number instead? Because for all
>>> >> that we care is simply if the file is there.
>>> >
>>> > Urgh, that is a bit strange and not the usual semantics for
>>> > timestamps :-(
>>>
>>> Indeed.
>>>
>>> >
>>> > We can't stat the inode - the code was explicitly trying to cope with
>>> > the
>>> > way /dev/kvm can change permissions when udev rules get applied. We
>>> > would
>>> > have to compare the user, group, permissions mask and even ACL, or a
>>> > hash
>>> > of those.
>>>
>>> Well, we can use ctime as suggested and post a patch for kernel to fix
>>> ctime behaviour. Until the patch is merged our behaviour would be
>>> suboptimal, but still better than it is now.
>>> >>>
>>> >>> I guess lets talk to KVM team for their input on this and then decide
>>> >>> what todo.
>>> >>
>>> >> Hmm, I wonder if it is not actually a kernel problem, but rather
>>> >> something
>>> >> in userspace genuinely touching the device in a way that caues these
>>> >> timestamps to be updated.
>>> >>
>>> >
>>> > It is kernel problem. In my testing, the moment I call:
>>> >
>>> > ioctl(kvm, KVM_CREATE_VM, 0);
>>>
>>> Okay, I have to retract this claim. 'udevadm monitor' shows some events:
>>>
>>> KERNEL[3631.129645] change /devices/virtual/misc/kvm (misc)
>>> UDEV [3631.130816] change /devices/virtual/misc/kvm (misc)
>>>
>>> and stopping udevd leaves all three times untouched. So it is udev after
>>> all. I just don't know how to find the rule that is causing the issue.
>>> Anyway, as for this patch, I think we can merge it in the end, can't
Re: [libvirt] [PATCHv7 10/18] conf: Remove virDomainResctrlAppend and introduce virDomainResctrlNew
> -Original Message-
> From: John Ferlan [mailto:jfer...@redhat.com]
> Sent: Wednesday, November 7, 2018 12:15 AM
> To: Wang, Huaqiang ; libvir-list@redhat.com
> Cc: Feng, Shaohe ; bing@intel.com; Ding, Jian-
> feng ; Zang, Rui
> Subject: Re: [PATCHv7 10/18] conf: Remove virDomainResctrlAppend and
> introduce virDomainResctrlNew
>
>
>
> On 11/6/18 4:51 AM, Huaqiang,Wang wrote:
> >
> >
> > On 2018年11月06日 01:26, John Ferlan wrote:
> >>
> >> On 10/22/18 4:01 AM, Wang Huaqiang wrote:
> >>> Introduced virDomainResctrlNew to do the most part of
> >>> virDomainResctrlAppend and move the operation of appending resctrl
> >>> to @def->resctrls out of function.
> >>>
> >>> Rather than rely on virDomainResctrlAppend to perform the
> >>> allocation, move the onus to the caller and make use of
> >>> virBitmapNewCopy for @vcpus and virObjectRef for @alloc, thus
> >>> removing the need to set each to NULL after the call.
> >>>
> >>> Signed-off-by: Wang Huaqiang
> >>> ---
> >>> src/conf/domain_conf.c | 60
> >>> +-
> >>> 1 file changed, 35 insertions(+), 25 deletions(-)
> >>>
> >>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index
> >>> e8e0adc..39bd396 100644
> >>> --- a/src/conf/domain_conf.c
> >>> +++ b/src/conf/domain_conf.c
> >>> @@ -18920,26 +18920,22 @@
> >>> virDomainCachetuneDefParseCache(xmlXPathContextPtr ctxt,
> >>> }
> >>> -static int
> >>> -virDomainResctrlAppend(virDomainDefPtr def,
> >>> - xmlNodePtr node,
> >>> - virResctrlAllocPtr alloc,
> >>> - virBitmapPtr vcpus,
> >>> - unsigned int flags)
> >>> +static virDomainResctrlDefPtr
> >>> +virDomainResctrlNew(xmlNodePtr node,
> >>> + virResctrlAllocPtr *alloc,
> >>> + virBitmapPtr *vcpus,
> >> Because we're not "stealing" @*alloc and/or @*vcpus, they do not need
> >> to be passed by reference. I can change these. There's some minor
> >> merge impact in later patches too, but no big deal.
> >
> > Agree. Please help make change.
> >
> >
> >>
> >>> + unsigned int flags)
> >>> {
> >>> char *vcpus_str = NULL;
> >>> char *alloc_id = NULL;
> >>> - virDomainResctrlDefPtr tmp_resctrl = NULL;
> >>> - int ret = -1;
> >>> -
> >>> - if (VIR_ALLOC(tmp_resctrl) < 0)
> >>> - goto cleanup;
> >>> + virDomainResctrlDefPtr resctrl = NULL;
> >>> + virDomainResctrlDefPtr ret = NULL;
> >>> /* We need to format it back because we need to be
> >>> consistent in the naming
> >>> * even when users specify some "sub-optimal" string there. */
> >>> - vcpus_str = virBitmapFormat(vcpus);
> >>> + vcpus_str = virBitmapFormat(*vcpus);
> >>> if (!vcpus_str)
> >>> - goto cleanup;
> >>> + return NULL;
> >>> if (!(flags & VIR_DOMAIN_DEF_PARSE_INACTIVE))
> >>> alloc_id = virXMLPropString(node, "id"); @@ -18954,18
> >>> +18950,23 @@ virDomainResctrlAppend(virDomainDefPtr def,
> >>> goto cleanup;
> >>> }
> >>>
> >> /* NB: Callers assume new @alloc, need to fill in ID now */
> >>
> >> Not that it would prevent someone in the future from passing an
> >> @alloc w/ ->id already filled in and overwriting, but at least for
> >> now it's not the case.
> >
> > Yes, it might happen.
> > If @alloc->id is specified through XML and is not following the naming
> > convention
> > virAsprintf(_id, "vcpus_%s", vcpus_str)
> >
> > If you think it is necessary we might need to through a warning for
> > this case.
> >
>
> Let's see - virDomainResctrlNew has two callers:
>
> 1. virDomainCachetuneDefParse
>
>In this case, we "know" we have a new/empty @alloc because if
> virDomainResctrlVcpuMatch found one, then there'd be a failure.
>
>The virDomainCachetuneDefParseCache calls don't seem to fill in
> alloc->id, but virDomainResctrlNew will for the first time.
>
> 2. virDomainMemorytuneDefParse
>
>The virDomainResctrlVcpuMatch may find a preexisting @alloc, but
> @new_alloc is set to true. The virDomainMemorytuneDefParseMemory won't
> fill alloc->id. Then only if @new_alloc do we call virDomainResctrlNew
>
> So I think both are safe "for now".
Yes. Agree. Thanks for the analysis.
> If you want I could modify the
> virResctrlAllocSetID code to :
>
> if (alloc->id) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
>_("Attempt to overwrite alloc->id='%s' with id='%s'"),
>alloc->id, id);
> return -1;
> }
>
> Let me know.
>
virResctrlMonitorSetID should also do similar patch, right?
Then the body of two functions, virRresctrlAllocSetID and
virResctrlMonitorSetID,
are very similar.
I will introduce two patches, the first patch will refactor
virRresctrlAllocSetID
and the second patch will reuse the refactor for virResctrlMonitorSetID.
I know you have a solution solving
Re: [libvirt] [PATCH v2] qemu: Introduce caching whether /dev/kvm is accessible
On Thu, Nov 01, 2018 at 10:18 AM +0100, "Daniel P. Berrangé" wrote: > On Thu, Nov 01, 2018 at 09:31:50AM +0100, Martin Kletzander wrote: >> On Tue, Oct 30, 2018 at 03:07:59PM +, Daniel P. Berrangé wrote: >> > On Tue, Oct 30, 2018 at 03:45:36PM +0100, Michal Privoznik wrote: >> > > On 10/30/2018 02:46 PM, Michal Privoznik wrote: >> > > > >> > > > It is kernel problem. In my testing, the moment I call: >> > > > >> > > > ioctl(kvm, KVM_CREATE_VM, 0); >> > > >> > > Okay, I have to retract this claim. 'udevadm monitor' shows some events: >> > > >> > > KERNEL[3631.129645] change /devices/virtual/misc/kvm (misc) >> > > UDEV [3631.130816] change /devices/virtual/misc/kvm (misc) >> > > >> > > and stopping udevd leaves all three times untouched. So it is udev after >> > > all. I just don't know how to find the rule that is causing the issue. >> > > Anyway, as for this patch, I think we can merge it in the end, can't we? >> > >> > Not really. Udev is in use everywhere, so this behaviour makes the >> > patch useless in practice, even though it is technically right in >> > theory :-( >> > >> >> Does it? With this behaviour we still do the "expensive" work after any >> machine >> has started. But for one machine starting it still has the effect of >> running it >> only once. And we *need* to run it for each machine unless we also cache the >> result per (at least) user:group of that machine as every machine can run >> under >> different user:group. > > Yes, with the current patch it still rechecks it for each VM startup But that happens only because of udev (see Michal’s answer). > , but > I was going to suggest the caching of this is simply put in a global static > variable as there's no reason to record this device permissions state in > the per VM caps cache. I’m not sure I understand you correctly, but this patch doesn’t use the VM/QEMU capabilities cache (that would be 'struct _virQEMUCaps') for the caching. Instead it uses 'struct _virQEMUCapsCachePriv' and this struct is initialized only once when initializing the QEMU driver. This should be pretty similar to your proposal with the global static variable. > > > Regards, > Daniel > -- > |: 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 :| > -- Kind regards / Beste Grüße Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzende des Aufsichtsrats: Martina Koederitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [RFC PATCH] Add new migration flag VIR_MIGRATE_DRY_RUN
On Fri, Nov 02, 2018 at 04:34:02PM -0600, Jim Fehlig wrote: > A dry run can be used as a best-effort check that a migration command > will succeed. The destination host will be checked to see if it can > accommodate the resources required by the domain. DRY_RUN will fail if > the destination host is not capable of running the domain. Although a > subsequent migration will likely succeed, the success of DRY_RUN does not > ensure a future migration will succeed. Resources on the destination host > could become unavailable between a DRY_RUN and actual migration. I'm not really convinced this is a particularly useful concept, as it is only going to catch a very small number of the reasons why migration can fail. So you still have to expect the real migration invokation to have a strong chance of failing. > > Signed-off-by: Jim Fehlig > --- > > If it is agreed this is useful, my thought was to use the begin and > prepare phases of migration to implement it. qemuMigrationDstPrepareAny() > already does a lot of the heavy lifting wrt checking the host can > accommodate the domain. Some of it, and the remaining migration phases, > can be short-circuited in the case of dry run. > > One interesting wrinkle I've observed is the check for cpu compatibility. > AFAICT qemu is actually invoked on the dst, "filtered-features" of the cpu > are requested via qmp, and results are checked against cpu in domain config. > If cpu on dst is insufficient, migration fails in the prepare phase with > something like "guest CPU doesn't match specification: missing features: z y > z". > I was hoping to avoid launching qemu in the case of dry run, but that may > be unavoidable if we'd like a dependable dry run result. Even launching QEMU isn't good enough - it has to actually process the migration data stream for devices to get a good indication of success, at which point you're basically doing a real migration. Regards, Daniel -- |: 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 :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [tck PATCH 1/3] Add tests for virtual network <-> guest connections
On Fri, Nov 02, 2018 at 04:23:02PM -0400, Laine Stump wrote:
> On 11/2/18 11:52 AM, Daniel P. Berrangé wrote:
> > Signed-off-by: Daniel P. Berrangé
> > ---
> > lib/Sys/Virt/TCK.pm | 33
> > lib/Sys/Virt/TCK/NetworkBuilder.pm| 33 +++-
> > scripts/networks/300-guest-network-isolated.t | 82 ++
> > scripts/networks/310-guest-network-nat.t | 83 +++
> > scripts/networks/320-guest-network-route.t| 83 +++
> > scripts/networks/330-guest-network-open.t | 83 +++
> > scripts/networks/340-guest-network-bridge.t | 79 ++
> > scripts/networks/350-guest-network-private.t | 81 ++
> > scripts/networks/360-guest-network-vepa.t | 81 ++
> > .../networks/370-guest-network-passthrough.t | 81 ++
> > scripts/networks/380-guest-network-hostdev.t | 82 ++
> > t/080-network-builder.t | 2 +-
> > 12 files changed, 800 insertions(+), 3 deletions(-)
> > create mode 100644 scripts/networks/300-guest-network-isolated.t
> > create mode 100644 scripts/networks/310-guest-network-nat.t
> > create mode 100644 scripts/networks/320-guest-network-route.t
> > create mode 100644 scripts/networks/330-guest-network-open.t
> > create mode 100644 scripts/networks/340-guest-network-bridge.t
> > create mode 100644 scripts/networks/350-guest-network-private.t
> > create mode 100644 scripts/networks/360-guest-network-vepa.t
> > create mode 100644 scripts/networks/370-guest-network-passthrough.t
> > create mode 100644 scripts/networks/380-guest-network-hostdev.t
>
>
> You added all these new tests, but forgot that you made the MANIFEST
> file static/manually edited in commit a140e4f61 back in May, so the new
> tests don't get installed.
Hah, forgot that :-)
> Also, the tests don't actually boot up an OS and try to pass traffic; I
> guess that's something you're counting on someone adding later? :-)
I was really only interested in testing the code in libvirtd that
connects guests to virtual networks, to exercise code paths i'm
refactoring right now.
I'll leave testing of guest traffic as an exercise for future contribs.
> > +sub find_free_ipv4_subnet {
> > +my $index;
> > +
> > +my %used;
> > +
> > +foreach my $iface (IO::Interface::Simple->interfaces()) {
>
>
> This works to eliminate conflicts with active interfaces, but doesn't
> take into account any routes that have the same destination
> address/prefix as the desired network. For example, you may have a
> static route for 192.168.125.0/25 pointing to another host that has a
> routed virtual network with that address. If that's the case, the test
> will fail.
>
>
> I don't have a quick alternative at hand to suggest though, and the
> likelyhood of a host having a static route that conflicts with the
> lowest numbered available 192.168.blah.0/24 network is probably pretty
> low, so I'm going to overlook this :-)
We could allow for a config file parameter to override this for
people how have that scenario.
> > + if ($iface->netmask eq "255.255.255.0" &&
> > + $iface->address =~ /^192.168.(\d+).\d+/) {
> > + $used{"$1"} = 1;
> > + print "Used $1\n";
> > + } else {
> > + print "Not used ", $iface->address, "\n";
> > + }
> > +}
> > +
> > +for (my $i = 1; $i < 255; $i++) {
> > + if (!exists $used{"$i"}) {
> > + $index = $i;
> > + last;
> > + }
> > +}
> > +
> > +return () unless defined $index;
> > +
> > +return (
> > + address => "192.168.$index.1",
> > + netmask => "255.255.255.0",
> > + dhcpstart => "192.168.$index.100",
> > + dhcpend => "192.168.$index.200"
> > + );
> > +}
> > +
> > diff --git a/scripts/networks/300-guest-network-isolated.t
> > b/scripts/networks/300-guest-network-isolated.t
> > new file mode 100644
> > index 000..487e864
> > --- /dev/null
> > +++ b/scripts/networks/300-guest-network-isolated.t
> > @@ -0,0 +1,82 @@
> > +# -*- perl -*-
> > +#
> > +# Copyright (C) 2018 Red Hat, Inc.
> > +#
> > +# This program is free software; You can redistribute it and/or modify
> > +# it under the GNU General Public License as published by the Free
> > +# Software Foundation; either version 2, or (at your option) any
> > +# later version
> > +#
> > +# The file "LICENSE" distributed along with this file provides full
> > +# details of the terms and conditions
> > +#
> > +
> > +=pod
> > +
> > +=head1 NAME
> > +
> > +network/300-guest-network-isolated.t - guest connect to isolated network
> > +
> > +=head1 DESCRIPTION
> > +
> > +This test case validates that a guest is connected to an isolated
> > +virtual network
> > +
> > +=cut
> > +
> > +use strict;
> > +use warnings;
> > +
> > +use Test::More tests => 4;
> > +
> > +use Sys::Virt::TCK;
> > +
> > +my $tck = Sys::Virt::TCK->new();
> > +my $conn = eval { $tck->setup(); };
> > +BAIL_OUT "failed to
Re: [libvirt] [PATCH v3] lxc: Include support to lxc version 3.0 or higher.
On Fri, Nov 09, 2018 at 03:30:59PM -0200, Julio Faracco wrote: > This patch introduce the new settings for LXC 3.0 or higher. The older > versions keep the compatibility to deprecated settings for LXC, but > after release 3.0, the compatibility was removed. This commit adds the > support to the refactored settings. > > v1-v2: Michal's suggestions to handle differences between versions. > v2-v3: Adding suggestions from Pino and John too. > > Signed-off-by: Julio Faracco > --- > src/lxc/lxc_native.c | 45 +++- > 1 file changed, 32 insertions(+), 13 deletions(-) I'd expect to additions to the test suite to cover these changes eg lxcconf2xmltest data files Regards, Daniel -- |: 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 :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] bt: Mark the bluetooth subsystem as deprecated
On Mon, Nov 12, 2018 at 11:00:30 +0100, Thomas Huth wrote: > It has been unmaintained since years, and there were only trivial or > tree-wide changes to the related files since many years, so the > code is likely very bitrotten and broken. For example the following > segfaults as soon as as you press a key: > > qemu-system-x86_64 -usb -device usb-bt-dongle -bt hci -bt device:keyboard > > Since we are not aware of anybody using bluetooth with the current > version of QEMU, let's mark the subsystem as deprecated, with a special > request for the users to write to the qemu-devel mailing list in case > they still use it (so we could revert the deprecation status in that > case). > > Signed-off-by: Thomas Huth > --- > qemu-deprecated.texi | 7 +++ > qemu-options.hx | 4 > vl.c | 4 > 3 files changed, 15 insertions(+) libvirt never implemented any configuration option for bluetooth signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] bt: Mark the bluetooth subsystem as deprecated
It has been unmaintained since years, and there were only trivial or
tree-wide changes to the related files since many years, so the
code is likely very bitrotten and broken. For example the following
segfaults as soon as as you press a key:
qemu-system-x86_64 -usb -device usb-bt-dongle -bt hci -bt device:keyboard
Since we are not aware of anybody using bluetooth with the current
version of QEMU, let's mark the subsystem as deprecated, with a special
request for the users to write to the qemu-devel mailing list in case
they still use it (so we could revert the deprecation status in that
case).
Signed-off-by: Thomas Huth
---
qemu-deprecated.texi | 7 +++
qemu-options.hx | 4
vl.c | 4
3 files changed, 15 insertions(+)
diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
index 5d2d7a3..cb4291f 100644
--- a/qemu-deprecated.texi
+++ b/qemu-deprecated.texi
@@ -128,6 +128,13 @@ The @option{[hub_id name]} parameter tuple of the
'hostfwd_add' and
The ``ivshmem'' device type is replaced by either the ``ivshmem-plain''
or ``ivshmem-doorbell`` device types.
+@subsection bluetooth (since 3.1)
+
+The bluetooth subsystem is unmaintained since many years and likely bitrotten
+quite a bit. It will be removed without replacement unless some users speaks
+up at the @email{qemu-devel@@nongnu.org} mailing list with information about
+their usecases.
+
@section System emulator machines
@subsection pc-0.10 and pc-0.11 (since 3.0)
diff --git a/qemu-options.hx b/qemu-options.hx
index 38c7a97..ee379b3 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -2772,6 +2772,10 @@ logic. The Transport Layer is decided by the machine
type. Currently
the machines @code{n800} and @code{n810} have one HCI and all other
machines have none.
+Note: This option and the whole bluetooth subsystem is considered as
deprecated.
+If you still use it, please send a mail to @email{qemu-devel@@nongnu.org} where
+you describe your usecase.
+
@anchor{bt-hcis}
The following three types are recognized:
diff --git a/vl.c b/vl.c
index 55bab00..fa25d1a 100644
--- a/vl.c
+++ b/vl.c
@@ -3269,6 +3269,10 @@ int main(int argc, char **argv, char **envp)
break;
#endif
case QEMU_OPTION_bt:
+warn_report("The bluetooth subsystem is deprecated and will "
+"be removed soon. If the bluetooth subsystem is "
+"still useful for you, please send a mail to "
+"qemu-de...@nongnu.org with your usecase.");
add_device_config(DEV_BT, optarg);
break;
case QEMU_OPTION_audio_help:
--
1.8.3.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv7 05/18] util: Refactor code for adding PID to the resource group
> -Original Message- > From: John Ferlan [mailto:jfer...@redhat.com] > Sent: Tuesday, November 6, 2018 10:41 PM > To: Wang, Huaqiang ; libvir-list@redhat.com > Cc: Feng, Shaohe ; bing@intel.com; Ding, Jian- > feng ; Zang, Rui > Subject: Re: [PATCHv7 05/18] util: Refactor code for adding PID to the > resource > group > > > > On 11/6/18 3:41 AM, Huaqiang,Wang wrote: > > > > > > On 2018年11月05日 23:03, John Ferlan wrote: > >> > >> On 10/22/18 4:01 AM, Wang Huaqiang wrote: > >>> The code of adding PID to the allocation could be reused, refactor > >>> it for later reuse. > >>> > >>> Signed-off-by: Wang Huaqiang > >>> --- > >>> src/util/virresctrl.c | 30 +++--- > >>> 1 file changed, 19 insertions(+), 11 deletions(-) > >>> > >> Reviewed-by: John Ferlan > >> > >> John > > > > Thanks for the review. > > Huaqiang > > > > While working through patch1 adjustments, I noted an extra space in a > comment, so I fixed it: > > /* If the allocation is empty, then it is impossible to add a PID to > * allocation due to lacking of its 'tasks' file. Just return */ > ^^ > Of course that resulted in a merge conflict in this patch where I (now) note > you > changed the comment to: > > /* If allocation is empty, then no resctrl directory and the 'tasks' > file > * exists, just return */ > > I'm going to restore the original comment; however, it made me stop and think > about future patch14 which used the *tasks (and a new local *pid > list) - perhaps you need to rethink the changes... Even patch1... > > What's the use of altering the *tasks file at all then? Fortunately it seems > it's not > really used for much more than logging the tids that are running the vcpu. > > I'll hold off on pushing anything... So far there's no real impact, but you > may > decide that you need to 'design in' a way to handle this default/system > path/issue. > I'll send out my v8 patches today, and will be covered in my new series, please make a review. Thanks for review. > John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
