[libvirt] [PATCH v5 06/13] conf: Introduce address caching for PCI extensions

[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 <zyimin at linux.ibm.com>
> Reviewed-by: Boris Fiuczynski <fiuczy at linux.vnet.ibm.com>
> Reviewed-by: Bjoern Walk <bwalk at linux.vnet.ibm.com>
> Reviewed-by: Ján Tomko <jtomko at redhat.com>
> ---
>  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