[libvirt] [PATCH] cpumap: optimize for clients that don't need online count

[Laine Stump]

On 11/01/2012 09:57 PM, Eric Blake wrote:
> It turns out that calling virNodeGetCPUMap(conn, NULL, NULL, 0)
> is both useful, and with Viktor's patches, common enough to
> optimize.  Since this interface hasn't been released yet, we
> can change the RPC call.
>
> * src/nodeinfo.c (nodeGetCPUMap): Avoid bitmap when not needed.
> * src/remote/remote_protocol.x (remote_node_get_cpu_map_args):
> Supply two separate flags for needed arguments.
> * src/remote/remote_driver.c (remoteNodeGetCPUMap): Update
> caller.
> * daemon/remote.c (remoteDispatchNodeGetCPUMap): Likewise.
> * src/remote_protocol-structs: Regenerate.
> ---
>
> This has to be applied before 1.0.0 if we want it; otherwise the
> change to RPC wire protocol will render this patch an ABI break.
>
> I thought of one alternative, that wouldn't change the RPC size,
> but still would be a difference.  That would be using the old
> need_results int as a bitmap of which of the two arguments are
> needed, instead of just a 1 or 0.  But even that is risky enough
> that it should get in before we commit to this interface.
>
>  daemon/remote.c              | 8 ++++----
>  src/nodeinfo.c               | 3 +++
>  src/remote/remote_driver.c   | 3 ++-
>  src/remote/remote_protocol.x | 3 ++-
>  src/remote_protocol-structs  | 3 ++-
>  5 files changed, 13 insertions(+), 7 deletions(-)
>
> diff --git a/daemon/remote.c b/daemon/remote.c
> index 7a9df60..340d07d 100644
> --- a/daemon/remote.c
> +++ b/daemon/remote.c
> @@ -4578,7 +4578,7 @@ remoteDispatchNodeGetCPUMap(virNetServerPtr server ATTRIBUTE_UNUSED,
>                              remote_node_get_cpu_map_ret *ret)
>  {
>      unsigned char *cpumap = NULL;
> -    unsigned int online;
> +    unsigned int online = 0;
>      unsigned int flags;
>      int cpunum;
>      int rv = -1;
> @@ -4592,13 +4592,13 @@ remoteDispatchNodeGetCPUMap(virNetServerPtr server ATTRIBUTE_UNUSED,
>
>      flags = args->flags;
>
> -    cpunum = virNodeGetCPUMap(priv->conn, args->need_results ? &cpumap : NULL,
> -                              &online, flags);
> +    cpunum = virNodeGetCPUMap(priv->conn, args->need_map ? &cpumap : NULL,
> +                              args->need_online ? &online : NULL, flags);
>      if (cpunum < 0)
>          goto cleanup;
>
>      /* 'serialize' return cpumap */
> -    if (args->need_results) {
> +    if (args->need_map) {
>          ret->cpumap.cpumap_len = VIR_CPU_MAPLEN(cpunum);
>          ret->cpumap.cpumap_val = (char *) cpumap;
>          cpumap = NULL;
> diff --git a/src/nodeinfo.c b/src/nodeinfo.c
> index 35c5f96..3348ae7 100644
> --- a/src/nodeinfo.c
> +++ b/src/nodeinfo.c
> @@ -1277,6 +1277,9 @@ nodeGetCPUMap(virConnectPtr conn ATTRIBUTE_UNUSED,
>
>      virCheckFlags(0, -1);
>
> +    if (!cpumap && !online)
> +        return nodeGetCPUCount();
> +
>      if (!(cpus = nodeGetCPUBitmap(&maxpresent)))
>          goto cleanup;
>
> diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
> index 71218f0..5eca0fa 100644
> --- a/src/remote/remote_driver.c
> +++ b/src/remote/remote_driver.c
> @@ -5793,7 +5793,8 @@ remoteNodeGetCPUMap(virConnectPtr conn,
>
>      remoteDriverLock(priv);
>
> -    args.need_results = !!cpumap;
> +    args.need_map = !!cpumap;
> +    args.need_online = !!online;
>      args.flags = flags;
>
>      memset (&ret, 0, sizeof(ret));
> diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x
> index 765ffcd..d6ac3c1 100644
> --- a/src/remote/remote_protocol.x
> +++ b/src/remote/remote_protocol.x
> @@ -2671,7 +2671,8 @@ struct remote_node_get_memory_parameters_ret {
>  };
>
>  struct remote_node_get_cpu_map_args {
> -    int need_results;
> +    int need_map;
> +    int need_online;
>      unsigned int flags;
>  };
>
> diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs
> index 567864a..6fe7213 100644
> --- a/src/remote_protocol-structs
> +++ b/src/remote_protocol-structs
> @@ -2127,7 +2127,8 @@ struct remote_node_get_memory_parameters_ret {
>          int                        nparams;
>  };
>  struct remote_node_get_cpu_map_args {
> -        int                        need_results;
> +        int                        need_map;
> +        int                        need_online;
>          u_int                      flags;
>  };
>  struct remote_node_get_cpu_map_ret {


After a small bit of explaining the context surrounding the change on
IRC, I understand what's going on and the benefit, and I agree that it's
either now or never.

ACK.