On 1/25/22 17:19, Praveen K Paladugu wrote:
> From: Vineeth Pillai <viremana@xxxxxxxxxxxxxxxxxxx>
>
> Signed-off-by: Vineeth Pillai <viremana@xxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Praveen K Paladugu <prapal@xxxxxxxxxxxxxxxxxxx>
> ---
> src/ch/ch_driver.c | 260 +++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 260 insertions(+)
>
> diff --git a/src/ch/ch_driver.c b/src/ch/ch_driver.c
> index 32ae15f940..d257c025ef 100644
> --- a/src/ch/ch_driver.c
> +++ b/src/ch/ch_driver.c
> @@ -43,6 +43,7 @@
> #include "viruri.h"
> #include "virutil.h"
> #include "viruuid.h"
> +#include "virnuma.h"
>
> #define VIR_FROM_THIS VIR_FROM_CH
>
> @@ -1302,6 +1303,263 @@ chDomainPinVcpu(virDomainPtr dom,
> VIR_DOMAIN_AFFECT_LIVE);
> }
>
> +#define CH_NB_NUMA_PARAM 2
> +
> +static int
> +chDomainGetNumaParameters(virDomainPtr dom,
> + virTypedParameterPtr params,
> + int *nparams,
> + unsigned int flags)
> +{
> + size_t i;
> + virDomainObj *vm = NULL;
> + virDomainNumatuneMemMode tmpmode = VIR_DOMAIN_NUMATUNE_MEM_STRICT;
> + virCHDomainObjPrivate *priv;
> + g_autofree char *nodeset = NULL;
> + int ret = -1;
> + virDomainDef *def = NULL;
> + bool live = false;
> + virBitmap *autoNodeset = NULL;
> +
> + virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
> + VIR_DOMAIN_AFFECT_CONFIG |
> + VIR_TYPED_PARAM_STRING_OKAY, -1);
> +
> + if (!(vm = virCHDomainObjFromDomain(dom)))
> + return -1;
> + priv = vm->privateData;
> +
> + if (virDomainGetNumaParametersEnsureACL(dom->conn, vm->def) < 0)
> + goto cleanup;
> +
> + if (!(def = virDomainObjGetOneDefState(vm, flags, &live)))
> + goto cleanup;
> +
> + if (live)
> + autoNodeset = priv->autoNodeset;
> +
> + if ((*nparams) == 0) {
> + *nparams = CH_NB_NUMA_PARAM;
> + ret = 0;
> + goto cleanup;
> + }
> +
> + for (i = 0; i < CH_NB_NUMA_PARAM && i < *nparams; i++) {
> + virMemoryParameterPtr param = ¶ms[i];
> +
> + switch (i) {
> + case 0: /* fill numa mode here */
> + ignore_value(virDomainNumatuneGetMode(def->numa, -1, &tmpmode));
> +
> + if (virTypedParameterAssign(param, VIR_DOMAIN_NUMA_MODE,
> + VIR_TYPED_PARAM_INT, tmpmode) < 0)
> + goto cleanup;
> +
> + break;
> +
> + case 1: /* fill numa nodeset here */
> + nodeset = virDomainNumatuneFormatNodeset(def->numa, autoNodeset, -1);
> +
> + if (!nodeset ||
> + virTypedParameterAssign(param, VIR_DOMAIN_NUMA_NODESET,
> + VIR_TYPED_PARAM_STRING, nodeset) < 0)
> + goto cleanup;
> +
> + nodeset = NULL;
> + break;
> +
> + /* coverity[dead_error_begin] */
I don't think this is correct. Does coverity really complain?
> + default:
> + break;
> + /* should not hit here */
> + }
> + }
> +
> + if (*nparams > CH_NB_NUMA_PARAM)
> + *nparams = CH_NB_NUMA_PARAM;
> + ret = 0;
> +
> + cleanup:
> + virDomainObjEndAPI(&vm);
> + return ret;
> +}
> +
> +static int
> +chDomainSetNumaParamsLive(virDomainObj *vm,
> + virBitmap *nodeset)
> +{
> + g_autoptr(virCgroup) cgroup_temp = NULL;
> + virCHDomainObjPrivate *priv = vm->privateData;
> + g_autofree char *nodeset_str = NULL;
> + virDomainNumatuneMemMode mode;
> + size_t i = 0;
> +
> +
> + if (virDomainNumatuneGetMode(vm->def->numa, -1, &mode) == 0 &&
> + mode != VIR_DOMAIN_NUMATUNE_MEM_STRICT) {
> + virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> + _("change of nodeset for running domain requires strict numa mode"));
> + return -1;
> + }
> +
> + if (!virNumaNodesetIsAvailable(nodeset))
> + return -1;
> +
> + /* Ensure the cpuset string is formatted before passing to cgroup */
> + if (!(nodeset_str = virBitmapFormat(nodeset)))
> + return -1;
> +
> + if (virCgroupNewThread(priv->cgroup, VIR_CGROUP_THREAD_EMULATOR, 0,
> + false, &cgroup_temp) < 0 ||
> + virCgroupSetCpusetMems(cgroup_temp, nodeset_str) < 0)
> + return -1;
> +
> +
> + for (i = 0; i < virDomainDefGetVcpusMax(vm->def); i++) {
> + virDomainVcpuDef *vcpu = virDomainDefGetVcpu(vm->def, i);
> +
> + if (!vcpu->online)
> + continue;
> +
> + if (virCgroupNewThread(priv->cgroup, VIR_CGROUP_THREAD_VCPU, i,
> + false, &cgroup_temp) < 0 ||
> + virCgroupSetCpusetMems(cgroup_temp, nodeset_str) < 0)
> + return -1;
> +
> + return 0;
> + }
> +
If anything, coverity should complain about the block below. Because of
this return 0 ^^^ the block below is effectivelly a dead code.
> + for (i = 0; i < vm->def->niothreadids; i++) {
> + if (virCgroupNewThread(priv->cgroup, VIR_CGROUP_THREAD_IOTHREAD,
> + vm->def->iothreadids[i]->iothread_id,
> + false, &cgroup_temp) < 0 ||
> + virCgroupSetCpusetMems(cgroup_temp, nodeset_str) < 0)
> + return -1;
> + }
> +
> + /* set nodeset for root cgroup */
> + if (virCgroupSetCpusetMems(priv->cgroup, nodeset_str) < 0)
> + return -1;
> +
> + return 0;
> +}
> +
> +static int
> +chDomainSetNumaParameters(virDomainPtr dom,
> + virTypedParameterPtr params,
> + int nparams,
> + unsigned int flags)
> +{
> + virCHDriver *driver = dom->conn->privateData;
> + size_t i;
> + virDomainDef *def;
> + virDomainDef *persistentDef;
> + virDomainObj *vm = NULL;
> + int ret = -1;
> + g_autoptr(virCHDriverConfig) cfg = NULL;
> + virCHDomainObjPrivate *priv;
> + virBitmap *nodeset = NULL;
> + virDomainNumatuneMemMode config_mode;
> + int mode = -1;
> +
> + virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
> + VIR_DOMAIN_AFFECT_CONFIG, -1);
> +
> + if (virTypedParamsValidate(params, nparams,
> + VIR_DOMAIN_NUMA_MODE,
> + VIR_TYPED_PARAM_INT,
> + VIR_DOMAIN_NUMA_NODESET,
> + VIR_TYPED_PARAM_STRING,
> + NULL) < 0)
> + return -1;
> +
> + if (!(vm = virCHDomainObjFromDomain(dom)))
> + return -1;
> +
> + priv = vm->privateData;
> + cfg = virCHDriverGetConfig(driver);
> +
> + if (virDomainSetNumaParametersEnsureACL(dom->conn, vm->def, flags) < 0)
> + goto cleanup;
> +
> + for (i = 0; i < nparams; i++) {
> + virTypedParameterPtr param = ¶ms[i];
> +
> + if (STREQ(param->field, VIR_DOMAIN_NUMA_MODE)) {
> + mode = param->value.i;
> +
> + if (mode < 0 || mode >= VIR_DOMAIN_NUMATUNE_MEM_LAST) {
> + virReportError(VIR_ERR_INVALID_ARG,
> + _("unsupported numatune mode: '%d'"), mode);
> + goto cleanup;
> + }
> +
> + } else if (STREQ(param->field, VIR_DOMAIN_NUMA_NODESET)) {
> + if (virBitmapParse(param->value.s, &nodeset,
> + VIR_DOMAIN_CPUMASK_LEN) < 0)
> + goto cleanup;
> +
> + if (virBitmapIsAllClear(nodeset)) {
> + virReportError(VIR_ERR_OPERATION_INVALID,
> + _("Invalid nodeset of 'numatune': %s"),
> + param->value.s);
> + goto cleanup;
> + }
> + }
> + }
> +
> + if (virCHDomainObjBeginJob(vm, CH_JOB_MODIFY) < 0)
> + goto cleanup;
> +
> + if (virDomainObjGetDefs(vm, flags, &def, &persistentDef) < 0)
This @persistentDef is never used. I know CH driver doesn't store XMLs
on the disk yet, but it does implement virDomainDefineXML() API (and
holds the domain definitions in memory).
Firstly, we should really address that, because with socket activation
it's going to drive users mad. I mean, if a client connects, defines a
domain, disconnects, waits for 2 minutes the daemon dies and domain is
gone. This is all because of the --timeout=120 argument we are passing
to daemon(s) (both monolithic daemon and driver daemons). What's the
reason we are not storing the XML?
Secondly, leaving --timeout= asaide, since virDomainDefineXML() is
defined an user can define a domain and then call APIs to change it. For
instance, it could call virDomainSetNumaParameters() to change the
domain definition.
> + goto endjob;
> +
> + if (def) {
> + if (!driver->privileged) {
> + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
> + _("NUMA tuning is not available in session mode"));
> + goto endjob;
> + }
> +
> + if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPUSET)) {
> + virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> + _("cgroup cpuset controller is not mounted"));
> + goto endjob;
> + }
> +
> + if (mode != -1 &&
> + virDomainNumatuneGetMode(def->numa, -1, &config_mode) == 0 &&
> + config_mode != mode) {
> + virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> + _("can't change numatune mode for running domain"));
> + goto endjob;
> + }
> +
> + if (nodeset &&
> + chDomainSetNumaParamsLive(vm, nodeset) < 0)
> + goto endjob;
> +
> + if (virDomainNumatuneSet(def->numa,
> + def->placement_mode ==
> + VIR_DOMAIN_CPU_PLACEMENT_MODE_STATIC,
> + -1, mode, nodeset) < 0)
> + goto endjob;
Meanwhile, in QEMU driver we've disabled changing of NUMA nodeset for
static mode, because we can't guarantee that QEMU will move all of its
memory to the new nodeset. How does CH behave in this case? I'm going to
merge these patches, but we should figure out whether CH behaves the
same (which I suspect it does), and deny the operation. Can you please
cook a patch in that case?
Michal
