[libvirt] [PATCH 09/17] Introduce virDomainPinHypervisorFlags and virDomainGetHypervisorPinInfo functions.

[Eric Blake]

On 08/03/2012 12:36 AM, Hu Tao wrote:
> From: Tang Chen <tangchen at cn.fujitsu.com>
> 
> Introduce 2 APIs for client to use.
>     1) virDomainPinHypervisorFlags: call remote driver api, such as remoteDomainPinHypervisorFlags.
>     2) virDomainGetHypervisorPinInfo: call remote driver api, such as remoteDomainGetHypervisorPinInfo.
> 
> Signed-off-by: Tang Chen <tangchen at cn.fujitsu.com>
> Signed-off-by: Hu Tao <hutao at cn.fujitsu.com>
> ---
>  include/libvirt/libvirt.h.in |   10 +++
>  src/driver.h                 |   13 +++-
>  src/libvirt.c                |  147 ++++++++++++++++++++++++++++++++++++++++++
>  src/libvirt_public.syms      |    2 +
>  4 files changed, 171 insertions(+), 1 deletion(-)

Here's where I start to have questions on whether this approach makes
sense.  We already have a function for doing pinning, so why not add a
new flag and reuse the existing function instead of adding a new one?
That is, it might be nicer to change virDomainPinVcpuFlags (side note,
why did we name it with 'Flags' in the name? In retrospect, that was a
dumb naming choice) to have the 'vcpu' argument become signed, with -1
now being a valid value for all hypervisor threads not tied to a vcpu
thread.  Since the function has extern "C" linkage, it would not be an
ABI break (it _would_ be a minor API break, but the only code that would
fail to recompile is code that uses a function pointer to
virDomainPinVcpuFlags, since most code would just get C's implicit
casting rules).

That is, instead of adding a new function, why can't:

virDomainPinVcpuFlags(dom, -1, map, len, flags)

serve as a way to pin all non-vcpu threads?  Or if changing from
'unsigned int vcpu' to 'int vcpu' in the pinning function is
unpalatable, then we could use:

virDomainPinVcpuFlags(dom, 0, map, len,
 flags | VIR_DOMAIN_PIN_VCPU_HYPERVISOR)

And for querying the hypervisor pinning, how about:

virDomainGetVcpuPinInfo(dom, 1, map, len,
 flags | VIR_DOMAIN_GET_VCPU_PIN_HYPERVISOR)

> +++ b/include/libvirt/libvirt.h.in
> @@ -1914,6 +1914,16 @@ int                     virDomainGetVcpuPinInfo (virDomainPtr domain,
>                                                   int maplen,
>                                                   unsigned int flags);
>  
> +int                     virDomainPinHypervisorFlags   (virDomainPtr domain,
> +                                                       unsigned char *cpumap,
> +                                                       int maplen,
> +                                                       unsigned int flags);

_If_ we agree that a new API is needed instead of reusing the existing
API with better semantics, then at least name it sensibly:

virDomainPinHypervisor()

by omitting the Flags suffix (no need to repeat the stupid naming
mistake of vcpu pinning)

> +
> +int                     virDomainGetHypervisorPinInfo (virDomainPtr domain,
> +                                                      unsigned char *cpumaps,
> +                                                      int maplen,
> +                                                      unsigned int flags);

Indentation looks off.

> +++ b/src/libvirt.c
> @@ -8864,6 +8864,153 @@ error:
>  }
>  
>  /**
> + * virDomainPinHypervisorFlags:
> + * @domain: pointer to domain object, or NULL for Domain0
> + * @cpumap: pointer to a bit map of real CPUs (in 8-bit bytes) (IN)
> + *      Each bit set to 1 means that corresponding CPU is usable.
> + *      Bytes are stored in little-endian order: CPU0-7, 8-15...
> + *      In each byte, lowest CPU number is least significant bit.
> + * @maplen: number of bytes in cpumap, from 1 up to size of CPU map in
> + *      underlying virtualization system (Xen...).
> + *      If maplen < size, missing bytes are set to zero.
> + *      If maplen > size, failure code is returned.
> + * @flags: bitwise-OR of virDomainModificationImpact
> + *
> + * Dynamically change the real CPUs which can be allocated to all hypervisor
> + * threads. This function may require privileged access to the hypervisor.

This terminology of 'all hypervisor threads' feels rather loose, I'm
thinking it might be better as 'all hypervisor threads not related to a
specific vcpu', to make it clear that this is the catch-all for all
remaining threads in the domain.


> +/**
> + * virDomainGetHypervisorPinInfo:
> + * @domain: pointer to domain object, or NULL for Domain0
> + * @cpumap: pointer to a bit map of real CPUs for all hypervisor threads of
> + *     this domain (in 8-bit bytes) (OUT)
> + *     There is only one cpumap for all hypervisor threads.
> + *     Must not be NULL.
> + * @maplen: the number of bytes in one cpumap, from 1 up to size of CPU map.
> + *     Must be positive.
> + * @flags: bitwise-OR of virDomainModificationImpact
> + *     Must not be VIR_DOMAIN_AFFECT_LIVE and
> + *     VIR_DOMAIN_AFFECT_CONFIG concurrently.
> + *
> + * Query the CPU affinity setting of all hypervisor threads of domain, store
> + * it in cpumap.
> + *
> + * Returns 1 in case of success,
> + * 0 in case of no hypervisor threads are pined to pcpus,

s/pined/pinned/

> +++ b/src/libvirt_public.syms
> @@ -549,6 +549,8 @@ LIBVIRT_0.10.0 {
>          virDomainGetHostname;
>          virConnectRegisterCloseCallback;
>          virConnectUnregisterCloseCallback;
> +        virDomainPinHypervisorFlags;
> +        virDomainGetHypervisorPinInfo;

I tend to sort these lists, but you're starting with unsorted text. :)

I'd like an opinion from anyone else on whether reusing existing API
with new flags makes more sense than adding new API.

-- 
Eric Blake   eblake at redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 620 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20120806/f328641a/attachment-0001.sig>