Thank you for your contribution! Sashiko AI review found 3 potential issues to consider: - [Low] selftests/ve: cgroup2 mount fd leaked on early FIXTURE_SETUP failure - [Low] selftests/ve: VE cgroup left behind when FIXTURE_SETUP fails after make_ve() - [Low] selftests/ve: test models possible CPUs with sysconf(_SC_NPROCESSORS_CONF) --
commit 5c946723cd7135b742e593bb7b560ea1869bd370 Author: Pavel Tikhomirov <[email protected]> selftests/ve: check the AIO ring size in a pseudosuper VE This adds a selftest, ve_aio_ring_test, that checks the io_setup() ring size on the host, in a VE, and in a pseudosuper VE. It checks that the host and a VE keep the CPU heuristic, that a pseudosuper VE skips it, and that io_setup((nr - 2) / 2) in a pseudosuper VE gives a ring of nr events for rings of 1 to 4 pages. > diff --git a/tools/testing/selftests/ve/ve_aio_ring_test.c > b/tools/testing/selftests/ve/ve_aio_ring_test.c > new file mode 100644 > index 000000000000..c37094157422 > --- /dev/null > +++ b/tools/testing/selftests/ve/ve_aio_ring_test.c [ ... ] > +FIXTURE_SETUP(ve_aio_ring) > +{ > + long cpus; > + > + self->ctid = -1; > + self->arg = MAP_FAILED; > + > + self->page_size = sysconf(_SC_PAGESIZE); > + cpus = sysconf(_SC_NPROCESSORS_CONF); > + ASSERT_GT(cpus, 0); > + self->cpus = cpus; [Severity: Low] Does sysconf(_SC_NPROCESSORS_CONF) really give the number of possible CPUs that ring_nr() needs? The comments and the skip message call it "possible CPUs", and ioctx_alloc() uses num_possible_cpus(). As far as I can tell, glibc derives this value from the cpuN entries under /sys/devices/system/cpu. Those are registered only for present CPUs (see cpu_dev_register_generic()). On a VM or host with possible-but-not-present CPUs, such as hot-pluggable vCPUs or possible_cpus= on the command line, the count can be lower than num_possible_cpus(). The expected ring sizes then no longer match the kernel's, so the test fails spuriously, or the skip check wrongly skips the pseudosuper checks. Would parsing /sys/devices/system/cpu/possible be more reliable? > + if (ring_nr(self, 1) == ring_nr_pseudosuper(self, 1)) > + SKIP(return, "%u possible CPUs are too few for the CPU > heuristic to matter", > + self->cpus); > + > + self->arg = mmap(NULL, sizeof(*self->arg), PROT_READ | PROT_WRITE, > + MAP_SHARED | MAP_ANONYMOUS, -1, 0); > + ASSERT_NE(self->arg, MAP_FAILED); > + > + self->cgv2_fd = mount_cg2_fd(); > + ASSERT_GE(self->cgv2_fd, 0); > + > + ASSERT_EQ(write_file_at(self->cgv2_fd, "cgroup.subtree_control", > + VE_CONTROLLERS), 0); > + > + self->ctid = make_ve(self->cgv2_fd, CTID_MIN); > + ASSERT_GE(self->ctid, 0); [Severity: Low] Does this leak the cgroup2 fd when one of these ASSERTs fails? If write_file_at() or make_ve() fails after mount_cg2_fd() succeeded, ctid is still -1. FIXTURE_TEARDOWN() then skips close(self->cgv2_fd), because the close sits under "if (self->ctid >= 0)". The harness also does not run teardown at all when setup does not complete, so the fd stays open either way. The impact is small, since the test process exits soon afterwards. > + > + ASSERT_EQ(prepare_namespaces(&self->ns_ctx, CLONE_NEWVE, > + self->cgv2_fd, self->ctid), 0); > +} [Severity: Low] Is the VE left behind if prepare_namespaces() fails? make_ve() has already created the VE cgroup by then. The harness runs FIXTURE_TEARDOWN() only if setup completed, so destroy_ve() never runs. The CTID then stays in the cgroup2 hierarchy, and later runs find it already taken. -- Sashiko AI review ยท https://sashiko.vzint.dev/#/patchset/20261113-pr-ptikhomirov-vstor-154129-aio-pseudosuper-ring-v1-0-7d35d143f...@virtuozzo.com?part=2 _______________________________________________ Devel mailing list [email protected] https://lists.openvz.org/mailman/listinfo/devel
