Re: [libvirt] [PATCH 1/2] qemu_alias:Set dimm alias name to dimmX where X=slot number.
On Sun, Sep 25, 2016 at 23:32:05 +0530, Nitesh Konkar wrote:
When we hotplug a memory module, qemu will assign
the smallest slot available. At the time of setting
the alias we find the smallest available slot and
assign alias name as dimmX where X=slot number.
This commit message doesn't really describe why this is necessary. The
problem with qemu is that the migration stream references the memory
blocks by name (which is supplied by libvirt) rather than by the order
of those. This means that we must keep them stable accross migrations.
With the current code that is assigning aliases for memory backend
objects this won't happen and since qemu is treating the memory object
links differently migration does not work in such case.
This means that the requirement is to keep the slot number consistent
with the alias.
To achieve this the below code is not really necessary as once the VM is
started the slot numbers are populated (and thankfully correctly since
qemu numbers the slots starting from 0).
The code below is based on parsing aliases which is not necessary since
you already have the slot numbers available.
The general idea is to do slot number alocation for the memory modules
beforehand so that we are 100% sure which slot number we will get and
then generate alias based on the slot number. I'm not sure though
whether qemu accepts a partial address information consisting of the
slot number only and I did not get around to check it yet.
Signed-off-by: Nitesh Konkar <nitkon12(a)linux.vnet.ibm.com>
---
src/qemu/qemu_alias.c | 51 +++++++++++++++++++++++++++++++++++++++++----------
1 file changed, 41 insertions(+), 10 deletions(-)
diff --git a/src/qemu/qemu_alias.c b/src/qemu/qemu_alias.c
index 0102c96..23ffdfd 100644
--- a/src/qemu/qemu_alias.c
+++ b/src/qemu/qemu_alias.c
@@ -28,6 +28,7 @@
#include "virlog.h"
#include "virstring.h"
#include "network/bridge_driver.h"
+#include "qemu_process.h"
Why do you need this? The code below doesn't seem to use it in any way.
#define QEMU_DRIVE_HOST_PREFIX "drive-"
@@ -331,21 +332,47 @@ qemuAssignDeviceRNGAlias(virDomainDefPtr def,
return 0;
}
+/* Find the smallest available slot. */
+static int
+qemuGetSmallestSlotIdx(virDomainDefPtr def)
+{
+ size_t i, j;
Please don't declare multiple variables on a line.
+ int minidx = 0, idx;
Also declaring multiple on same line and initializing some of them is
even worse.
+
+ for (i = 0; i < def->nmems; i++) {
+ if ((idx = qemuDomainDeviceAliasIndex(&def->mems[i]->info,
"dimm")) != i) {
[1] You've parsed the alias here once now.
+ for (j = 0; j < def->nmems; j++) {
+ if ((idx = qemuDomainDeviceAliasIndex(&def->mems[j]->info,
"dimm")) == i)
+ break;
+ }
+
+ if (j >= def->nmems) {
+ minidx = i;
+ break;
+ }
This is a rather weird algorithm.
+ }
+
+ if ((idx = qemuDomainDeviceAliasIndex(&def->mems[i]->info,
"dimm")) >= minidx)
You are doing the same as in [1]. Seems like a waste.
+ minidx = idx + 1;
+ }
+
+ return minidx;
+}
I don't see a reason to have the function above if we switch to
pre-allocating the numbers properly in the first place. Also then it
will be possible to find the index just by looking at the slot number
rather than parsing the alias over and over.
If you're not into making sure that qemu accepts slots and doing the
slot allocation I can do that since I was up to fix this problem
anyways.
Peter