[libvirt] [PATCH 4/4] test_driver: Implement virNodeAllocPages

[John Ferlan]

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 at redhat.com>
> ---
>  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) ?


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...

> +        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++;
> +            }
> +        }
> +    }
> +
> +    ret = ncounts;
> + cleanup:
> +    testDriverUnlock(privconn);
> +    return ret;
> +}
> +
>  static int testDomainCreateWithFlags(virDomainPtr domain, unsigned int flags)
>  {
>      testDriverPtr privconn = domain->conn->privateData;
> @@ -6872,6 +6949,7 @@ static virHypervisorDriver testHypervisorDriver = {
>      .nodeGetCPUStats = testNodeGetCPUStats, /* 2.3.0 */
>      .nodeGetFreeMemory = testNodeGetFreeMemory, /* 2.3.0 */
>      .nodeGetFreePages = testNodeGetFreePages, /* 2.3.0 */
> +    .nodeAllocPages = testNodeAllocPages, /* 2.4.0 */
>      .connectGetCapabilities = testConnectGetCapabilities, /* 0.2.1 */
>      .connectGetSysinfo = testConnectGetSysinfo, /* 2.3.0 */
>      .connectGetType = testConnectGetType, /* 2.3.0 */
>