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