[libvirt] [PATCH 0 of 3] Add cpu.shares support to LXC driver
This set adds {Get,Set}SchedulerParameters support to the LXC driver. A single parameter of "cpu_shares" is added, which gets/sets the cpu.shares key in the domain's cgroup. I also added bits to "virsh schedinfo" to support this.
This brings get/set of U64 out of the #if 0, which looks messier than it is. diff -r 64f19f607bc6 -r 6fb284fa200a src/cgroup.c --- a/src/cgroup.c Fri Oct 03 17:58:02 2008 +0000 +++ b/src/cgroup.c Tue Oct 07 08:21:49 2008 -0700 @@ -224,26 +224,6 @@ return rc; } -#if 0 -/* This is included for completeness, but not yet used */ - -static int virCgroupSetValueI64(virCgroupPtr group, - const char *key, - int64_t value) -{ - char *strval = NULL; - int rc; - - if (asprintf(&strval, "%" PRIi64, value) == -1) - return -ENOMEM; - - rc = virCgroupSetValueStr(group, key, strval); - - VIR_FREE(strval); - - return rc; -} - static int virCgroupGetValueStr(virCgroupPtr group, const char *key, char **value) @@ -293,20 +273,21 @@ return rc; } -static int virCgroupGetValueU64(virCgroupPtr group, +#if 0 +/* This is included for completeness, but not yet used */ + +static int virCgroupSetValueI64(virCgroupPtr group, const char *key, - uint64_t *value) + int64_t value) { char *strval = NULL; - int rc = 0; + int rc; - rc = virCgroupGetValueStr(group, key, &strval); - if (rc != 0) - goto out; + if (asprintf(&strval, "%" PRIi64, value) == -1) + return -ENOMEM; - if (sscanf(strval, "%" SCNu64, value) != 1) - rc = -EINVAL; -out: + rc = virCgroupSetValueStr(group, key, strval); + VIR_FREE(strval); return rc; @@ -331,6 +312,25 @@ return rc; } #endif + +static int virCgroupGetValueU64(virCgroupPtr group, + const char *key, + uint64_t *value) +{ + char *strval = NULL; + int rc = 0; + + rc = virCgroupGetValueStr(group, key, &strval); + if (rc != 0) + goto out; + + if (sscanf(strval, "%" SCNu64, value) != 1) + rc = -EINVAL; +out: + VIR_FREE(strval); + + return rc; +} static int _virCgroupInherit(const char *path, const char *key) @@ -760,3 +760,13 @@ return rc; } + +int virCgroupSetCpuShares(virCgroupPtr group, unsigned long shares) +{ + return virCgroupSetValueU64(group, "cpu.shares", (uint64_t)shares); +} + +int virCgroupGetCpuShares(virCgroupPtr group, unsigned long *shares) +{ + return virCgroupGetValueU64(group, "cpu.shares", (uint64_t *)shares); +} diff -r 64f19f607bc6 -r 6fb284fa200a src/cgroup.h --- a/src/cgroup.h Fri Oct 03 17:58:02 2008 +0000 +++ b/src/cgroup.h Tue Oct 07 08:21:49 2008 -0700 @@ -36,6 +36,9 @@ int major, int minor); +int virCgroupSetCpuShares(virCgroupPtr group, unsigned long shares); +int virCgroupGetCpuShares(virCgroupPtr group, unsigned long *shares); + int virCgroupRemove(virCgroupPtr group); void virCgroupFree(virCgroupPtr *group);
diff -r 6fb284fa200a -r ebecbe5caa03 src/lxc_driver.c --- a/src/lxc_driver.c Tue Oct 07 08:21:49 2008 -0700 +++ b/src/lxc_driver.c Tue Oct 07 08:21:50 2008 -0700 @@ -35,6 +35,7 @@ #include <unistd.h> #include <wait.h> +#include "internal.h" #include "lxc_conf.h" #include "lxc_container.h" #include "lxc_driver.h" @@ -1149,6 +1150,94 @@ return 0; } +static char *lxcGetSchedulerType(virDomainPtr domain, int *nparams) +{ + if (nparams) + *nparams = 1; + + return strdup("cgroup"); +} + +static int lxcSetSchedulerParameters(virDomainPtr _domain, + virSchedParameterPtr params, + int nparams) +{ + int i; + int rc; + virCgroupPtr group; + virDomainObjPtr domain; + + if (virCgroupHaveSupport() != 0) + return 0; + + domain = virDomainFindByUUID(lxc_driver->domains, _domain->uuid); + if (domain == NULL) { + lxcError(NULL, _domain, VIR_ERR_INTERNAL_ERROR, + _("No such domain %s"), _domain->uuid); + return -EINVAL; + } + + rc = virCgroupForDomain(domain->def, "lxc", &group); + if (rc != 0) + return rc; + + for (i = 0; i < nparams; i++) { + virSchedParameterPtr param = ¶ms[i]; + + if (STREQ(param->field, "cpu_shares")) { + rc = virCgroupSetCpuShares(group, params[i].value.ui); + } else { + lxcError(NULL, _domain, VIR_ERR_INVALID_ARG, + _("Invalid parameter `%s'"), param->field); + rc = -ENOENT; + goto out; + } + } + + rc = 0; +out: + virCgroupFree(&group); + + return rc; +} + +static int lxcGetSchedulerParameters(virDomainPtr _domain, + virSchedParameterPtr params, + int *nparams) +{ + int rc = 0; + virCgroupPtr group; + virDomainObjPtr domain; + + if (virCgroupHaveSupport() != 0) + return 0; + + if ((*nparams) != 1) { + lxcError(NULL, _domain, VIR_ERR_INVALID_ARG, + _("Invalid parameter count")); + return -1; + } + + domain = virDomainFindByUUID(lxc_driver->domains, _domain->uuid); + if (domain == NULL) { + lxcError(NULL, _domain, VIR_ERR_INTERNAL_ERROR, + _("No such domain %s"), _domain->uuid); + return -ENOENT; + } + + rc = virCgroupForDomain(domain->def, "lxc", &group); + if (rc != 0) + return rc; + + rc = virCgroupGetCpuShares(group, (unsigned long *)¶ms[0].value.ul); + strncpy(params[0].field, "cpu_shares", sizeof(params[0].field)); + params[0].type = VIR_DOMAIN_SCHED_FIELD_ULLONG; + + virCgroupFree(&group); + + return rc; +} + /* Function Tables */ static virDriver lxcDriver = { VIR_DRV_LXC, /* the number virDrvNo */ @@ -1198,9 +1287,9 @@ NULL, /* domainDetachDevice */ NULL, /* domainGetAutostart */ NULL, /* domainSetAutostart */ - NULL, /* domainGetSchedulerType */ - NULL, /* domainGetSchedulerParameters */ - NULL, /* domainSetSchedulerParameters */ + lxcGetSchedulerType, /* domainGetSchedulerType */ + lxcGetSchedulerParameters, /* domainGetSchedulerParameters */ + lxcSetSchedulerParameters, /* domainSetSchedulerParameters */ NULL, /* domainMigratePrepare */ NULL, /* domainMigratePerform */ NULL, /* domainMigrateFinish */ @@ -1211,7 +1300,6 @@ NULL, /* nodeGetCellsFreeMemory */ NULL, /* getFreeMemory */ }; - static virStateDriver lxcStateDriver = { lxcStartup,
On Tue, Oct 07, 2008 at 08:30:21AM -0700, Dan Smith wrote:
diff -r 6fb284fa200a -r ebecbe5caa03 src/lxc_driver.c --- a/src/lxc_driver.c Tue Oct 07 08:21:49 2008 -0700 +++ b/src/lxc_driver.c Tue Oct 07 08:21:50 2008 -0700 @@ -35,6 +35,7 @@ #include <unistd.h> #include <wait.h>
+#include "internal.h" #include "lxc_conf.h" #include "lxc_container.h" #include "lxc_driver.h" @@ -1149,6 +1150,94 @@ return 0; }
+static char *lxcGetSchedulerType(virDomainPtr domain, int *nparams) +{ + if (nparams) + *nparams = 1; + + return strdup("cgroup"); +}
cgroups isn't really the schedular here - its just the mechanism for supplying the cpu_shares parameter to the schedular. I sort of imagined that for Linux based VMs we'd key this off the various schedulars available for sched_getscheduler(2), but actually thinking about it more, its probably best to just say that the schedular is 'posix', and make the choice of SCHED_FIFO, SCHED_RR, SCHED_OTHER, etc be a tunable parameter. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
DB> cgroups isn't really the schedular here - its just the mechanism DB> for supplying the cpu_shares parameter to the schedular. That's true, although I was thinking along the lines of how kconfig asks you for the method for grouping tasks, of which "cgroups" is an option. But, yeah. DB> I sort of imagined that for Linux based VMs we'd key this off the DB> various schedulars available for sched_getscheduler(2), but DB> actually thinking about it more, its probably best to just say DB> that the schedular is 'posix', and make the choice of SCHED_FIFO, DB> SCHED_RR, SCHED_OTHER, etc be a tunable parameter. Okay, sounds good. I'll change it. -- Dan Smith IBM Linux Technology Center Open Hypervisor Team email: danms@us.ibm.com
diff -r ebecbe5caa03 -r b07cd92a30e9 src/virsh.c --- a/src/virsh.c Tue Oct 07 08:21:50 2008 -0700 +++ b/src/virsh.c Tue Oct 07 08:21:50 2008 -0700 @@ -1114,6 +1114,7 @@ {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, gettext_noop("domain name, id or uuid")}, {"weight", VSH_OT_INT, VSH_OFLAG_NONE, gettext_noop("weight for XEN_CREDIT")}, {"cap", VSH_OT_INT, VSH_OFLAG_NONE, gettext_noop("cap for XEN_CREDIT")}, + {"cpu_shares", VSH_OT_INT, VSH_OFLAG_NONE, gettext_noop("cpu shares for LXC")}, {NULL, 0, 0, NULL} }; @@ -1128,11 +1129,14 @@ int nr_inputparams = 0; int inputparams = 0; int weightfound = 0; + int sharesfound = 0; int weight = 0; int capfound = 0; + int shares = 0; int cap = 0; char str_weight[] = "weight"; char str_cap[] = "cap"; + char str_shares[] = "cpu_shares"; int ret_val = FALSE; if (!vshConnectionUsability(ctl, ctl->conn, TRUE)) @@ -1152,6 +1156,7 @@ } } + /* Currently supports Xen Credit only */ if(vshCommandOptBool(cmd, "cap")) { cap = vshCommandOptInt(cmd, "cap", &capfound); if (!capfound) { @@ -1162,6 +1167,17 @@ } } + /* Currently supports LXC cgroup only */ + if(vshCommandOptBool(cmd, "cpu_shares")) { + shares = vshCommandOptInt(cmd, "cpu_shares", &sharesfound); + if (!sharesfound) { + vshError(ctl, FALSE, "%s", _("Invalid value of cpu_shares")); + goto cleanup; + } else { + nr_inputparams++; + } + } + params = vshMalloc(ctl, sizeof (virSchedParameter) * nr_inputparams); if (params == NULL) { goto cleanup; @@ -1180,7 +1196,13 @@ params[inputparams].value.ui = cap; inputparams++; } - /* End Currently supports Xen Credit only */ + + if (sharesfound) { + strncpy(params[inputparams].field,str_shares,sizeof(str_shares)); + params[inputparams].type = VIR_DOMAIN_SCHED_FIELD_UINT; + params[inputparams].value.ul = shares; + inputparams++; + } assert (inputparams == nr_inputparams);
On Tue, Oct 07, 2008 at 08:30:22AM -0700, Dan Smith wrote:
diff -r ebecbe5caa03 -r b07cd92a30e9 src/virsh.c --- a/src/virsh.c Tue Oct 07 08:21:50 2008 -0700 +++ b/src/virsh.c Tue Oct 07 08:21:50 2008 -0700 @@ -1114,6 +1114,7 @@ {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, gettext_noop("domain name, id or uuid")}, {"weight", VSH_OT_INT, VSH_OFLAG_NONE, gettext_noop("weight for XEN_CREDIT")}, {"cap", VSH_OT_INT, VSH_OFLAG_NONE, gettext_noop("cap for XEN_CREDIT")}, + {"cpu_shares", VSH_OT_INT, VSH_OFLAG_NONE, gettext_noop("cpu shares for LXC")},
Not your fault, but this schedinfo command is a really dumb wrt to option handling. Requiring that we add new options for every possible schedular param for every possible hypervisor is just crazy. Instead of using a syntax schedinfo --weight 10 it should use schedinfo --set weight=10 so it automatically has support for every parameter supported by the hypervisor in question. It can validate that the user suplied param name is correct by calling 'virDomainGetSchedularParams' to see what are available. We need to implement a --set option, and deprecate use of the existing --weight and --cap options in the help message/man page. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
DB> Not your fault, but this schedinfo command is a really dumb wrt to DB> option handling. Requiring that we add new options for every DB> possible schedular param for every possible hypervisor is just DB> crazy. Agreed. I actually just wrote this patch for my own testing, but decided to send it out as part of the rest anyway. DB> it should use DB> schedinfo --set weight=10 Agreed :) DB> so it automatically has support for every parameter supported by DB> the hypervisor in question. It can validate that the user suplied DB> param name is correct by calling 'virDomainGetSchedularParams' to DB> see what are available. I can do that, but since the CIM providers don't depend on virsh for this, can we move forward with the first two patches before we get this fixed up? -- Dan Smith IBM Linux Technology Center Open Hypervisor Team email: danms@us.ibm.com
participants (2)
-
Dan Smith -
Daniel P. Berrange