[libvirt] [PATCH v5 06/13] conf: Introduce address caching for PCI extensions
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
Reviewed-by: Boris Fiuczynski
Reviewed-by: Bjoern Walk
Reviewed-by: Ján Tomko
---
src/conf/domain_addr.c | 85 ++
src/conf/domain_addr.h | 9 +
src/libvirt_private.syms | 1 +
src/qemu/qemu_domain_address.c | 7
4 files changed, 102 insertions(+)
diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c
index b62fd53c66..363e5b21dc 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)
{
@@ -741,6 +753,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,
+ virDomainPCIAddressExtensionFlags
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)
{
@@ -767,6 +851,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 5219d2f208..d1ec869da4 100644
--- a/src/conf/domain_addr.h
+++ b/src/conf/domain_addr.h
@@ -116,6 +116,11 @@ typedef struct {
} virDomainPCIAddressBus;
typedef virDomainPCIAddressBus *virDomainPCIAddressBusPtr;
+typedef struct {
+virHashTablePtr uids, fids;
+} virDomainZPCIAddressIds;
+typedef virDomainZPCIAddressIds *virDomainZPCIAddressIdsPtr;
+
struct _virDomainPCIAddressSet {
virDomainPCIAddressBus *buses;
size_t nbuses;
@@ -125,6 +130,7 @@ 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;
@@ -132,6 +138,9 @@ typedef virDomainPCIAddressSet *virDomainPCIAddressSetPtr;
char *virDomainPCIAddressAsString(virPCIDeviceAddressPtr addr)
A
Re: [libvirt] [PATCH v5 06/13] conf: Introduce address caching for PCI extensions
On Tue, 2018-09-04 at 16:39 +0800, Yi Min Zhao wrote:
> 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
> Reviewed-by: Boris Fiuczynski
> Reviewed-by: Bjoern Walk
> Reviewed-by: Ján Tomko
> ---
> src/conf/domain_addr.c | 85
> ++
> src/conf/domain_addr.h | 9 +
> src/libvirt_private.syms | 1 +
> src/qemu/qemu_domain_address.c | 7
> 4 files changed, 102 insertions(+)
Okay, I've spent some time actually digging into the hash table
implementation this time around :)
[...]
> +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);
> +}
This makes sense and seems to work just fine; however, you are
allocating and releasing a bunch of small integers, which seems
a bit wasteful.
vircgroup is AFAICT avoiding all that extra memory management by
stuffing the values straight into the pointers themselves, which
you should also be able to do since the biggest legal ID is a
32-bit integer.
That said, I haven't been able to get that to actually work, at
least with a quick attempt :( Would you mind exploring that route
and figuring out whether it's feasible at all?
[...]
> +int
> +virDomainPCIAddressSetExtensionAlloc(virDomainPCIAddressSetPtr addrs,
> + virDomainPCIAddressExtensionFlags
> extFlags)
> +{
> +if (extFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI) {
> +if (addrs->zpciIds)
> +return 0;
This check doesn't look necessary.
[...]
> +typedef struct {
> +virHashTablePtr uids, fids;
> +} virDomainZPCIAddressIds;
One member per line, please.
[...]
> +/* create zpci address set for s390 domain */
> +if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_ZPCI) &&
> +virDomainPCIAddressSetExtensionAlloc(addrs,
> +
> VIR_PCI_ADDRESS_EXTENSION_ZPCI)) {
> +goto error;
> +}
You should check for
virDomainPCIAddressSetExtensionAlloc() < 0
here.
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v5 06/13] conf: Introduce address caching for PCI extensions
在 2018/9/11 下午7:34, Andrea Bolognani 写道:
On Tue, 2018-09-04 at 16:39 +0800, Yi Min Zhao wrote:
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
Reviewed-by: Boris Fiuczynski
Reviewed-by: Bjoern Walk
Reviewed-by: Ján Tomko
---
src/conf/domain_addr.c | 85 ++
src/conf/domain_addr.h | 9 +
src/libvirt_private.syms | 1 +
src/qemu/qemu_domain_address.c | 7
4 files changed, 102 insertions(+)
Okay, I've spent some time actually digging into the hash table
implementation this time around :)
[...]
+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);
+}
This makes sense and seems to work just fine; however, you are
allocating and releasing a bunch of small integers, which seems
a bit wasteful.
vircgroup is AFAICT avoiding all that extra memory management by
stuffing the values straight into the pointers themselves, which
you should also be able to do since the biggest legal ID is a
32-bit integer.
That said, I haven't been able to get that to actually work, at
least with a quick attempt :( Would you mind exploring that route
and figuring out whether it's feasible at all?
I'm testing this. Actually I wanted to do so like vircgroup. I
remembered there's
error due to the previous code logic. I will reply to you later.
[...]
+int
+virDomainPCIAddressSetExtensionAlloc(virDomainPCIAddressSetPtr addrs,
+ virDomainPCIAddressExtensionFlags
extFlags)
+{
+if (extFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI) {
+if (addrs->zpciIds)
+return 0;
This check doesn't look necessary.
All right.
[...]
+typedef struct {
+virHashTablePtr uids, fids;
+} virDomainZPCIAddressIds;
One member per line, please.
ok
[...]
+/* create zpci address set for s390 domain */
+if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_ZPCI) &&
+virDomainPCIAddressSetExtensionAlloc(addrs,
+ VIR_PCI_ADDRESS_EXTENSION_ZPCI)) {
+goto error;
+}
You should check for
virDomainPCIAddressSetExtensionAlloc() < 0
here.
Thanks for your comments.
--
Yi Min
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v5 06/13] conf: Introduce address caching for PCI extensions
在 2018/9/12 下午3:35, Yi Min Zhao 写道:
+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);
+}
This makes sense and seems to work just fine; however, you are
allocating and releasing a bunch of small integers, which seems
a bit wasteful.
vircgroup is AFAICT avoiding all that extra memory management by
stuffing the values straight into the pointers themselves, which
you should also be able to do since the biggest legal ID is a
32-bit integer.
That said, I haven't been able to get that to actually work, at
least with a quick attempt :( Would you mind exploring that route
and figuring out whether it's feasible at all?
I'm testing this. Actually I wanted to do so like vircgroup. I
remembered there's
error due to the previous code logic. I will reply to you later.
I remebered the reason and test again. FID might be 0. It is treated as
an error
if we save 0 in void* pointer.
--
Yi Min
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v5 06/13] conf: Introduce address caching for PCI extensions
On Wed, 2018-09-12 at 16:34 +0800, Yi Min Zhao wrote: > 在 2018/9/12 下午3:35, Yi Min Zhao 写道: > > > This makes sense and seems to work just fine; however, you are > > > allocating and releasing a bunch of small integers, which seems > > > a bit wasteful. > > > > > > vircgroup is AFAICT avoiding all that extra memory management by > > > stuffing the values straight into the pointers themselves, which > > > you should also be able to do since the biggest legal ID is a > > > 32-bit integer. > > > > > > That said, I haven't been able to get that to actually work, at > > > least with a quick attempt :( Would you mind exploring that route > > > and figuring out whether it's feasible at all? > > > > I'm testing this. Actually I wanted to do so like vircgroup. I > > remembered there's > > error due to the previous code logic. I will reply to you later. > > I remebered the reason and test again. FID might be 0. It is treated as > an error > if we save 0 in void* pointer. Right. Too bad fid can go all the way to UINT32_MAX, otherwise we could have just stored them in the pointer after offsetting them by one and thus worked around the issue... I guess forbidding users from using UINT32_MAX as a fid is not an option, right? -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v5 06/13] conf: Introduce address caching for PCI extensions
在 2018/9/12 下午6:37, Andrea Bolognani 写道: On Wed, 2018-09-12 at 16:34 +0800, Yi Min Zhao wrote: 在 2018/9/12 下午3:35, Yi Min Zhao 写道: This makes sense and seems to work just fine; however, you are allocating and releasing a bunch of small integers, which seems a bit wasteful. vircgroup is AFAICT avoiding all that extra memory management by stuffing the values straight into the pointers themselves, which you should also be able to do since the biggest legal ID is a 32-bit integer. That said, I haven't been able to get that to actually work, at least with a quick attempt :( Would you mind exploring that route and figuring out whether it's feasible at all? I'm testing this. Actually I wanted to do so like vircgroup. I remembered there's error due to the previous code logic. I will reply to you later. I remebered the reason and test again. FID might be 0. It is treated as an error if we save 0 in void* pointer. Right. Too bad fid can go all the way to UINT32_MAX, otherwise we could have just stored them in the pointer after offsetting them by one and thus worked around the issue... Yes. Just one value makes all things complex. I guess forbidding users from using UINT32_MAX as a fid is not an option, right? Actually as my understanding, it's just a value to identify the pci function. IMO, it's not a big deal to decrease usable FID values. After all, UID set is smaller than FID set. The maximum number of pci devices is limited by UID. Anyway, I have to discuss this with my colleagues internally. I will tell you our discussion result first time. -- Yi Min -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v5 06/13] conf: Introduce address caching for PCI extensions
在 2018/9/12 下午6:37, Andrea Bolognani 写道: On Wed, 2018-09-12 at 16:34 +0800, Yi Min Zhao wrote: 在 2018/9/12 下午3:35, Yi Min Zhao 写道: This makes sense and seems to work just fine; however, you are allocating and releasing a bunch of small integers, which seems a bit wasteful. vircgroup is AFAICT avoiding all that extra memory management by stuffing the values straight into the pointers themselves, which you should also be able to do since the biggest legal ID is a 32-bit integer. That said, I haven't been able to get that to actually work, at least with a quick attempt :( Would you mind exploring that route and figuring out whether it's feasible at all? I'm testing this. Actually I wanted to do so like vircgroup. I remembered there's error due to the previous code logic. I will reply to you later. I remebered the reason and test again. FID might be 0. It is treated as an error if we save 0 in void* pointer. Right. Too bad fid can go all the way to UINT32_MAX, otherwise we could have just stored them in the pointer after offsetting them by one and thus worked around the issue... I guess forbidding users from using UINT32_MAX as a fid is not an option, right? We have discussed this. FID 0~UINT32_MAX are allowed in s390. We shouldn't forbid users. So we have to keep the hashtable code as it is. -- Yi Min -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v5 06/13] conf: Introduce address caching for PCI extensions
On Thu, 2018-09-13 at 15:58 +0800, Yi Min Zhao wrote: > 在 2018/9/12 下午6:37, Andrea Bolognani 写道: > > Too bad fid can go all the way to UINT32_MAX, otherwise we could > > have just stored them in the pointer after offsetting them by one > > and thus worked around the issue... > > > > I guess forbidding users from using UINT32_MAX as a fid is not an > > option, right? > > We have discussed this. FID 0~UINT32_MAX are allowed in s390. > We shouldn't forbid users. So we have to keep the hashtable code > as it is. That's a bit unfortunate, but okay. Just ignore the comments about it and keep the current implementation then. -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v5 06/13] conf: Introduce address caching for PCI extensions
在 2018/9/13 下午4:23, Andrea Bolognani 写道: On Thu, 2018-09-13 at 15:58 +0800, Yi Min Zhao wrote: 在 2018/9/12 下午6:37, Andrea Bolognani 写道: Too bad fid can go all the way to UINT32_MAX, otherwise we could have just stored them in the pointer after offsetting them by one and thus worked around the issue... I guess forbidding users from using UINT32_MAX as a fid is not an option, right? We have discussed this. FID 0~UINT32_MAX are allowed in s390. We shouldn't forbid users. So we have to keep the hashtable code as it is. That's a bit unfortunate, but okay. Just ignore the comments about it and keep the current implementation then. Thanks for your understanding. -- Yi Min -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
