Re: [PATCH 3/4] lxc: Implement virtual /proc/cpuinfo via LXC fuse

On 2/16/20 2:11 PM, Julio Faracco wrote: > This commit tries to fix a lots of issues related to LXC VCPUs. One of Extra 'a' there. "tries to fix lots of issues ..." > them is related to /proc/cpuinfo content. If only 1 VCPU is set, LXC > containers will show all CPUs available for host. The second one is > related to CPU share, if an user set only 1 VCPU, the container/process > will use all available CPUs. (This is not the case when `cpuset` This parentesis is never closed. > attribute is declared. So, this commit adds a virtual cpuinfo based on > VCPU mapping and it automatic limits the CPU usage according VCPU count. "and it automatically limits ..." > > Example (now): > LXC container - 8 CPUS with 2 VCPU: > lxc-root# stress --cpu 8 > > On host machine, only CPU 0 and 1 have 100% usage. > > Signed-off-by: Julio Faracco <jcfaracco(a)gmail.com&gt; > --- > src/lxc/lxc_cgroup.c | 31 ++++++++++++++++ > src/lxc/lxc_container.c | 15 ++++++++ > src/lxc/lxc_fuse.c | 78 ++++++++++++++++++++++++++++++++++++++--- > 3 files changed, 120 insertions(+), 4 deletions(-) > > diff --git a/src/lxc/lxc_cgroup.c b/src/lxc/lxc_cgroup.c > index 470337e675..a6c73d9d55 100644 > --- a/src/lxc/lxc_cgroup.c > +++ b/src/lxc/lxc_cgroup.c > @@ -59,6 +59,34 @@ static int virLXCCgroupSetupCpuTune(virDomainDefPtr def, > } > > > +static int virLXCCgroupSetupVcpuAuto(virDomainDefPtr def, > + virCgroupPtr cgroup) > +{ > + size_t i; > + int vcpumax; > + virBuffer buffer = VIR_BUFFER_INITIALIZER; > + virBufferPtr cpuset = &buffer; > + > + vcpumax = virDomainDefGetVcpusMax(def); > + for (i = 0; i < vcpumax; i++) { > + virDomainVcpuDefPtr vcpu = virDomainDefGetVcpu(def, i); > + /* Cgroup is smart enough to convert numbers separated > + * by comma into ranges. Example: "0,1,2,5," -> "0-2,5". > + * Libvirt does not need to process it here. */ > + if (vcpu) > + virBufferAsprintf(cpuset, "%zu,", i); > + } > + if (virCgroupSetCpusetCpus(cgroup, > + virBufferCurrentContent(cpuset)) < 0) { > + virBufferFreeAndReset(cpuset); > + return -1; > + } > + > + virBufferFreeAndReset(cpuset); > + return 0; > +} > + > + > static int virLXCCgroupSetupCpusetTune(virDomainDefPtr def, > virCgroupPtr cgroup, > virBitmapPtr nodemask) > @@ -76,6 +104,9 @@ static int virLXCCgroupSetupCpusetTune(virDomainDefPtr def, > goto cleanup; > /* free mask to make sure we won't use it in a wrong way later */ > VIR_FREE(mask); > + } else { > + /* auto mode for VCPU limits */ > + virLXCCgroupSetupVcpuAuto(def, cgroup); > } > > if (virDomainNumatuneGetMode(def->numa, -1, &mode) < 0 || > diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c > index 41efe43a14..1a2c97c9f4 100644 > --- a/src/lxc/lxc_container.c > +++ b/src/lxc/lxc_container.c > @@ -999,6 +999,7 @@ static int lxcContainerMountProcFuse(virDomainDefPtr def, > { > int ret; > char *meminfo_path = NULL; > + char *cpuinfo_path = NULL; You can use g_autofree on cpuinfo_path to avoid the need for VIR_FREE() in the end. > > VIR_DEBUG("Mount /proc/meminfo stateDir=%s", stateDir); > > @@ -1013,7 +1014,21 @@ static int lxcContainerMountProcFuse(virDomainDefPtr def, > meminfo_path); > } > > + VIR_DEBUG("Mount /proc/cpuinfo stateDir=%s", stateDir); > + > + cpuinfo_path = g_strdup_printf("/.oldroot/%s/%s.fuse/cpuinfo", > + stateDir, > + def->name); > + > + if ((ret = mount(cpuinfo_path, "/proc/cpuinfo", > + NULL, MS_BIND, NULL)) < 0) { > + virReportSystemError(errno, > + _("Failed to mount %s on /proc/cpuinfo"), > + cpuinfo_path); > + } > + > VIR_FREE(meminfo_path); > + VIR_FREE(cpuinfo_path); > return ret; > } > #else > diff --git a/src/lxc/lxc_fuse.c b/src/lxc/lxc_fuse.c > index 44f240a0b5..12fa69d494 100644 > --- a/src/lxc/lxc_fuse.c > +++ b/src/lxc/lxc_fuse.c > @@ -37,6 +37,7 @@ > #if WITH_FUSE > > static const char *fuse_meminfo_path = "/meminfo"; > +static const char *fuse_cpuinfo_path = "/cpuinfo"; > > static int lxcProcGetattr(const char *path, struct stat *stbuf) > { > @@ -54,7 +55,8 @@ static int lxcProcGetattr(const char *path, struct stat *stbuf) > if (STREQ(path, "/")) { > stbuf->st_mode = S_IFDIR | 0755; > stbuf->st_nlink = 2; > - } else if (STREQ(path, fuse_meminfo_path)) { > + } else if (STREQ(path, fuse_meminfo_path) || > + STREQ(path, fuse_cpuinfo_path)) { > if (stat(mempath, &sb) < 0) { > res = -errno; > goto cleanup; > @@ -90,6 +92,7 @@ static int lxcProcReaddir(const char *path, void *buf, > filler(buf, ".", NULL, 0); > filler(buf, "..", NULL, 0); > filler(buf, fuse_meminfo_path + 1, NULL, 0); > + filler(buf, fuse_cpuinfo_path + 1, NULL, 0); > > return 0; > } > @@ -97,7 +100,8 @@ static int lxcProcReaddir(const char *path, void *buf, > static int lxcProcOpen(const char *path G_GNUC_UNUSED, > struct fuse_file_info *fi G_GNUC_UNUSED) > { > - if (STRNEQ(path, fuse_meminfo_path)) > + if (STRNEQ(path, fuse_meminfo_path) && > + STRNEQ(path, fuse_cpuinfo_path)) > return -ENOENT; > > if ((fi->flags & 3) != O_RDONLY) > @@ -125,7 +129,7 @@ static int lxcProcHostRead(char *path, char *buf, size_t size, off_t offset) > static int lxcProcReadMeminfo(char *hostpath, virDomainDefPtr def, > char *buf, size_t size, off_t offset) > { > - int res; > + int res = -1; > FILE *fd = NULL; > char *line = NULL; > size_t n; > @@ -151,7 +155,6 @@ static int lxcProcReadMeminfo(char *hostpath, virDomainDefPtr def, > goto cleanup; > } > > - res = -1; > while (getline(&line, &n, fd) > 0) { > char *ptr = strchr(line, ':'); > if (!ptr) > @@ -235,6 +238,70 @@ static int lxcProcReadMeminfo(char *hostpath, virDomainDefPtr def, > return res; > } > > + > +static int lxcProcReadCpuinfo(char *hostpath, virDomainDefPtr def, > + char *buf, size_t size, off_t offset) > +{ > + int res = -1; > + FILE *fd = NULL; I believe that instead of creating the fd as FILE you can use VIR_AUTOCLOSE fd = -1; And it will auto close the fd for you when context is gone. Then you won't need VIR_FORCE_FCLOSE(). > + char *line = NULL; 'line' can use g_autofree as well. > + size_t n; > + virBuffer buffer = VIR_BUFFER_INITIALIZER; > + virBufferPtr new_cpuinfo = &buffer; > + size_t cpu; > + size_t nvcpu; > + size_t curcpu = 0; > + bool get_proc = false; > + > + fd = fopen(hostpath, "r"); Suggestion: fopen() here is fine, but I believe that you can use utilities like virFileReadAll() as well. It will give you a buffer with the file contents so you don't need to rely on sscanf() and getline() in the loop below.

> + if (fd == NULL) { > + virReportSystemError(errno, _("Cannot open %s"), hostpath); > + res = -errno; > + goto cleanup; > + } > + > + /* /proc/cpuinfo does not support fseek */ > + if (offset > 0) { > + res = 0; > + goto cleanup; > + } > + > + nvcpu = virDomainDefGetVcpus(def); > + while (getline(&line, &n, fd) > 0) { > + if (sscanf(line, "processor\t: %zu", &cpu) == 1) { > + virDomainVcpuDefPtr vcpu = virDomainDefGetVcpu(def, cpu); > + /* VCPU is mapped */ > + if (vcpu) { > + if (curcpu == nvcpu) > + break; > + > + virBufferAsprintf(new_cpuinfo, "processor\t: %zu\n", > + curcpu); > + curcpu++; > + get_proc = true; > + } else { > + get_proc = false; > + } > + } else { > + /* It is not a processor index */ > + if (get_proc) > + virBufferAdd(new_cpuinfo, line, -1); > + } > + } > + > + res = strlen(virBufferCurrentContent(new_cpuinfo)); > + if (res > size) > + res = size; > + memcpy(buf, virBufferCurrentContent(new_cpuinfo), res); > + > + cleanup: > + VIR_FREE(line); > + virBufferFreeAndReset(new_cpuinfo); > + VIR_FORCE_FCLOSE(fd); > + return res; > +} > + > + > static int lxcProcRead(const char *path G_GNUC_UNUSED, > char *buf G_GNUC_UNUSED, > size_t size G_GNUC_UNUSED, > @@ -254,6 +321,9 @@ static int lxcProcRead(const char *path G_GNUC_UNUSED, > if (STREQ(path, fuse_meminfo_path)) { > if ((res = lxcProcReadMeminfo(hostpath, def, buf, size, offset)) < 0) > res = lxcProcHostRead(hostpath, buf, size, offset); > + } else if (STREQ(path, fuse_cpuinfo_path)) { > + if ((res = lxcProcReadCpuinfo(hostpath, def, buf, size, offset)) < 0) > + res = lxcProcHostRead(hostpath, buf, size, offset); > } > > VIR_FREE(hostpath); >