Re: [libvirt] [PATCH 3/6] virDomainFormatSchedDef: factor out subset code

[Martin Kletzander]

On Mon, Nov 07, 2016 at 10:01:14AM +0100, Martin Polednik wrote:

The code within the function is too specific for priority attribute of
RT schedulers. To allow addition of schedulers that group by different
properties, we factor out the logic to calculate cpu subset. Instead
of comparing by priority, the new code accepts comparator for the 2
sched structs.
---
src/conf/domain_conf.c | 77 +++++++++++++++++++++++++++++++++++---------------
1 file changed, 55 insertions(+), 22 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index ca0fdfd..0ed755c 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -23058,6 +23058,47 @@ virDomainDefHasCapabilitiesFeatures(virDomainDefPtr 
def)
    return false;
}

+static bool
+virDomainSchedPriorityComparator(virDomainThreadSchedParamPtr baseSched,
+                                 virDomainThreadSchedParamPtr sched)
+{
+    bool ret = false;
+    ret = (baseSched->priority == sched->priority);
+
+    return ret;

Just return (baseSched->priority == sched->priority);  all else is not
needed here.

+}
+
+static virDomainThreadSchedParamPtr
+virDomainSchedSubsetCharacteristic(virDomainDefPtr def,
+                                   virBitmapPtr schedMap,
+                                   virBitmapPtr subsetMap,
+                                   virDomainThreadSchedParamPtr 
(*func)(virDomainDefPtr, unsigned int),
+                                   bool 
(*comparator)(virDomainThreadSchedParamPtr,
+                                                      
virDomainThreadSchedParamPtr))
+{
+    ssize_t nextidx;
+    virDomainThreadSchedParamPtr sched;
+    virDomainThreadSchedParamPtr baseSched = NULL;
+
+    virBitmapClearAll(subsetMap);
+
+    /* we need to find a subset of vCPUs with the given scheduler
+        * that share the priority */
+    nextidx = virBitmapNextSetBit(schedMap, -1);
+    if (!(sched = func(def, nextidx)))
+        return NULL;
+
+    baseSched = sched;
+    ignore_value(virBitmapSetBit(subsetMap, nextidx));
+
+    while ((nextidx = virBitmapNextSetBit(schedMap, nextidx)) > -1) {
+        sched = func(def, nextidx);
+        if (sched && comparator(baseSched, sched))
+            ignore_value(virBitmapSetBit(subsetMap, nextidx));
+    }
+
+    return baseSched;
+}

/**
 * virDomainFormatSchedDef:
@@ -23082,8 +23123,9 @@ virDomainFormatSchedDef(virDomainDefPtr def,
                        virBitmapPtr resourceMap)
{
    virBitmapPtr schedMap = NULL;
-    virBitmapPtr prioMap = NULL;
+    virBitmapPtr subsetMap = NULL;
    virDomainThreadSchedParamPtr sched;
+    virDomainThreadSchedParamPtr baseSched;
    char *tmp = NULL;
    ssize_t next;
    size_t i;
@@ -23098,7 +23140,7 @@ virDomainFormatSchedDef(virDomainDefPtr def,
     */

    if (!(schedMap = virBitmapNew(VIR_DOMAIN_CPUMASK_LEN)) ||
-        !(prioMap = virBitmapNew(VIR_DOMAIN_CPUMASK_LEN)))
+        !(subsetMap = virBitmapNew(VIR_DOMAIN_CPUMASK_LEN)))
        goto cleanup;

    for (i = VIR_PROC_POLICY_NONE + 1; i < VIR_PROC_POLICY_LAST; i++) {
@@ -23117,9 +23159,8 @@ virDomainFormatSchedDef(virDomainDefPtr def,
         * have them */
        while (!virBitmapIsAllClear(schedMap)) {
            virBitmapPtr currentMap = NULL;
-            ssize_t nextprio;
            bool hasPriority = false;
-            int priority = 0;
+            baseSched = NULL;

            switch ((virProcessSchedPolicy) i) {
            case VIR_PROC_POLICY_NONE:
@@ -23132,25 +23173,17 @@ virDomainFormatSchedDef(virDomainDefPtr def,

            case VIR_PROC_POLICY_FIFO:
            case VIR_PROC_POLICY_RR:
-                virBitmapClearAll(prioMap);
                hasPriority = true;

-                /* we need to find a subset of vCPUs with the given scheduler
-                 * that share the priority */
-                nextprio = virBitmapNextSetBit(schedMap, -1);
-                if (!(sched = func(def, nextprio)))
+                baseSched = virDomainSchedSubsetCharacteristic(def,
+                                                               schedMap,
+                                                               subsetMap,
+                                                               func,
+                                                               
virDomainSchedPriorityComparator);
+                if (baseSched == NULL)
                    goto cleanup;

-                priority = sched->priority;
-                ignore_value(virBitmapSetBit(prioMap, nextprio));
-
-                while ((nextprio = virBitmapNextSetBit(schedMap, nextprio)) > 
-1) {
-                    sched = func(def, nextprio);
-                    if (sched && sched->priority == priority)
-                        ignore_value(virBitmapSetBit(prioMap, nextprio));
-                }
-
-                currentMap = prioMap;
+                currentMap = subsetMap;
                break;

This looks like it was factored out but with the combination of renaming
the variable with the same patch makes it weird to review.

@@ -23164,8 +23197,8 @@ virDomainFormatSchedDef(virDomainDefPtr def,
                              virProcessSchedPolicyTypeToString(i));
            VIR_FREE(tmp);

-            if (hasPriority)
-                virBufferAsprintf(buf, " priority='%d'", priority);
+            if (hasPriority && baseSched != NULL)
+                virBufferAsprintf(buf, " priority='%d'", baseSched->priority);

            virBufferAddLit(buf, "/>\n");

@@ -23178,7 +23211,7 @@ virDomainFormatSchedDef(virDomainDefPtr def,

 cleanup:
    virBitmapFree(schedMap);
-    virBitmapFree(prioMap);
+    virBitmapFree(subsetMap);
    return ret;
}

--
2.8.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

signature.asc
Description: Digital signature

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list