[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 1/6] node_memory: Define the APIs to get/set memory parameters



On Fri, Sep 14, 2012 at 02:42:15PM +0800, Osier Yang wrote:
> * include/libvirt/libvirt.h.in: (Add macros for the param fields,
>   declare the APIs).
> * src/driver.h: (New methods for the driver struct)
> * src/libvirt.c: (Implement the public APIs)
> * src/libvirt_public.syms: (Export the public symbols)
> ---
>  include/libvirt/libvirt.h.in |   63 ++++++++++++++++++++++
>  python/generator.py          |    2 +
>  src/driver.h                 |   14 +++++
>  src/libvirt.c                |  121 ++++++++++++++++++++++++++++++++++++++++++
>  src/libvirt_public.syms      |    2 +
>  5 files changed, 202 insertions(+), 0 deletions(-)
> 
> diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
> index ca04f6c..5a26474 100644
> --- a/include/libvirt/libvirt.h.in
> +++ b/include/libvirt/libvirt.h.in
> @@ -4305,6 +4305,69 @@ typedef struct _virTypedParameter virMemoryParameter;
>   */
>  typedef virMemoryParameter *virMemoryParameterPtr;
>  
> +/*
> + * VIR_NODE_MEMORY_SHARED_PAGES_TO_SCAN:
> + *
> + * Macro for typed parameter that represents how many present pages
> + * to scan before the shared memory service goes to sleep.
> + */
> +# define VIR_NODE_MEMORY_SHARED_PAGES_TO_SCAN     "pages_to_scan"

  Hum, now that the API is generic and not just for shared memory
maybe the string need to be made more specific, for example
"pages_shared" as a general node memory information would not
be the data about the memory tuning of KSM, it would be expected
to be something else. I suggest to prefix all those propertes with
"shm" for shared memory:

  "shm_pages_to_scan"

> +/*
> + * VIR_NODE_MEMORY_SHARED_SLEEP_MILLISECS:
> + *
> + * Macro for typed parameter that represents how many milliseconds
> + * the shared memory service should sleep before next scan.
> + */
> +# define VIR_NODE_MEMORY_SHARED_SLEEP_MILLISECS    "sleep_millisecs"

  "shm_sleep_millisecs"

> +/*
> + * VIR_NODE_MEMORY_SHARED_PAGES_SHARED:
> + *
> + * Macro for typed parameter that represents how many the shared
> + * mmeory pages are being used.
> + */
> +# define VIR_NODE_MEMORY_SHARED_PAGES_SHARED       "pages_shared"

  "shm_pages_shared"

> +/*
> + * VIR_NODE_MEMORY_SHARED_PAGES_SHARING:
> + *
> + * Macro for typed parameter that represents how many sites are
> + * sharing the pages i.e. how much saved.
> + */
> +# define VIR_NODE_MEMORY_SHARED_PAGES_SHARING      "pages_sharing"

  "shm_pages_sharing"

> +/* VIR_NODE_MEMORY_SHARED_PAGES_UNSHARED:
> + *
> + * Macro for typed parameter that represents how many pages unique
> + * but repeatedly checked for merging.
> + */
> +# define VIR_NODE_MEMORY_SHARED_PAGES_UNSHARED     "pages_unshared"

 idem

> +/* VIR_NODE_MEMORY_SHARED_PAGES_VOLATILE:
> + *
> + * Macro for typed parameter that represents how many pages changing
> + * too fast to be placed in a tree.
> + */
> +# define VIR_NODE_MEMORY_SHARED_PAGES_VOLATILE     "pages_volatile"

 idem

> +/* VIR_NODE_MEMORY_SHARED_FULL_SCAN:
> + *
> + * Macro for typed parameter that represents how many times all
> + * mergeable areas have been scanned.
> + */
> +# define VIR_NODE_MEMORY_SHARED_FULL_SCANS         "full_scans"

 idem

> +int virNodeGetMemoryParameters(virConnectPtr conn,
> +                               virTypedParameterPtr params,
> +                               int *nparams,
> +                               unsigned int flags);
> +
> +int virNodeSetMemoryParameters(virConnectPtr conn,
> +                               virTypedParameterPtr params,
> +                               int nparams,
> +                               unsigned int flags);
> +
>  #ifdef __cplusplus
>  }
>  #endif
> diff --git a/python/generator.py b/python/generator.py
> index 8f6e455..8a73a9f 100755
> --- a/python/generator.py
> +++ b/python/generator.py
> @@ -427,6 +427,8 @@ skip_impl = (
>      'virDomainGetDiskErrors',
>      'virConnectUnregisterCloseCallback',
>      'virConnectRegisterCloseCallback',
> +    'virNodeGetMemoryParameters',
> +    'virNodeSetMemoryParameters',
>  )
>  
>  qemu_skip_impl = (
> diff --git a/src/driver.h b/src/driver.h
> index 518e9d4..0eae601 100644
> --- a/src/driver.h
> +++ b/src/driver.h
> @@ -882,6 +882,18 @@ typedef char *
>                                 const char *uri,
>                                 unsigned int flags);
>  
> +typedef int
> +    (*virDrvNodeGetMemoryParameters)(virConnectPtr conn,
> +                                     virTypedParameterPtr params,
> +                                     int *nparams,
> +                                     unsigned int flags);
> +
> +typedef int
> +    (*virDrvNodeSetMemoryParameters)(virConnectPtr conn,
> +                                     virTypedParameterPtr params,
> +                                     int nparams,
> +                                     unsigned int flags);
> +
>  /**
>   * _virDriver:
>   *
> @@ -1068,6 +1080,8 @@ struct _virDriver {
>      virDrvDomainGetDiskErrors           domainGetDiskErrors;
>      virDrvDomainSetMetadata             domainSetMetadata;
>      virDrvDomainGetMetadata             domainGetMetadata;
> +    virDrvNodeGetMemoryParameters       nodeGetMemoryParameters;
> +    virDrvNodeSetMemoryParameters       nodeSetMemoryParameters;
>  };
>  
>  typedef int
> diff --git a/src/libvirt.c b/src/libvirt.c
> index 6e25baf..d451ec8 100644
> --- a/src/libvirt.c
> +++ b/src/libvirt.c
> @@ -6720,6 +6720,127 @@ error:
>      return -1;
>  }
>  
> +/*
> + * virNodeGetMemoryParameters:
> + * @conn: pointer to the hypervisor connection
> + * @params: pointer to memory parameter object
> + *          (return value, allocated by the caller)
> + * @nparams: pointer to number of memory parameters; input and output
> + * @flags: extra flags; not used yet, so callers should always pass 0
> + *
> + * Get all node memory parameters.  On input, @nparams gives the size
> + * of the @params array; on output, @nparams gives how many slots were
> + * filled with parameter information, which might be less but will
> + * not exceed the input value.
> + *
> + * As a special case, calling with @params as NULL and @nparams as 0 on
> + * input will cause @nparams on output to contain the number of parameters
> + * supported by the hypervisor. The caller should then allocate @params
> + * array, i.e. (sizeof(@virTypedParameter) * @nparams) bytes and call the API
> + * again.  See virDomainGetMemoryParameters() for an equivalent usage
> + * example.
> + *
> + * Returns 0 in case of success, and -1 in case of failure.
> + */
> +int
> +virNodeGetMemoryParameters(virConnectPtr conn,
> +                           virTypedParameterPtr params,
> +                           int *nparams,
> +                           unsigned int flags)
> +{
> +    VIR_DEBUG("conn=%p, params=%p, nparams=%p, flags=%x",
> +              conn, params, nparams, flags);
> +
> +    virResetLastError();
> +
> +    if (!VIR_IS_CONNECT(conn)) {
> +        virLibConnError(VIR_ERR_INVALID_CONN, __FUNCTION__);
> +        virDispatchError(NULL);
> +        return -1;
> +    }
> +
> +    virCheckNonNullArgGoto(nparams, error);
> +    virCheckNonNegativeArgGoto(*nparams, error);
> +    if (*nparams != 0)
> +        virCheckNonNullArgGoto(params, error);
> +
> +    if (VIR_DRV_SUPPORTS_FEATURE(conn->driver, conn,
> +                                 VIR_DRV_FEATURE_TYPED_PARAM_STRING))
> +        flags |= VIR_TYPED_PARAM_STRING_OKAY;
> +
> +    if (conn->driver->nodeGetMemoryParameters) {
> +        int ret;
> +        ret = conn->driver->nodeGetMemoryParameters(conn, params,
> +                                                    nparams, flags);
> +        if (ret < 0)
> +            goto error;
> +        return ret;
> +    }
> +
> +    virLibConnError(VIR_ERR_NO_SUPPORT, __FUNCTION__);
> +
> +error:
> +    virDispatchError(conn);
> +    return -1;
> +}
> +
> +/*
> + * virNodeSetMemoryParameters:
> + * @conn: pointer to the hypervisor connection
> + * @params: pointer to scheduler parameter objects
> + * @nparams: number of scheduler parameter objects
> + *          (this value can be the same or less than the returned
> + *           value nparams of virDomainGetSchedulerType)
> + * @flags: extra flags; not used yet, so callers should always pass 0
> + *
> + * Change all or a subset of the node memory tunables.
> + * This function may require privileged access to the hypervisor.
> + *
> + * Returns 0 in case of success, -1 in case of failure.
> + */
> +int
> +virNodeSetMemoryParameters(virConnectPtr conn,
> +                           virTypedParameterPtr params,
> +                           int nparams,
> +                           unsigned int flags)
> +{
> +    VIR_DEBUG("conn=%p, params=%p, nparams=%d, flags=%x",
> +              conn, params, nparams, flags);
> +
> +    virResetLastError();
> +
> +    if (!VIR_IS_CONNECT(conn)) {
> +        virLibConnError(VIR_ERR_INVALID_CONN, __FUNCTION__);
> +        virDispatchError(NULL);
> +        return -1;
> +    }
> +
> +    if (conn->flags & VIR_CONNECT_RO) {
> +        virLibConnError(VIR_ERR_OPERATION_DENIED, __FUNCTION__);
> +        goto error;
> +    }
> +
> +    virCheckNonNullArgGoto(params, error);
> +    virCheckNonNegativeArgGoto(nparams, error);
> +
> +    if (virTypedParameterValidateSet(conn, params, nparams) < 0)
> +        goto error;
> +
> +    if (conn->driver->nodeSetMemoryParameters) {
> +        int ret;
> +        ret = conn->driver->nodeSetMemoryParameters(conn, params,
> +                                                          nparams, flags);
> +        if (ret < 0)
> +            goto error;
> +        return ret;
> +    }
> +
> +    virLibConnError(VIR_ERR_NO_SUPPORT, __FUNCTION__);
> +
> +error:
> +    virDispatchError(conn);
> +    return -1;
> +}
>  
>  /**
>   * virDomainGetSchedulerType:
> diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms
> index 8dda48b..7ec1177 100644
> --- a/src/libvirt_public.syms
> +++ b/src/libvirt_public.syms
> @@ -559,6 +559,8 @@ LIBVIRT_0.10.2 {
>          virConnectListAllInterfaces;
>          virConnectListAllNetworks;
>          virConnectListAllStoragePools;
> +        virNodeGetMemoryParameters;
> +        virNodeSetMemoryParameters;
>          virStoragePoolListAllVolumes;
>  } LIBVIRT_0.10.0;

  Otherwise looks fine to me, ACK with the parameter name change
as suggested.

Daniel

-- 
Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
daniel veillard com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]