On 10/04/2016 09:06 AM, Nitesh Konkar wrote:

Currently the migration stream references the memory blocks by name (which is supplied by libvirt) rather than by there order. 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 patch ensures slot number alocation for the memory

allocation

modules beforehand and assign alias accordingly. This keeps slot numbers consistent with the aliases always.

Signed-off-by: Nitesh Konkar <nitkon12@linux.vnet.ibm.com> --- src/conf/domain_conf.h | 1 + src/qemu/qemu_alias.c | 36 +++++++++++++++++++++++++----------- src/qemu/qemu_alias.h | 6 ++++-- src/qemu/qemu_domain.c | 3 +++ src/qemu/qemu_hotplug.c | 5 ++++- 5 files changed, 37 insertions(+), 14 deletions(-)

I believe Peter's review of your v2 mentioned a few other things... In particular test cases... but also a subtle point about partial manual assignment of slots by a user which I'm not sure you covered in the new AssignSlot method below. I'll make note of a few things below...

diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index fd3ae8e..22b5fe1 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2145,6 +2145,7 @@ struct _virDomainDef {

virDomainBlkiotune blkio; virDomainMemtune mem; + virBitmapPtr memslotsptr;

This wouldn't belong here - it would belong in _qemuDomainObjPrivate. You're not saving this data the user visible XML and this is not something generic, rather it's qemu specific. I think it would need to be saved in the "running" XML so that future restart/reloads of running domains would have it. You'll see that the structure already has a couple of virBitmapPtr fields which you could use as a "model" to find all the places you'd need to touch (and the history of how they got into private).

virDomainVcpuDefPtr *vcpus; size_t maxvcpus; diff --git a/src/qemu/qemu_alias.c b/src/qemu/qemu_alias.c index cc83fec..8deb054 100644 --- a/src/qemu/qemu_alias.c +++ b/src/qemu/qemu_alias.c @@ -332,21 +332,34 @@ qemuAssignDeviceRNGAlias(virDomainDefPtr def, }

-int -qemuAssignDeviceMemoryAlias(virDomainDefPtr def, - virDomainMemoryDefPtr mem) +void +qemuAssignDeviceMemorySlot(virDomainDefPtr def, + virDomainMemoryDefPtr mem)

This should not go in qemu_alias.c - it seems to belong in qemu_domain and thus would be appropriately named such as: qemuDomainDeviceAssignMemorySlot (or something that starts with qemuDomain - I'm not the "best" at creating names).

{ size_t i; - int maxidx = 0; - int idx; + int minidx = 0;

- for (i = 0; i < def->nmems; i++) { - if ((idx = qemuDomainDeviceAliasIndex(&def->mems[i]->info, "dimm")) >= maxidx) - maxidx = idx + 1; + if (mem->info.addr.dimm.base) { + minidx = mem->info.addr.dimm.slot; + } else { + for (i = 0; i < def->mem.memory_slots; i++) { + if (!virBitmapIsBitSet(def->memslotsptr, i)) { + minidx = i; + break; + } + }

Are you looking for something like virBitmapNextClearBit?

}

- if (virAsprintf(&mem->info.alias, "dimm%d", maxidx) < 0) - return -1; + ignore_value(virBitmapSetBit(def->memslotsptr, minidx)); + mem->info.addr.dimm.slot = minidx; +} + + +int +qemuAssignDeviceMemoryAlias(virDomainMemoryDefPtr mem) +{

Right here you'll want to call your method to assign the slot... Since the two places where slot assignment is done (qemuAssignDeviceAliases and qemuDomainAttachMemory) have that call first...

+ if (virAsprintf(&mem->info.alias, "dimm%d", mem->info.addr.dimm.slot) < 0) + return -1;

return 0;

FWIW: If this only had the virAsprintf, it could have just simply been: return virAsprintf(...);

} @@ -475,7 +488,8 @@ qemuAssignDeviceAliases(virDomainDefPtr def, virQEMUCapsPtr qemuCaps) return -1; } for (i = 0; i < def->nmems; i++) { - if (virAsprintf(&def->mems[i]->info.alias, "dimm%zu", i) < 0) + qemuAssignDeviceMemorySlot(def, def->mems[i]); + if (virAsprintf(&def->mems[i]->info.alias, "dimm%d", def->mems[i]->info.addr.dimm.slot) < 0)

Similar to [1] below, when these two things are "paired up" - that says to me the qemuAssignDeviceAliases should be handling that memory slot Not doing the virAsprintf here allows one common place to generate the alias...

return -1; }

diff --git a/src/qemu/qemu_alias.h b/src/qemu/qemu_alias.h index 11d9fde..c6cb568 100644 --- a/src/qemu/qemu_alias.h +++ b/src/qemu/qemu_alias.h @@ -57,8 +57,10 @@ int qemuAssignDeviceRedirdevAlias(virDomainDefPtr def, int qemuAssignDeviceRNGAlias(virDomainDefPtr def, virDomainRNGDefPtr rng);

-int qemuAssignDeviceMemoryAlias(virDomainDefPtr def, - virDomainMemoryDefPtr mems); +void qemuAssignDeviceMemorySlot(virDomainDefPtr def, + virDomainMemoryDefPtr); + +int qemuAssignDeviceMemoryAlias(virDomainMemoryDefPtr mems);

int qemuAssignDeviceShmemAlias(virDomainDefPtr def, virDomainShmemDefPtr shmem, diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 9b1a32e..263e78f 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2386,6 +2386,9 @@ qemuDomainDefPostParse(virDomainDefPtr def, if (qemuDomainDefVcpusPostParse(def) < 0) goto cleanup;

+ if (def->mem.memory_slots) + def->memslotsptr = virBitmapNew(def->mem.memory_slots); +

When is this virBitmapFree()'d? If it moves to the qemu domain private, then check out qemuDomainObjPrivateFree.

ret = 0; cleanup: virObjectUnref(qemuCaps); diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 72dd93b..5a3af10 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1898,7 +1898,9 @@ qemuDomainAttachMemory(virQEMUDriverPtr driver, if (qemuDomainDefValidateMemoryHotplug(vm->def, priv->qemuCaps, mem) < 0) goto cleanup;

- if (qemuAssignDeviceMemoryAlias(vm->def, mem) < 0) + qemuAssignDeviceMemorySlot(vm->def, mem); + + if (qemuAssignDeviceMemoryAlias(mem) < 0)

[1] Things paired like this are what cause me to believe the Alias function can do the setting... Secondarily, if we go to cleanup due to failure after this point, then your bitmap slot is not cleared. One way we've been handling this type of situation is by adding a boolean to indicate that we have to undo something we did "on failure". In this particular case, you may need to alter subsequent goto cleanup's to be a different label to clear the bit similar to how there's a removedef label... The new label would be below removedef and above the goto audit to allow the removedef label code to just fall into the new label (hope that makes sense!).

goto cleanup;

if (virAsprintf(&objalias, "mem%s", mem->info.alias) < 0) @@ -4427,6 +4429,7 @@ qemuDomainDetachMemoryDevice(virQEMUDriverPtr driver, }

mem = vm->def->mems[idx]; + ignore_value(virBitmapClearBit(vm->def->memslotsptr, memdef->info.addr.dimm.slot));

BTW: Since the alias and the slot are tightly coupled, if the alias isn't found, then do we want to be clearing this? IOW: Should this go after the following alias check John

if (!mem->info.alias) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s",