On 10/12/2016 10:57 PM, Michal Privoznik wrote:
> On 13.10.2016 05:38, John Ferlan wrote:
>>
>>
>> On 10/05/2016 03:30 AM, Michal Privoznik wrote:
>>> Now that our cells in test driver are huge pages aware, we can
>>> implement virNodeAllocPages. Basically there's just one catch. If
>>> users specify startCell == -1, they want the huge pages change to
>>> be stretched over all the nodes. Therefore we just recalculate
>>> the change they want to make to each node and continue.
>>>
>>> Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
>>> ---
>>> src/test/test_driver.c | 78 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>> 1 file changed, 78 insertions(+)
>>>
>>> diff --git a/src/test/test_driver.c b/src/test/test_driver.c
>>> index a3f74f8..d9e408e 100644
>>> --- a/src/test/test_driver.c
>>> +++ b/src/test/test_driver.c
>>> @@ -2867,6 +2867,83 @@ testNodeGetFreePages(virConnectPtr conn,
>>> return ret;
>>> }
>>>
>>> +static int
>>> +testNodeAllocPages(virConnectPtr conn,
>>> + unsigned int npages,
>>> + unsigned int *pageSizes,
>>> + unsigned long long *pageCounts,
>>> + int startCell,
>>> + unsigned int cellCount,
>>> + unsigned int flags)
>>> +{
>>> + testDriverPtr privconn = conn->privateData;
>>> + bool add = !(flags & VIR_NODE_ALLOC_PAGES_SET);
>>> + ssize_t i, ncounts = 0;
>>> + size_t j;
>>> + int lastCell;
>>> + int ret = -1;
>>> +
>>> + virCheckFlags(VIR_NODE_ALLOC_PAGES_SET, -1);
>>> +
>>> + testDriverLock(privconn);
>>> +
>>> + lastCell = privconn->numCells - 1;
>>
>> lastCell would be 1 then.
>>
>>> +
>>> + if (startCell < -1 || startCell > lastCell) {
>>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>>> + _("start cell %d out of range (0-%d)"),
>>> + startCell, lastCell);
>>> + goto cleanup;
>>> + }
>>> +
>>> + lastCell = MIN(lastCell, startCell + (int) cellCount - 1);
>>
>> if cellCount is 2, then lastCell = MIN(1, -1 + 2 - 1) ?
>
> Ah. So I haven't though of the case where user wants both behaviours:
> startcell = -1 cellCount = 2. He/she is basically telling us: in the
> first step I want you to allocate the pages through all NUMA nodes, and
> in the second step, I want you to allocate some additional pages on node
> #0. Damn, this is gonna be twice as complicated. Okay, let me rework
> this and post v2.
>
Well it is a test of some basic world - I'm unclear of the rules of the
game of parameters, I'm reacting to what I see.
It is complicated as you continue to describe below... Glad I'm not the
expert!
John
>>
>>
>> Does the following hunk belong before the startCell < -1 if?
>>
>>> +
>>> + if (startCell == -1) {
>>> + /* Okay, hold on to your hats because this is gonna get wild.
>>> + * startCell == -1 means that user wants us to allocate
>>> + * pages over all NUMA nodes proportionally. Just
>>> + * recalculate the pageCounts and we should be good. */
>>
>> Color me confounded.
>>
>>> + for (j = 0; j < npages; j++)
>>> + pageCounts[j] /= privconn->numCells;
>>
>> Why does this work? pageCounts[j] would essentially be divided by 2,
>> but isn't that an input value? How does that make us ensure we're
>> pulling "equally" from across all cells. That is what's the difference
>> between supplying 0 or -1 for startCell? I'm missing something...
>
> Exactly. I mean, if user tells us the following:
>
> npages = 1, pageSizes= {2048}, pageCounts = {678}, startCell = -1,
> cellCount = 1
>
> then they want to allocate 678 of 2M pages across the whole host. They
> don't care whether they will be distributed equally throughout all NUMA
> nodes. For what they know, all 678 pages can be allocated on node #0. Or
> you know, any other combination that adds up to 678.
> If, however, they care where the pages are allocated, they have to
> provide startCell != -1. So for instance, if host has 2 NUMA nodes, and
> they want to allocate 678 pages in total and have them distributed
> equally between the two nodes:
>
> npages =1, pageSizes = {2048}, pageCounts = {339}, startCell = 0,
> cellCount = 2
>
> Yup, it is very complicated API, because it allows you to allocate more
> page sizes across multiple NUMA nodes.
>
> Anyway, because this is all emulated in our test driver (and we
> specifically document that we guarantee no equal distribution for
> startCell = -1 case), we can distribute the allocation as we please. So
> I went with equal distribution.
>
>>
>>> + startCell = 0;
>>> + lastCell = privconn->numCells - 1;
>>
>> So this doesn't change from our default?
>>
>>> + }
>>> +
>>
>> You could probably explain the following to me 5 different ways and each
>> time I'd probably respond, huh?
>>
>>> + for (i = startCell; i <= lastCell; i++) {
>>> + testCellPtr cell = &privconn->cells[i];
>>> + size_t k;
>>> +
>>> + for (j = 0; j < cell->npages; j++) {
>>> + for (k = 0; k < npages; k++) {
>>> + unsigned long long pagesFree = cell->pages[j].pagesFree;
>>> +
>>> + if (pageSizes[k] != cell->pages[j].pagesize)
>>> + continue;
>>
>> Going back to patch 3, this seems to match what I was thinking...
>>
>> The rest of this I guess I'm not sure what to say other than I'm lost.
>>
>>
>> John
>>
>>> +
>>> + if (add)
>>> + pagesFree += pageCounts[k];
>>> + else
>>> + pagesFree = pageCounts[k];
>>> +
>>> + if (pagesFree > cell->pages[j].pages) {
>>> + virReportError(VIR_ERR_OPERATION_FAILED,
>>> + _("Unable to allocate %llu pages"),
>>> + pagesFree);
>>> + goto cleanup;
>>> + }
>>> +
>>> + cell->pages[j].pagesFree = pagesFree;
>>> + ncounts++;
>
> Oh, just figured. I need to modify the pool allocation size, not how
> much free pages is there. That is I need to modify cell-pages[j].pages.
> And also, I should probably recalculate the pagesFree then...
> Sigh. It is complicated and easy to get lost in.
>
> Will send v2.
>
> Michal
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list