[libvirt] [PATCH 1/6] domMemStats: Add domainMemoryStats method to struct _virDriver

[Adam Litke]

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