libct/intelrdt: skip reading /proc/cpuinfo

[corhere]

• Relates to runc exec is 5x slower than crun exec #3181

Reading /proc/cpuinfo is a surprisingly expensive operation. Since kernel version 4.12, 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 which gets rid of the delay, but has yet to make it into the mainline kernel. 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 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. 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.

Benchmarks: runc run is 17 ms faster when "intelRdt" is enabled in config.json

• Hardware: AWS EC2 c5n.metal
• OS: Ubuntu 20.04 LTS
• /sys/fs/resctrl mounted
• OCI config:
  • "args": ["/bin/true"]
  • "linux": {"intelRdt": {"memBwSchema": "MB:0=20;1=70"}}

	3f0daac	This PR
Run 1	real 0m0.098s user 0m0.029s sys 0m0.043s	real 0m0.092s user 0m0.023s sys 0m0.048s
Run 2	real 0m0.110s user 0m0.019s sys 0m0.052s	real 0m0.074s user 0m0.022s sys 0m0.047s
Run 3	real 0m0.087s user 0m0.019s sys 0m0.051s	real 0m0.077s user 0m0.025s sys 0m0.046s

[kolyshkin]

Since commit edeb3b3 (#3306) we only read cpuinfo if intelrdt is available in hardware, yet you're right, it does not make sense to read it at all, since we can get the needed info with a few stat(2) calls.

Can we take one more step and ditch mountinfo parsing as well? This is only used to check for mbaScEnabled, which is apparently not used anywhere except to avoid stat of $root/info/MB file. Would be nice to remove this.

@xiaochenshen @marquiz PTAL

[xiaochenshen]

@corhere Thank you very much for working on it.

• Relates to runc exec is 5x slower than crun exec #3181

Reading /proc/cpuinfo is a surprisingly expensive operation. Since kernel version 4.12, opening /proc/cpuinfo on an x86 system can block for around 20 milliseconds while the kernel samples the current CPU frequency.

I am really surprised for the performance hurt from reading /proc/cpuinfo. This issue should be fixed. Thank you.

There is a very recent patch which gets rid of the delay, but has yet to make it into the mainline kernel. 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.

Yes, in most of the cases the production kernels or older upstream kernels are without this fix. Reading /proc/cpuinfo is too expensive.

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.
Benchmarks: runc run is 17 ms faster when "intelRdt" is enabled in config.json

I agree with you. We could remove the redundant check on /proc/cpuinfo.

[xiaochenshen]

LGTM. Thank you!

[xiaochenshen]

@kolyshkin

Can we take one more step and ditch mountinfo parsing as well? This is only used to check for mbaScEnabled, which is apparently not used anywhere except to avoid stat of $root/info/MB file. Would be nice to remove this.

I don't think the mountinfo check could be removed so far. MBA Software Controller is a "pure" software feature based on MBA by kernel, the only way of exposing the capability to user is through mount option "mba_MBps" in current kernel.

An alternative hacking way is to check "MB:" line in default schemata file: without mount option "mba_MBps", the data is a percentage in range [0, 100%], but with mount option "mba_MBps", the data is a u32 value in range [0, ((u32)~0U)].

$ cat /sys/fs/resctrl/schemata | grep MB
(1) w/o "mba_MBps" mount option: "MB:0= 100;1= 100"
(2) w/  "mba_MBps" mount option: "MB:0=4294967295;1=4294967295"

[corhere]

Given that Intel's own libraries also parse mountinfo to check whether the mba_MBps option is enabled, I don't think there are any alternatives to parsing mountinfo.
https://github.com/intel/goresctrl/blob/1b61a39df7b692eb89e41a75a27b7f786461012d/pkg/rdt/info.go#L243-L251
https://github.com/intel/intel-cmt-cat/blob/c76f7a34195fba05c96ea3d6450a24a790041ef1/lib/os_cap.c#L745-L746

I suspect that @xiaochenshen's hacky check might result in false negatives. The /sys/fs/resctrl/schemata file describes the configuration for the default resource group so a system with the resctrl filesystem mounted with the mba_MBps option could also have the default group's memory bandwidth set to a value <= 100 MBps.

That all being said, it looks like the only other place the mbaScEnabled variable is referenced is in the function intelrdt.IsMBAScEnabled(), which is unused in libcontainer. So we could elide parsing mountinfo in rootOnce without breaking compatibility with external users by lazily parsing mountinfo the first time IsMBAScEnabled is called. But mountinfo is not only parsed to check if mba_sc is enabled; it is also used to find the mount-point of the resctrl fs. Otherwise libcontainer would need to assume that the resctrl fs is always mounted to /sys/fs/resctrl, which is a pretty reasonable assumption IMO and aligns with Intel's C library (but not their Go library).

[xiaochenshen]

@corhere

I suspect that @xiaochenshen's hacky check might result in false negatives. The /sys/fs/resctrl/schemata file describes the configuration for the default resource group so a system with the resctrl filesystem mounted with the mba_MBps option could also have the default group's memory bandwidth set to a value <= 100 MBps.

Correct. This hacking way is not reliable.

That all being said, it looks like the only other place the mbaScEnabled variable is referenced is in the function intelrdt.IsMBAScEnabled(), which is unused in libcontainer. So we could elide parsing mountinfo in rootOnce without breaking compatibility with external users by lazily parsing mountinfo the first time IsMBAScEnabled is called.

Sounds good to me.

But mountinfo is not only parsed to check if mba_sc is enabled; it is also used to find the mount-point of the resctrl fs. Otherwise libcontainer would need to assume that the resctrl fs is always mounted to /sys/fs/resctrl, which is a pretty reasonable assumption IMO and aligns with Intel's C library (but not their Go library).

Yes. Parsing mountinfo is necessary to find the mount point of resctrl fs. We could not assume resctrl fs is always mounted to the recommended default mount point - /sys/fs/resctrl.
Actually, resctrl fs could be mounted at any proper place instead of /sys/fs/resctrl.

[kolyshkin]

intelrdt.IsMBAScEnabled(), which is unused in libcontainer

... and elsewhere (I tried to search through some well known users of runc/libcontainer and haven't found any).

But mountinfo is not only parsed to check if mba_sc is enabled; it is also used to find the mount-point of the resctrl fs. Otherwise libcontainer would need to assume that the resctrl fs is always mounted to /sys/fs/resctrl, which is a pretty reasonable assumption IMO and aligns with Intel's C library (but not their Go library).

Yes. Parsing mountinfo is necessary to find the mount point of resctrl fs. We could not assume resctrl fs is always mounted to the recommended default mount point - /sys/fs/resctrl. Actually, resctrl fs could be mounted at any proper place instead of /sys/fs/resctrl.

Well, this is easy -- first we do statfs(2) on default path (/sys/fs/resctrl), checking that it exists and comparing the returned fstype (it should be RDTGROUP_SUPER_MAGIC as defined in kernel headers as well as x/sys/unix). This is very fast and will work in 99.9% cases.

If not, we fallback to parsing /proc/mountinfo.

Currently we can't do the fast path because unfortunately there's no other way to read mount options (and I've actually added the comment explaining that sad situation in commit 5e201e7).

If we can get rid of unconditional parsing of mountinfo, that would be awesome.

@corhere @xiaochenshen thanks for looking into it!

[corhere]

@kolyshkin @xiaochenshen I have updated the PR with the discussed fast path, plus some more improvements such that the slow path is only executed if RDT is configured in the container's OCI spec. PTAL.

[xiaochenshen]

LGTM for the fast path updates. Thank you.

[marquiz]

I like the idea in this PR very much 😄 I haven't closely reviewed the code yet, though

[marquiz]

Hmm, there's one consideration that I don't know if we should care about here. If resctrlfs is mounted with -o cdp we will have L3CODE/ and L3DATA/ (instead of L3/) under the info/ dir.

I don't remember if CDP is officially supported in the runtime spec but I guess specifying something like L3DATA:0=ff\nL3CODE:0=ff in intelRdt.l3CacheSchema would work in practice.

Any thoughts?

[corhere]

That certainly sounds like we could get false negatives on IsCATEnabled() if resctrlfs is mounted with -o cdp and is worth investigating further. However, as the code in main would have the exact same deficiency, any improvements to feature detection or how a container's L3 allocation config is applied when RDT/CDP is enabled in the kernel probably should be made in a follow-up PR.

[kolyshkin]

Yes, it seems this can be addressed in a follow-up. @marquiz feel free to create it :)

[marquiz]

K, sounds good. Especially as the old code has the same limitation as you pointed out

[thaJeztah]

Opened #3537 so that we don't forget 👍

[marquiz]

I had just one comment/question. Looks very good to me 👍

[marquiz]

LGTM

[kolyshkin]

Left a couple of nits wrt two last commits, but overall LGTM

[kolyshkin]

LGTM

[kolyshkin]

@AkihiroSuda @thaJeztah @cyphar PTAL

[kolyshkin]

@AkihiroSuda @thaJeztah @cyphar PTAL

[thaJeztah]

Had a go at reviewing this, and found some unused code that (as a result) could simplify this patch.

[thaJeztah]

LGTM, thanks!

[thaJeztah]

heh (very!) random comment 😅; I recall some "lunch time" discussions I had with @kolyshkin about "where" comments should go for conditional statements.

My take was that putting the comment before the condition is an easy pitfal to describe the code, not the intent (and you may end up with describing the "if" statement, only in words instead of code), whereas putting it inside the branch "gently" forces one to write the branch itself (why we're doing it).

// check if foo is enabled
if foo {
    do stuff
}

if foo {
  // foo is enabled; start the flux capacitor to go back to the future.
  do stuff
}

(Of course, it highly depends on the situation; in some cases you'd want to describe a more complex conditional block as a whole to describe the whole intent.)

[thaJeztah]

@kolyshkin PTAL

[kolyshkin]

LGTM, thanks!