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

[Yi Min Zhao]

在 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 <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?
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