Re: [libvirt] [PATCH v2 RESEND 06/12] conf: Introduce address caching for PCI extensions

2018-07-25 Thread Yi Min Zhao



在 2018/7/24 下午10:03, Andrea Bolognani 写道:

On Tue, 2018-07-10 at 16:02 +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.

Pretty much assuming your hash table implementation doesn't have
any issues, because I lack the expertise to review it properly :)

Some code style issues below.

[...]

+static uint32_t virZPCIAddrCode(const void *name, uint32_t seed)

The return type and each of the function arguments should be on
separate lines, like

   static uint32_t
   virZPCIAddrCode(const void *name,
   uint32_t seed)

[...]

+static bool virZPCIAddrEqual(const void *namea, const void *nameb)

Same.

[...]

+static void *virZPCIAddrCopy(const void *name)

Same.

[...]

+static void virZPCIAddrKeyFree(void *name)

Same.

[...]

+int
+virDomainPCIAddressSetExtensionAlloc(virDomainPCIAddressSetPtr addrs,
+ virDomainPCIAddressExtensionFlags 
extFlags)
+{
+if (extFlags == VIR_PCI_ADDRESS_EXTENSION_ZPCI) {

This should probably be

   if (extFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI)

since we're dealing with flags, but given the way you end up
calling the function it might be okay as it is.


Thanks for your comments!

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 RESEND 06/12] conf: Introduce address caching for PCI extensions

2018-07-24 Thread Andrea Bolognani
On Tue, 2018-07-10 at 16:02 +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.

Pretty much assuming your hash table implementation doesn't have
any issues, because I lack the expertise to review it properly :)

Some code style issues below.

[...]
> +static uint32_t virZPCIAddrCode(const void *name, uint32_t seed)

The return type and each of the function arguments should be on
separate lines, like

  static uint32_t
  virZPCIAddrCode(const void *name,
  uint32_t seed)

[...]
> +static bool virZPCIAddrEqual(const void *namea, const void *nameb)

Same.

[...]
> +static void *virZPCIAddrCopy(const void *name)

Same.

[...]
> +static void virZPCIAddrKeyFree(void *name)

Same.

[...]
> +int
> +virDomainPCIAddressSetExtensionAlloc(virDomainPCIAddressSetPtr addrs,
> + virDomainPCIAddressExtensionFlags 
> extFlags)
> +{
> +if (extFlags == VIR_PCI_ADDRESS_EXTENSION_ZPCI) {

This should probably be

  if (extFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI)

since we're dealing with flags, but given the way you end up
calling the function it might be okay as it is.

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list


[libvirt] [PATCH v2 RESEND 06/12] conf: Introduce address caching for PCI extensions

2018-07-10 Thread Yi Min Zhao
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 
---
 src/conf/domain_addr.c | 79 ++
 src/conf/domain_addr.h |  9 +
 src/libvirt_private.syms   |  1 +
 src/qemu/qemu_domain_address.c |  5 +++
 4 files changed, 94 insertions(+)

diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c
index 16f7ffa928..82f7889679 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,72 @@ 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 +845,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)
   ATTRIBUTE_NONNULL(1);
 
+int virDomainPCIAddressSetExtensionAlloc(