[Libvir] RFC PATCH - Initial NodeGetCellsFreeMemory patch

[beth kon]

oops... meant to post to list as well
beth kon wrote:

> Daniel Veillard wrote:
>
>> On Thu, Sep 20, 2007 at 05:10:49PM -0400, beth kon wrote:
>>
>>
>>> Here is my first pass at a patch for accessing the available memory 
>>> associated with each NUMA cell through libvirt.
>>>
>>
>> Thanks !
>>
>>
> Thanks for the quick comments!
>
>>> +struct xen_v2s4_availheap {
>>> + uint32_t min_bitwidth; /* Smallest address width (zero if don't 
>>> care). */
>>> + uint32_t max_bitwidth; /* Largest address width (zero if don't 
>>> care). */
>>>
>>
>> I'm a bit puzzled by those 2 fields I still wonder what it is about :-)
>>
>>
>>
> I was puzzled too! These fields are related to the case when 32 bit 
> guests need to reserve certain ranges of memory for DMA, for example. 
> I checked with Ryan and he assured me we don't need to worry about 
> those fields for these purposes.
>
>>> + int32_t node; /* NUMA node (-1 for sum across all nodes). */
>>> + uint64_t avail_bytes; /* Bytes available in the specified region. */
>>> +};
>>> +
>>> +typedef struct xen_v2s4_availheap xen_v2s4_availheap;
>>> +
>>>
>> [...]
>>
>>
> What does [...] mean? :-)
>
>>
>>
>>> +xenHypervisorNodeGetCellsFreeMemory(virConnectPtr conn, long long 
>>> *freeMems,
>>> + int startCell, int maxCells)
>>> {
>>> - if ((conn == NULL) || (freeMems == NULL) || (nbCells < 0))
>>> - return -1;
>>> + xen_op_v2_sys op_sys;
>>> + int i, ret, nbCells;
>>> + virNodeInfo nodeInfo;
>>> + virNodeInfoPtr nodeInfoPtr = &nodeInfo;
>>> + xenUnifiedPrivatePtr priv;
>>> +
>>> + if ((conn == NULL) || (freeMems == NULL) || (maxCells < 0))
>>> + return -1;
>>>
>>
>> Hum, actually, that would catch the (maxCells == -1) so that won't work
>> and won't catch maxCells == 0 which could lead to a segfault. Maybe
>> (maxCells == 0) || (maxCells < -1) should be used instead.
>>
>>
>>
> Yes, you're right. I added maxcells = -1 and startCell as an 
> afterthought, and it shows!
>
>>> + /* get actual number of cells */
>>> + if (xenDaemonNodeGetInfo(conn, nodeInfoPtr)) {
>>>
>>
>> Hum, since the number of cells is static, maybe this should be stored
>> permanently in a variable on init. The xenDaemon RPC will be orders 
>> of magnitude
>> more expensive than the direct hypercall below.
>>
>>
>>
>>> + virXenError(VIR_ERR_XEN_CALL, " cannot determine actual number of 
>>> cells", 0);
>>> + return -1;
>>> + }
>>> + nbCells = nodeInfoPtr->nodes;
>>> +
>>> + if (maxCells > nbCells)
>>> + maxCells = nbCells;
>>>
>>
>>
>>
> I wondered about that. Would you like me to make that change as part 
> of this patch?
>
>> Hum ... maxCells is the number of entries in the array, I'm afraid 
>> you will
>> need 2 counters or I misunderstood the way the API works (possible :-),
>> I would fill in freeMems[] starting at index 0 for startCell, and 
>> going up.
>> That feels more coherent than leaving uninitialized values at the 
>> start of
>> the array:
>>
>> for (i = startCell, j = 0;(i < nbCells) && (j < maxCells);i++,j++) {
>> op_sys.u.availheap.node = i;
>> ret = xenHypervisorDoV2Sys(priv->handle, &op_sys);
>> if (ret < 0)
>> return(-1);
>> freeMems[j] = op_sys.u.availheap.avail_bytes;
>> }
>> return (j);
>>
>>
>>
> Yes, right again!
>
> Thanks so much for the great feedback!
>


-- 
Elizabeth Kon (Beth)
IBM Linux Technology Center
Open Hypervisor Team
email: eak at us.ibm.com