[1.1] fix intelrdt #3978

In case resctrl filesystem can not be found in /proc/self/mountinfo (which is pretty common on non-server or non-x86 hardware), subsequent calls to Root() will result in parsing it again and again. Use sync.Once to avoid it. Make unit tests call it so that Root() won't actually parse mountinfo in tests. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com> (cherry picked from commit 02e961b) Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

This test was written back in the day when findIntelRdtMountpointDir was using its own mountinfo parser. Commit f1c1fdf changed that, and thus this test is actually testing moby/sys/mountinfo parser, which does not make much sense. Remove the test, and drop the io.Reader argument since we no longer need to parse a custom file. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com> (cherry picked from commit 6c6b14e) Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

In a (quite common) case RDT is not supported by the kernel/hardware, it does not make sense to parse /proc/cpuinfo and /proc/self/mountinfo, and yet the current code does it (on every runc exec, for example). Fortunately, there is a quick way to check whether RDT is available -- if so, kernel creates /sys/fs/resctrl directory. Check its existence, and skip all the other initialization if it's not present. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com> (cherry picked from commit edeb3b3) Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

For the Nth time I wanted to replace parsing mountinfo with statfs and the check for superblock magic, but it is not possible since some code relies of mount options check which can only be obtained via mountinfo. Add a note about it. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com> (cherry picked from commit 5e201e7) Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

TestGetContainerStats test a function that is smaller than the test itself, and only calls a couple of other functions (which are represented by mocks). It does not make sense to have it. mockIntelRdtManager is only needed for TestGetContainerStats and TestGetContainerState, which basically tests that Path is called. Also, it does not make much sense to have it. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com> (cherry picked from commit 8593285) Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

[This is a manual port of commit dbd990d to release-1.1 branch. Original description follows.] Remove intelrtd.Manager interface, since we only have a single implementation, and do not expect another one. Rename intelRdtManager to Manager, and modify its users accordingly. Remove NewIntelRdtManager from factory. Remove IntelRdtfs. Instead, make intelrdt.NewManager return nil if the feature is not available. Remove TestFactoryNewIntelRdt as it is now identical to TestFactoryNew. Add internal function newManager to be used for tests (to make sure some testing is done even when the feature is not available in kernel/hardware). Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

This function is unused, and removing it simplifies the changes which follow this commit. Signed-off-by: Cory Snider <csnider@mirantis.com> (cherry picked from commit 13674f4) Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

Reading /proc/cpuinfo is a surprisingly expensive operation. Since kernel version 4.12 [1], opening /proc/cpuinfo on an x86 system can block for around 20 milliseconds while the kernel samples the current CPU frequency. There is a very recent patch [2] which gets rid of the delay, but has yet to make it into the mainline kenel. Regardless, kernels for which opening /proc/cpuinfo takes 20ms will continue to be run in production for years to come. libcontainer only opens /proc/cpuinfo to read the processor feature flags so all the delays to get an accurate snapshot of the CPU frequency are just wasted time. If we wanted to, we could interrogate the CPU features directly from userspace using the `CPUID` instruction. However, Intel and AMD CPUs have flags in different positions for their analogous sub-features and there are CPU quirks [3] which would need to be accounted for. Some Haswell server CPUs support RDT/CAT but are missing the `CPUID` flags advertising their support; the kernel checks for support on that processor family by probing the the hardware using privileged RDMSR/WRMSR instructions [4]. This sort of probing could not be implemented in userspace so it would not be possible to check for RDT feature support in userspace without false negatives on some hardware configurations. It looks like libcontainer reads the CPU feature flags as a kind of optimization so that it can skip checking whether the kernel supports an RDT sub-feature if the hardware support is missing. As the kernel only exposes subtrees in the `resctrl` filesystem for RDT sub-features with hardware and kernel support, checking the CPU feature flags is redundant from a correctness point of view. Remove the /proc/cpuinfo check as it is an optimization which actually hurts performance. [1]: https://unix.stackexchange.com/a/526679 [2]: https://lore.kernel.org/all/20220415161206.875029458@linutronix.de/ [3]: https://github.com/torvalds/linux/blob/7cf6a8a17f5b134b7e783c2d45c53298faef82a7/arch/x86/kernel/cpu/resctrl/core.c#L834-L851 [4]: https://github.com/torvalds/linux/blob/a6b450573b912316ad36262bfc70e7c3870c56d1/arch/x86/kernel/cpu/resctrl/core.c#L111-L153 Signed-off-by: Cory Snider <csnider@mirantis.com> (cherry picked from commit 9f10748) Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

The intelrdt package only needs to parse mountinfo to find the mount point of the resctrl filesystem. Users are generally going to mount the resctrl filesystem to the pre-created /sys/fs/resctrl directory, so there is a common case where mountinfo parsing is not required. Optimize for the common case with a fast path which checks both for the existence of the /sys/fs/resctrl directory and whether the resctrl filesystem was mounted to that path using a single statfs syscall. Signed-off-by: Cory Snider <csnider@mirantis.com> (cherry picked from commit c156bde) Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

The OCI runtime spec mandates "[i]f intelRdt is not set, the runtime MUST NOT manipulate any resctrl pseudo-filesystems." Attempting to delete files counts as manipulating, so stop doing that when the container's RDT configuration is nil. Signed-off-by: Cory Snider <csnider@mirantis.com> (cherry picked from commit 56daf36) Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

Unless the container's runtime config has intelRdt configuration set, any checks for whether Intel RDT is supported or the resctrl filesystem is mounted are a waste of time as, per the OCI Runtime Spec, "the runtime MUST NOT manipulate any resctrl pseudo-filesystems." And in the likely case where Intel RDT is supported by both the hardware and kernel but the resctrl filesystem is not mounted, these checks can get expensive as the intelrdt package needs to parse mountinfo to check whether the filesystem has been mounted to a non-standard path. Optimize for the common case of containers with no intelRdt configuration by only performing the checks when the container has opted in. Signed-off-by: Cory Snider <csnider@mirantis.com> (cherry picked from commit ea0bd78) Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[1.1] fix intelrdt #3978

[1.1] fix intelrdt #3978

Commits on Aug 10, 2023