[libvirt PATCH v5 2/7] ch: methods for cgroup mgmt in ch driver

[Michal Prívozník]

On 1/25/22 17:19, Praveen K Paladugu wrote:
> From: Vineeth Pillai <viremana at linux.microsoft.com>
> 
> Signed-off-by: Vineeth Pillai <viremana at linux.microsoft.com>
> Signed-off-by: Praveen K Paladugu <prapal at linux.microsoft.com>
> ---
>  src/ch/ch_conf.c    |   2 +
>  src/ch/ch_conf.h    |   4 +-
>  src/ch/ch_domain.c  |  34 +++++
>  src/ch/ch_domain.h  |  11 +-
>  src/ch/ch_monitor.c |  96 ++++++++++++++
>  src/ch/ch_monitor.h |  54 +++++++-
>  src/ch/ch_process.c | 308 ++++++++++++++++++++++++++++++++++++++++++--
>  src/ch/ch_process.h |   3 +
>  8 files changed, 492 insertions(+), 20 deletions(-)
> 

> diff --git a/src/ch/ch_domain.c b/src/ch/ch_domain.c
> index a746d0f5fd..6f0cec8c6e 100644
> --- a/src/ch/ch_domain.c
> +++ b/src/ch/ch_domain.c
> @@ -319,6 +319,40 @@ chValidateDomainDeviceDef(const virDomainDeviceDef *dev,
>                         _("Serial can only be enabled for a PTY"));
>          return -1;
>      }
> +    return 0;
> +}
> +
> +int
> +virCHDomainRefreshThreadInfo(virDomainObj *vm)
> +{
> +    size_t maxvcpus = virDomainDefGetVcpusMax(vm->def);
> +    virCHMonitorThreadInfo *info = NULL;
> +    size_t nthreads, ncpus = 0;

We like one variable per line more, because it allows smaller diffs
should this area of code be ever changed.

> +    size_t i;
> +
> +    nthreads = virCHMonitorGetThreadInfo(virCHDomainGetMonitor(vm),
> +                                         true, &info);
> +
> +    for (i = 0; i < nthreads; i++) {
> +        virCHDomainVcpuPrivate *vcpupriv;
> +        virDomainVcpuDef *vcpu;
> +        virCHMonitorCPUInfo *vcpuInfo;
> +
> +        if (info[i].type != virCHThreadTypeVcpu)
> +            continue;
> +
> +        /* TODO: hotplug support */
> +        vcpuInfo = &info[i].vcpuInfo;
> +        vcpu = virDomainDefGetVcpu(vm->def, vcpuInfo->cpuid);
> +        vcpupriv = CH_DOMAIN_VCPU_PRIVATE(vcpu);
> +        vcpupriv->tid = vcpuInfo->tid;
> +        ncpus++;
> +    }
> +
> +    /* TODO: Remove the warning when hotplug is implemented.*/
> +    if (ncpus != maxvcpus)
> +        VIR_WARN("Mismatch in the number of cpus, expected: %ld, actual: %ld",
> +                 maxvcpus, ncpus);
>  
>      return 0;
>  }
> diff --git a/src/ch/ch_domain.h b/src/ch/ch_domain.h
> index 4d0b5479b8..cb94905b94 100644
> --- a/src/ch/ch_domain.h
> +++ b/src/ch/ch_domain.h
> @@ -53,11 +53,13 @@ typedef struct _virCHDomainObjPrivate virCHDomainObjPrivate;
>  struct _virCHDomainObjPrivate {
>      struct virCHDomainJobObj job;
>  
> -    virChrdevs *chrdevs;
> -    virCHDriver *driver;
> -    virCHMonitor *monitor;
>      char *machineName;
>      virBitmap *autoCpuset;
> +    virBitmap *autoNodeset;
> +    virCHDriver *driver;
> +    virCHMonitor *monitor;
> +    virCgroup *cgroup;
> +    virChrdevs *chrdevs;

Looks like you can't make up your mind about the order. I remember
seeing the order of the members being changed over and over. Any reason
for that?

>  };
>  
>  #define CH_DOMAIN_PRIVATE(vm) \
> @@ -87,7 +89,8 @@ void
>  virCHDomainObjEndJob(virDomainObj *obj);
>  
>  int
> -virCHDomainRefreshVcpuInfo(virDomainObj *vm);
> +virCHDomainRefreshThreadInfo(virDomainObj *vm);
> +
>  pid_t
>  virCHDomainGetVcpuPid(virDomainObj *vm,
>                        unsigned int vcpuid);
> diff --git a/src/ch/ch_monitor.c b/src/ch/ch_monitor.c
> index a19f0c7e33..d984bf9822 100644
> --- a/src/ch/ch_monitor.c
> +++ b/src/ch/ch_monitor.c
> @@ -41,6 +41,7 @@ VIR_LOG_INIT("ch.ch_monitor");
>  
>  static virClass *virCHMonitorClass;
>  static void virCHMonitorDispose(void *obj);
> +static void virCHMonitorThreadInfoFree(virCHMonitor * mon);
>  
>  static int virCHMonitorOnceInit(void)
>  {
> @@ -578,6 +579,7 @@ static void virCHMonitorDispose(void *opaque)
>      virCHMonitor *mon = opaque;
>  
>      VIR_DEBUG("mon=%p", mon);
> +    virCHMonitorThreadInfoFree(mon);
>      virObjectUnref(mon->vm);
>  }
>  
> @@ -743,6 +745,100 @@ virCHMonitorGet(virCHMonitor *mon, const char *endpoint, virJSONValue **response
>      return ret;
>  }
>  
> +static void
> +virCHMonitorThreadInfoFree(virCHMonitor *mon)
> +{
> +    mon->nthreads = 0;
> +    if (mon->threads)
> +        VIR_FREE(mon->threads);

Since VIR_FREE() is really just g_clear_pointer() there's no point in
checking the pointer. In fact, glib will do that for us.

> +}
> +
> +static size_t
> +virCHMonitorRefreshThreadInfo(virCHMonitor *mon)
> +{
> +    virCHMonitorThreadInfo *info = NULL;
> +    g_autofree pid_t *tids = NULL;
> +    virDomainObj *vm = mon->vm;
> +    size_t ntids = 0;
> +    size_t i;
> +
> +
> +    virCHMonitorThreadInfoFree(mon);
> +    if (virProcessGetPids(vm->pid, &ntids, &tids) < 0) {
> +        mon->threads = NULL;

No need to set to NULL. It was done by virCHMonitorThreadInfoFree() above.

> +        return 0;
> +    }
> +

> diff --git a/src/ch/ch_process.c b/src/ch/ch_process.c
> index 49976d769e..1a0730a4d1 100644
> --- a/src/ch/ch_process.c
> +++ b/src/ch/ch_process.c

> +/**
> + * virCHProcessSetupPid:
> + *
> + * This function sets resource properties (affinity, cgroups,
> + * scheduler) for any PID associated with a domain.  It should be used
> + * to set up emulator PIDs as well as vCPU and I/O thread pids to
> + * ensure they are all handled the same way.
> + *
> + * Returns 0 on success, -1 on error.
> + */
> +static int
> +virCHProcessSetupPid(virDomainObj *vm,
> +                     pid_t pid,
> +                     virCgroupThreadName nameval,
> +                     int id,
> +                     virBitmap *cpumask,
> +                     unsigned long long period,
> +                     long long quota,
> +                     virDomainThreadSchedParam *sched)
> +{
> +    virCHDomainObjPrivate *priv = vm->privateData;
> +    virDomainNumatuneMemMode mem_mode;
> +    virCgroup *cgroup = NULL;

g_autoptr(virCgroup)

> +    virBitmap *use_cpumask = NULL;
> +    virBitmap *affinity_cpumask = NULL;
> +    g_autoptr(virBitmap) hostcpumap = NULL;
> +    g_autofree char *mem_mask = NULL;
> +    int ret = -1;

Michal