12:35 p.m.
changes in v2:
- changed the parameter order in the function calls
- gave up on moving virPCIDeviceSetX(pci, true) calls to
virPCIDeviceReattach(). The attributes being set changes
the behavior of virPCIDeviceReattach() in a more complex
way than I expected. I still believe a simplification
can be done there, but it became out of scope for a more
simplistic cleanup such as this one.
These are cleanups that I made together with an attempt to
enable parcial PCI Multifunction assignment with managed=true.
That work will be scrapped after discussions with Laine in
[1], but these cleanups kind of make sense on their own, so
here they are.
[1] https://www.redhat.com/archives/libvir-list/2019-July/msg01175.html
*** BLURB HERE ***
Daniel Henrique Barboza (3):
virhostdev: introduce virHostdevResetAllPCIDevices
virhostdev: remove virHostdevReattachPCIDevice
virhostdev: introduce virHostdevReattachAllPCIDevices
src/util/virhostdev.c | 148 +++++++++++++++++-------------------------
src/util/virpci.c | 14 ++++
2 files changed, 75 insertions(+), 87 deletions(-)
--
2.21.0
12:35 p.m.
New subject: [libvirt] [PATCH v2 1/3] virhostdev: introduce virHostdevResetAllPCIDevices
This code that executes virPCIDeviceReset in all virPCIDevicePtr
objects of a given virPCIDeviceListPtr list is replicated twice
in the code. Putting it in a helper function helps with
readability.
Signed-off-by: Daniel Henrique Barboza <danielhb413(a)gmail.com>
---
src/util/virhostdev.c | 54 +++++++++++++++++++++++--------------------
1 file changed, 29 insertions(+), 25 deletions(-)
diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c
index a3647a6cf4..4dd24a8f65 100644
--- a/src/util/virhostdev.c
+++ b/src/util/virhostdev.c
@@ -613,6 +613,32 @@ virHostdevRestoreNetConfig(virDomainHostdevDefPtr hostdev,
}
}
+static int
+virHostdevResetAllPCIDevices(virHostdevManagerPtr mgr,
+ virPCIDeviceListPtr pcidevs)
+{
+ int ret = 0;
+ size_t i;
+
+ for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) {
+ virPCIDevicePtr pci = virPCIDeviceListGet(pcidevs, i);
+
+ /* We can avoid looking up the actual device here, because performing
+ * a PCI reset on a device doesn't require any information other than
+ * the address, which 'pci' already contains */
+ VIR_DEBUG("Resetting PCI device %s", virPCIDeviceGetName(pci));
+ if (virPCIDeviceReset(pci, mgr->activePCIHostdevs,
+ mgr->inactivePCIHostdevs) < 0) {
+ VIR_ERROR(_("Failed to reset PCI device: %s"),
+ virGetLastErrorMessage());
+ virResetLastError();
+ ret = -1;
+ }
+ }
+
+ return ret;
+}
+
int
virHostdevPreparePCIDevices(virHostdevManagerPtr mgr,
const char *drv_name,
@@ -765,17 +791,8 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr,
/* Step 3: Now that all the PCI hostdevs have been detached, we
* can safely reset them */
- for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) {
- virPCIDevicePtr pci = virPCIDeviceListGet(pcidevs, i);
-
- /* We can avoid looking up the actual device here, because performing
- * a PCI reset on a device doesn't require any information other than
- * the address, which 'pci' already contains */
- VIR_DEBUG("Resetting PCI device %s", virPCIDeviceGetName(pci));
- if (virPCIDeviceReset(pci, mgr->activePCIHostdevs,
- mgr->inactivePCIHostdevs) < 0)
- goto reattachdevs;
- }
+ if (virHostdevResetAllPCIDevices(mgr, pcidevs) < 0)
+ goto reattachdevs;
/* Step 4: For SRIOV network devices, Now that we have detached the
* the network device, set the new netdev config */
@@ -1046,20 +1063,7 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr mgr,
}
/* Step 4: perform a PCI Reset on all devices */
- for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) {
- virPCIDevicePtr pci = virPCIDeviceListGet(pcidevs, i);
-
- /* We can avoid looking up the actual device here, because performing
- * a PCI reset on a device doesn't require any information other than
- * the address, which 'pci' already contains */
- VIR_DEBUG("Resetting PCI device %s", virPCIDeviceGetName(pci));
- if (virPCIDeviceReset(pci, mgr->activePCIHostdevs,
- mgr->inactivePCIHostdevs) < 0) {
- VIR_ERROR(_("Failed to reset PCI device: %s"),
- virGetLastErrorMessage());
- virResetLastError();
- }
- }
+ virHostdevResetAllPCIDevices(mgr, pcidevs);
/* Step 5: Reattach managed devices to their host drivers; unmanaged
* devices don't need to be processed further */
--
2.21.0
7:53 a.m.
New subject: [libvirt] [PATCH v2 1/3] virhostdev: introduce virHostdevResetAllPCIDevices
On 7/23/19 7:35 PM, Daniel Henrique Barboza wrote:
This code that executes virPCIDeviceReset in all virPCIDevicePtr
objects of a given virPCIDeviceListPtr list is replicated twice
in the code. Putting it in a helper function helps with
readability.
Signed-off-by: Daniel Henrique Barboza <danielhb413(a)gmail.com>
---
src/util/virhostdev.c | 54 +++++++++++++++++++++++--------------------
1 file changed, 29 insertions(+), 25 deletions(-)
diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c
index a3647a6cf4..4dd24a8f65 100644
--- a/src/util/virhostdev.c
+++ b/src/util/virhostdev.c
@@ -613,6 +613,32 @@ virHostdevRestoreNetConfig(virDomainHostdevDefPtr hostdev,
}
}
+static int
+virHostdevResetAllPCIDevices(virHostdevManagerPtr mgr,
+ virPCIDeviceListPtr pcidevs)
+{
+ int ret = 0;
+ size_t i;
+
+ for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) {
+ virPCIDevicePtr pci = virPCIDeviceListGet(pcidevs, i);
+
+ /* We can avoid looking up the actual device here, because performing
+ * a PCI reset on a device doesn't require any information other than
+ * the address, which 'pci' already contains */
+ VIR_DEBUG("Resetting PCI device %s", virPCIDeviceGetName(pci));
+ if (virPCIDeviceReset(pci, mgr->activePCIHostdevs,
+ mgr->inactivePCIHostdevs) < 0) {
+ VIR_ERROR(_("Failed to reset PCI device: %s"),
+ virGetLastErrorMessage());
+ virResetLastError();
I don't think this is a good idea. VIR_ERROR() logs the error message,
but then virResetLastError() clears the thread local error object and
thus if a PCI fails to reset the caller is left with 'uknown error' and
sysadmin has to go to logs to find out what's happening. In fact, I
think all those virResetLastError() calls should be removed from
virHostdevReAttachPCIDevices(). I'll fix it here and in 3/3 too.
+ ret = -1;
+ }
+ }
+
+ return ret;
+}
+
Michal
8:21 a.m.
New subject: [libvirt] [PATCH v2 1/3] virhostdev: introduce virHostdevResetAllPCIDevices
On 7/31/19 9:53 AM, Michal Privoznik wrote:
On 7/23/19 7:35 PM, Daniel Henrique Barboza wrote:
> This code that executes virPCIDeviceReset in all virPCIDevicePtr
> objects of a given virPCIDeviceListPtr list is replicated twice
> in the code. Putting it in a helper function helps with
> readability.
>
> Signed-off-by: Daniel Henrique Barboza <danielhb413(a)gmail.com>
> ---
> src/util/virhostdev.c | 54 +++++++++++++++++++++++--------------------
> 1 file changed, 29 insertions(+), 25 deletions(-)
>
> diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c
> index a3647a6cf4..4dd24a8f65 100644
> --- a/src/util/virhostdev.c
> +++ b/src/util/virhostdev.c
> @@ -613,6 +613,32 @@
> virHostdevRestoreNetConfig(virDomainHostdevDefPtr hostdev,
> }
> }
> +static int
> +virHostdevResetAllPCIDevices(virHostdevManagerPtr mgr,
> + virPCIDeviceListPtr pcidevs)
> +{
> + int ret = 0;
> + size_t i;
> +
> + for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) {
> + virPCIDevicePtr pci = virPCIDeviceListGet(pcidevs, i);
> +
> + /* We can avoid looking up the actual device here, because
> performing
> + * a PCI reset on a device doesn't require any information
> other than
> + * the address, which 'pci' already contains */
> + VIR_DEBUG("Resetting PCI device %s", virPCIDeviceGetName(pci));
> + if (virPCIDeviceReset(pci, mgr->activePCIHostdevs,
> + mgr->inactivePCIHostdevs) < 0) {
> + VIR_ERROR(_("Failed to reset PCI device: %s"),
> + virGetLastErrorMessage());
> + virResetLastError();
I don't think this is a good idea. VIR_ERROR() logs the error message,
but then virResetLastError() clears the thread local error object and
thus if a PCI fails to reset the caller is left with 'uknown error'
and sysadmin has to go to logs to find out what's happening. In fact,
I think all those virResetLastError() calls should be removed from
virHostdevReAttachPCIDevices(). I'll fix it here and in 3/3 too.
Thanks for explaining and fixing it. I blindly moved the code to this new
function (here and in 3/3) without giving too much thought about whether
the existing error handling makes sense.
DHB
> + ret = -1;
> + }
> + }
> +
> + return ret;
> +}
> +
Michal
12:35 p.m.
New subject: [libvirt] [PATCH v2 2/3] virhostdev: remove virHostdevReattachPCIDevice
virHostdevReattachPCIDevice() is a static that simply does
a wait loop with virPCIDeviceWaitForCleanup() before
calling virPCIDeviceReattach().
This loop traces back to commit d1e5676c0d, aiming to
solve a race condition between Libvirt returning the
device back to the host and QEMU trying to access it in
the meantime, which resulted in QEMU exiting on error
and killing the guest. This happens because device_del
is asynchronous, returning OK even if the guest didn't
release the device. Commit 01abc8a1b8 moved this code
to qemu_hostdev.c, 82e8dd4cf8 added the pci-stub conditional
for the loop, 899b261127 moved the code to virhostdev.c
where it stood until now.
The intent of this wait loop is still valid: device_del
is still not bullet proof into preventing the conditions
that commit d1e5676c0d aimed to fix, especially when considering
all the architectures we must support. However, this loop
is executed only in virHostdevReattachPCIDevice(), leaving
every other virPCIDeviceReattach() call prone to that error.
Let's move the wait loop code to virPCIDeviceReattach(). This
will:
- make every reattach call safe from this race condition
with the pci-stub;
- allow for a bit of code cleanup (virHostdevReattachPCIDevice()
can be erased, and virHostdevReAttachPCIDevices() can use
virPCIDeviceReattach() directly);
- make it easier to understand the overall reattach mechanisms in
Libvirt, without the risk of a newcomer wondering why reattach
is done slightly different in some instances.
Signed-off-by: Daniel Henrique Barboza <danielhb413(a)gmail.com>
---
src/util/virhostdev.c | 40 ++++++++++------------------------------
src/util/virpci.c | 14 ++++++++++++++
2 files changed, 24 insertions(+), 30 deletions(-)
diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c
index 4dd24a8f65..31d075a11a 100644
--- a/src/util/virhostdev.c
+++ b/src/util/virhostdev.c
@@ -927,33 +927,6 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr,
return ret;
}
-/*
- * Pre-condition: inactivePCIHostdevs & activePCIHostdevs
- * are locked
- */
-static void
-virHostdevReattachPCIDevice(virHostdevManagerPtr mgr,
- virPCIDevicePtr actual)
-{
- /* Wait for device cleanup if it is qemu/kvm */
- if (virPCIDeviceGetStubDriver(actual) == VIR_PCI_STUB_DRIVER_KVM) {
- int retries = 100;
- while (virPCIDeviceWaitForCleanup(actual, "kvm_assigned_device")
- && retries) {
- usleep(100*1000);
- retries--;
- }
- }
-
- VIR_DEBUG("Reattaching PCI device %s", virPCIDeviceGetName(actual));
- if (virPCIDeviceReattach(actual, mgr->activePCIHostdevs,
- mgr->inactivePCIHostdevs) < 0) {
- VIR_ERROR(_("Failed to re-attach PCI device: %s"),
- virGetLastErrorMessage());
- virResetLastError();
- }
-}
-
/* @oldStateDir:
* For upgrade purpose: see virHostdevRestoreNetConfig
*/
@@ -1072,12 +1045,19 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr mgr,
virPCIDevicePtr actual;
/* We need to look up the actual device because that's what
- * virHostdevReattachPCIDevice() expects as its argument */
+ * virPCIDeviceReattach() expects as its argument */
if (!(actual = virPCIDeviceListFind(mgr->inactivePCIHostdevs, pci)))
continue;
- if (virPCIDeviceGetManaged(actual))
- virHostdevReattachPCIDevice(mgr, actual);
+ if (virPCIDeviceGetManaged(actual)) {
+ if (virPCIDeviceReattach(actual,
+ mgr->activePCIHostdevs,
+ mgr->inactivePCIHostdevs) < 0) {
+ VIR_ERROR(_("Failed to re-attach PCI device: %s"),
+ virGetLastErrorMessage());
+ virResetLastError();
+ }
+ }
else
VIR_DEBUG("Not reattaching unmanaged PCI device %s",
virPCIDeviceGetName(actual));
diff --git a/src/util/virpci.c b/src/util/virpci.c
index 6dc0a2711c..05232888c3 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -1509,6 +1509,10 @@ virPCIDeviceDetach(virPCIDevicePtr dev,
return 0;
}
+/*
+ * Pre-condition: inactivePCIHostdevs & activePCIHostdevs
+ * are locked
+ */
int
virPCIDeviceReattach(virPCIDevicePtr dev,
virPCIDeviceListPtr activeDevs,
@@ -1520,6 +1524,16 @@ virPCIDeviceReattach(virPCIDevicePtr dev,
return -1;
}
+ /* Wait for device cleanup if it is qemu/kvm */
+ if (virPCIDeviceGetStubDriver(dev) == VIR_PCI_STUB_DRIVER_KVM) {
+ int retries = 100;
+ while (virPCIDeviceWaitForCleanup(dev, "kvm_assigned_device")
+ && retries) {
+ usleep(100*1000);
+ retries--;
+ }
+ }
+
if (virPCIDeviceUnbindFromStub(dev) < 0)
return -1;
--
2.21.0
12:35 p.m.
New subject: [libvirt] [PATCH v2 3/3] virhostdev: introduce virHostdevReattachAllPCIDevices
This code that executes virPCIDeviceReattach in all
virPCIDevicePtr objects of a given virPCIDeviceListPtr
list is replicated twice in the code. Putting it in a helper
function helps with readability.
Signed-off-by: Daniel Henrique Barboza <danielhb413(a)gmail.com>
---
src/util/virhostdev.c | 74 +++++++++++++++++++------------------------
1 file changed, 32 insertions(+), 42 deletions(-)
diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c
index 31d075a11a..d2474aa140 100644
--- a/src/util/virhostdev.c
+++ b/src/util/virhostdev.c
@@ -639,6 +639,36 @@ virHostdevResetAllPCIDevices(virHostdevManagerPtr mgr,
return ret;
}
+static void
+virHostdevReattachAllPCIDevices(virHostdevManagerPtr mgr,
+ virPCIDeviceListPtr pcidevs)
+{
+ size_t i;
+
+ for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) {
+ virPCIDevicePtr pci = virPCIDeviceListGet(pcidevs, i);
+ virPCIDevicePtr actual;
+
+ /* We need to look up the actual device because that's what
+ * virPCIDeviceReattach() expects as its argument */
+ if (!(actual = virPCIDeviceListFind(mgr->inactivePCIHostdevs, pci)))
+ continue;
+
+ if (virPCIDeviceGetManaged(actual)) {
+ if (virPCIDeviceReattach(actual,
+ mgr->activePCIHostdevs,
+ mgr->inactivePCIHostdevs) < 0) {
+ VIR_ERROR(_("Failed to re-attach PCI device: %s"),
+ virGetLastErrorMessage());
+ virResetLastError();
+ }
+ }
+ else
+ VIR_DEBUG("Not reattaching unmanaged PCI device %s",
+ virPCIDeviceGetName(actual));
+ }
+}
+
int
virHostdevPreparePCIDevices(virHostdevManagerPtr mgr,
const char *drv_name,
@@ -899,26 +929,7 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr,
}
reattachdevs:
- for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) {
- virPCIDevicePtr pci = virPCIDeviceListGet(pcidevs, i);
- virPCIDevicePtr actual;
-
- /* We need to look up the actual device because that's what
- * virPCIDeviceReattach() expects as its argument */
- if (!(actual = virPCIDeviceListFind(mgr->inactivePCIHostdevs, pci)))
- continue;
-
- if (virPCIDeviceGetManaged(actual)) {
- VIR_DEBUG("Reattaching managed PCI device %s",
- virPCIDeviceGetName(pci));
- ignore_value(virPCIDeviceReattach(actual,
- mgr->activePCIHostdevs,
- mgr->inactivePCIHostdevs));
- } else {
- VIR_DEBUG("Not reattaching unmanaged PCI device %s",
- virPCIDeviceGetName(pci));
- }
- }
+ virHostdevReattachAllPCIDevices(mgr, pcidevs);
cleanup:
virObjectUnlock(mgr->activePCIHostdevs);
@@ -1040,28 +1051,7 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr mgr,
/* Step 5: Reattach managed devices to their host drivers; unmanaged
* devices don't need to be processed further */
- for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) {
- virPCIDevicePtr pci = virPCIDeviceListGet(pcidevs, i);
- virPCIDevicePtr actual;
-
- /* We need to look up the actual device because that's what
- * virPCIDeviceReattach() expects as its argument */
- if (!(actual = virPCIDeviceListFind(mgr->inactivePCIHostdevs, pci)))
- continue;
-
- if (virPCIDeviceGetManaged(actual)) {
- if (virPCIDeviceReattach(actual,
- mgr->activePCIHostdevs,
- mgr->inactivePCIHostdevs) < 0) {
- VIR_ERROR(_("Failed to re-attach PCI device: %s"),
- virGetLastErrorMessage());
- virResetLastError();
- }
- }
- else
- VIR_DEBUG("Not reattaching unmanaged PCI device %s",
- virPCIDeviceGetName(actual));
- }
+ virHostdevReattachAllPCIDevices(mgr, pcidevs);
virObjectUnlock(mgr->activePCIHostdevs);
virObjectUnlock(mgr->inactivePCIHostdevs);
--
2.21.0
7:53 a.m.
On 7/23/19 7:35 PM, Daniel Henrique Barboza wrote:
changes in v2:
- changed the parameter order in the function calls
- gave up on moving virPCIDeviceSetX(pci, true) calls to
virPCIDeviceReattach(). The attributes being set changes
the behavior of virPCIDeviceReattach() in a more complex
way than I expected. I still believe a simplification
can be done there, but it became out of scope for a more
simplistic cleanup such as this one.
These are cleanups that I made together with an attempt to
enable parcial PCI Multifunction assignment with managed=true.
That work will be scrapped after discussions with Laine in
[1], but these cleanups kind of make sense on their own, so
here they are.
[1] https://www.redhat.com/archives/libvir-list/2019-July/msg01175.html
*** BLURB HERE ***
Daniel Henrique Barboza (3):
virhostdev: introduce virHostdevResetAllPCIDevices
virhostdev: remove virHostdevReattachPCIDevice
virhostdev: introduce virHostdevReattachAllPCIDevices
src/util/virhostdev.c | 148 +++++++++++++++++-------------------------
src/util/virpci.c | 14 ++++
2 files changed, 75 insertions(+), 87 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn(a)redhat.com>
Michal
1974
days inactive
1982
days old
6 comments
2 participants
participants (2)
-
Daniel Henrique Barboza -
Michal Privoznik