[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v3] qemu: Support numad

On 03/16/2012 06:12 AM, Eric Blake wrote:
On 03/08/2012 06:36 AM, Osier Yang wrote:
numad is an user-level daemon that monitors NUMA topology and
processes resource consumption to facilitate good NUMA resource
alignment of applications/virtual machines to improve performance
and minimize cost of remote memory latencies. It provides a
pre-placement advisory interface, so significant processes can
be pre-bound to nodes with sufficient available resources.

Better late than never, but here's a few things you might want to tweak:

+#if defined(NUMAD)
+static char *
+qemuGetNumadAdvice(virDomainDefPtr def)
+    virCommandPtr cmd = NULL;
+    char *args = NULL;
+    char *output = NULL;
+    if (virAsprintf(&args, "%d:%lu", def->vcpus, def->mem.cur_balloon)<  0) {
+        virReportOOMError();
+        goto out;
+    }

This prints into a temporary,

+    cmd = virCommandNewArgList(NUMAD, "-w", args, NULL);

only to then copy it to cmd and have to free the temporary.  It's fewer
lines of code, and less malloc pressure, to avoid the temporary by
printing directly into cmd:

cmd = virCommandNewArgList(NUMAD, "-w", NULL);
virCommandAddArgFormat(&cmd, "%d:%llu", def->vcpus, def->mem.cur_balloon);

+        if (vm->def->cpumask) {
+            /* XXX why don't we keep 'cpumask' in the libvirt cpumap
+             * format to start with ?!?! */
+            for (i = 0 ; i<  maxcpu&&  i<  vm->def->cpumasklen ; i++)
+                if (vm->def->cpumask[i])
+                    VIR_USE_CPU(cpumap, i);

This comment just verbalizes what has already been bothering me.  It
would be really nice if someone could convert all of our internal uses
of cpumask (with its inefficient one-byte-per-cpu representation) into
virBitmask(), as well as making virBitmask provide convenience functions
for converting to and from our stringized format as well as from the
public api cpumap (8 cpus per byte).

I will do these work in seperate patches. Thanks.


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]