Re: [libvirt] [PATCH 1/6] domMemStats: Add domainMemoryStats method to struct _virDriver
On Thu, 2009-12-17 at 10:43 +0000, Daniel P. Berrange wrote:
On Fri, Dec 11, 2009 at 03:26:12PM -0500, Adam Litke wrote:
> Set up the types for the domainMemoryStats function and insert it into the
>
> +/**
> + * Memory Statistics Tags:
> + */
> +typedef enum {
> + /* The total amount of data read from swap space (in bytes). */
> + VIR_MEMSTAT_SWAP_IN = 0,
> + /* The total amount of memory written out to swap space (in bytes). */
> + VIR_MEMSTAT_SWAP_OUT = 1,
> +
> + /*
> + * Page faults occur when a process makes a valid access to virtual memory
> + * that is not available. When servicing the page fault, if disk IO is
> + * required, it is considered a major fault. If not, it is a minor fault.
> + * These are expressed as the number of faults that have occurred.
> + */
> + VIR_MEMSTAT_MAJOR_FAULT = 2,
> + VIR_MEMSTAT_MINOR_FAULT = 3,
> +
> + /*
> + * The amount of memory left completely unused by the system. Memory that
> + * is available but used for reclaimable caches should NOT be reported as
> + * free. This value is expressed in bytes.
> + */
> + VIR_MEMSTAT_MEM_FREE = 4,
> +
> + /*
> + * The total amount of usable memory as seen by the domain. This value
> + * may be less than the amount of memory assigned to the domain if a
> + * balloon driver is in use or if the guest OS does not initialize all
> + * assigned pages. This value is expressed in bytes.
> + */
> + VIR_MEMSTAT_MEM_TOTAL = 5,
> +
> + /*
> + * The number of statistics supported by this version of the interface.
> + * To add new statistics, add them to the enum and increase this value.
> + */
> + VIR_MEMSTAT_NR_TAGS = 6,
> +} virDomainMemoryStatTags;
I'm a little wary of this last element 'VIR_MEMSTAT_NR_TAGS', since
it is something that will obviously change as we add more stats. I
see your intent is that the caller simply statically declare an array
that is VIR_MEMSTAT_NR_TAGS in size. The only other alternative I see
is to have an API for querying the number of supported statistics, but
then that has the downside of requiring an extra API call (network
round trip) for something we expect to be called quite frequently.
Yep, this dilemma is apparent to me as well. I think the only two
alternatives are: a query API (as you mention above), or going back to a
static stats structure. I agree that forcing two separate calls for
each operation would be unfortunate. And if we revert to using a static
stats structure, we just end up creating churn in that definition (while
introducing a new set of problems). So, it seems that
VIR_MEMSTAT_NR_TAGS is the least offensive way to solve the problem.
Would it help to rename it to something else (eg. VIR_MEMSTAT_NR_STATS)?
> +
> +typedef struct _virDomainMemoryStat virDomainMemoryStatStruct;
> +
> +struct _virDomainMemoryStat {
> + virDomainMemoryStatTags tag;
This should be an 'int', since enums have an ill-defined size for ABI
Ok.
--
Thanks,
Adam