On Tue, Nov 26, 2013 at 07:59:31PM +0530, Shivaprasad G Bhat wrote:
> Version 3:
> Addressed comments on V2.
>
> Version 2:
> Fixed the string formatting errors in v1.
>
> The patch contains the fix for defect 1009880 reported at redhat bugzilla.
> The root cause is, ever since the subcpusets(vcpu,emulator) were introduced, the
> parent cpuset cannot be modified to remove the nodes that are in use by the
> subcpusets.
> The fix is to break the memory node modification into three steps as to assign
> new nodes into the parent first. Change the nodes in the child nodes. Then
> remove the old nodes on the parent node.
>
> Signed-off-by: Shivaprasad G Bhat <sbhat@xxxxxxxxxxxxxxxxxx>
> ---
> src/qemu/qemu_driver.c | 115 ++++++++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 110 insertions(+), 5 deletions(-)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 8a1eefd..4bc9d1d 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -8132,6 +8132,47 @@ cleanup:
> }
>
> static int
> +qemuSetVcpuCpusetMems(virDomainObjPtr vm,
> + char *nodeset_str)
> +{
> + size_t j = 0;
> + qemuDomainObjPrivatePtr priv = vm->privateData;
> + virCgroupPtr cgroup_vcpu = NULL;
> +
> + for (j = 0; j < priv->nvcpupids; j++) {
> + if (virCgroupNewVcpu(priv->cgroup, j, false, &cgroup_vcpu) < 0) {
> + return -1;
> + }
> + if (virCgroupSetCpusetMems(cgroup_vcpu, nodeset_str) < 0) {
> + virCgroupFree(&cgroup_vcpu);
> + return -1;
> + }
> + virCgroupFree(&cgroup_vcpu);
> + }
> +
> + return 0;
> +}
> +
> +static int
> +qemuSetEmulatorCpusetMems(virDomainObjPtr vm,
> + char *nodeset_str)
> +{
> + qemuDomainObjPrivatePtr priv = vm->privateData;
> + virCgroupPtr cgroup_emulator = NULL;
> +
> + if (virCgroupNewEmulator(priv->cgroup, false, &cgroup_emulator) < 0) {
> + return -1;
> + }
> + if (virCgroupSetCpusetMems(cgroup_emulator, nodeset_str) < 0) {
> + virCgroupFree(&cgroup_emulator);
> + return -1;
> + }
> + virCgroupFree(&cgroup_emulator);
> +
> + return 0;
> +}
> +
I suggested to offload this to a different function just in case it is
used in more places than this one. If it is not then it just adds
more code.
> +static int
> qemuDomainSetNumaParameters(virDomainPtr dom,
> virTypedParameterPtr params,
> int nparams,
> @@ -8198,7 +8239,11 @@ qemuDomainSetNumaParameters(virDomainPtr dom,
> }
> } else if (STREQ(param->field, VIR_DOMAIN_NUMA_NODESET)) {
> virBitmapPtr nodeset = NULL;
> + virBitmapPtr old_nodeset = NULL;
> + virBitmapPtr temp_nodeset = NULL;
> char *nodeset_str = NULL;
> + char *old_nodeset_str = NULL;
> + char *temp_nodeset_str = NULL;
>
> if (virBitmapParse(params[i].value.s,
> 0, &nodeset,
> @@ -8208,32 +8253,92 @@ qemuDomainSetNumaParameters(virDomainPtr dom,
> }
>
> if (flags & VIR_DOMAIN_AFFECT_LIVE) {
> + size_t j;
> +
> if (vm->def->numatune.memory.mode !=
> VIR_DOMAIN_NUMATUNE_MEM_STRICT) {
> virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> _("change of nodeset for running domain "
> "requires strict numa mode"));
> - virBitmapFree(nodeset);
> ret = -1;
> - continue;
> + goto next;
> }
>
> /* Ensure the cpuset string is formated before passing to cgroup */
> if (!(nodeset_str = virBitmapFormat(nodeset))) {
> virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> _("Failed to format nodeset"));
> - virBitmapFree(nodeset);
> ret = -1;
> - continue;
> + goto next;
> + }
> +
> + /*Get Exisitng nodeset values */
> + if (virCgroupGetCpusetMems(priv->cgroup, &old_nodeset_str) < 0) {
> + ret = -1;
> + goto next;
> + }
> + if (virBitmapParse(old_nodeset_str, 0, &old_nodeset,
> + VIR_DOMAIN_CPUMASK_LEN) < 0){
> + ret = -1;
> + goto next;
> + }
> +
> + /* Merge the existing and new nodeset values */
> + if ((temp_nodeset = virBitmapNewCopy(old_nodeset)) == NULL) {
> + ret = -1;
> + goto next;
> + }
> +
> + for (j = 0; j < caps->host.nnumaCell; j++) {
> + bool result;
> + if (virBitmapGetBit(nodeset, j, &result) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("Failed to get cpuset bit values"));
> + ret = -1;
> + goto next;
> + }
> + if (result && (virBitmapSetBit(temp_nodeset, j) < 0)) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("Failed to set temporary cpuset bit values"));
> + ret = -1;
> + goto next;
> + }
> + }
> +
> + if (!(temp_nodeset_str = virBitmapFormat(temp_nodeset))) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("Failed to format nodeset"));
> + ret = -1;
> + goto next;
> + }
> +
> + if (virCgroupSetCpusetMems(priv->cgroup, temp_nodeset_str) < 0) {
> + ret = -1;
> + goto next;
> + }
> +
> + if (qemuSetVcpuCpusetMems(vm, nodeset_str) ||
> + qemuSetEmulatorCpusetMems(vm, nodeset_str)) {
> + ret = -1;
> + goto next;
> }
>
> if (virCgroupSetCpusetMems(priv->cgroup, nodeset_str) < 0) {
> virBitmapFree(nodeset);
> VIR_FREE(nodeset_str);
> ret = -1;
> - continue;
> + goto next;
> }
> +next :
> VIR_FREE(nodeset_str);
> + VIR_FREE(old_nodeset_str);
> + virBitmapFree(old_nodeset);
> + VIR_FREE(temp_nodeset_str);
> + virBitmapFree(temp_nodeset);
> + if (ret) {
> + virBitmapFree(nodeset);
> + continue;
> + }
>
This label makes the code unclean, but when I tried "refactoring" it
in the way I had on my mind (Free-ing the pointers on start of the
cycle, changing it to continue, etc.), it looked also ugly. This is
however perfect piece of code to make it into a function. You can
then do a 'cleanup:' label there, clean all the memory after it, set
'ret = -1' on start and reset it before the label; basically the same
way we do it everywhere else. The problem is that this for() cycle is
special in the way that it continues working with other options when a
problem appears. Sorry I haven't mentioned it at first, but when
looking at the code today it seems like it should be done that way.
You'll get rid of those "ret = -1; goto next;" thanks to that.
Thanks,
Martin
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list