This patch provides a caching mechanism for the device address
extensions uid and fid on S390. For efficient sparse address allocation,
we introduce two hash tables for uid/fid which hold the address set
information per domain. Also in order to improve performance of
searching available value, we introduce our own callbacks for the two
hashtables. In this way, uid/fid is saved in hash key and hash value
could be any non-NULL pointer due to no operation on hash value. That is
also the reason why we don't introduce hash value free callback.
Signed-off-by: Yi Min Zhao <zyimin@linux.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com>
Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
---
src/conf/domain_addr.c | 85 ++++++++++++++++++++++++++++++++++
src/conf/domain_addr.h | 13 ++++++
src/libvirt_private.syms | 1 +
src/qemu/qemu_domain_address.c | 7 +++
4 files changed, 106 insertions(+)
diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c
index 442e6aab94..deb58fa7c9 100644
--- a/src/conf/domain_addr.c
+++ b/src/conf/domain_addr.c
@@ -27,11 +27,23 @@
#include "virlog.h"
#include "virstring.h"
#include "domain_addr.h"
+#include "virhashcode.h"
#define VIR_FROM_THIS VIR_FROM_DOMAIN
VIR_LOG_INIT("conf.domain_addr");
+static void
+virDomainPCIAddressSetExtensionFree(virDomainPCIAddressSetPtr addrs)
+{
+ if (!addrs || !addrs->zpciIds)
+ return;
+
+ virHashFree(addrs->zpciIds->uids);
+ virHashFree(addrs->zpciIds->fids);
+ VIR_FREE(addrs->zpciIds);
+}
+
virDomainPCIConnectFlags
virDomainPCIControllerModelToConnectType(virDomainControllerModelPCI model)
{
@@ -727,6 +739,78 @@ virDomainPCIAddressReleaseAddr(virDomainPCIAddressSetPtr addrs,
addrs->buses[addr->bus].slot[addr->slot].functions &= ~(1 << addr->function);
}
+
+static uint32_t
+virZPCIAddrCode(const void *name,
+ uint32_t seed)
+{
+ unsigned int value = *((unsigned int *)name);
+ return virHashCodeGen(&value, sizeof(value), seed);
+}
+
+
+static bool
+virZPCIAddrEqual(const void *namea,
+ const void *nameb)
+{
+ return *((unsigned int *)namea) == *((unsigned int *)nameb);
+}
+
+
+static void *
+virZPCIAddrCopy(const void *name)
+{
+ unsigned int *copy;
+
+ if (VIR_ALLOC(copy) < 0)
+ return NULL;
+
+ *copy = *((unsigned int *)name);
+ return (void *)copy;
+}
+
+
+static void
+virZPCIAddrKeyFree(void *name)
+{
+ VIR_FREE(name);
+}
+
+
+int
+virDomainPCIAddressSetExtensionAlloc(virDomainPCIAddressSetPtr addrs,
+ virPCIDeviceAddressExtensionFlags extFlags)
+{
+ if (extFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI) {
+ if (addrs->zpciIds)
+ return 0;
+
+ if (VIR_ALLOC(addrs->zpciIds) < 0)
+ return -1;
+
+ if (!(addrs->zpciIds->uids = virHashCreateFull(10, NULL,
+ virZPCIAddrCode,
+ virZPCIAddrEqual,
+ virZPCIAddrCopy,
+ virZPCIAddrKeyFree)))
+ goto error;
+
+ if (!(addrs->zpciIds->fids = virHashCreateFull(10, NULL,
+ virZPCIAddrCode,
+ virZPCIAddrEqual,
+ virZPCIAddrCopy,
+ virZPCIAddrKeyFree)))
+ goto error;
+ }
+
+ return 0;
+
+ error:
+ virDomainPCIAddressSetExtensionFree(addrs);
+ return -1;
+}
+
+
virDomainPCIAddressSetPtr
virDomainPCIAddressSetAlloc(unsigned int nbuses)
{
@@ -753,6 +837,7 @@ virDomainPCIAddressSetFree(virDomainPCIAddressSetPtr addrs)
if (!addrs)
return;
+ virDomainPCIAddressSetExtensionFree(addrs);
VIR_FREE(addrs->buses);
VIR_FREE(addrs);
}
diff --git a/src/conf/domain_addr.h b/src/conf/domain_addr.h
index b8525e1403..c87e30786a 100644
--- a/src/conf/domain_addr.h
+++ b/src/conf/domain_addr.h
@@ -116,6 +116,12 @@ typedef struct {
} virDomainPCIAddressBus;
typedef virDomainPCIAddressBus *virDomainPCIAddressBusPtr;
+typedef struct {
+ virHashTablePtr uids;
+ virHashTablePtr fids;
+} virDomainZPCIAddressIds;
+typedef virDomainZPCIAddressIds *virDomainZPCIAddressIdsPtr;
+
struct _virDomainPCIAddressSet {
virDomainPCIAddressBus *buses;
size_t nbuses;
@@ -125,10 +131,17 @@ struct _virDomainPCIAddressSet {
bool areMultipleRootsSupported;
/* If true, the guest can use the pcie-to-pci-bridge controller */
bool isPCIeToPCIBridgeSupported;
+ virDomainZPCIAddressIdsPtr zpciIds;
};
typedef struct _virDomainPCIAddressSet virDomainPCIAddressSet;
typedef virDomainPCIAddressSet *virDomainPCIAddressSetPtr;
+char *virDomainPCIAddressAsString(virPCIDeviceAddressPtr addr)
+ ATTRIBUTE_NONNULL(1);
+
+int virDomainPCIAddressSetExtensionAlloc(virDomainPCIAddressSetPtr addrs,
+ virPCIDeviceAddressExtensionFlags extFlags);
+
virDomainPCIAddressSetPtr virDomainPCIAddressSetAlloc(unsigned int nbuses);
void virDomainPCIAddressSetFree(virDomainPCIAddressSetPtr addrs);
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 92363913e3..b662d2f01e 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -124,6 +124,7 @@ virDomainPCIAddressReserveAddr;
virDomainPCIAddressReserveNextAddr;
virDomainPCIAddressSetAllMulti;
virDomainPCIAddressSetAlloc;
+virDomainPCIAddressSetExtensionAlloc;
virDomainPCIAddressSetFree;
virDomainPCIAddressSlotInUse;
virDomainPCIAddressValidate;
diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
index f8241bed3a..90f2afadcd 100644
--- a/src/qemu/qemu_domain_address.c
+++ b/src/qemu/qemu_domain_address.c
@@ -1509,6 +1509,13 @@ qemuDomainPCIAddressSetCreate(virDomainDefPtr def,
addrs->dryRun = dryRun;
+ /* create zpci address set for s390 domain */
+ if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_ZPCI) &&
+ virDomainPCIAddressSetExtensionAlloc(addrs,
+ VIR_PCI_ADDRESS_EXTENSION_ZPCI)) {
+ goto error;
+ }
+
/* pSeries domains support multiple pci-root controllers */
if (qemuDomainIsPSeries(def))
addrs->areMultipleRootsSupported = true;
--
Yi Min
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-listOn Fri, 2018-09-28 at 16:46 +0800, Yi Min Zhao wrote:
[...]
> +static void
> +virDomainPCIAddressSetExtensionFree(virDomainPCIAddressSetPtr addrs)
> +{
> + if (!addrs || !addrs->zpciIds)
> + return;
> +
> + virHashFree(addrs->zpciIds->uids);
> + virHashFree(addrs->zpciIds->fids);
> + VIR_FREE(addrs->zpciIds);
> +}
Please keep the Free() function closer to the Alloc() function.
[...]
> +int
> +virDomainPCIAddressSetExtensionAlloc(virDomainPCIAddressSetPtr addrs,
> + virPCIDeviceAddressExtensionFlags extFlags)
> +{
> + if (extFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI) {
> + if (addrs->zpciIds)
> + return 0;
> +
> + if (VIR_ALLOC(addrs->zpciIds) < 0)
> + return -1;
> +
> + if (!(addrs->zpciIds->uids = virHashCreateFull(10, NULL,
> + virZPCIAddrCode,
> + virZPCIAddrEqual,
> + virZPCIAddrCopy,
> + virZPCIAddrKeyFree)))
The function names are inconsistent here: they all deal with hash
keys, but only the free function contains the word "key" in its
name.
Please change them like so:
virZPCIAddrCode() => virZPCIAddrKeyCode()
virZPCIAddrEqual() => virZPCIAddrKeyEqual()
virZPCIAddrCopy() => virZPCIAddrKeyCopy()
[...]
> typedef struct _virDomainPCIAddressSet virDomainPCIAddressSet;
> typedef virDomainPCIAddressSet *virDomainPCIAddressSetPtr;
>
> +char *virDomainPCIAddressAsString(virPCIDeviceAddressPtr addr)
> + ATTRIBUTE_NONNULL(1);
This hunk is a leftover from a previous respin, please drop it.
[...]
> @@ -124,6 +124,7 @@ virDomainPCIAddressReserveAddr;
> virDomainPCIAddressReserveNextAddr;
> virDomainPCIAddressSetAllMulti;
> virDomainPCIAddressSetAlloc;
> +virDomainPCIAddressSetExtensionAlloc;
Hm. Instead of exporting this function just so you can call it from
qemuDomainPCIAddressSetCreate(), wouldn't it make more sense to call
it directly from inside virDomainPCIAddressSetAlloc()?
You'd have to pass virPCIDeviceAddressExtensionFlags to the latter,
of course, but that sounds fairly sensible; the only other user,
the bhyve driver, can just pass _NONE.
[...]
> @@ -1509,6 +1509,13 @@ qemuDomainPCIAddressSetCreate(virDomainDefPtr def,
>
> addrs->dryRun = dryRun;
>
> + /* create zpci address set for s390 domain */
> + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_ZPCI) &&
> + virDomainPCIAddressSetExtensionAlloc(addrs,
> + VIR_PCI_ADDRESS_EXTENSION_ZPCI)) {
This should be
if (... &&
virDomainPCIAddressSetExtensionAlloc(...) < 0) {
...
With all of the above addressed,
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
在 2018/10/11 下午6:31, Andrea Bolognani 写道:
> On Fri, 2018-09-28 at 16:46 +0800, Yi Min Zhao wrote:
> [...]
>> +static void
>> +virDomainPCIAddressSetExtensionFree(virDomainPCIAddressSetPtr addrs)
>> +{
>> + if (!addrs || !addrs->zpciIds)
>> + return;
>> +
>> + virHashFree(addrs->zpciIds->uids);
>> + virHashFree(addrs->zpciIds->fids);
>> + VIR_FREE(addrs->zpciIds);
>> +}
> Please keep the Free() function closer to the Alloc() function.
OK.
>
> [...]
>> +int
>> +virDomainPCIAddressSetExtensionAlloc(virDomainPCIAddressSetPtr addrs,
>> + virPCIDeviceAddressExtensionFlags extFlags)
>> +{
>> + if (extFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI) {
>> + if (addrs->zpciIds)
>> + return 0;
>> +
>> + if (VIR_ALLOC(addrs->zpciIds) < 0)
>> + return -1;
>> +
>> + if (!(addrs->zpciIds->uids = virHashCreateFull(10, NULL,
>> + virZPCIAddrCode,
>> + virZPCIAddrEqual,
>> + virZPCIAddrCopy,
>> + virZPCIAddrKeyFree)))
> The function names are inconsistent here: they all deal with hash
> keys, but only the free function contains the word "key" in its
> name.
>
> Please change them like so:
>
> virZPCIAddrCode() => virZPCIAddrKeyCode()
> virZPCIAddrEqual() => virZPCIAddrKeyEqual()
> virZPCIAddrCopy() => virZPCIAddrKeyCopy()
Sure.
> [...]
>> typedef struct _virDomainPCIAddressSet virDomainPCIAddressSet;
>> typedef virDomainPCIAddressSet *virDomainPCIAddressSetPtr;
>>
>> +char *virDomainPCIAddressAsString(virPCIDeviceAddressPtr addr)
>> + ATTRIBUTE_NONNULL(1);
> This hunk is a leftover from a previous respin, please drop it.
ok.
>
> [...]
>> @@ -124,6 +124,7 @@ virDomainPCIAddressReserveAddr;
>> virDomainPCIAddressReserveNextAddr;
>> virDomainPCIAddressSetAllMulti;
>> virDomainPCIAddressSetAlloc;
>> +virDomainPCIAddressSetExtensionAlloc;
> Hm. Instead of exporting this function just so you can call it from
> qemuDomainPCIAddressSetCreate(), wouldn't it make more sense to call
> it directly from inside virDomainPCIAddressSetAlloc()?
>
> You'd have to pass virPCIDeviceAddressExtensionFlags to the latter,
> of course, but that sounds fairly sensible; the only other user,
> the bhyve driver, can just pass _NONE.
Yeah.
> [...]
>> @@ -1509,6 +1509,13 @@ qemuDomainPCIAddressSetCreate(virDomainDefPtr def,
>>
>> addrs->dryRun = dryRun;
>>
>> + /* create zpci address set for s390 domain */
>> + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_ZPCI) &&
>> + virDomainPCIAddressSetExtensionAlloc(addrs,
>> + VIR_PCI_ADDRESS_EXTENSION_ZPCI)) {
> This should be
>
> if (... &&
> virDomainPCIAddressSetExtensionAlloc(...) < 0) {
> ...
OK
>
> With all of the above addressed,
>
> Reviewed-by: Andrea Bolognani <abologna@redhat.com>
>
Thanks!
--
Yi Min
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2025 Red Hat, Inc.