hmm. my git configuration seems to have stripped out all my comments... sorry. This should have been included: --------- Patch to earlier cpu cgroup support in libvirt-cim to accommodate support in latest versions of libvirt for persisting cpu cgroup setting for KVM guests in new <cputune> section of XML config. Patch is conditionally compiled based on libvirt version built against. New libvirt versions will exploit this patch and libvirt-cim will no longer save cpu cgroup setting in infostore, and is better interoperable with virsh management commands. Older libvirt versions will continue to exploit previous libvirt-cim cpu cgroup support (ie save cpu cgroup setting in infostore, less interoperable with vish management commands). ---------- - G Dr. Gareth S. Bestor IBM Senior Software Engineer Systems & Technology Group - Systems Management Standards 971-285-6375 (mobile) bestor(a)us.ibm.com [PATCH] cpu cgroup patch 2 Gareth S Bestor to: libvirt-cim 12/20/11 11:48 AM Cc: Gareth S Bestor Signed-off-by: Gareth S. Bestor <bestor(a)us.ibm.com&gt; --- libxkutil/xmlgen.c | 45 +++++++++++++++++++++++++++++ src/Virt_ComputerSystem.c | 6 ++++ src/Virt_RASD.c | 6 +++- src/Virt_VirtualSystemManagementService.c | 30 +++++++++++++++++++ 4 files changed, 86 insertions(+), 1 deletions(-) diff --git a/libxkutil/xmlgen.c b/libxkutil/xmlgen.c index 4cca75b..2fe41bf 100644 --- a/libxkutil/xmlgen.c +++ b/libxkutil/xmlgen.c @@ -365,6 +365,44 @@ static const char *vcpu_xml(xmlNodePtr root, struct domain *dominfo) return NULL; } +#if LIBVIR_VERSION_NUMBER >= 9000 +static const char *cputune_xml(xmlNodePtr root, struct domain *dominfo) +{ + struct vcpu_device *vcpu; + xmlNodePtr cputune, tmp; + int ret; + char *string = NULL; + + if (dominfo->dev_vcpu == NULL) + return NULL; + + vcpu = &dominfo->dev_vcpu[0].dev.vcpu; + + /* CPU cgroup setting saved by libvirt under <cputune> XML section */ + cputune = xmlNewChild(root, NULL, BAD_CAST "cputune", NULL); + if (cputune == NULL) + return XML_ERROR; + + /* Get the CPU cgroup setting from the VCPU RASD.Weight property */ + ret = asprintf(&string, + "%d", + vcpu->weight); + if (ret == -1) + return XML_ERROR; + + tmp = xmlNewChild(cputune, + NULL, + BAD_CAST "shares", + BAD_CAST string); + free(string); + + if (tmp == NULL) + return XML_ERROR; + else + return NULL; +} +#endif + static const char *mem_xml(xmlNodePtr root, struct domain *dominfo) { struct mem_device *mem; @@ -941,6 +979,13 @@ char *system_to_xml(struct domain *dominfo) if (msg != NULL) goto out; +#if LIBVIR_VERSION_NUMBER >= 9000 + /* Recent libvirt versions add new <cputune> section to XML */ + msg = cputune_xml(root, dominfo); + if (msg != NULL) + goto out; +#endif + devices = xmlNewChild(root, NULL, BAD_CAST "devices", NULL); if (devices == NULL) { msg = XML_ERROR; diff --git a/src/Virt_ComputerSystem.c b/src/Virt_ComputerSystem.c index e1f1cec..098b07a 100644 --- a/src/Virt_ComputerSystem.c +++ b/src/Virt_ComputerSystem.c @@ -861,6 +861,11 @@ static int lxc_scheduler_params(struct infostore_ctx *ctx, static int kvm_scheduler_params(struct infostore_ctx *ctx, virSchedParameter **params) { +#if LIBVIR_VERSION_NUMBER < 9000 + /* Old versions of libvirt only support CPU cgroups for running guests */ + /* so instead read cpu cgroup setting for inactive guest from infostore. */ + /* New versions there is nothing to do because libvirt takes care of it. */ + unsigned long long value; *params = calloc(1, sizeof(virSchedParameter)); @@ -878,6 +883,7 @@ static int kvm_scheduler_params(struct infostore_ctx *ctx, return 1; } +#endif return 0; } diff --git a/src/Virt_RASD.c b/src/Virt_RASD.c index 9305c8d..0a2de7f 100644 --- a/src/Virt_RASD.c +++ b/src/Virt_RASD.c @@ -157,8 +157,12 @@ static CMPIStatus set_proc_rasd_params(const CMPIBroker *broker, goto out; } - /* Currently only support CPU cgroups for running KVM guests */ + /* Early versions of libvirt only support CPU cgroups for *running* KVM guests */ +#if LIBVIR_VERSION_NUMBER < 9000 if (domain_online(dom) && STREQC(virConnectGetType(conn), "QEMU")) { +#else + if (STREQC(virConnectGetType(conn), "QEMU")) { +#endif char *sched; int nparams; unsigned int i; diff --git a/src/Virt_VirtualSystemManagementService.c b/src/Virt_VirtualSystemManagementService.c index 21979c3..f6b191e 100644 --- a/src/Virt_VirtualSystemManagementService.c +++ b/src/Virt_VirtualSystemManagementService.c @@ -1783,7 +1783,37 @@ static CMPIStatus update_dominfo(const struct domain *dominfo, goto out; } +#if LIBVIR_VERSION_NUMBER < 9000 + /* Old libvirt versions dont save cpu cgroup setting for inactive */ + /* guests, so save in infostore instead */ infostore_set_u64(ctx, "weight", dev->dev.vcpu.weight); +#else + /* New libvirt versions save cpu cgroup setting in KVM guest config */ + if (STREQC(virConnectGetType(conn), "QEMU")) { + int ret; + virSchedParameter params; + strncpy(params.field, + "cpu_shares", + VIR_DOMAIN_SCHED_FIELD_LENGTH); + params.type = VIR_DOMAIN_SCHED_FIELD_ULLONG; + params.value.ul = dev->dev.vcpu.weight; + + CU_DEBUG("setting %s scheduler param cpu_shares=%d", + dominfo->name, + dev->dev.vcpu.weight); + ret = virDomainSetSchedulerParametersFlags(dom, &params, 1, + VIR_DOMAIN_AFFECT_CONFIG); + if (ret != 0) { + CU_DEBUG("Failed to set config scheduler param"); + cu_statusf(_BROKER, &s, + CMPI_RC_ERR_FAILED, + "Failed to set config scheduler param"); + goto out; + } + } + else + infostore_set_u64(ctx, "weight", dev->dev.vcpu.weight); +#endif infostore_set_u64(ctx, "limit", dev->dev.vcpu.limit); out: -- 1.7.1

On 12/20/2011 08:59 PM, Wayne Xia wrote: The current (default) approach has been to a build time check and link certain APIs that way. This does mean that we need to have different libvirt-cim RPMs for different libvirt versions, which is problematic. I have a few ideas and I think others might to. I believe Gareth is going to put down some initial thoughts and we can discuss on a dedicated thread. -- Chip Vincent Open Virtualization IBM Linux Technology Center cvincent(a)linux.vnet.ibm.com

Correct. libvirt behaves very differently when it comes to handling cgroup settings for *inactive* guests between recent ("yes it does") and older ("no it doesn't") versions, which means turning on/off code in libvirt-cim to compensate. It seemed simpler to compile libvirt-cim accordingly, and we have some existing libvirt version-based compile-time directives already. Or is there a better, more established approach to handling such matters in libvirt-cim? - Gareth Dr. Gareth S. Bestor IBM Senior Software Engineer Systems & Technology Group - Systems Management Standards 971-285-6375 (mobile) bestor(a)us.ibm.com Re: [Libvirt-cim] [PATCH] cpu cgroup patch 2 Wayne Xia to: libvirt-cim, Gareth S Bestor 12/20/11 06:01 PM Seems it choose the function at compile time, I guess this result in two RPMs with different configuration of libvirt-cim. Could we use the latest the libvirt to compile the codes, but choose the function at runtime? 于 2011-12-20 20:43, Gareth S. Bestor 写道: +++++++++++++++++++++++++++++ domain *dominfo) section */ property */ infostore_ctx *ctx, guests */ infostore. */ care of it. */ *ctx, CMPIBroker *broker, *running* KVM guests */ "QEMU")) { b/src/Virt_VirtualSystemManagementService.c domain *dominfo, inactive */ config */ 1, param"); param"); -- Best Regards Wayne Xia mail:xiawenc@linux.vnet.ibm.com tel:86-010-82450803

Gareth, Patch looks good. I have two minor comments (see below) and got some "trailing whitespace" errors when I tried to apply. Applying: cpu cgroup patch 2 /home/cvincent/proj/libvirt-cim/.git/rebase-apply/patch:123: trailing whitespace. /* guests, so save in infostore instead */ /home/cvincent/proj/libvirt-cim/.git/rebase-apply/patch:137: trailing whitespace. dominfo->name, /home/cvincent/proj/libvirt-cim/.git/rebase-apply/patch:139: trailing whitespace. ret = virDomainSetSchedulerParametersFlags(dom, &params, 1, warning: 3 lines add whitespace errors. On 12/20/2011 07:43 AM, Gareth S. Bestor wrote: Shouldn't you also be checking that dominfor->dev_vcpu[0] is not NULL above? Do you think simply "weight" is unique enough? I'm concerned that we may have other types of "weight" in the future that may require more unique naming. How about "cpu_weight"? -- Chip Vincent Open Virtualization IBM Linux Technology Center cvincent(a)linux.vnet.ibm.com