On 06/05/2012 02:17 AM, tangchen wrote:
> This patch enables cpuset cgroup, and synchronous vcpupin info set by sched_setaffinity() to cgroup.
>
This doesn't really give many details about what you are trying to do here.
> Signed-off-by: Tang Chen <tangchen@xxxxxxxxxxxxxx>
> ---
> src/libvirt_private.syms | 2 +
> src/qemu/qemu_cgroup.c | 51 ++++++++++++++++++++++++++++++++++++++++++++-
> src/qemu/qemu_cgroup.h | 2 +
> src/qemu/qemu_driver.c | 43 +++++++++++++++++++++++++++++++-------
> src/util/cgroup.c | 35 ++++++++++++++++++++++++++++++-
> src/util/cgroup.h | 3 ++
> 6 files changed, 125 insertions(+), 11 deletions(-)
>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 6ff1a3b..88cc37a 100644
> --- a/src/libvirt_private.syms
> +++ b/src/qemu/qemu_cgroup.c
> @@ -473,18 +473,57 @@ cleanup:
> rc = virCgroupSetCpuCfsPeriod(cgroup, old_period);
> if (rc < 0)
> virReportSystemError(-rc,
> - _("%s"),
> - "Unable to rollback cpu bandwidth period");
> + "%s",
> + _("Unable to rollback cpu bandwidth period"));
This hunk is an independent bug fix, and should be pushed separately. I
will take care of that shortly.
> }
>
> return -1;
> }
>
> +int qemuSetupCgroupVcpuPin(virCgroupPtr cgroup, virDomainDefPtr def,
> + int vcpuid)
> +{
> + int i, rc;
> + char *new_cpus = NULL;
> +
> + if (vcpuid < 0 || vcpuid >= def->vcpus) {
> + virReportSystemError(EINVAL,
> + "%s: %d", _("invalid vcpuid"), vcpuid);
I would write this:
virReportSystemError(EINVAL,
_("invalid vcpuid: %d"), vcpuid);
> + return -1;
> + }
> +
> + for (i = 0; i < def->cputune.nvcpupin; i++) {
> + if (vcpuid == def->cputune.vcpupin[i]->vcpuid) {
> + new_cpus = virDomainCpuSetFormat(def->cputune.vcpupin[i]->cpumask,
> + VIR_DOMAIN_CPUMASK_LEN);
> + if (!new_cpus) {
> + qemuReportError(VIR_ERR_INTERNAL_ERROR,
> + _("failed to convert cpu mask"));
> + goto cleanup;
> + }
> + rc = virCgroupSetCpusetCpus(cgroup, new_cpus);
> + if (rc < 0) {
> + virReportSystemError(-rc,
> + "%s", _("Unable to set cpuset.cpus"));
> + goto cleanup;
> + }
> + }
> + }
> + VIR_FREE(new_cpus);
> + return 0;
> +
> +cleanup:
> + if (new_cpus)
> + VIR_FREE(new_cpus);
This fails 'make syntax-check':
src/qemu/qemu_cgroup.c: if (new_cpus)
VIR_FREE(new_cpus);
maint.mk: found useless "if" before "free" above
> + return -1;
> +}
And since you call VIR_FREE(new_cpus) on both success and failure path,
I'd consolidate things. Declare this up front:
int ret = -1;
then the tail of the function becomes:
ret = 0;
cleanup:
VIR_FREE(new_cpus);
return ret;
}
> @@ -556,6 +595,14 @@ int qemuSetupCgroupForVcpu(struct qemud_driver *driver, virDomainObjPtr vm)
> }
> }
>
> + /* Set vcpupin in cgroup if vcpupin xml is provided */
> + if (def->cputune.nvcpupin) {
> + if (qemuCgroupControllerActive(driver, VIR_CGROUP_CONTROLLER_CPUSET)) {
> + if (qemuSetupCgroupVcpuPin(cgroup_vcpu, def, i) < 0)
> + goto cleanup;
Rather than nesting this deeply, you could use &&, as in:
if (def->cputune.nvcpupin &&
qemuCgroupControllerActive(driver, VIR_CGROUP_CONTROLLER_CPUSET) &&
qemuSetupCgroupVcpuPin(cgroup_vcpu, def, i) < 0)
goto cleanup;
> @@ -3605,9 +3607,37 @@ qemudDomainPinVcpuFlags(virDomainPtr dom,
> if (flags & VIR_DOMAIN_AFFECT_LIVE) {
>
> if (priv->vcpupids != NULL) {
> + /* Add config to vm->def first, because cgroup APIs need it. */
> + if (virDomainVcpuPinAdd(vm->def, cpumap, maplen, vcpu) < 0) {
> + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("failed to update or add vcpupin xml of "
> + "a running domain"));
> + goto cleanup;
> + }
> +
> + /* Configure the corresponding cpuset cgroup before set affinity. */
> + if (qemuCgroupControllerActive(driver,
> + VIR_CGROUP_CONTROLLER_CPUSET)) {
> + if (virCgroupForDomain(driver->cgroup, vm->def->name,
> + &cgroup_dom, 0) == 0) {
> + if (virCgroupForVcpu(cgroup_dom, vcpu, &cgroup_vcpu, 0) == 0) {
> + if (qemuSetupCgroupVcpuPin(cgroup_vcpu, vm->def, vcpu) < 0) {
> + qemuReportError(VIR_ERR_OPERATION_INVALID, "%s %d",
> + _("failed to set cpuset.cpus in cgroup"
> + " for vcpu"), vcpu);
Another place where I would embed the %d into the message, as in:
_("failed to set cpuset.cpus in cgroup for vcpu %d"), vcpu
> + goto cleanup;
> + }
> + }
> + }
> + }
> +
> if (virProcessInfoSetAffinity(priv->vcpupids[vcpu],
> - cpumap, maplen, maxcpu) < 0)
> + cpumap, maplen, maxcpu) < 0) {
> + qemuReportError(VIR_ERR_SYSTEM_ERROR, "%s %d",
> + _("failed to set cpu affinity for vcpu"),
> + vcpu);
and again
--
Eric Blake eblake@xxxxxxxxxx +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list