Re: [libvirt] [PATCH] lxc: Clang is complaining about possible NULL pointer.
On Mon, Nov 05, 2018 at 04:58:14PM -0200, Julio Faracco wrote:
> The array "mount" inside lxc_container is not being checked before for
> loop. Clang syntax scan is complaining about this segmentation fault.
Which in this specific case is a false positive since nmounts pretty much
protects us from that happening. Usually I'm not very keen on making changes to
silence false positives from analyzers like coverity, however, in this case we
can actually make things better I believe.
>
> Signed-off-by: Julio Faracco
> ---
> src/lxc/lxc_container.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c
> index 918194dacd..6b7bcd8eb6 100644
> --- a/src/lxc/lxc_container.c
> +++ b/src/lxc/lxc_container.c
> @@ -871,7 +871,7 @@ static int lxcContainerSetReadOnly(void)
How about we revert the logic HERE (in the missing context) and instead do:
if (!mounts) {
ret = 0;
goto cleanup;
}
> qsort(mounts, nmounts, sizeof(mounts[0]),
>virStringSortRevCompare);
>
> -for (i = 0; i < nmounts; i++) {
> +for (i = 0; i < nmounts && mounts; i++) {
> VIR_DEBUG("Bind readonly %s", mounts[i]);
> if (mount(mounts[i], mounts[i], "none",
> MS_BIND|MS_REC|MS_RDONLY|MS_REMOUNT, NULL) < 0) {
> virReportSystemError(errno,
> @@ -883,7 +883,7 @@ static int lxcContainerSetReadOnly(void)
>
> ret = 0;
> cleanup:
> -for (i = 0; i < nmounts; i++)
> +for (i = 0; i < nmounts && mounts; i++)
and then ^here replace this hunk with virStringListFreeCount which already
checks @src for NULL, so that should suffice from Clang's perspective, right?
Erik
> VIR_FREE(mounts[i]);
> VIR_FREE(mounts);
> endmntent(procmnt);
> --
> 2.17.1
>
> --
> 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
Re: [libvirt] [PATCH] lxc: Clang is complaining about possible NULL pointer.
On Monday, 5 November 2018 19:58:14 CET Julio Faracco wrote:
> The array "mount" inside lxc_container is not being checked before for
> loop. Clang syntax scan is complaining about this segmentation fault.
>
> Signed-off-by: Julio Faracco
> ---
> src/lxc/lxc_container.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c
> index 918194dacd..6b7bcd8eb6 100644
> --- a/src/lxc/lxc_container.c
> +++ b/src/lxc/lxc_container.c
> @@ -871,7 +871,7 @@ static int lxcContainerSetReadOnly(void)
> qsort(mounts, nmounts, sizeof(mounts[0]),
>virStringSortRevCompare);
The code here is:
if (mounts)
qsort(mounts, nmounts, sizeof(mounts[0]),
virStringSortRevCompare);
Hence ...
> -for (i = 0; i < nmounts; i++) {
> +for (i = 0; i < nmounts && mounts; i++) {
> VIR_DEBUG("Bind readonly %s", mounts[i]);
> if (mount(mounts[i], mounts[i], "none",
> MS_BIND|MS_REC|MS_RDONLY|MS_REMOUNT, NULL) < 0) {
> virReportSystemError(errno,
... IMHO it is a better idea to move the whole for loop inside the
'if (mounts)' above.
> @@ -883,7 +883,7 @@ static int lxcContainerSetReadOnly(void)
>
> ret = 0;
> cleanup:
> -for (i = 0; i < nmounts; i++)
> +for (i = 0; i < nmounts && mounts; i++)
> VIR_FREE(mounts[i]);
Same idea here: just use a simple if to detect when 'mounts' is non-null.
--
Pino Toscano
signature.asc
Description: This is a digitally signed message part.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] lxc: Include support to lxc version 3.0 or higher.
On Monday, 5 November 2018 19:57:24 CET 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. Shouldn't it be better to check first for the new names of the configuration strings, using the pre-3.0 ones as fallback option? -- Pino Toscano signature.asc Description: This is a digitally signed message part. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2] qemu: Process RDMA GID state change event
This event is emitted on the monitor when a GID table in pvrdma device
is modified and the change needs to be propagate to the backend RDMA
device's GID table.
The control over the RDMA device's GID table is done by updating the
device's Ethernet function addresses.
Usually the first GID entry is determine by the MAC address, the second
by the first IPv6 address and the third by the IPv4 address. Other
entries can be added by adding more IP addresses. The opposite is the
same, i.e. whenever an address is removed, the corresponding GID entry
is removed.
The process is done by the network and RDMA stacks. Whenever an address
is added the ib_core driver is notified and calls the device driver's
add_gid function which in turn update the device.
To support this in pvrdma device we need to hook into the create_bind
and destroy_bind HW commands triggered by pvrdma driver in guest.
Whenever a changed is made to the pvrdma device's GID table a special
QMP messages is sent to be processed by libvirt to update the address of
the backend Ethernet device.
Signed-off-by: Yuval Shaia
---
v1 -> v2:
* Address all comments from Michal Privoznik
---
src/qemu/qemu_domain.c | 3 +++
src/qemu/qemu_domain.h | 1 +
src/qemu/qemu_driver.c | 44 ++
src/qemu/qemu_monitor.c | 27 +++
src/qemu/qemu_monitor.h | 27 +++
src/qemu/qemu_monitor_json.c | 36 +
src/qemu/qemu_process.c | 52
7 files changed, 190 insertions(+)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index ba3fff607a..8da54c7ee9 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -13479,6 +13479,9 @@ qemuProcessEventFree(struct qemuProcessEvent *event)
case QEMU_PROCESS_EVENT_GUESTPANIC:
qemuMonitorEventPanicInfoFree(event->data);
break;
+case QEMU_PROCESS_EVENT_RDMA_GID_STATUS_CHANGED:
+qemuMonitorEventRdmaGidStatusFree(event->data);
+break;
case QEMU_PROCESS_EVENT_WATCHDOG:
case QEMU_PROCESS_EVENT_DEVICE_DELETED:
case QEMU_PROCESS_EVENT_NIC_RX_FILTER_CHANGED:
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index 80bd4bde91..64bceb9a98 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -487,6 +487,7 @@ typedef enum {
QEMU_PROCESS_EVENT_BLOCK_JOB,
QEMU_PROCESS_EVENT_MONITOR_EOF,
QEMU_PROCESS_EVENT_PR_DISCONNECT,
+QEMU_PROCESS_EVENT_RDMA_GID_STATUS_CHANGED,
QEMU_PROCESS_EVENT_LAST
} qemuProcessEventType;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index a52e2495d5..4e541abd05 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4788,6 +4788,47 @@ processPRDisconnectEvent(virDomainObjPtr vm)
}
+static void
+processRdmaGidStatusChangedEvent(virDomainObjPtr vm,
+ qemuMonitorRdmaGidStatusChangedPrivatePtr
info)
+{
+unsigned int prefix_len;
+virSocketAddr addr = {0};
+int rc;
+
+if (!virDomainObjIsActive(vm))
+return;
+
+
VIR_DEBUG("netdev=%s,gid_status=%d,subnet_prefix=0x%llx,interface_id=0x%llx",
+ info->netdev, info->gid_status, info->subnet_prefix,
+ info->interface_id);
+
+if (info->subnet_prefix) {
+prefix_len = 64;
+uint32_t ipv6[4];
+memcpy([0], >subnet_prefix, sizeof(info->subnet_prefix));
+memcpy([2], >interface_id, sizeof(info->interface_id));
+virSocketAddrSetIPv6AddrNetOrder(, ipv6);
+} else {
+prefix_len = 24;
+virSocketAddrSetIPv4AddrNetOrder(, info->interface_id >> 32);
+}
+
+if (info->gid_status) {
+VIR_DEBUG("Adding %s to %s", virSocketAddrFormat(), info->netdev);
+rc = virNetDevIPAddrAdd(info->netdev, , NULL, prefix_len);
+} else {
+VIR_DEBUG("Removing %s from %s", virSocketAddrFormat(),
+ info->netdev);
+rc = virNetDevIPAddrDel(info->netdev, , prefix_len);
+}
+
+if (rc < 0)
+VIR_WARN("Fail to update address %s to %s", virSocketAddrFormat(),
+ info->netdev);
+}
+
+
static void qemuProcessEventHandler(void *data, void *opaque)
{
struct qemuProcessEvent *processEvent = data;
@@ -4828,6 +4869,9 @@ static void qemuProcessEventHandler(void *data, void
*opaque)
case QEMU_PROCESS_EVENT_PR_DISCONNECT:
processPRDisconnectEvent(vm);
break;
+case QEMU_PROCESS_EVENT_RDMA_GID_STATUS_CHANGED:
+processRdmaGidStatusChangedEvent(vm, processEvent->data);
+break;
case QEMU_PROCESS_EVENT_LAST:
break;
}
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index 7f7013e115..4bf71dbf8c 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -1686,6 +1686,22 @@ qemuMonitorEmitPRManagerStatusChanged(qemuMonitorPtr mon,
}
+int
+qemuMonitorEmitRdmaGidStatusChanged(qemuMonitorPtr mon, const char
Re: [libvirt] [PATCH v2 8/8] news: Cold(un)plug and hot(un)plug support for usb hub device
On Tue, Nov 6, 2018 at 6:52 AM John Ferlan wrote: > > > On 10/12/18 4:50 AM, Han Han wrote: > > Signed-off-by: Han Han > > --- > > docs/news.xml | 5 + > > 1 file changed, 5 insertions(+) > > > > diff --git a/docs/news.xml b/docs/news.xml > > index dc08c96352..0efad21758 100644 > > --- a/docs/news.xml > > +++ b/docs/news.xml > > @@ -35,6 +35,11 @@ > > > > > > > > + > > + > > + qemu: Add support for cold(un)plugging and hot(un)plugging > usb hub device > > + > > + > > How about: > > > Add active and inactive device add or remove for QEMU USB Hub devices > > > Add the ability to attach or detach a USB Hub device either for > active or inactive guests. > > Well. Your advice is nice. 'active' or 'inactive' is to describe the guests not the devices. > > ? > > John > > > > > > > > > > -- Best regards, --- Han Han Quality Engineer Redhat. Email: h...@redhat.com Phone: +861065339333 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] snapshot: Don't hose list on deletion failure
ping
On 10/17/18 8:45 PM, Eric Blake wrote:
If qemuDomainSnapshotDiscard() fails for any reason (rare,
but possible with an ill-timed ENOMEM or if
qemuDomainSnapshotForEachQcow2() has problems talking to the
qemu guest monitor), then an attempt to retry the snapshot
deletion API will crash because we didn't undo the effects
of virDomainSnapshotDropParent() temporarily rearranging the
internal list structures, and the second attempt to drop
parents will dereference NULL. Fix it by instead noting that
there are only two callers to qemuDomainSnapshotDiscard(),
and only one of the two callers wants the parent to be updated;
thus we can move the call to virDomainSnapshotDropParent()
into a code path that only gets executed on success.
Signed-off-by: Eric Blake
---
v2: avoid use-after-free
---
src/qemu/qemu_domain.c | 6 --
src/qemu/qemu_driver.c | 1 -
2 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index f00f1b3fdb..dd67be5e2a 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -8246,7 +8246,7 @@ int
qemuDomainSnapshotDiscard(virQEMUDriverPtr driver,
virDomainObjPtr vm,
virDomainSnapshotObjPtr snap,
- bool update_current,
+ bool update_parent,
bool metadata_only)
{
char *snapFile = NULL;
@@ -8275,7 +8275,7 @@ qemuDomainSnapshotDiscard(virQEMUDriverPtr driver,
goto cleanup;
if (snap == vm->current_snapshot) {
-if (update_current && snap->def->parent) {
+if (update_parent && snap->def->parent) {
parentsnap = virDomainSnapshotFindByName(vm->snapshots,
snap->def->parent);
if (!parentsnap) {
@@ -8298,6 +8298,8 @@ qemuDomainSnapshotDiscard(virQEMUDriverPtr driver,
if (unlink(snapFile) < 0)
VIR_WARN("Failed to unlink %s", snapFile);
+if (update_parent)
+virDomainSnapshotDropParent(snap);
virDomainSnapshotObjListRemove(vm->snapshots, snap);
ret = 0;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index a52e2495d5..9f71641dfa 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -16526,7 +16526,6 @@ qemuDomainSnapshotDelete(virDomainSnapshotPtr snapshot,
snap->first_child = NULL;
ret = 0;
} else {
-virDomainSnapshotDropParent(snap);
ret = qemuDomainSnapshotDiscard(driver, vm, snap, true,
metadata_only);
}
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 8/8] news: Cold(un)plug and hot(un)plug support for usb hub device
On 10/12/18 4:50 AM, Han Han wrote: > Signed-off-by: Han Han > --- > docs/news.xml | 5 + > 1 file changed, 5 insertions(+) > > diff --git a/docs/news.xml b/docs/news.xml > index dc08c96352..0efad21758 100644 > --- a/docs/news.xml > +++ b/docs/news.xml > @@ -35,6 +35,11 @@ > > > > + > + > + qemu: Add support for cold(un)plugging and hot(un)plugging usb hub > device > + > + How about: Add active and inactive device add or remove for QEMU USB Hub devices Add the ability to attach or detach a USB Hub device either for active or inactive guests. ? John > > > > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 7/8] qemu: implement usb hub device hotunplug
$SUBJ:
s/implement/Implement
On 10/12/18 4:50 AM, Han Han wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1375423
>
Add the infrastructure to allow a USB Hub device to be hot unplugged.
> Signed-off-by: Han Han
> ---
> src/qemu/qemu_driver.c | 5 ++-
> src/qemu/qemu_hotplug.c | 74 -
> src/qemu/qemu_hotplug.h | 4 +++
> 3 files changed, 81 insertions(+), 2 deletions(-)
>
This is where things get a bit dicey. Are you sure we can allow this
given that we automagically create hub devices when there's too many USB
devices, see:
tests/qemuxml2argvdata/usb-hub-autoadd-deluxe.args
tests/qemuxml2argvdata/usb-hub-autoadd-deluxe.xml
and in the code qemuDomainUSBAddressAddHubs?
Note that the test XML doesn't have a hub device defined, but yet some
are created. If someone decides to hot unplug one that has something
plugged into it (e.g. in the case of that test XML, some USB Input
device), then what happens?
Can you test that and ensure the results that you get?
I see you've essentially copied the steps that an Input device would
take; however, I'd suspect a Hub device is a bit more special and we may
need to process the various USB devices to make sure there isn't one
using the Hub device by port... Even worse if it's port=1 and there's a
port=1.1 around that equates to more ports (like from the test).
The question becomes - can we determine which port a USB device is using
via the guest status/live XML? Would the qemu del device error out or
happily accept deleting a hub with attached ports? Or would it just drop
all those attached devices?
Logically what's here would appear to work and is essentially similar to
the Input devices code, so from that aspect things look OK - it's this
one (hah) minor detail.
Tks -
John
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index de764a7f1c..c8a6d98dc0 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -7822,11 +7822,14 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm,
> ret = qemuDomainDetachVsockDevice(vm, dev->data.vsock, async);
> break;
>
> +case VIR_DOMAIN_DEVICE_HUB:
> +ret = qemuDomainDetachHubDevice(vm, dev->data.hub, async);
> +break;
> +
> case VIR_DOMAIN_DEVICE_FS:
> case VIR_DOMAIN_DEVICE_SOUND:
> case VIR_DOMAIN_DEVICE_VIDEO:
> case VIR_DOMAIN_DEVICE_GRAPHICS:
> -case VIR_DOMAIN_DEVICE_HUB:
> case VIR_DOMAIN_DEVICE_SMARTCARD:
> case VIR_DOMAIN_DEVICE_MEMBALLOON:
> case VIR_DOMAIN_DEVICE_NVRAM:
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 1b6cc36bc8..87749ec011 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -4965,6 +4965,32 @@ qemuDomainRemoveRedirdevDevice(virQEMUDriverPtr driver,
> }
>
>
> +static int
> +qemuDomainRemoveHubDevice(virDomainObjPtr vm,
> + virDomainHubDefPtr dev)
> +{
> +qemuDomainObjPrivatePtr priv = vm->privateData;
> +virQEMUDriverPtr driver = priv->driver;
> +virObjectEventPtr event = NULL;
> +size_t i;
> +
> +VIR_DEBUG("Removing hub device %s from domain %p %s",
> + dev->info.alias, vm, vm->def->name);
> +
> +event = virDomainEventDeviceRemovedNewFromObj(vm, dev->info.alias);
> +virObjectEventStateQueue(driver->domainEventState, event);
> +for (i = 0; i < vm->def->nhubs; i++) {
> +if (vm->def->hubs[i] == dev)
> +break;
> +}
> +qemuDomainReleaseDeviceAddress(vm, >info, NULL);
> +
> +virDomainHubDefFree(vm->def->hubs[i]);
> +VIR_DELETE_ELEMENT(vm->def->hubs, i, vm->def->nhubs);
> +return 0;
> +}
> +
> +
> int
> qemuDomainRemoveDevice(virQEMUDriverPtr driver,
> virDomainObjPtr vm,
> @@ -5016,13 +5042,16 @@ qemuDomainRemoveDevice(virQEMUDriverPtr driver,
> ret = qemuDomainRemoveVsockDevice(vm, dev->data.vsock);
> break;
>
> +case VIR_DOMAIN_DEVICE_HUB:
> +ret = qemuDomainRemoveHubDevice(vm, dev->data.hub);
> +break;
> +
> case VIR_DOMAIN_DEVICE_NONE:
> case VIR_DOMAIN_DEVICE_LEASE:
> case VIR_DOMAIN_DEVICE_FS:
> case VIR_DOMAIN_DEVICE_SOUND:
> case VIR_DOMAIN_DEVICE_VIDEO:
> case VIR_DOMAIN_DEVICE_GRAPHICS:
> -case VIR_DOMAIN_DEVICE_HUB:
> case VIR_DOMAIN_DEVICE_SMARTCARD:
> case VIR_DOMAIN_DEVICE_MEMBALLOON:
> case VIR_DOMAIN_DEVICE_NVRAM:
> @@ -6955,3 +6984,46 @@ qemuDomainDetachVsockDevice(virDomainObjPtr vm,
> qemuDomainResetDeviceRemoval(vm);
> return ret;
> }
> +
> +
> +int
> +qemuDomainDetachHubDevice(virDomainObjPtr vm,
> + virDomainHubDefPtr def,
> + bool async)
> +{
> +qemuDomainObjPrivatePtr priv = vm->privateData;
> +virQEMUDriverPtr driver = priv->driver;
> +virDomainHubDefPtr hub;
> +int ret = -1;
> +int idx;
> +
> +if ((idx = virDomainHubDefFind(vm->def, def)) < 0) {
> +
Re: [libvirt] [PATCH v3 1/6] qemu-nbd: add support for authorization of TLS clients
On 10/9/18 8:23 AM, Daniel P. Berrangé wrote:
From: "Daniel P. Berrange"
Currently any client which can complete the TLS handshake is able to use
the NBD server. The server admin can turn on the 'verify-peer' option
for the x509 creds to require the client to provide a x509 certificate.
This means the client will have to acquire a certificate from the CA
before they are permitted to use the NBD server. This is still a fairly
low bar to cross.
This adds a '--tls-authz OBJECT-ID' option to the qemu-nbd command which
takes the ID of a previously added 'QAuthZ' object instance. This will
be used to validate the client's x509 distinguished name. Clients
failing the authorization check will not be permitted to use the NBD
server.
For example to setup authorization that only allows connection from a client
whose x509 certificate distinguished name is
CN=laptop.example.com,O=Example Org,L=London,ST=London,C=GB
use:
qemu-nbd --object tls-creds-x509,id=tls0,dir=/home/berrange/qemutls,\
endpoint=server,verify-peer=yes \
--object authz-simple,id=auth0,identity=CN=laptop.example.com,,\
O=Example Org,,L=London,,ST=London,,C=GB \
Missing shell quoting around the space in 'Example Org'. It's also
fairly obvious that actual shell commands can't have leading space
between \-newline line continuations.
--tls-creds tls0 \
--tls-authz authz0
other qemu-nbd args...
Signed-off-by: Daniel P. Berrange
---
include/block/nbd.h | 2 +-
nbd/server.c| 10 +-
qemu-nbd.c | 13 -
qemu-nbd.texi | 4
4 files changed, 22 insertions(+), 7 deletions(-)
+++ b/qemu-nbd.c
@@ -52,6 +52,7 @@
#define QEMU_NBD_OPT_TLSCREDS 261
#define QEMU_NBD_OPT_IMAGE_OPTS262
#define QEMU_NBD_OPT_FORK 263
+#define QEMU_NBD_OPT_TLSAUTHZ 264
@@ -532,6 +534,7 @@ int main(int argc, char **argv)
{ "image-opts", no_argument, NULL, QEMU_NBD_OPT_IMAGE_OPTS },
{ "trace", required_argument, NULL, 'T' },
{ "fork", no_argument, NULL, QEMU_NBD_OPT_FORK },
+{ "tls-authz", no_argument, NULL, QEMU_NBD_OPT_TLSAUTHZ },
{ NULL, 0, NULL, 0 }
};
Missing a change to qemu-nbd --help to describe the new option.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 6/8] qemu: implement usb hub device hotplug
$SUBJ: s/implement/Implement On 10/12/18 4:50 AM, Han Han wrote: > https://bugzilla.redhat.com/show_bug.cgi?id=1375423 Add the infrastructure to allow a USB Hub device to be hotplugged. > > Signed-off-by: Han Han > --- > src/qemu/qemu_driver.c | 9 ++- > src/qemu/qemu_hotplug.c | 58 + > src/qemu/qemu_hotplug.h | 8 ++ > 3 files changed, 74 insertions(+), 1 deletion(-) > [...] > diff --git a/src/qemu/qemu_hotplug.h b/src/qemu/qemu_hotplug.h > index 0297e42a98..444333c4df 100644 > --- a/src/qemu/qemu_hotplug.h > +++ b/src/qemu/qemu_hotplug.h > @@ -135,10 +135,18 @@ int qemuDomainAttachInputDevice(virQEMUDriverPtr driver, > virDomainObjPtr vm, > virDomainInputDefPtr input); > > +int qemuDomainAttachHubDevice(virQEMUDriverPtr driver, > + virDomainObjPtr vm, > + virDomainHubDefPtr hub); > + > int qemuDomainAttachVsockDevice(virQEMUDriverPtr driver, > virDomainObjPtr vm, > virDomainVsockDefPtr vsock); > > +int qemuDomainAttachInputDevice(virQEMUDriverPtr driver, > +virDomainObjPtr vm, > +virDomainInputDefPtr input); > + cut-n-paste error perhaps? I'll remove before pushing though... Reviewed-by: John Ferlan John > int qemuDomainAttachLease(virQEMUDriverPtr driver, >virDomainObjPtr vm, >virDomainLeaseDefPtr lease); > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 4/8] qemu_command: Make qemuBuildHubDevStr global
$SUBJ: s/qemu_command/qemu/ On 10/12/18 4:50 AM, Han Han wrote: > Make function qemuBuildHubDevStr global for reuse on hub hotplug. > > Signed-off-by: Han Han > --- > src/qemu/qemu_command.c | 2 +- > src/qemu/qemu_command.h | 4 > 2 files changed, 5 insertions(+), 1 deletion(-) > Reviewed-by: John Ferlan John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 2/8] qemu: Allow coldunplugging of hub device
On 10/12/18 4:50 AM, Han Han wrote: > https://bugzilla.redhat.com/show_bug.cgi?id=1375423 > > Signed-off-by: Han Han > --- > src/conf/domain_conf.c | 30 ++ > src/conf/domain_conf.h | 3 +++ > src/libvirt_private.syms | 1 + > src/qemu/qemu_driver.c | 10 +- > 4 files changed, 43 insertions(+), 1 deletion(-) > I assume no concerns over something that could be attached to a port on the hub... I guess for a cold unplug it probably won't matter so much. Reviewed-by: John Ferlan John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 3/8] qemu_alias: Refactor hub alias assignment for hotplug
$SUBJ: s/qemu_alias/qemu/ On 10/12/18 4:50 AM, Han Han wrote: > Make qemuAssignDeviceHubAlias global and allow alias > generating for reuse on hotplug. Following the model of Input devices I see... > > Signed-off-by: Han Han > --- > src/qemu/qemu_alias.c | 22 ++ > src/qemu/qemu_alias.h | 4 > 2 files changed, 22 insertions(+), 4 deletions(-) > Reviewed-by: John Ferlan John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 1/8] qemu: Allow coldplugging of hub device
On 10/12/18 4:50 AM, Han Han wrote: > https://bugzilla.redhat.com/show_bug.cgi?id=1375423 > > Signed-off-by: Han Han > --- > src/qemu/qemu_driver.c | 6 +- > 1 file changed, 5 insertions(+), 1 deletion(-) > sorry for the delay. Lost in the pre KVM Forum "shuffle" Reviewed-by: John Ferlan John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 5/8] private_syms: add virDomainHubDefFree to libvirt_private.syms
$SUBJ: s/private_syms: a/conf: A/ On 10/12/18 4:50 AM, Han Han wrote: > Add virDomainHubDefFree to for the preparation of usb hub hotplug. > > Signed-off-by: Han Han > --- > src/libvirt_private.syms | 1 + > 1 file changed, 1 insertion(+) > Reviewed-by: John Ferlan John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 00/20] Incremental Backup API additions
On 10/25/2018 03:20 PM, Eric Blake wrote:
> The following is the latest version of my API proposal for
> incremental backups, and matches the demo I will be presenting
> as part of my KVM Forum 2018 talk tomorrow afternoon:
> https://kvmforum2018.sched.com/event/FzuB/facilitating-incremental-backup-eric-blake-red-hat
>
> The patches are also available via https://repo.or.cz/libvirt/ericb.git
> at the tag backup-v3.
>
> The qemu integration is still incomplete (I don't have push
> mode backups working, only pull mode; and there are still some
> things to fine-tune as I figure out how to better support
> external snapshots and block commit/stream), and I still haven't
> gotten any opinions on whether it is better to have:
>
> virDomainSnapshotCreateXML("...") # existing
> virDomainBackupBegin("...", "...") # this
> series
> virDomainSnapshotCheckpointCreateXML("...",
> "...") # new
>
> or to make checkpoint creation part of the snapshot and backup XML, as in:
>
> virDomainSnapshotCreateXML("...")
> # extension of existing
> virDomainBackupBegin("...") #
> tweak to this series
>
> It's also obvious that we don't want to check in the qemu implementation
> until after Peter's blockdev work has landed AND qemu has gotten rid
> of the x- prefix in its QMP interactions, although the state of this
> series SHOULD be sufficient to prove that the proposed interface is
> at least flexible enough to adapt to whatever qemu tweaks land and
> whatever other hypervisors may support.
For what it's worth, the x prefixes for bitmap manipulation were only
because there was no user for those features; if the overall design of
this API proposal looks palatable to the libvirt community, I'll happily
drop the prefix for any commands actually used by this patchset, before
it's merged.
(From the looks of it, it looks like all of them.)
--js
> Whether to accept the API now> (to make backporting qemu support downstream
> across versions without
> rebasing to an even later date for the API) is a harder question;
> Jirka has already expressed distaste at the idea.
>
> There are probably still loads of things needing fixing in this version,
> I already know I did not address all of Peter's review comments on v2.
> My unit test coverage is not very complete on the XML parsing, and there
> are definitely some hacks in place to get things to the point of a working
> demo that need to be polished for robustness.
>
> Below is a diff since the v2 posting (patches 1-10 here, for later
> patches a comparison against the state of my tree at the time I posted v2):
> https://www.redhat.com/archives/libvir-list/2018-October/msg00700.html
>
> 001/20:[] [--] 'snapshots: Avoid term 'checkpoint' for full system
> snapshot'
> 002/20:[] [--] 'domain_conf: Expose virDomainStorageNetworkParseHost'
> 003/20:[down] 'qemu: Allow optional export name during NBD export'
> 004/20:[] [--] 'backup: Document nuances between different state capture
> APIs'
> 005/20:[] [--] 'backup: Introduce virDomainCheckpointPtr'
> 006/20:[0012] [FC] 'backup: Document new XML for backups'
> 007/20:[] [--] 'backup: Introduce virDomainCheckpoint APIs'
> 008/20:[0024] [FC] 'backup: Introduce virDomainBackup APIs'
> 009/20:[] [--] 'backup: Add new domain:checkpoint access control'
> 010/20:[0001] [FC] 'backup: Implement backup APIs for remote driver'
> 011/20:[0007] [FC] 'wip: backup: Parse and output checkpoint XML'
> 012/20:[0199] [FC] 'wip: backup: Parse and output backup XML'
> 013/20:[] [--] 'backup: Implement virsh support for checkpoints'
> 014/20:[] [--] 'wip: backup: virsh support for backup'
> 015/20:[0223] [FC] 'backup: Add new qemu monitor interactions'
> 016/20:[0123] [FC] 'backup: qemu: Implement metadata tracking for checkpoint
> APIs'
> 017/20:[down] 'wip: backup: Wire up qemu checkpoint commands over QMP'
> 018/20:[0072] [FC] 'wip: backup: qemu: Implement framework for backup job
> APIs'
> 019/20:[down] 'backup: Wire up qemu full pull backup commands over QMP'
> 020/20:[down] 'backup: implement qemu incremental pull backup'
>
> No direct dependency on this one, but included in my git tag:
> https://www.redhat.com/archives/libvir-list/2018-October/msg00899.html
>
> Eric Blake (20):
> snapshots: Avoid term 'checkpoint' for full system snapshot
> domain_conf: Expose virDomainStorageNetworkParseHost
> qemu: Allow optional export name during NBD export
> backup: Document nuances between different state capture APIs
> backup: Introduce virDomainCheckpointPtr
> backup: Document new XML for backups
> backup: Introduce virDomainCheckpoint APIs
> backup: Introduce virDomainBackup APIs
> backup: Add new domain:checkpoint access control
> backup: Implement backup APIs for remote driver
> wip: backup: Parse and output checkpoint XML
> wip: backup: Parse and output backup XML
> backup: Implement virsh support for checkpoints
> wip: backup: virsh support for backup
> backup: Add new qemu
Re: [libvirt] [RFC PATCH] Add new migration flag VIR_MIGRATE_DRY_RUN
On 11/5/18 1:46 AM, Michal Privoznik wrote:
On 11/02/2018 11:34 PM, 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.
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.
Thanks for considering the idea!
(BTW, if it is considered useful I will follow up with a V1 series that
includes this patch and and impl for the qemu driver.)
include/libvirt/libvirt-domain.h | 12
src/qemu/qemu_migration.h| 3 ++-
tools/virsh-domain.c | 7 +++
tools/virsh.pod | 10 +-
4 files changed, 30 insertions(+), 2 deletions(-)
diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
index fdd2d6b8ea..6d52f6ce50 100644
--- a/include/libvirt/libvirt-domain.h
+++ b/include/libvirt/libvirt-domain.h
@@ -830,6 +830,18 @@ typedef enum {
*/
VIR_MIGRATE_TLS = (1 << 16),
+/* Setting the VIR_MIGRATE_DRY_RUN flag will cause libvirt to make a
+ * best-effort attempt to check if migration will succeed. The destination
+ * host will be checked to see if it can accommodate the resources required
+ * by the domain. For example are the network, disk, memory, and CPU
While this is a honourable goal to achieve I don't think we can
guarantee it (without running qemu). At least in qemu world.
I don't think it can be guaranteed even if qemu is run. That's why the rest of
the comment warns about relying on dry run's success. Dry run succeeding should
give the user warm fuzzies, but it can't guarantee success of a future migration.
For instance, libvirt doesn't check if there's enough memory (nor regular
nor hugepages) when domain is started/migrated. We just run qemu and let
it fail. However, for network, CPU and hostdev we do run checks so these
might work. Disks are in grey area - we check their presence but not
their labels. And if domain is relabel=no then the only way to learn if
qemu would succeed is to run it.
I'll have to check but I think starting qemu for dry run is a no-go if host
resources are actually consumed. E.g. if host memory is given to the dry run
qemu and not available for non dry run instances.
But I don't see much problem with starting qemu in paused state. I mean,
we can get through Prepare phase but never actually reach Perform stage.
The API/flag would return success if Prepare succeeded.
Yep, my though exactly, along with doing less preparation in the prepare phase.
I bet it's easier to check if migration would succeed in xen world, or?
I suppose so, if anything because it supports less options. E.g. there's only
one type of cpu for Xen PV domains.
The other thing is how are apps expected to use this? I mean, if an app
wants to work without admin intervention then it would need to learn how
to fix any possible error (missing disk, perms issue, missing hostdev,
etc.). This is not a trivial task IMO.
That's the case today if an actual migration fails. Dry run simply allows
checking the possible success of migration without actually performing it. Admin
intervention can occur before there is any attempt to perform a doomed migration
(which in worse case can result in domain not running on src or dst).
Regards,
Jim
--
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 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. 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. 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. 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. 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. John > 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 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
Re: [libvirt] [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. John > /* 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 17/18] qemu: Report cache occupancy (CMT) with domstats
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...
>
> #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:
>
Re: [libvirt] [PATCHv7 16/18] qemu: refactor qemuDomainGetStatsCpu
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
> 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] [PATCH RESEND] qemu: Process RDMA GID state change event
On Mon, Nov 05, 2018 at 05:42:10PM +0100, Michal Privoznik wrote:
> [There is no need to resend your patch just to put yourself onto CC
> list. The review policy is always "reply to all"]
Hi Michal,
Actually i posted the "RESEND" as my original mail was not shown up in
https://www.redhat.com/archives/libvir-list so I followed the
recommendations (register to mailing list etc) and re-post.
>
> On 11/05/2018 11:50 AM, Yuval Shaia wrote:
> > This event is emitted on the monitor when a GID table in pvrdma device
> > is modified and the change needs to be propagate to the backend RDMA
> > device's GID table.
>
> Not yet, your qemu patches are not merged yet ;-) I will provide review
> anyway, but even if this patch was good as is we couldn't merge it
> unless it's merged into qemu repo first. We have our history with that.
Yeah, not yet merged but assuming both should be merged at the same time
due to dependency i had to start the reviews in parallel.
The qemu patches will probably merged in the in the next few weeks (v3.2)
so I'd like to do the best to be ready with this one too.
Anyways, appreciate much your review i took all you comments and will post
v2 soon. Please check my answers/questions below.
>
> >
> > The control over the RDMA device's GID table is done by updating the
> > device's Ethernet function addresses.
> > Usually the first GID entry is determine by the MAC address, the second
> > by the first IPv6 address and the third by the IPv4 address. Other
> > entries can be added by adding more IP addresses. The opposite is the
> > same, i.e. whenever an address is removed, the corresponding GID entry
> > is removed.
> >
> > The process is done by the network and RDMA stacks. Whenever an address
> > is added the ib_core driver is notified and calls the device driver's
> > add_gid function which in turn update the device.
> >
> > To support this in pvrdma device we need to hook into the create_bind
> > and destroy_bind HW commands triggered by pvrdma driver in guest.
> > Whenever a changed is made to the pvrdma device's GID table a special
> > QMP messages is sent to be processed by libvirt to update the address of
> > the backend Ethernet device.
> >
> > Signed-off-by: Yuval Shaia
> > ---
> > src/qemu/qemu_domain.c | 3 ++
> > src/qemu/qemu_domain.h | 15 +
> > src/qemu/qemu_driver.c | 40
> > src/qemu/qemu_monitor.c | 28 +
> > src/qemu/qemu_monitor.h | 13
> > src/qemu/qemu_monitor_json.c | 36 ++
> > src/qemu/qemu_process.c | 59
> > 7 files changed, 194 insertions(+)
> >
> > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> > index ba3fff607a..8da54c7ee9 100644
> > --- a/src/qemu/qemu_domain.c
> > +++ b/src/qemu/qemu_domain.c
> > @@ -13479,6 +13479,9 @@ qemuProcessEventFree(struct qemuProcessEvent *event)
> > case QEMU_PROCESS_EVENT_GUESTPANIC:
> > qemuMonitorEventPanicInfoFree(event->data);
> > break;
> > +case QEMU_PROCESS_EVENT_RDMA_GID_STATUS_CHANGED:
> > +qemuMonitorEventRdmaGidStatusFree(event->data);
> > +break;
> > case QEMU_PROCESS_EVENT_WATCHDOG:
> > case QEMU_PROCESS_EVENT_DEVICE_DELETED:
> > case QEMU_PROCESS_EVENT_NIC_RX_FILTER_CHANGED:
> > diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
> > index 80bd4bde91..1b188843e3 100644
> > --- a/src/qemu/qemu_domain.h
> > +++ b/src/qemu/qemu_domain.h
> > @@ -478,6 +478,18 @@ struct _qemuDomainVsockPrivate {
> > };
> >
> >
> > +typedef struct _qemuDomainRdmaGidStatusChangedPrivate
> > qemuDomainRdmaGidStatusChangedPrivate;
> > +typedef qemuDomainRdmaGidStatusChangedPrivate
> > *qemuDomainRdmaGidStatusChangedPrivatePtr;
> > +struct _qemuDomainRdmaGidStatusChangedPrivate {
> > +virObject parent;
> > +
> > +char *netdev;
> > +bool gid_status;
> > +uint64_t subnet_prefix;
> > +uint64_t interface_id;
>
> We use ULL instead of uint64_t.
Sure, will do.
>
> > +};
> > +
> > +
> > typedef enum {
> > QEMU_PROCESS_EVENT_WATCHDOG = 0,
> > QEMU_PROCESS_EVENT_GUESTPANIC,
> > @@ -487,6 +499,7 @@ typedef enum {
> > QEMU_PROCESS_EVENT_BLOCK_JOB,
> > QEMU_PROCESS_EVENT_MONITOR_EOF,
> > QEMU_PROCESS_EVENT_PR_DISCONNECT,
> > +QEMU_PROCESS_EVENT_RDMA_GID_STATUS_CHANGED,
> >
> > QEMU_PROCESS_EVENT_LAST
> > } qemuProcessEventType;
> > @@ -499,6 +512,8 @@ struct qemuProcessEvent {
> > void *data;
> > };
> >
> > +void
> > qemuMonitorEventRdmaGidStatusFree(qemuDomainRdmaGidStatusChangedPrivatePtr
> > info);
>
> If this function has prefix qemuMonitor then is should be in
> qemu_monitor* file. Just like qemuMonitorEventPanicInfoFree() is.
Sure,
I did a quite mass here :( will change it.
>
> > +
> > void qemuProcessEventFree(struct qemuProcessEvent *event);
> >
> > typedef struct _qemuDomainLogContext qemuDomainLogContext;
> > diff --git
Re: [libvirt] [PATCHv7 15/18] conf: Add 'id' to virDomainResctrlDef
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
> 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] [PATCHv7 14/18] util: Add function for checking if monitor is running
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) {
> +VIR_ERROR(_("Monitor %s PID list mismatch in length"),
> monitor->path);
> +goto cleanup;
> +}
See this doesn't make sense - why have a lookaside list then? Either
you trust what's in your file or you don't.
Having boolean logic return true/false where false can either be an
error generated or a truly false value just doesn't seem right.
> +
>
[libvirt] [PATCH] lxc: Clang is complaining about possible NULL pointer.
The array "mount" inside lxc_container is not being checked before for
loop. Clang syntax scan is complaining about this segmentation fault.
Signed-off-by: Julio Faracco
---
src/lxc/lxc_container.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c
index 918194dacd..6b7bcd8eb6 100644
--- a/src/lxc/lxc_container.c
+++ b/src/lxc/lxc_container.c
@@ -871,7 +871,7 @@ static int lxcContainerSetReadOnly(void)
qsort(mounts, nmounts, sizeof(mounts[0]),
virStringSortRevCompare);
-for (i = 0; i < nmounts; i++) {
+for (i = 0; i < nmounts && mounts; i++) {
VIR_DEBUG("Bind readonly %s", mounts[i]);
if (mount(mounts[i], mounts[i], "none",
MS_BIND|MS_REC|MS_RDONLY|MS_REMOUNT, NULL) < 0) {
virReportSystemError(errno,
@@ -883,7 +883,7 @@ static int lxcContainerSetReadOnly(void)
ret = 0;
cleanup:
-for (i = 0; i < nmounts; i++)
+for (i = 0; i < nmounts && mounts; i++)
VIR_FREE(mounts[i]);
VIR_FREE(mounts);
endmntent(procmnt);
--
2.17.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2] lxc: Include support to lxc version 3.0 or higher.
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.
Signed-off-by: Julio Faracco
---
src/lxc/lxc_native.c | 18 +-
1 file changed, 13 insertions(+), 5 deletions(-)
diff --git a/src/lxc/lxc_native.c b/src/lxc/lxc_native.c
index cbdec723dd..8604cbaf46 100644
--- a/src/lxc/lxc_native.c
+++ b/src/lxc/lxc_native.c
@@ -200,7 +200,9 @@ lxcSetRootfs(virDomainDefPtr def,
int type = VIR_DOMAIN_FS_TYPE_MOUNT;
VIR_AUTOFREE(char *) value = NULL;
-if (virConfGetValueString(properties, "lxc.rootfs", ) <= 0)
+if (virConfGetValueString(properties, "lxc.rootfs", ) <= 0 &&
+/* Legacy config keys were removed after release 3.0. */
+virConfGetValueString(properties, "lxc.rootfs.path", ) <= 0)
return -1;
if (STRPREFIX(value, "/dev/"))
@@ -684,7 +686,9 @@ lxcCreateConsoles(virDomainDefPtr def, virConfPtr
properties)
virDomainChrDefPtr console;
size_t i;
-if (virConfGetValueString(properties, "lxc.tty", ) <= 0)
+if (virConfGetValueString(properties, "lxc.tty", ) <= 0 &&
+/* Legacy config keys were removed after release 3.0. */
+virConfGetValueString(properties, "lxc.tty.max", ) <= 0)
return 0;
if (virStrToLong_i(value, NULL, 10, ) < 0) {
@@ -724,7 +728,7 @@ lxcIdmapWalkCallback(const char *name, virConfValuePtr
value, void *data)
char type;
unsigned long start, target, count;
-if (STRNEQ(name, "lxc.id_map") || !value->str)
+if (STRNEQ(name, "lxc.id_map") || STRNEQ(name, "lxc.idmap") || !value->str)
return 0;
if (sscanf(value->str, "%c %lu %lu %lu", ,
@@ -1041,7 +1045,9 @@ lxcParseConfigString(const char *config,
if (VIR_STRDUP(vmdef->os.init, "/sbin/init") < 0)
goto error;
-if (virConfGetValueString(properties, "lxc.utsname", ) <= 0 ||
+if ((virConfGetValueString(properties, "lxc.utsname", ) <= 0 &&
+ /* Legacy config keys were removed after release 3.0. */
+ virConfGetValueString(properties, "lxc.uts.name", ) <= 0) ||
VIR_STRDUP(vmdef->name, value) < 0)
goto error;
if (!vmdef->name && (VIR_STRDUP(vmdef->name, "unnamed") < 0))
@@ -1051,7 +1057,9 @@ lxcParseConfigString(const char *config,
goto error;
/* Look for fstab: we shouldn't have it */
-if (virConfGetValue(properties, "lxc.mount")) {
+if (virConfGetValue(properties, "lxc.mount") ||
+/* Legacy config keys were removed after release 3.0. */
+virConfGetValue(properties, "lxc.mount.fstab")) {
virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
_("lxc.mount found, use lxc.mount.entry lines
instead"));
goto error;
--
2.17.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 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.
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...
3. In the reconnect logic - it would seem that we would be starting from
scratch again, right? So would the *tasks file even be valid since vcpus
could be added/deleted and we didn't get notified... So perhaps we need
some logic in the reconnect code to reinitialize the tasks file (IOW:
delete it since we're going to recreate it - I assume).
Perhaps more questions now vis-a-vis any sort of ACK/push for patches
starting here. At least 1-12 are in decent shape.
John
> @@ -7207,8 +7250,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);
>
> @@ -7939,9 +7992,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;
> +
> +
Re: [libvirt] [PATCHv7 13/18] qemu: enable resctrl monitor in qemu
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)
> +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
> +}
> +
The next loop is duplicitous and can be removed, right?
with some adjustments (which I can make as described),
Reviewed-by: John Ferlan
John
[...]
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv7 12/18] conf: Introduce cache monitor element in cachetune
On 10/22/18 4:01 AM, Wang Huaqiang wrote:
> Introducing element under to represent
> a cache monitor.
>
> Signed-off-by: Wang Huaqiang
> ---
> 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 b1651e3..2fd665c 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 @@
>
>
>
> + monitor
> +
> +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 associating
the associated
> +allocation. The default monitor has the same vCPU list as the
> +associating allocation. For non-default monitors, there
associated
> +are no vCPU overlap permitted.
For non-default monitors, overlapping vCPUs are not permitted.
> +
> +
> +
>
>
>
[...]
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index a068d4d..01f795f 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -2955,13 +2955,31 @@ virDomainLoaderDefFree(virDomainLoaderDefPtr loader)
>
>
[...]
> @@ -19026,7 +19215,14 @@ virDomainCachetuneDefParse(virDomainDefPtr def,
> if (!resctrl)
> goto cleanup;
>
> -if (virResctrlAllocIsEmpty(alloc)) {
> +if (virDomainResctrlMonDefParse(def, ctxt, node,
> +VIR_RESCTRL_MONITOR_TYPE_CACHE,
> +resctrl) < 0)
> +goto cleanup;
> +
> +/* If no element or element in , do not
> + * append any resctrl element */
> +if (virResctrlAllocIsEmpty(alloc) && !resctrl->nmonitors) {
Swap order since *IsEmpty is more compute intensive, also change to:
if (resctrl->nmonitors == 0 &&
> ret = 0;
> goto cleanup;
> }
> @@ -27063,12 +27259,41 @@ virDomainCachetuneDefFormatHelper(unsigned int
> level,
>
>
> static int
> +virDomainResctrlMonDefFormatHelper(virDomainResctrlMonDefPtr domresmon,
> + virResctrlMonitorType tag,
> + virBufferPtr buf)
> +{
> +char *vcpus = NULL;
> +
> +if (domresmon->tag != tag)
> +return 0;
> +
> +virBufferAddLit(buf, " +
> +if (tag == VIR_RESCTRL_MONITOR_TYPE_CACHE) {
> +virBufferAsprintf(buf, "level='%u' ",
> + VIR_DOMAIN_RESCTRL_MONITOR_CACHELEVEL);
> +}
So is this because when is introduced it won't use a cache
level, right? Just trying to recall (and perhaps add a comment).
> +
> +vcpus = virBitmapFormat(domresmon->vcpus);
> +if (!vcpus)
> +return -1;
> +
> +virBufferAsprintf(buf, "vcpus='%s'/>\n", vcpus);
> +
> +VIR_FREE(vcpus);
> +return 0;
> +}
> +
> +
[...]
I can fix the minor nits, just ack them...
Reviewed-by: John Ferlan
John
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv7 10/18] conf: Remove virDomainResctrlAppend and introduce virDomainResctrlNew
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.
> +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.
With the changes (that I can make),
Reviewed-by: John Ferlan
John
[...]
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv7 11/18] conf: move virResctrlAllocIsEmpty to a place a litter lower
$SUBJ: s/litter/little Perhaps better said: conf: Alter order of Cachetune virDomainResctrlNew call On 10/22/18 4:01 AM, Wang Huaqiang wrote: > This refactor allows to add some code between virDomainResctrlNew > and virResctrlAllocIsEmpty to extend the scope of resctrl. this then becomes: Refactor the logic to handle subsequent generation of a resource monitor which would effect whether a non default cache exists. > > Signed-off-by: Wang Huaqiang > --- > src/conf/domain_conf.c | 8 > 1 file changed, 4 insertions(+), 4 deletions(-) > Reviewed-by: John Ferlan John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv7 04/18] util: Add interface to determine monitor path
On 10/22/18 4:01 AM, Wang Huaqiang wrote:
> Add interface for resctrl monitor to determine the path.
>
> Signed-off-by: Wang Huaqiang
> ---
> 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 956aca8..1d0eb01 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)
Just ran the changes through Coverity - because of the above
"monitor->alloc" check, Coverity notes right here that monitor->alloc
could be NULL, so I think a check prior to here would be in order, such
as either before or after the monitor->path check:
if (!monitor->alloc) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("Missing resctrl monitor allocation"));
return -1;
}
Then the monitor->alloc check wouldn't be necessary. Thus the above
STRDUP becomes:
if (STREQ_NULLABLE(monitor->id, monitor->alloc->id)) {
if (VIR_STRDUP(monitor->path, monitor->alloc->path) < 0)
return -1;
return 0;
}
Let me know where you want it.
John
> +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__ */
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH RESEND] qemu: Process RDMA GID state change event
[There is no need to resend your patch just to put yourself onto CC
list. The review policy is always "reply to all"]
On 11/05/2018 11:50 AM, Yuval Shaia wrote:
> This event is emitted on the monitor when a GID table in pvrdma device
> is modified and the change needs to be propagate to the backend RDMA
> device's GID table.
Not yet, your qemu patches are not merged yet ;-) I will provide review
anyway, but even if this patch was good as is we couldn't merge it
unless it's merged into qemu repo first. We have our history with that.
>
> The control over the RDMA device's GID table is done by updating the
> device's Ethernet function addresses.
> Usually the first GID entry is determine by the MAC address, the second
> by the first IPv6 address and the third by the IPv4 address. Other
> entries can be added by adding more IP addresses. The opposite is the
> same, i.e. whenever an address is removed, the corresponding GID entry
> is removed.
>
> The process is done by the network and RDMA stacks. Whenever an address
> is added the ib_core driver is notified and calls the device driver's
> add_gid function which in turn update the device.
>
> To support this in pvrdma device we need to hook into the create_bind
> and destroy_bind HW commands triggered by pvrdma driver in guest.
> Whenever a changed is made to the pvrdma device's GID table a special
> QMP messages is sent to be processed by libvirt to update the address of
> the backend Ethernet device.
>
> Signed-off-by: Yuval Shaia
> ---
> src/qemu/qemu_domain.c | 3 ++
> src/qemu/qemu_domain.h | 15 +
> src/qemu/qemu_driver.c | 40
> src/qemu/qemu_monitor.c | 28 +
> src/qemu/qemu_monitor.h | 13
> src/qemu/qemu_monitor_json.c | 36 ++
> src/qemu/qemu_process.c | 59
> 7 files changed, 194 insertions(+)
>
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index ba3fff607a..8da54c7ee9 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -13479,6 +13479,9 @@ qemuProcessEventFree(struct qemuProcessEvent *event)
> case QEMU_PROCESS_EVENT_GUESTPANIC:
> qemuMonitorEventPanicInfoFree(event->data);
> break;
> +case QEMU_PROCESS_EVENT_RDMA_GID_STATUS_CHANGED:
> +qemuMonitorEventRdmaGidStatusFree(event->data);
> +break;
> case QEMU_PROCESS_EVENT_WATCHDOG:
> case QEMU_PROCESS_EVENT_DEVICE_DELETED:
> case QEMU_PROCESS_EVENT_NIC_RX_FILTER_CHANGED:
> diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
> index 80bd4bde91..1b188843e3 100644
> --- a/src/qemu/qemu_domain.h
> +++ b/src/qemu/qemu_domain.h
> @@ -478,6 +478,18 @@ struct _qemuDomainVsockPrivate {
> };
>
>
> +typedef struct _qemuDomainRdmaGidStatusChangedPrivate
> qemuDomainRdmaGidStatusChangedPrivate;
> +typedef qemuDomainRdmaGidStatusChangedPrivate
> *qemuDomainRdmaGidStatusChangedPrivatePtr;
> +struct _qemuDomainRdmaGidStatusChangedPrivate {
> +virObject parent;
> +
> +char *netdev;
> +bool gid_status;
> +uint64_t subnet_prefix;
> +uint64_t interface_id;
We use ULL instead of uint64_t.
> +};
> +
> +
> typedef enum {
> QEMU_PROCESS_EVENT_WATCHDOG = 0,
> QEMU_PROCESS_EVENT_GUESTPANIC,
> @@ -487,6 +499,7 @@ typedef enum {
> QEMU_PROCESS_EVENT_BLOCK_JOB,
> QEMU_PROCESS_EVENT_MONITOR_EOF,
> QEMU_PROCESS_EVENT_PR_DISCONNECT,
> +QEMU_PROCESS_EVENT_RDMA_GID_STATUS_CHANGED,
>
> QEMU_PROCESS_EVENT_LAST
> } qemuProcessEventType;
> @@ -499,6 +512,8 @@ struct qemuProcessEvent {
> void *data;
> };
>
> +void
> qemuMonitorEventRdmaGidStatusFree(qemuDomainRdmaGidStatusChangedPrivatePtr
> info);
If this function has prefix qemuMonitor then is should be in
qemu_monitor* file. Just like qemuMonitorEventPanicInfoFree() is.
> +
> void qemuProcessEventFree(struct qemuProcessEvent *event);
>
> typedef struct _qemuDomainLogContext qemuDomainLogContext;
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index a52e2495d5..dc088d844f 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -4788,6 +4788,43 @@ processPRDisconnectEvent(virDomainObjPtr vm)
> }
>
>
> +static void
> +processRdmaGidStatusChangedEvent(virDomainObjPtr vm,
> + qemuDomainRdmaGidStatusChangedPrivatePtr
> info)
> +{
> +unsigned int prefix_len;
> +virSocketAddr addr = {0};
> +int rc;
> +
> +if (!virDomainObjIsActive(vm))
> +return;
> +
> +
> VIR_DEBUG("netdev=%s,gid_status=%d,subnet_prefix=0x%lx,interface_id=0x%lx",
> + info->netdev, info->gid_status, info->subnet_prefix,
> + info->interface_id);
> +
> +if (info->subnet_prefix) {
> +prefix_len = 64;
> +uint32_t ipv6[4];
> +memcpy([0], >subnet_prefix, sizeof(info->subnet_prefix));
> +memcpy([2], >interface_id,
Re: [libvirt] [PATCHv7 09/18] util: Add more interfaces for resctrl monitor
On 10/22/18 4:01 AM, Wang Huaqiang wrote:
> Add interfaces monitor group to support operations such
> as add PID, set ID, remove group ... etc.
>
> Signed-off-by: Wang Huaqiang
> ---
> src/libvirt_private.syms | 5 +
> src/util/virresctrl.c| 47 +++
> src/util/virresctrl.h| 14 ++
> 3 files changed, 66 insertions(+)
>
[...]
> +int
> +virResctrlMonitorRemove(virResctrlMonitorPtr monitor)
> +{
> +int ret = 0;
> +
> +if (!monitor->path)
> +return 0;
Similar to patch1 - if we are using a default path, then we don't want
to removed even if it exists, so I *think* (but you need to confirm for
me) that the following should be done:
if (STREQ(monitor->path, monitor->alloc->path))
return 0;
Although I wonder if a !monitor->alloc guard should be used as well.
Whether it's part of a || !monitor->path return 0 or this check should
be "if (monitor->alloc && STREQ(...))"... Thoughts?
> +
> +VIR_DEBUG("Removing resctrl monitor%s", monitor->path);
s/monitor%s/monitor path='%s'
> +if (rmdir(monitor->path) != 0 && errno != ENOENT) {
> +ret = -errno;
> +VIR_ERROR(_("Unable to remove %s (%d)"), monitor->path, errno);
> +}
> +
> +return ret;
> +}
I can make the changes - just let me know your preferred way to proceed...
Reviewed-by: John Ferlan
John
[...]
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv7 06/18] util: Add interface for adding PID to the monitor
On 10/22/18 4:01 AM, Wang Huaqiang wrote: > Add interface for adding task PID to the monitor. > > Signed-off-by: Wang Huaqiang > --- > src/libvirt_private.syms | 1 + > src/util/virresctrl.c| 8 > src/util/virresctrl.h| 4 > 3 files changed, 13 insertions(+) > Reviewed-by: John Ferlan John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv7 08/18] util: Add interface for creating monitor group
On 10/22/18 4:01 AM, Wang Huaqiang wrote: > Add interface for creating the resource monitoring group according > to '@virResctrlMonitor->path'. > > Signed-off-by: Wang Huaqiang > --- > src/libvirt_private.syms | 1 + > src/util/virresctrl.c| 24 > src/util/virresctrl.h| 4 > 3 files changed, 29 insertions(+) > Reviewed-by: John Ferlan John -- 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
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 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv7 07/18] util: Refactor code for creating resctrl group
On 10/22/18 4:01 AM, Wang Huaqiang wrote: > 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 > --- > src/util/virresctrl.c | 37 +++-- > 1 file changed, 23 insertions(+), 14 deletions(-) > Reviewed-by: John Ferlan John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv7 02/18] util: Introduce resctrl monitor for CMT
On 10/22/18 4:01 AM, Wang Huaqiang wrote:
> 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| 79
>
> src/util/virresctrl.h| 9 ++
> 3 files changed, 83 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
[...]
> @@ -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
>From a
> + * bandwidth technology (MBM), as well as the CAT and MBA, are all orthogonal
> + * features. The monitor will be created under the scope of default resctl
*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,29 @@ 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;
> +
> +/* In resctrl, each monitor is associated with one specific allocation,
Each ResctrlMonitor is associated...
> + * either the allocation under / sys / fs / resctrl or allocation of the
either the root directory allocation /sys/fs/resctrl or a specific
allocation defined under the root directory.
With these simple changes that I can make for you,
Reviewed-by: John Ferlan
John
[...]
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv7 01/18] docs, util: Refactor schemas and virresctrl to support optional cache
On 10/22/18 4:01 AM, Wang Huaqiang wrote:
> 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 | 28
> 3 files changed, 32 insertions(+), 4 deletions(-)
>
[...]
> +/* 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;
> +}
> +
Because of ...
> if (!alloc->path &&
> virAsprintf(>path, "%s/%s-%s",
> SYSFS_RESCTRL_PATH, machinename, alloc->id) < 0)
[...]
> @@ -2334,6 +2358,10 @@ virResctrlAllocRemove(virResctrlAllocPtr alloc)
> {
> int ret = 0;
>
> +/* No directory have ever been created. Just return */
> +if (virResctrlAllocIsEmpty(alloc))
> +return 0;
... the change to virResctrlAllocDeterminePath to fill in alloc->path
when virResctrlAllocIsEmpty to be a default path, this should be:
if (STREQ_NULLABLE(alloc->path, SYSFS_RESCTRL_PATH))
return 0;
or moved after the next check and the _NULLABLE removed.
Whether the AllocIsEmpty is true or not shouldn't be the bearing on
whether the directory created because of that
> +
> if (!alloc->path)
> return 0;
>
>
I can adjust for you, let me know; otherwise, things are fine for the
Reviewed-by: John Ferlan
John
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv7 04/18] util: Add interface to determine monitor path
On 10/22/18 4:01 AM, Wang Huaqiang wrote: > Add interface for resctrl monitor to determine the path. > > Signed-off-by: Wang Huaqiang > --- > src/libvirt_private.syms | 1 + > src/util/virresctrl.c| 55 > > src/util/virresctrl.h| 5 - > 3 files changed, 60 insertions(+), 1 deletion(-) > Reviewed-by: John Ferlan John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv7 03/18] util: Refactor code for determining allocation path
On 10/22/18 4:01 AM, Wang Huaqiang wrote:
> 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 6530801..956aca8 100644
> --- a/src/util/virresctrl.c
> +++ b/src/util/virresctrl.c
> @@ -2265,28 +2265,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'"), parentpath);
Add "for prefix='%s'" w/ prefix as argument especially since for Alloc's
the parent path is SYSFS_RESCTRL_PATH so it's perhaps not specific enough.
With this change that I can make for you,
Reviewed-by: John Ferlan
John
[...]
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] qemu: Don't use -mem-prealloc among with .prealloc=yes
https://bugzilla.redhat.com/show_bug.cgi?id=1624223
There are two ways to request memory preallocation on cmd line:
-mem-prealloc and .prealloc attribute to memory-backend-file.
However, as it turns out it's not safe to use both at the same
time. Prefer -mem-prealloc as it is more backward compatible
compared to switching to "-numa node,memdev= + -object
memory-backend-file".
Signed-off-by: Michal Privoznik
---
src/qemu/qemu_command.c | 37 +--
src/qemu/qemu_command.h | 1 +
src/qemu/qemu_domain.c| 2 +
src/qemu/qemu_domain.h| 3 ++
src/qemu/qemu_hotplug.c | 3 +-
.../hugepages-numa-default-dimm.args | 2 +-
6 files changed, 35 insertions(+), 13 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index e338d3172e..0294030f0e 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -3123,6 +3123,7 @@ qemuBuildControllerDevCommandLine(virCommandPtr cmd,
* @def: domain definition object
* @mem: memory definition object
* @autoNodeset: fallback nodeset in case of automatic NUMA placement
+ * @forbidPrealloc: don't set prealloc attribute
* @force: forcibly use one of the backends
*
* Creates a configuration object that represents memory backend of given guest
@@ -3136,6 +3137,9 @@ qemuBuildControllerDevCommandLine(virCommandPtr cmd,
* Then, if one of the two memory-backend-* should be used, the @qemuCaps is
* consulted to check if qemu does support it.
*
+ * If @forbidPrealloc is true then 'prealloc' attribute of the backend is not
+ * set. This may come handy when global -mem-prealloc is already specified.
+ *
* Returns: 0 on success,
* 1 on success and if there's no need to use memory-backend-*
* -1 on error.
@@ -3148,6 +3152,7 @@ qemuBuildMemoryBackendProps(virJSONValuePtr *backendProps,
virDomainDefPtr def,
virDomainMemoryDefPtr mem,
virBitmapPtr autoNodeset,
+bool forbidPrealloc,
bool force)
{
const char *backendType = "memory-backend-file";
@@ -3265,11 +3270,13 @@ qemuBuildMemoryBackendProps(virJSONValuePtr
*backendProps,
if (mem->nvdimmPath) {
if (VIR_STRDUP(memPath, mem->nvdimmPath) < 0)
goto cleanup;
-prealloc = true;
+if (!forbidPrealloc)
+prealloc = true;
} else if (useHugepage) {
if (qemuGetDomainHupageMemPath(def, cfg, pagesize, ) < 0)
goto cleanup;
-prealloc = true;
+if (!forbidPrealloc)
+prealloc = true;
} else {
/* We can have both pagesize and mem source. If that's the case,
* prefer hugepages as those are more specific. */
@@ -3398,7 +3405,8 @@ qemuBuildMemoryCellBackendStr(virDomainDefPtr def,
mem.info.alias = alias;
if ((rc = qemuBuildMemoryBackendProps(, alias, cfg, priv->qemuCaps,
- def, , priv->autoNodeset,
false)) < 0)
+ def, , priv->autoNodeset,
+ priv->memPrealloc, false)) < 0)
goto cleanup;
if (virQEMUBuildObjectCommandlineFromJSON(buf, props) < 0)
@@ -3435,7 +3443,8 @@ qemuBuildMemoryDimmBackendStr(virBufferPtr buf,
goto cleanup;
if (qemuBuildMemoryBackendProps(, alias, cfg, priv->qemuCaps,
-def, mem, priv->autoNodeset, true) < 0)
+def, mem, priv->autoNodeset,
+priv->memPrealloc, true) < 0)
goto cleanup;
if (virQEMUBuildObjectCommandlineFromJSON(buf, props) < 0)
@@ -7443,7 +7452,8 @@ qemuBuildSmpCommandLine(virCommandPtr cmd,
static int
qemuBuildMemPathStr(virQEMUDriverConfigPtr cfg,
const virDomainDef *def,
-virCommandPtr cmd)
+virCommandPtr cmd,
+qemuDomainObjPrivatePtr priv)
{
const long system_page_size = virGetSystemPageSizeKB();
char *mem_path = NULL;
@@ -7465,8 +7475,10 @@ qemuBuildMemPathStr(virQEMUDriverConfigPtr cfg,
return 0;
}
-if (def->mem.allocation != VIR_DOMAIN_MEMORY_ALLOCATION_IMMEDIATE)
+if (def->mem.allocation != VIR_DOMAIN_MEMORY_ALLOCATION_IMMEDIATE) {
virCommandAddArgList(cmd, "-mem-prealloc", NULL);
+priv->memPrealloc = true;
+}
virCommandAddArgList(cmd, "-mem-path", mem_path, NULL);
VIR_FREE(mem_path);
@@ -7479,7 +7491,8 @@ static int
qemuBuildMemCommandLine(virCommandPtr cmd,
virQEMUDriverConfigPtr cfg,
const virDomainDef *def,
-virQEMUCapsPtr qemuCaps)
+
Re: [libvirt] [PATCH v3 6/6] monitor: deprecate acl_show, acl_reset, acl_policy, acl_add, acl_remove
Daniel P. Berrangé wrote: > The various ACL related commands are obsolete now that the QAuthZ > framework for authorization is fully integrated throughout QEMU network > services. Mark it as deprecated with no replacement to be provided. > > Authorization is now provided by using 'object_add' together with > the 'tls-authz' or 'sasl-authz' parameters to the VNC server, and > equivalent for other network services. > > Signed-off-by: Daniel P. Berrangé Reviewed-by: Juan Quintela -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 5/6] vnc: allow specifying a custom authorization object name
Daniel P. Berrangé wrote: > From: "Daniel P. Berrange" > > The VNC server has historically had support for ACLs to check both the > SASL username and the TLS x509 distinguished name. The VNC server was > responsible for creating the initial ACL, and the client app was then > responsible for populating it with rules using the HMP 'acl_add' command. > > This is not satisfactory for a variety of reasons. There is no way to > populate the ACLs from the command line, users are forced to use the > HMP. With multiple network services all supporting TLS and ACLs now, it > is desirable to be able to define a single ACL that is referenced by all > services. > > To address these limitations, two new options are added to the VNC > server CLI. The 'tls-authz' option takes the ID of a QAuthZ object to > use for checking TLS x509 distinguished names, and the 'sasl-authz' > option takes the ID of another object to use for checking SASL usernames. > > In this example, we setup two authorization rules. The first allows any > client with a certificate issued by the 'RedHat' organization in the > 'London' locality. The second ACL allows clients with either the > 'j...@redhat.com' or 'f...@redhat.com' kerberos usernames. Both checks > must pass for the user to be allowed. > > $QEMU -object tls-creds-x509,id=tls0,dir=/home/berrange/qemutls,\ > endpoint=server,verify-peer=yes \ > -object authz-simple,id=authz0,policy=deny,\ > rules.0.match=O=RedHat,,L=London,rules.0.policy=allow \ > -object authz-simple,id=authz1,policy=deny,\ > rules.0.match=f...@redhat.com,rules.0.policy=allow \ > rules.0.match=j...@redhat.com,rules.0.policy=allow \ > -vnc 0.0.0.0:1,tls-creds=tls0,tls-authz=authz0, > sasl,sasl-authz=authz1 \ > ...other QEMU args... > > Signed-off-by: Daniel P. Berrange Reviewed-by: Juan Quintela -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] RFC: VIR_DOMAIN_SNAPSHOT_LIST_NO_METADATA is not functional?
On Fri, Nov 02, 2018 at 15:52:54 +0800, Han Han wrote: > Hello, > I found snapshot APIs(like virDomainSnapshotNum) invoked with > VIR_DOMAIN_SNAPSHOT_LIST_NO_METADATA will return 0 even there are internal > no-metadata snapshots in the domain. We've never implemented the support for loading internal snapshots without libvirt metadata, thus none will be listed. > > Then I find the reason is in virDomainSnapshotObjListGetNames(): >937 /* If this common code is being used, we assume that all > snapshots >938 ┆* have metadata, and thus can handle METADATA up front as > an >939 ┆* all-or-none filter. XXX This might not always be true, if > we >940 ┆* add the ability to track qcow2 internal snapshots without > the >941 ┆* use of metadata. > */ >942 if ((data.flags & VIR_DOMAIN_SNAPSHOT_FILTERS_METADATA) > == >943 ┆ > VIR_DOMAIN_SNAPSHOT_LIST_NO_METADATA) >944 ┆ return 0; > > So currently, with VIR_DOMAIN_SNAPSHOT_LIST_NO_METADATA, no snapshot will > be returned. > > I checked the commit. It is really old(6478ec1673aEric Blake > 2012-08-13 18:09:12 -0600) . I guess it was initially designed for the > function to list internal snapshots without metadata. > > However, now internal snapshot seems lower prioritized than external > snapshot. > Do we have plan to design this function? Since the APIs invoked with this > flag don't return the **right** result, if the function for > VIR_DOMAIN_SNAPSHOT_LIST_NO_METADATA will not be designed, how will we deal > with the flag? Remove it from code OR update the docs to mark it > unsupported? We cannot remove that flag. It will return no results until somebody actually implements the support for snapshots with no libvirt metadata. Also note that for other hypervisor drivers there certainly is more possibility for modelling that as well so removing the flag does not make sense. signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v7 00/14] PCI passthrough support on s390
On Fri, 2018-10-19 at 11:40 +0800, Yi Min Zhao wrote: > Abstract > > The PCI representation in QEMU has been extended for S390 > allowing configuration of zPCI attributes like uid (user-defined > identifier) and fid (PCI function identifier). > The details can be found here: > https://lists.gnu.org/archive/html/qemu-devel/2016-06/msg07262.html > > To support the new zPCI feature of the S390 platform, a new element of > PCI address is introduced. It has two optional attributes, @uid and > @fid. For example: > > > > > > > > > > > If they are defined by the user, unique values within the guest domain > must be used. If they are not specified and the architecture requires > them, they are automatically generated with non-conflicting values. > > zPCI address as an extension of the PCI address are stored in a new > structure 'virZPCIDeviceAddress' which is a member of common PCI > Address structure. Additionally, two hashtables are used for assignment > and reservation of zPCI uid/fid. > > In support of extending the PCI address, a new PCI address extension flag is > introduced. This extension flag allows is not only dedicated for the S390 > platform but also other architectures needing certain extensions to PCI > address space. So, I was able to sit down with Laine during KVM Forum and we looked over the entire series together. He didn't spot any issues with it except for the couple minor ones that I pointed out separately in response to the corresponding patches; as for myself, I've already provided R-bs for all patches and I have no further objections to this being merged. I also asked Dan to weigh in explicitly on the list regarding his concerns about the XML design, and he promised he'd do that... I think you can post a v8 which addresses the small issues mentioned above (and rebased on top of current master) in the meantime. -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v7 14/14] news: Update news for PCI address extension attributes
On Fri, 2018-10-19 at 11:40 +0800, Yi Min Zhao wrote: [...] > > + > + > + qemu: Added support for PCI device on S390 > + s/device/devices/ -- Andrea Bolognani / Red Hat / Virtualization -- 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)
On 11/05/2018 01:58 PM, 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
For the feature itself consider the patch series
Acked-by: Christian Borntraeger
I always wanted this kind of control.
>
> 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 +
>
Re: [libvirt] [PATCH v7 13/14] docs: Add 'uid' and 'fid' information
On Fri, 2018-10-19 at 11:40 +0800, Yi Min Zhao wrote: [...] > @@ -3925,7 +3925,15 @@ > (since 0.9.7, requires QEMU > 0.13). multifunction defaults to 'off', > but should be set to 'on' for function 0 of a slot that will > -have multiple functions used. > +have multiple functions used. > +(Since 4.9.0), PCI address extensions Since 4.10.0 now ;) -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v7 08/14] conf: Introduce parser, formatter for uid and fid
On Fri, 2018-10-19 at 11:40 +0800, Yi Min Zhao wrote: > This patch introduces new XML parser/formatter functions. Uid is > 16-bit and non-zero. Fid is 32-bit. They are the two attributes of zpci > which is introduced as PCI address element. Zpci element is parsed and > formatted along with PCI address. And add the related test cases. > > Signed-off-by: Yi Min Zhao > Reviewed-by: Boris Fiuczynski > Reviewed-by: Stefan Zimmermann > Reviewed-by: Bjoern Walk > Reviewed-by: Ján Tomko > --- > docs/schemas/basictypes.rng | 27 ++ > docs/schemas/domaincommon.rng | 1 + > src/conf/device_conf.c| 53 +++ > src/conf/domain_addr.c| 3 ++ > src/conf/domain_conf.c| 12 - > src/libvirt_private.syms | 2 + > src/util/virpci.c | 26 + > src/util/virpci.h | 6 +++ > .../disk-virtio-s390-zpci.args| 25 + > .../disk-virtio-s390-zpci.xml | 19 +++ > tests/qemuxml2argvdata/hostdev-vfio-zpci.args | 23 > tests/qemuxml2argvdata/hostdev-vfio-zpci.xml | 21 > tests/qemuxml2argvtest.c | 7 +++ > .../disk-virtio-s390-zpci.xml | 31 +++ > .../qemuxml2xmloutdata/hostdev-vfio-zpci.xml | 32 +++ > tests/qemuxml2xmltest.c | 6 +++ > 16 files changed, 293 insertions(+), 1 deletion(-) > create mode 100644 tests/qemuxml2argvdata/disk-virtio-s390-zpci.args > create mode 100644 tests/qemuxml2argvdata/disk-virtio-s390-zpci.xml > create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci.args > create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci.xml > create mode 100644 tests/qemuxml2xmloutdata/disk-virtio-s390-zpci.xml > create mode 100644 tests/qemuxml2xmloutdata/hostdev-vfio-zpci.xml Reviewed-by: Andrea Bolognani -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v7 07/14] conf: use virXMLFormatElement() in virDomainDeviceInfoFormat()
On Fri, 2018-10-19 at 11:40 +0800, Yi Min Zhao wrote: > In order to add zPCI child element for PCI address, we update > virDomainDeviceInfoFormat() to format device info by helper function > virXMLFormatElement(). Then we could simply format zPCI address into > child buffer later. > > Signed-off-by: Yi Min Zhao > Reviewed-by: Boris Fiuczynski > --- > src/conf/domain_conf.c | 40 ++-- > 1 file changed, 22 insertions(+), 18 deletions(-) Reviewed-by: Andrea Bolognani -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [REPOST PATCH v2 10/12] qemu: Introduce qemuDomainSetIOThreadParams
https://bugzilla.redhat.com/show_bug.cgi?id=1545732
Implement the QEMU driver mechanism in order to set the polling
parameters for an IOThread within the bounds specified by the
QEMU qapi parameter passing.
Based heavily on patches originally posted by Pavel Hrdina
, but modified to only handle alterations
for a running guest. For the most part the API names changed,
the typed parameters removed the poll enabled value, and the
capabilities check was moved to just before the live attempt
to set. Since changes are only supported for a running guest,
no guest XML alterations were kept.
Signed-off-by: John Ferlan
---
src/qemu/qemu_driver.c | 201 +
1 file changed, 201 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 4ddca2f765..61b1ce717c 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -5902,6 +5902,35 @@ qemuDomainHotplugAddIOThread(virQEMUDriverPtr driver,
goto cleanup;
}
+
+static int
+qemuDomainHotplugModIOThread(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ qemuMonitorIOThreadInfo iothread)
+{
+qemuDomainObjPrivatePtr priv = vm->privateData;
+int rc;
+
+if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_IOTHREAD_POLLING)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("IOThreads polling is not supported for this QEMU"));
+return -1;
+}
+
+qemuDomainObjEnterMonitor(driver, vm);
+
+rc = qemuMonitorSetIOThread(priv->mon, );
+
+if (qemuDomainObjExitMonitor(driver, vm) < 0)
+return -1;
+
+if (rc < 0)
+return -1;
+
+return 0;
+}
+
+
static int
qemuDomainHotplugDelIOThread(virQEMUDriverPtr driver,
virDomainObjPtr vm,
@@ -6018,9 +6047,106 @@ qemuDomainDelIOThreadCheck(virDomainDefPtr def,
return 0;
}
+
+/**
+ * @params: Pointer to params list
+ * @nparams: Number of params to be parsed
+ * @iothread: Buffer to store the values
+ *
+ * The following is a description of each value parsed:
+ *
+ * - "poll-max-ns" for each IOThread is the maximum time in nanoseconds
+ *to allow each polling interval to occur. A polling interval is a
+ *period of time allowed for a thread to process data before it returns
+ *the CPU quantum back to the host. A value set too small will not allow
+ *the IOThread to run long enough on a CPU to process data. A value set
+ *too high will consume too much CPU time per IOThread failing to allow
+ *other threads running on the CPU to get time. A value of 0 (zero) will
+ *disable the polling.
+ *
+ * - "poll-grow" - factor to grow the current polling time when deemed
+ * necessary. If a 0 (zero) value is provided, QEMU currently doubles
+ * its polling interval unless the current value is greater than the
+ * poll-max-ns.
+ *
+ * - "poll-shrink" - divisor to reduced the current polling time when deemed
+ * necessary. If a 0 (zero) value is provided, QEMU resets the polling
+ * interval to 0 (zero) allowing the poll-grow to manipulate the time.
+ *
+ * QEMU keeps track of the polling time elapsed and may grow or shrink the
+ * its polling interval based upon its heuristic algorithm. It is possible
+ * that calculations determine that it has found a "sweet spot" and no
+ * ajustments are made. The polling time value is not available.
+ *
+ * Returns 0 on success, -1 on failure with error set.
+ */
+static int
+qemuDomainIOThreadParseParams(virTypedParameterPtr params,
+ int nparams,
+ qemuMonitorIOThreadInfoPtr iothread)
+{
+int rc;
+
+if (virTypedParamsValidate(params, nparams,
+ VIR_DOMAIN_IOTHREAD_POLL_MAX_NS,
+ VIR_TYPED_PARAM_ULLONG,
+ VIR_DOMAIN_IOTHREAD_POLL_GROW,
+ VIR_TYPED_PARAM_UINT,
+ VIR_DOMAIN_IOTHREAD_POLL_SHRINK,
+ VIR_TYPED_PARAM_UINT,
+ NULL) < 0)
+return -1;
+
+if ((rc = virTypedParamsGetULLong(params, nparams,
+ VIR_DOMAIN_IOTHREAD_POLL_MAX_NS,
+ >poll_max_ns)) < 0)
+return -1;
+if (rc == 1)
+iothread->set_poll_max_ns = true;
+
+if ((rc = virTypedParamsGetUInt(params, nparams,
+VIR_DOMAIN_IOTHREAD_POLL_GROW,
+>poll_grow)) < 0)
+return -1;
+if (rc == 1)
+iothread->set_poll_grow = true;
+
+if ((rc = virTypedParamsGetUInt(params, nparams,
+VIR_DOMAIN_IOTHREAD_POLL_SHRINK,
+>poll_shrink)) < 0)
+return -1;
+if (rc == 1)
+iothread->set_poll_shrink = true;
+
+if
[libvirt] [REPOST PATCH v2 08/12] qemu: Alter qemuDomainChgIOThread to take qemuMonitorIOThreadInfo
Rather than passing an iothread_id, let's pass a qemuMonitorIOThreadInfo
structure so that a subsequent change to modify the iothread info can
just generate and pass one.
Signed-off-by: John Ferlan
ACKed-by: Michal Privoznik
---
src/qemu/qemu_driver.c | 26 +++---
1 file changed, 15 insertions(+), 11 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 6835d3e875..4ddca2f765 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -6026,7 +6026,7 @@ typedef enum {
static int
qemuDomainChgIOThread(virQEMUDriverPtr driver,
virDomainObjPtr vm,
- unsigned int iothread_id,
+ qemuMonitorIOThreadInfo iothread,
virDomainIOThreadAction action,
unsigned int flags)
{
@@ -6055,19 +6055,19 @@ qemuDomainChgIOThread(virQEMUDriverPtr driver,
switch (action) {
case VIR_DOMAIN_IOTHREAD_ACTION_ADD:
-if (qemuDomainAddIOThreadCheck(def, iothread_id) < 0)
+if (qemuDomainAddIOThreadCheck(def, iothread.iothread_id) < 0)
goto endjob;
-if (qemuDomainHotplugAddIOThread(driver, vm, iothread_id) < 0)
+if (qemuDomainHotplugAddIOThread(driver, vm, iothread.iothread_id)
< 0)
goto endjob;
break;
case VIR_DOMAIN_IOTHREAD_ACTION_DEL:
-if (qemuDomainDelIOThreadCheck(def, iothread_id) < 0)
+if (qemuDomainDelIOThreadCheck(def, iothread.iothread_id) < 0)
goto endjob;
-if (qemuDomainHotplugDelIOThread(driver, vm, iothread_id) < 0)
+if (qemuDomainHotplugDelIOThread(driver, vm, iothread.iothread_id)
< 0)
goto endjob;
break;
@@ -6080,19 +6080,19 @@ qemuDomainChgIOThread(virQEMUDriverPtr driver,
if (persistentDef) {
switch (action) {
case VIR_DOMAIN_IOTHREAD_ACTION_ADD:
-if (qemuDomainAddIOThreadCheck(persistentDef, iothread_id) < 0)
+if (qemuDomainAddIOThreadCheck(persistentDef,
iothread.iothread_id) < 0)
goto endjob;
-if (!virDomainIOThreadIDAdd(persistentDef, iothread_id))
+if (!virDomainIOThreadIDAdd(persistentDef, iothread.iothread_id))
goto endjob;
break;
case VIR_DOMAIN_IOTHREAD_ACTION_DEL:
-if (qemuDomainDelIOThreadCheck(persistentDef, iothread_id) < 0)
+if (qemuDomainDelIOThreadCheck(persistentDef,
iothread.iothread_id) < 0)
goto endjob;
-virDomainIOThreadIDDel(persistentDef, iothread_id);
+virDomainIOThreadIDDel(persistentDef, iothread.iothread_id);
break;
}
@@ -6119,6 +6119,7 @@ qemuDomainAddIOThread(virDomainPtr dom,
{
virQEMUDriverPtr driver = dom->conn->privateData;
virDomainObjPtr vm = NULL;
+qemuMonitorIOThreadInfo iothread = {0};
int ret = -1;
virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
@@ -6136,7 +6137,8 @@ qemuDomainAddIOThread(virDomainPtr dom,
if (virDomainAddIOThreadEnsureACL(dom->conn, vm->def, flags) < 0)
goto cleanup;
-ret = qemuDomainChgIOThread(driver, vm, iothread_id,
+iothread.iothread_id = iothread_id;
+ret = qemuDomainChgIOThread(driver, vm, iothread,
VIR_DOMAIN_IOTHREAD_ACTION_ADD, flags);
cleanup:
@@ -6152,6 +6154,7 @@ qemuDomainDelIOThread(virDomainPtr dom,
{
virQEMUDriverPtr driver = dom->conn->privateData;
virDomainObjPtr vm = NULL;
+qemuMonitorIOThreadInfo iothread = {0};
int ret = -1;
virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
@@ -6169,7 +6172,8 @@ qemuDomainDelIOThread(virDomainPtr dom,
if (virDomainDelIOThreadEnsureACL(dom->conn, vm->def, flags) < 0)
goto cleanup;
-ret = qemuDomainChgIOThread(driver, vm, iothread_id,
+iothread.iothread_id = iothread_id;
+ret = qemuDomainChgIOThread(driver, vm, iothread,
VIR_DOMAIN_IOTHREAD_ACTION_DEL, flags);
cleanup:
--
2.17.2
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [REPOST PATCH v2 07/12] qemu: Alter qemuDomainChgIOThread to take enum instead of bool
We're about to add a new state "modify" and thus the function
goes from just Add/Del. Use an enum to manage.
Extracted from code originally posted by Pavel Hrdina
, but placed into a separate patch.
Signed-off-by: John Ferlan
ACKed-by: Michal Privoznik
---
src/qemu/qemu_driver.c | 32 +---
1 file changed, 25 insertions(+), 7 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index b50d805bf1..6835d3e875 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -6018,11 +6018,16 @@ qemuDomainDelIOThreadCheck(virDomainDefPtr def,
return 0;
}
+typedef enum {
+VIR_DOMAIN_IOTHREAD_ACTION_ADD,
+VIR_DOMAIN_IOTHREAD_ACTION_DEL,
+} virDomainIOThreadAction;
+
static int
qemuDomainChgIOThread(virQEMUDriverPtr driver,
virDomainObjPtr vm,
unsigned int iothread_id,
- bool add,
+ virDomainIOThreadAction action,
unsigned int flags)
{
virQEMUDriverConfigPtr cfg = NULL;
@@ -6048,18 +6053,24 @@ qemuDomainChgIOThread(virQEMUDriverPtr driver,
goto endjob;
}
-if (add) {
+switch (action) {
+case VIR_DOMAIN_IOTHREAD_ACTION_ADD:
if (qemuDomainAddIOThreadCheck(def, iothread_id) < 0)
goto endjob;
if (qemuDomainHotplugAddIOThread(driver, vm, iothread_id) < 0)
goto endjob;
-} else {
+
+break;
+
+case VIR_DOMAIN_IOTHREAD_ACTION_DEL:
if (qemuDomainDelIOThreadCheck(def, iothread_id) < 0)
goto endjob;
if (qemuDomainHotplugDelIOThread(driver, vm, iothread_id) < 0)
goto endjob;
+
+break;
}
if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm,
driver->caps) < 0)
@@ -6067,18 +6078,23 @@ qemuDomainChgIOThread(virQEMUDriverPtr driver,
}
if (persistentDef) {
-if (add) {
+switch (action) {
+case VIR_DOMAIN_IOTHREAD_ACTION_ADD:
if (qemuDomainAddIOThreadCheck(persistentDef, iothread_id) < 0)
goto endjob;
if (!virDomainIOThreadIDAdd(persistentDef, iothread_id))
goto endjob;
-} else {
+break;
+
+case VIR_DOMAIN_IOTHREAD_ACTION_DEL:
if (qemuDomainDelIOThreadCheck(persistentDef, iothread_id) < 0)
goto endjob;
virDomainIOThreadIDDel(persistentDef, iothread_id);
+
+break;
}
if (virDomainSaveConfig(cfg->configDir, driver->caps,
@@ -6120,7 +6136,8 @@ qemuDomainAddIOThread(virDomainPtr dom,
if (virDomainAddIOThreadEnsureACL(dom->conn, vm->def, flags) < 0)
goto cleanup;
-ret = qemuDomainChgIOThread(driver, vm, iothread_id, true, flags);
+ret = qemuDomainChgIOThread(driver, vm, iothread_id,
+VIR_DOMAIN_IOTHREAD_ACTION_ADD, flags);
cleanup:
virDomainObjEndAPI();
@@ -6152,7 +6169,8 @@ qemuDomainDelIOThread(virDomainPtr dom,
if (virDomainDelIOThreadEnsureACL(dom->conn, vm->def, flags) < 0)
goto cleanup;
-ret = qemuDomainChgIOThread(driver, vm, iothread_id, false, flags);
+ret = qemuDomainChgIOThread(driver, vm, iothread_id,
+VIR_DOMAIN_IOTHREAD_ACTION_DEL, flags);
cleanup:
virDomainObjEndAPI();
--
2.17.2
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [REPOST PATCH v2 00/12] Allow modification of IOThread polling values (redux)
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 +-
src/remote_protocol-structs | 10 +
.../caps_2.10.0.aarch64.xml | 1 +
.../caps_2.10.0.ppc64.xml | 1 +
.../caps_2.10.0.s390x.xml | 1 +
.../caps_2.10.0.x86_64.xml
[libvirt] [REPOST PATCH v2 06/12] qemu: Add monitor functions to set IOThread params
Add functions to set the IOThreadInfo param data for the live guest.
Modify the _qemuMonitorIOThreadInfo to have a flag to indicate when
a value was set so that we don't set a value unless it was desired
to be set.
Based on code originally posted by Pavel Hrdina ,
but extracted into a separate patch. Note that qapi expects to receive
integer parameters rather than unsigned long long or unsigned int's.
QEMU does save the value in larger signed 64 bit values eventually.
Signed-off-by: John Ferlan
---
src/qemu/qemu_monitor.c | 19 +++
src/qemu/qemu_monitor.h | 5 +
src/qemu/qemu_monitor_json.c | 35 +++
src/qemu/qemu_monitor_json.h | 4
4 files changed, 63 insertions(+)
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index 7f7013e115..a65d638ab8 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -4135,6 +4135,25 @@ qemuMonitorGetIOThreads(qemuMonitorPtr mon,
}
+/**
+ * qemuMonitorSetIOThread:
+ * @mon: Pointer to the monitor
+ * @iothreadInfo: filled IOThread info with data
+ *
+ * Alter the specified IOThread's IOThreadInfo values.
+ */
+int
+qemuMonitorSetIOThread(qemuMonitorPtr mon,
+ qemuMonitorIOThreadInfoPtr iothreadInfo)
+{
+VIR_DEBUG("iothread=%p", iothreadInfo);
+
+QEMU_CHECK_MONITOR(mon);
+
+return qemuMonitorJSONSetIOThread(mon, iothreadInfo);
+}
+
+
/**
* qemuMonitorGetMemoryDeviceInfo:
* @mon: pointer to the monitor
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
index c2991e2b16..66bfdb0e5c 100644
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -1120,9 +1120,14 @@ struct _qemuMonitorIOThreadInfo {
unsigned long long poll_max_ns;
unsigned int poll_grow;
unsigned int poll_shrink;
+bool set_poll_max_ns;
+bool set_poll_grow;
+bool set_poll_shrink;
};
int qemuMonitorGetIOThreads(qemuMonitorPtr mon,
qemuMonitorIOThreadInfoPtr **iothreads);
+int qemuMonitorSetIOThread(qemuMonitorPtr mon,
+ qemuMonitorIOThreadInfoPtr iothreadInfo);
typedef struct _qemuMonitorMemoryDeviceInfo qemuMonitorMemoryDeviceInfo;
typedef qemuMonitorMemoryDeviceInfo *qemuMonitorMemoryDeviceInfoPtr;
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 2e92984b44..5a806f6c0e 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -7474,6 +7474,41 @@ qemuMonitorJSONGetIOThreads(qemuMonitorPtr mon,
}
+int
+qemuMonitorJSONSetIOThread(qemuMonitorPtr mon,
+ qemuMonitorIOThreadInfoPtr iothreadInfo)
+{
+int ret = -1;
+char *path = NULL;
+qemuMonitorJSONObjectProperty prop;
+
+if (virAsprintf(, "/objects/iothread%u",
+iothreadInfo->iothread_id) < 0)
+goto cleanup;
+
+#define VIR_IOTHREAD_SET_PROP(propName, propVal) \
+if (iothreadInfo->set_##propVal) { \
+memset(, 0, sizeof(qemuMonitorJSONObjectProperty)); \
+prop.type = QEMU_MONITOR_OBJECT_PROPERTY_INT; \
+prop.val.iv = iothreadInfo->propVal; \
+if (qemuMonitorJSONSetObjectProperty(mon, path, propName, ) < 0) \
+goto cleanup; \
+}
+
+VIR_IOTHREAD_SET_PROP("poll-max-ns", poll_max_ns);
+VIR_IOTHREAD_SET_PROP("poll-grow", poll_grow);
+VIR_IOTHREAD_SET_PROP("poll-shrink", poll_shrink);
+
+#undef VIR_IOTHREAD_SET_PROP
+
+ret = 0;
+
+ cleanup:
+VIR_FREE(path);
+return ret;
+}
+
+
int
qemuMonitorJSONGetMemoryDeviceInfo(qemuMonitorPtr mon,
virHashTablePtr info)
diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h
index da267b15b0..c3abd0ddf0 100644
--- a/src/qemu/qemu_monitor_json.h
+++ b/src/qemu/qemu_monitor_json.h
@@ -502,6 +502,10 @@ int qemuMonitorJSONGetIOThreads(qemuMonitorPtr mon,
qemuMonitorIOThreadInfoPtr **iothreads)
ATTRIBUTE_NONNULL(2);
+int qemuMonitorJSONSetIOThread(qemuMonitorPtr mon,
+ qemuMonitorIOThreadInfoPtr iothreadInfo)
+ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
+
int qemuMonitorJSONGetMemoryDeviceInfo(qemuMonitorPtr mon,
virHashTablePtr info)
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
--
2.17.2
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [REPOST PATCH v2 09/12] qemu: Detect whether iothread polling is supported
Add a capability check for IOThread polling (all were added at the
same time, so only one check is necessary).
Based on code originally posted by Pavel Hrdina
with the only changes to include the more recent QEMU releases.
Signed-off-by: John Ferlan
ACKed-by: Michal Privoznik
---
src/qemu/qemu_capabilities.c | 2 ++
src/qemu/qemu_capabilities.h | 1 +
tests/qemucapabilitiesdata/caps_2.10.0.aarch64.xml | 1 +
tests/qemucapabilitiesdata/caps_2.10.0.ppc64.xml | 1 +
tests/qemucapabilitiesdata/caps_2.10.0.s390x.xml | 1 +
tests/qemucapabilitiesdata/caps_2.10.0.x86_64.xml | 1 +
tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml | 1 +
tests/qemucapabilitiesdata/caps_2.11.0.x86_64.xml | 1 +
tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml | 1 +
tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml | 1 +
tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml | 1 +
tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml | 1 +
tests/qemucapabilitiesdata/caps_2.9.0.ppc64.xml| 1 +
tests/qemucapabilitiesdata/caps_2.9.0.s390x.xml| 1 +
tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml | 1 +
tests/qemucapabilitiesdata/caps_3.0.0.ppc64.xml| 1 +
tests/qemucapabilitiesdata/caps_3.0.0.riscv32.xml | 1 +
tests/qemucapabilitiesdata/caps_3.0.0.riscv64.xml | 1 +
tests/qemucapabilitiesdata/caps_3.0.0.s390x.xml| 1 +
tests/qemucapabilitiesdata/caps_3.0.0.x86_64.xml | 1 +
20 files changed, 21 insertions(+)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 2ca5af3297..4fdb0e66a5 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -509,6 +509,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
"vfio-pci.display",
"blockdev",
"vfio-ap",
+ "iothread.poll-max-ns",
);
@@ -1239,6 +1240,7 @@ static struct virQEMUCapsStringFlags
virQEMUCapsQMPSchemaQueries[] = {
{ "nbd-server-start/arg-type/tls-creds", QEMU_CAPS_NBD_TLS },
{ "screendump/arg-type/device", QEMU_CAPS_SCREENDUMP_DEVICE },
{ "block-commit/arg-type/*top", QEMU_CAPS_ACTIVE_COMMIT },
+{ "query-iothreads/ret-type/poll-max-ns", QEMU_CAPS_IOTHREAD_POLLING },
};
typedef struct _virQEMUCapsObjectTypeProps virQEMUCapsObjectTypeProps;
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index 6bb9a2c8f0..f53288bc81 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -493,6 +493,7 @@ typedef enum { /* virQEMUCapsFlags grouping marker for
syntax-check */
QEMU_CAPS_VFIO_PCI_DISPLAY, /* -device vfio-pci.display */
QEMU_CAPS_BLOCKDEV, /* -blockdev and blockdev-add are supported */
QEMU_CAPS_DEVICE_VFIO_AP, /* -device vfio-ap */
+QEMU_CAPS_IOTHREAD_POLLING, /* -object iothread.poll-max-ns */
QEMU_CAPS_LAST /* this must always be the last item */
} virQEMUCapsFlags;
diff --git a/tests/qemucapabilitiesdata/caps_2.10.0.aarch64.xml
b/tests/qemucapabilitiesdata/caps_2.10.0.aarch64.xml
index b9c4182a66..7061ba8f7e 100644
--- a/tests/qemucapabilitiesdata/caps_2.10.0.aarch64.xml
+++ b/tests/qemucapabilitiesdata/caps_2.10.0.aarch64.xml
@@ -151,6 +151,7 @@
+
201
0
305067
diff --git a/tests/qemucapabilitiesdata/caps_2.10.0.ppc64.xml
b/tests/qemucapabilitiesdata/caps_2.10.0.ppc64.xml
index 66b25601e7..2a48b63efe 100644
--- a/tests/qemucapabilitiesdata/caps_2.10.0.ppc64.xml
+++ b/tests/qemucapabilitiesdata/caps_2.10.0.ppc64.xml
@@ -150,6 +150,7 @@
+
201
0
384412
diff --git a/tests/qemucapabilitiesdata/caps_2.10.0.s390x.xml
b/tests/qemucapabilitiesdata/caps_2.10.0.s390x.xml
index e000aac384..c35e014b32 100644
--- a/tests/qemucapabilitiesdata/caps_2.10.0.s390x.xml
+++ b/tests/qemucapabilitiesdata/caps_2.10.0.s390x.xml
@@ -113,6 +113,7 @@
+
201
0
306247
diff --git a/tests/qemucapabilitiesdata/caps_2.10.0.x86_64.xml
b/tests/qemucapabilitiesdata/caps_2.10.0.x86_64.xml
index ebc5e771d9..a8d787f99a 100644
--- a/tests/qemucapabilitiesdata/caps_2.10.0.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_2.10.0.x86_64.xml
@@ -192,6 +192,7 @@
+
201
0
364386
diff --git a/tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml
b/tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml
index 4eb8a39d94..6ee53a1f21 100644
--- a/tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml
+++ b/tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml
@@ -120,6 +120,7 @@
+
2011000
0
345099
diff --git a/tests/qemucapabilitiesdata/caps_2.11.0.x86_64.xml
b/tests/qemucapabilitiesdata/caps_2.11.0.x86_64.xml
index 857a9a9f9a..4ba2a82b60 100644
--- a/tests/qemucapabilitiesdata/caps_2.11.0.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_2.11.0.x86_64.xml
@@ -198,6 +198,7 @@
+
2011000
0
368875
diff --git a/tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml
b/tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml
index
[libvirt] [REPOST PATCH v2 04/12] virsh: Add ability to display IOThread stats
Add an --iothread qualifier to domstats and an explanation in
the man page. Describe the values in as generic terms as possible
allowing each hypervisor to provide a specific algorithm to utilize
the values as it sees fit.
Signed-off-by: John Ferlan
ACKed-by: Michal Privoznik
---
tools/virsh-domain-monitor.c | 7 +++
tools/virsh.pod | 26 --
2 files changed, 31 insertions(+), 2 deletions(-)
diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c
index 3a2636377d..5187c1e248 100644
--- a/tools/virsh-domain-monitor.c
+++ b/tools/virsh-domain-monitor.c
@@ -2066,6 +2066,10 @@ static const vshCmdOptDef opts_domstats[] = {
.type = VSH_OT_BOOL,
.help = N_("report domain perf event statistics"),
},
+{.name = "iothread",
+ .type = VSH_OT_BOOL,
+ .help = N_("report domain IOThread information"),
+},
{.name = "list-active",
.type = VSH_OT_BOOL,
.help = N_("list only active domains"),
@@ -2179,6 +2183,9 @@ cmdDomstats(vshControl *ctl, const vshCmd *cmd)
if (vshCommandOptBool(cmd, "perf"))
stats |= VIR_DOMAIN_STATS_PERF;
+if (vshCommandOptBool(cmd, "iothread"))
+stats |= VIR_DOMAIN_STATS_IOTHREAD;
+
if (vshCommandOptBool(cmd, "list-active"))
flags |= VIR_CONNECT_GET_ALL_DOMAINS_STATS_ACTIVE;
diff --git a/tools/virsh.pod b/tools/virsh.pod
index 86c041d575..90f3c1ef5c 100644
--- a/tools/virsh.pod
+++ b/tools/virsh.pod
@@ -975,7 +975,8 @@ or unique source names printed by this command.
=item B [I<--raw>] [I<--enforce>] [I<--backing>] [I<--nowait>]
[I<--state>] [I<--cpu-total>] [I<--balloon>] [I<--vcpu>] [I<--interface>]
-[I<--block>] [I<--perf>] [[I<--list-active>] [I<--list-inactive>]
+[I<--block>] [I<--perf>] [I<--iothread>]
+[[I<--list-active>] [I<--list-inactive>]
[I<--list-persistent>] [I<--list-transient>] [I<--list-running>]
[I<--list-paused>] [I<--list-shutoff>] [I<--list-other>]] | [I ...]
@@ -993,7 +994,7 @@ behavior use the I<--raw> flag.
The individual statistics groups are selectable via specific flags. By
default all supported statistics groups are returned. Supported
statistics groups flags are: I<--state>, I<--cpu-total>, I<--balloon>,
-I<--vcpu>, I<--interface>, I<--block>, I<--perf>.
+I<--vcpu>, I<--interface>, I<--block>, I<--perf>, I<--iothread>.
Note that - depending on the hypervisor type and version or the domain state
- not all of the following statistics may be returned.
@@ -1126,6 +1127,27 @@ Information listed includes:
VIR_DOMAIN_EVENT_ID_BLOCK_THRESHOLD event
See domblkthreshold.
+I<--iothread> returns information about IOThreads on the running guest
+if supported by the hypervisor.
+
+The "poll-max-ns" for each thread is the maximum nanoseconds to allow
+each polling interval to occur. A polling interval is a period of time
+allowed for a thread to process data before being the guest gives up
+its CPU quantum back to the host. A value set too small will not allow
+the IOThread to run long enough on a CPU to process data. A value set
+too high will consume too much CPU time per IOThread failing to allow
+other threads running on the CPU to get time. The polling interval is
+not available for statistical purposes.
+
+ "iothread..poll-max-ns" - maximum polling time in nanoseconds used
+ by the IOThread. A value of 0 (zero)
+ indicates polling is disabled.
+ "iothread..poll-grow" - polling time grow value. A value of 0 (zero)
+ indicates growth is managed by the hypervisor.
+ "iothread..poll-shrink" - polling time shrink value. A value of
+ 0 (zero) indicates shrink is managed by
+ the hypervisor.
+
Selecting a specific statistics groups doesn't guarantee that the
daemon supports the selected group of stats. Flag I<--enforce>
forces the command to fail if the daemon doesn't support the
--
2.17.2
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [REPOST PATCH v2 01/12] qemu: Check for and return IOThread polling values if available
If there are IOThread polling values in the query-iothreads return
buffer, then fill them in and set a bool indicating their presence.
This will allow for displaying in a domain stats output eventually.
Note that the QEMU values are managed a bit differently (as int's
stored in int64_t's) than we will manage them (as unsigned long and
int values). This is intentional to allow for value validation
checking when it comes time to provide the values to QEMU.
Signed-off-by: John Ferlan
ACKed-by: Michal Privoznik
---
src/qemu/qemu_monitor.h | 4
src/qemu/qemu_monitor_json.c | 15 +++
2 files changed, 19 insertions(+)
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
index 48b142a4f4..c2991e2b16 100644
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -1116,6 +1116,10 @@ typedef qemuMonitorIOThreadInfo
*qemuMonitorIOThreadInfoPtr;
struct _qemuMonitorIOThreadInfo {
unsigned int iothread_id;
int thread_id;
+bool poll_valid;
+unsigned long long poll_max_ns;
+unsigned int poll_grow;
+unsigned int poll_shrink;
};
int qemuMonitorGetIOThreads(qemuMonitorPtr mon,
qemuMonitorIOThreadInfoPtr **iothreads);
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 3de298c9e2..2e92984b44 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -7441,6 +7441,21 @@ qemuMonitorJSONGetIOThreads(qemuMonitorPtr mon,
"'thread-id' data"));
goto cleanup;
}
+
+/* Fetch poll values (since QEMU 2.9 ) if available. QEMU
+ * stores these values as int64_t's; however, the qapi type
+ * is an int. The qapi/misc.json also mis-describes the grow
+ * and shrink values as pure add/remove values. The source
+ * util/aio-posix.c function aio_poll uses them as a factor
+ * or divisor in it's calculation. We will fetch and store
+ * them as defined in our structures. */
+if (virJSONValueObjectGetNumberUlong(child, "poll-max-ns",
+ >poll_max_ns) == 0 &&
+virJSONValueObjectGetNumberUint(child, "poll-grow",
+>poll_grow) == 0 &&
+virJSONValueObjectGetNumberUint(child, "poll-shrink",
+>poll_shrink) == 0)
+info->poll_valid = true;
}
ret = n;
--
2.17.2
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [REPOST PATCH v2 11/12] tools: Add virsh iothreadset command
Add a command to allow for setting various dynamic IOThread polling
interval scope (poll-max-ns, poll-grow, and poll-shrink). Describe
the values in the virsh.pod in as generic terms as possible. The
more specific QEMU algorithm has been divulged in the previous patch.
Based heavily on code originally posted by Pavel Hrdina
, but altered to only provide one command
and to not managed a poll disabled state.
Signed-off-by: John Ferlan
---
tools/virsh-domain.c | 110 +++
tools/virsh.pod | 21 +
2 files changed, 131 insertions(+)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 372bdb95d3..4ee6ddf956 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -7734,6 +7734,110 @@ cmdIOThreadAdd(vshControl *ctl, const vshCmd *cmd)
return ret;
}
+
+ /*
+ * "iothreadset" command
+ */
+static const vshCmdInfo info_iothreadset[] = {
+{.name = "help",
+ .data = N_("modifies an existing IOThread of the guest domain")
+},
+{.name = "desc",
+ .data = N_("Modifies an existing IOThread of the guest domain.")
+},
+{.name = NULL}
+};
+
+static const vshCmdOptDef opts_iothreadset[] = {
+VIRSH_COMMON_OPT_DOMAIN_FULL(0),
+{.name = "id",
+ .type = VSH_OT_INT,
+ .flags = VSH_OFLAG_REQ,
+ .help = N_("iothread id of existing IOThread")
+},
+{.name = "poll-max-ns",
+ .type = VSH_OT_INT,
+ .help = N_("set the maximum IOThread polling time in ns")
+},
+{.name = "poll-grow",
+ .type = VSH_OT_INT,
+ .help = N_("set the value to increase the IOThread polling time")
+},
+{.name = "poll-shrink",
+ .type = VSH_OT_INT,
+ .help = N_("set the value for reduction of the IOThread polling time ")
+},
+VIRSH_COMMON_OPT_DOMAIN_LIVE,
+VIRSH_COMMON_OPT_DOMAIN_CURRENT,
+{.name = NULL}
+};
+
+static bool
+cmdIOThreadSet(vshControl *ctl, const vshCmd *cmd)
+{
+virDomainPtr dom;
+int id = 0;
+bool ret = false;
+bool live = vshCommandOptBool(cmd, "live");
+unsigned int flags = VIR_DOMAIN_AFFECT_CURRENT;
+virTypedParameterPtr params = NULL;
+int nparams = 0;
+int maxparams = 0;
+unsigned long long poll_max;
+unsigned int poll_val;
+int rc;
+
+if (live)
+flags |= VIR_DOMAIN_AFFECT_LIVE;
+
+if (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))
+return false;
+
+if (vshCommandOptInt(ctl, cmd, "id", ) < 0)
+goto cleanup;
+if (id <= 0) {
+vshError(ctl, _("Invalid IOThread id value: '%d'"), id);
+goto cleanup;
+}
+
+poll_val = 0;
+if ((rc = vshCommandOptULongLong(ctl, cmd, "poll-max-ns", _max)) < 0)
+goto cleanup;
+if (rc > 0 && virTypedParamsAddULLong(, , ,
+ VIR_DOMAIN_IOTHREAD_POLL_MAX_NS,
+ poll_max) < 0)
+goto save_error;
+
+#define VSH_IOTHREAD_SET_UINT_PARAMS(opt, param) \
+poll_val = 0; \
+if ((rc = vshCommandOptUInt(ctl, cmd, opt, _val)) < 0) \
+goto cleanup; \
+if (rc > 0 && \
+virTypedParamsAddUInt(, , , \
+ param, poll_val) < 0) \
+goto save_error;
+
+VSH_IOTHREAD_SET_UINT_PARAMS("poll-grow", VIR_DOMAIN_IOTHREAD_POLL_GROW)
+VSH_IOTHREAD_SET_UINT_PARAMS("poll-shrink",
VIR_DOMAIN_IOTHREAD_POLL_SHRINK)
+
+#undef VSH_IOTHREAD_SET_UINT_PARAMS
+
+if (virDomainSetIOThreadParams(dom, id, params, nparams, flags) < 0)
+goto cleanup;
+
+ret = true;
+
+ cleanup:
+virTypedParamsFree(params, nparams);
+virshDomainFree(dom);
+return ret;
+
+ save_error:
+vshSaveLibvirtError();
+goto cleanup;
+}
+
+
/*
* "iothreaddel" command
*/
@@ -14149,6 +14253,12 @@ const vshCmdDef domManagementCmds[] = {
.info = info_iothreadadd,
.flags = 0
},
+{.name = "iothreadset",
+ .handler = cmdIOThreadSet,
+ .opts = opts_iothreadset,
+ .info = info_iothreadset,
+ .flags = 0
+},
{.name = "iothreaddel",
.handler = cmdIOThreadDel,
.opts = opts_iothreaddel,
diff --git a/tools/virsh.pod b/tools/virsh.pod
index 90f3c1ef5c..48766567f8 100644
--- a/tools/virsh.pod
+++ b/tools/virsh.pod
@@ -1732,6 +1732,27 @@ If I<--config> is specified, affect the next boot of a
persistent guest.
If I<--current> is specified or I<--live> and I<--config> are not specified,
affect the current guest state.
+=item B I I
+[[I<--poll-max-ns> B] [I<--poll-grow> B]
+[I<--poll-shrink> B]]
+[[I<--config>] [I<--live>] | [I<--current>]]
+
+Modifies an existing iothread of the domain using the specified
+I. The I<--poll-max-ns> provides the maximum polling
+interval to be allowed for an IOThread in ns. If a 0 (zero) is provided,
+then polling for the IOThread is disabled. The I<--poll-grow> is the
+factor by which the current polling time will be adjusted in order to
+reach the maximum polling time. If a 0 (zero) is provided, then the
+default
[libvirt] [REPOST PATCH v2 12/12] docs: Add news article for IOThread polling
Signed-off-by: John Ferlan --- docs/news.xml | 13 + 1 file changed, 13 insertions(+) diff --git a/docs/news.xml b/docs/news.xml index 9d98c34df2..c7c101fd47 100644 --- a/docs/news.xml +++ b/docs/news.xml @@ -35,6 +35,19 @@ + + + Support changing IOThread polling parameters for a live guest + + + Introduced virDomainSetIOThreadParams which allows dynamically + setting the IOThread polling parameters used by QEMU to manage + the thread polling interval and the algorithm for growth or + shrink of the polling time. The values only affect a running + guest with IOThreads. The guest's IOThread polling values can + be viewed via the domain statistics. + + -- 2.17.2 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [REPOST PATCH v2 03/12] qemu: Implement the ability to return IOThread stats
Process the IOThreads polling stats if available. Generate the
output params record to be returned to the caller with the three
values - poll-max-ns, poll-grow, and poll-shrink.
Signed-off-by: John Ferlan
---
include/libvirt/libvirt-domain.h | 1 +
src/libvirt-domain.c | 38 +++
src/qemu/qemu_driver.c | 81
3 files changed, 120 insertions(+)
diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
index fdd2d6b8ea..58fd4bc10c 100644
--- a/include/libvirt/libvirt-domain.h
+++ b/include/libvirt/libvirt-domain.h
@@ -2048,6 +2048,7 @@ typedef enum {
VIR_DOMAIN_STATS_INTERFACE = (1 << 4), /* return domain interfaces info */
VIR_DOMAIN_STATS_BLOCK = (1 << 5), /* return domain block info */
VIR_DOMAIN_STATS_PERF = (1 << 6), /* return domain perf event info */
+VIR_DOMAIN_STATS_IOTHREAD = (1 << 7), /* return iothread poll info */
} virDomainStatsTypes;
typedef enum {
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
index 7690339521..9fda56d660 100644
--- a/src/libvirt-domain.c
+++ b/src/libvirt-domain.c
@@ -11499,6 +11499,44 @@ virConnectGetDomainCapabilities(virConnectPtr conn,
* long long. It is produced by the
* emulation_faults perf event
*
+ * VIR_DOMAIN_STATS_IOTHREAD:
+ * Return IOThread statistics if available. IOThread polling is a
+ * timing mechanism that allows the hypervisor to generate a longer
+ * period of time in which the guest will perform operations on the
+ * CPU being used by the IOThread. The higher the value for poll-max-ns
+ * the longer the guest will keep the CPU. This may affect other host
+ * threads using the CPU. The poll-grow and poll-shrink values allow
+ * the hypervisor to generate a mechanism to add or remove polling time
+ * within the confines of 0 and poll-max-ns. For QEMU, the poll-grow is
+ * multiplied by the polling interval, while poll-shrink is used as a
+ * divisor. When not provided, QEMU may double the polling time until
+ * poll-max-ns is reached. When poll-shrink is 0 (zero) QEMU may reset
+ * the polling interval to 0 until it finds its "sweet spot". Setting
+ * poll-grow too large may cause frequent fluctution of the time; however,
+ * this can be tempered by a high poll-shrink to reduce the polling
+ * interval. For example, a poll-grow of 3 will triple the polling time
+ * which could quickly exceed poll-max-ns; however, a poll-shrink of
+ * 10 would cut that polling time more gradually.
+ *
+ * The typed parameter keys are in this format:
+ *
+ * "iothread.cnt" - maximum number of IOThreads in the subsequent list
+ * as unsigned int. Each IOThread in the list will
+ * will use it's iothread_id value as the . There
+ * may be fewer entries than the iothread.cnt
+ * value if the polling values are not supported.
+ * "iothread..poll-max-ns" - maximum polling time in ns as an unsigned
+ * long long. A 0 (zero) means polling is
+ * disabled.
+ * "iothread..poll-grow" - polling time factor as an unsigned int.
+ * A 0 (zero) indicates to allow the underlying
+ * hypervisor to choose how to grow the
+ * polling time.
+ * "iothread..poll-shrink" - polling time divisor as an unsigned int.
+ * A 0 (zero) indicates to allow the underlying
+ * hypervisor to choose how to shrink the
+ * polling time.
+ *
* Note that entire stats groups or individual stat fields may be missing from
* the output in case they are not supported by the given hypervisor, are not
* applicable for the current state of the guest domain, or their retrieval
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index e13633c1e0..b50d805bf1 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -20437,6 +20437,86 @@ qemuDomainGetStatsBlock(virQEMUDriverPtr driver,
#undef QEMU_ADD_NAME_PARAM
+#define QEMU_ADD_IOTHREAD_PARAM_UI(record, maxparams, id, name, value) \
+do { \
+char param_name[VIR_TYPED_PARAM_FIELD_LENGTH]; \
+snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH, \
+ "iothread.%u.%s", id, name); \
+if (virTypedParamsAddUInt(&(record)->params, \
+ &(record)->nparams, \
+ maxparams, \
+ param_name, \
+ value) < 0) \
+goto cleanup; \
+} while (0)
+
+#define QEMU_ADD_IOTHREAD_PARAM_ULL(record, maxparams, id, name, value) \
+do { \
+char
[libvirt] [REPOST PATCH v2 05/12] lib: Introduce virDomainSetIOThreadParams
Create a new API that will allow an adjustment of IOThread
polling parameters for the specified IOThread. These parameters
will not be saved in the guest XML. Currently the only parameters
supported will allow the hypervisor to adjust the parameters used
to limit and alter the scope of the polling interval. The polling
interval allows the IOThread to spend more or less time processing
in the guest.
Based on code originally posted by Pavel Hrdina
to add virDomainAddIOThreadParams and virDomainModIOThreadParams.
Modification of those changes to use virDomainSetIOThreadParams
instead and remove concepts related to saving the data in guest
XML as well as the way to specifically enable the polling parameters.
Signed-off-by: John Ferlan
ACKed-by: Michal Privoznik
---
include/libvirt/libvirt-domain.h | 44
src/driver-hypervisor.h | 8
src/libvirt-domain.c | 70
src/libvirt_public.syms | 5 +++
src/remote/remote_driver.c | 1 +
src/remote/remote_protocol.x | 21 +-
src/remote_protocol-structs | 10 +
7 files changed, 158 insertions(+), 1 deletion(-)
diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
index 58fd4bc10c..bf89d0149f 100644
--- a/include/libvirt/libvirt-domain.h
+++ b/include/libvirt/libvirt-domain.h
@@ -1911,6 +1911,50 @@ int virDomainDelIOThread(virDomainPtr
domain,
unsigned int iothread_id,
unsigned int flags);
+/* IOThread set parameters */
+
+/**
+ * VIR_DOMAIN_IOTHREAD_POLL_MAX_NS:
+ *
+ * The maximum polling time that can be used by polling algorithm in ns.
+ * The polling time starts at 0 (zero) and is the time spent by the guest
+ * to process IOThread data before returning the CPU to the host. The
+ * polling time will be dynamically modified over time based on the
+ * poll_grow and poll_shrink parameters provided. A value set too large
+ * will cause more CPU time to be allocated the guest. A value set too
+ * small will not provide enough cycles for the guest to process data.
+ * The polling interval is not available for statistical purposes.
+ */
+# define VIR_DOMAIN_IOTHREAD_POLL_MAX_NS "poll_max_ns"
+
+/**
+ * VIR_DOMAIN_IOTHREAD_POLL_GROW:
+ *
+ * This provides a value for the dynamic polling adjustment algorithm to
+ * use to grow its polling interval up to the poll_max_ns value. A value
+ * of 0 (zero) allows the hypervisor to choose its own value. The algorithm
+ * to use for adjustment is hypervisor specific.
+ */
+# define VIR_DOMAIN_IOTHREAD_POLL_GROW "poll_grow"
+
+/**
+ * VIR_DOMAIN_IOTHREAD_POLL_SHRINK:
+ *
+ * This provides a value for the dynamic polling adjustment algorithm to
+ * use to shrink its polling interval when the polling interval exceeds
+ * the poll_max_ns value. A value of 0 (zero) allows the hypervisor to
+ * choose its own value. The algorithm to use for adjustment is hypervisor
+ * specific.
+ */
+# define VIR_DOMAIN_IOTHREAD_POLL_SHRINK "poll_shrink"
+
+int virDomainSetIOThreadParams(virDomainPtr domain,
+unsigned int iothread_id,
+virTypedParameterPtr params,
+int nparams,
+unsigned int flags);
+
+
/**
* VIR_USE_CPU:
* @cpumap: pointer to a bit map of real CPUs (in 8-bit bytes) (IN/OUT)
diff --git a/src/driver-hypervisor.h b/src/driver-hypervisor.h
index eef31eb1f0..6be3e175ce 100644
--- a/src/driver-hypervisor.h
+++ b/src/driver-hypervisor.h
@@ -406,6 +406,13 @@ typedef int
unsigned int iothread_id,
unsigned int flags);
+typedef int
+(*virDrvDomainSetIOThreadParams)(virDomainPtr domain,
+ unsigned int iothread_id,
+ virTypedParameterPtr params,
+ int nparams,
+ unsigned int flags);
+
typedef int
(*virDrvDomainGetSecurityLabel)(virDomainPtr domain,
virSecurityLabelPtr seclabel);
@@ -1407,6 +1414,7 @@ struct _virHypervisorDriver {
virDrvDomainPinIOThread domainPinIOThread;
virDrvDomainAddIOThread domainAddIOThread;
virDrvDomainDelIOThread domainDelIOThread;
+virDrvDomainSetIOThreadParams domainSetIOThreadParams;
virDrvDomainGetSecurityLabel domainGetSecurityLabel;
virDrvDomainGetSecurityLabelList domainGetSecurityLabelList;
virDrvNodeGetSecurityModel nodeGetSecurityModel;
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
index 9fda56d660..5b76458f11 100644
--- a/src/libvirt-domain.c
+++ b/src/libvirt-domain.c
@@ -7812,6 +7812,76 @@ virDomainDelIOThread(virDomainPtr domain,
}
+/**
+ * virDomainSetIOThreadParams:
+ * @domain: a
[libvirt] [REPOST PATCH v2 02/12] qemu: Split qemuDomainGetIOThreadsLive
Separate out the fetch of the IOThread monitor call into a separate
helper so that a subsequent domain statistics change can fetch the raw
IOThread data and parse it as it sees fit.
Signed-off-by: John Ferlan
---
src/qemu/qemu_driver.c | 48 ++
1 file changed, 30 insertions(+), 18 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index a52e2495d5..e13633c1e0 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -5486,39 +5486,52 @@ qemuDomainGetMaxVcpus(virDomainPtr dom)
VIR_DOMAIN_VCPU_MAXIMUM));
}
+
static int
-qemuDomainGetIOThreadsLive(virQEMUDriverPtr driver,
- virDomainObjPtr vm,
- virDomainIOThreadInfoPtr **info)
+qemuDomainGetIOThreadsMon(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ qemuMonitorIOThreadInfoPtr **iothreads)
{
qemuDomainObjPrivatePtr priv;
-qemuMonitorIOThreadInfoPtr *iothreads = NULL;
-virDomainIOThreadInfoPtr *info_ret = NULL;
int niothreads = 0;
-size_t i;
-int ret = -1;
-
-if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0)
-goto cleanup;
if (!virDomainObjIsActive(vm)) {
virReportError(VIR_ERR_OPERATION_INVALID, "%s",
_("cannot list IOThreads for an inactive domain"));
-goto endjob;
+return -1;
}
priv = vm->privateData;
if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_OBJECT_IOTHREAD)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("IOThreads not supported with this binary"));
-goto endjob;
+return -1;
}
qemuDomainObjEnterMonitor(driver, vm);
-niothreads = qemuMonitorGetIOThreads(priv->mon, );
-if (qemuDomainObjExitMonitor(driver, vm) < 0)
-goto endjob;
-if (niothreads < 0)
+niothreads = qemuMonitorGetIOThreads(priv->mon, iothreads);
+if (qemuDomainObjExitMonitor(driver, vm) < 0 || niothreads < 0)
+return -1;
+
+return niothreads;
+}
+
+
+static int
+qemuDomainGetIOThreadsLive(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ virDomainIOThreadInfoPtr **info)
+{
+qemuMonitorIOThreadInfoPtr *iothreads = NULL;
+virDomainIOThreadInfoPtr *info_ret = NULL;
+int niothreads = 0;
+size_t i;
+int ret = -1;
+
+if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0)
+goto cleanup;
+
+if ((niothreads = qemuDomainGetIOThreadsMon(driver, vm, )) < 0)
goto endjob;
/* Nothing to do */
@@ -5548,8 +5561,7 @@ qemuDomainGetIOThreadsLive(virQEMUDriverPtr driver,
virBitmapFree(map);
}
-*info = info_ret;
-info_ret = NULL;
+VIR_STEAL_PTR(*info, info_ret);
ret = niothreads;
endjob:
--
2.17.2
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v7 01/14] conf: Add definitions for 'uid' and 'fid' PCI address attributes
On Fri, 2018-10-19 at 11:40 +0800, Yi Min Zhao wrote:
> struct _virPCIDeviceAddress {
> unsigned int domain;
> unsigned int bus;
> unsigned int slot;
> unsigned int function;
> int multi; /* virTristateSwitch */
> +virZPCIDeviceAddress zpci;
Laine pointed out that it's weird that 'zpci' is not added in the
same patch as 'extFlags', and more specifically that the former is
introduced before the latter.
I believe I pointed out the same thing at some point, or at least
I though about doing so :) Now that it's two people noticing the
same, I guess it would be nice to address it. Shouldn't be hard at
all... Can you please take care of it?
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH RESEND] qemu: Process RDMA GID state change event
This event is emitted on the monitor when a GID table in pvrdma device
is modified and the change needs to be propagate to the backend RDMA
device's GID table.
The control over the RDMA device's GID table is done by updating the
device's Ethernet function addresses.
Usually the first GID entry is determine by the MAC address, the second
by the first IPv6 address and the third by the IPv4 address. Other
entries can be added by adding more IP addresses. The opposite is the
same, i.e. whenever an address is removed, the corresponding GID entry
is removed.
The process is done by the network and RDMA stacks. Whenever an address
is added the ib_core driver is notified and calls the device driver's
add_gid function which in turn update the device.
To support this in pvrdma device we need to hook into the create_bind
and destroy_bind HW commands triggered by pvrdma driver in guest.
Whenever a changed is made to the pvrdma device's GID table a special
QMP messages is sent to be processed by libvirt to update the address of
the backend Ethernet device.
Signed-off-by: Yuval Shaia
---
src/qemu/qemu_domain.c | 3 ++
src/qemu/qemu_domain.h | 15 +
src/qemu/qemu_driver.c | 40
src/qemu/qemu_monitor.c | 28 +
src/qemu/qemu_monitor.h | 13
src/qemu/qemu_monitor_json.c | 36 ++
src/qemu/qemu_process.c | 59
7 files changed, 194 insertions(+)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index ba3fff607a..8da54c7ee9 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -13479,6 +13479,9 @@ qemuProcessEventFree(struct qemuProcessEvent *event)
case QEMU_PROCESS_EVENT_GUESTPANIC:
qemuMonitorEventPanicInfoFree(event->data);
break;
+case QEMU_PROCESS_EVENT_RDMA_GID_STATUS_CHANGED:
+qemuMonitorEventRdmaGidStatusFree(event->data);
+break;
case QEMU_PROCESS_EVENT_WATCHDOG:
case QEMU_PROCESS_EVENT_DEVICE_DELETED:
case QEMU_PROCESS_EVENT_NIC_RX_FILTER_CHANGED:
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index 80bd4bde91..1b188843e3 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -478,6 +478,18 @@ struct _qemuDomainVsockPrivate {
};
+typedef struct _qemuDomainRdmaGidStatusChangedPrivate
qemuDomainRdmaGidStatusChangedPrivate;
+typedef qemuDomainRdmaGidStatusChangedPrivate
*qemuDomainRdmaGidStatusChangedPrivatePtr;
+struct _qemuDomainRdmaGidStatusChangedPrivate {
+virObject parent;
+
+char *netdev;
+bool gid_status;
+uint64_t subnet_prefix;
+uint64_t interface_id;
+};
+
+
typedef enum {
QEMU_PROCESS_EVENT_WATCHDOG = 0,
QEMU_PROCESS_EVENT_GUESTPANIC,
@@ -487,6 +499,7 @@ typedef enum {
QEMU_PROCESS_EVENT_BLOCK_JOB,
QEMU_PROCESS_EVENT_MONITOR_EOF,
QEMU_PROCESS_EVENT_PR_DISCONNECT,
+QEMU_PROCESS_EVENT_RDMA_GID_STATUS_CHANGED,
QEMU_PROCESS_EVENT_LAST
} qemuProcessEventType;
@@ -499,6 +512,8 @@ struct qemuProcessEvent {
void *data;
};
+void
qemuMonitorEventRdmaGidStatusFree(qemuDomainRdmaGidStatusChangedPrivatePtr
info);
+
void qemuProcessEventFree(struct qemuProcessEvent *event);
typedef struct _qemuDomainLogContext qemuDomainLogContext;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index a52e2495d5..dc088d844f 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4788,6 +4788,43 @@ processPRDisconnectEvent(virDomainObjPtr vm)
}
+static void
+processRdmaGidStatusChangedEvent(virDomainObjPtr vm,
+ qemuDomainRdmaGidStatusChangedPrivatePtr info)
+{
+unsigned int prefix_len;
+virSocketAddr addr = {0};
+int rc;
+
+if (!virDomainObjIsActive(vm))
+return;
+
+VIR_DEBUG("netdev=%s,gid_status=%d,subnet_prefix=0x%lx,interface_id=0x%lx",
+ info->netdev, info->gid_status, info->subnet_prefix,
+ info->interface_id);
+
+if (info->subnet_prefix) {
+prefix_len = 64;
+uint32_t ipv6[4];
+memcpy([0], >subnet_prefix, sizeof(info->subnet_prefix));
+memcpy([2], >interface_id, sizeof(info->subnet_prefix));
+virSocketAddrSetIPv6AddrNetOrder(, ipv6);
+} else {
+prefix_len = 24;
+virSocketAddrSetIPv4AddrNetOrder(, info->interface_id >> 32);
+}
+
+if (info->gid_status)
+rc = virNetDevIPAddrAdd(info->netdev, , NULL, prefix_len);
+else
+rc = virNetDevIPAddrDel(info->netdev, , prefix_len);
+
+if (rc)
+VIR_ERROR(_("Fail to update address 0x%lx to %s"), info->interface_id,
+ info->netdev);
+}
+
+
static void qemuProcessEventHandler(void *data, void *opaque)
{
struct qemuProcessEvent *processEvent = data;
@@ -4828,6 +4865,9 @@ static void qemuProcessEventHandler(void *data, void
*opaque)
case QEMU_PROCESS_EVENT_PR_DISCONNECT:
processPRDisconnectEvent(vm);
Re: [libvirt] [RFC PATCH] Add new migration flag VIR_MIGRATE_DRY_RUN
On 11/02/2018 11:34 PM, 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.
>
> 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.
>
> Thanks for considering the idea!
>
> (BTW, if it is considered useful I will follow up with a V1 series that
> includes this patch and and impl for the qemu driver.)
>
> include/libvirt/libvirt-domain.h | 12
> src/qemu/qemu_migration.h| 3 ++-
> tools/virsh-domain.c | 7 +++
> tools/virsh.pod | 10 +-
> 4 files changed, 30 insertions(+), 2 deletions(-)
>
> diff --git a/include/libvirt/libvirt-domain.h
> b/include/libvirt/libvirt-domain.h
> index fdd2d6b8ea..6d52f6ce50 100644
> --- a/include/libvirt/libvirt-domain.h
> +++ b/include/libvirt/libvirt-domain.h
> @@ -830,6 +830,18 @@ typedef enum {
> */
> VIR_MIGRATE_TLS = (1 << 16),
>
> +/* Setting the VIR_MIGRATE_DRY_RUN flag will cause libvirt to make a
> + * best-effort attempt to check if migration will succeed. The
> destination
> + * host will be checked to see if it can accommodate the resources
> required
> + * by the domain. For example are the network, disk, memory, and CPU
While this is a honourable goal to achieve I don't think we can
guarantee it (without running qemu). At least in qemu world. For
instance, libvirt doesn't check if there's enough memory (nor regular
nor hugepages) when domain is started/migrated. We just run qemu and let
it fail. However, for network, CPU and hostdev we do run checks so these
might work. Disks are in grey area - we check their presence but not
their labels. And if domain is relabel=no then the only way to learn if
qemu would succeed is to run it.
But I don't see much problem with starting qemu in paused state. I mean,
we can get through Prepare phase but never actually reach Perform stage.
The API/flag would return success if Prepare succeeded.
I bet it's easier to check if migration would succeed in xen world, or?
The other thing is how are apps expected to use this? I mean, if an app
wants to work without admin intervention then it would need to learn how
to fix any possible error (missing disk, perms issue, missing hostdev,
etc.). This is not a trivial task IMO.
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
