libcontainer/intelrdt: cleanups #3041
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@marquiz @Creatone @xiaochenshen PTAL; I'd appreciate your reviews. |
|
@kolyshkin Thank you very much for the code cleanups and bug fixing for Intel RDT. Three tiny typos in subject of this PR, description of this PR and subject of last commit: 😃 |
xiaochenshen
approved these changes
Jun 25, 2021
Comment on lines
-303
to
+305
| config.IntelRdt = &configs.IntelRdt{} | ||
| if spec.Linux.IntelRdt.L3CacheSchema != "" { | ||
| config.IntelRdt.L3CacheSchema = spec.Linux.IntelRdt.L3CacheSchema | ||
| } | ||
| if spec.Linux.IntelRdt.MemBwSchema != "" { | ||
| config.IntelRdt.MemBwSchema = spec.Linux.IntelRdt.MemBwSchema | ||
| config.IntelRdt = &configs.IntelRdt{ | ||
| L3CacheSchema: spec.Linux.IntelRdt.L3CacheSchema, | ||
| MemBwSchema: spec.Linux.IntelRdt.MemBwSchema, |
xiaochenshen
approved these changes
Jun 25, 2021
Comment on lines
-550
to
+552
| if err != nil && !IsNotFound(err) { | ||
| if err != nil { |
xiaochenshen
previously approved these changes
Jun 25, 2021
@xiaochenshen thanks for your review! Fixed, and I also found a 4th one, in a commit before last, also fixed: @@ -400,7 +400,7 @@ func writeFile(dir, file, data string) error {
return fmt.Errorf("no such directory for %s", file)
}
if err := ioutil.WriteFile(filepath.Join(dir, file), []byte(data+"\n"), 0o600); err != nil {
- return NewLastCmdError(fmt.Errorf("intelrdt: unable to write %v: %w", data, err))
+ return NewLastCmdError(fmt.Errorf("intelrtd: unable to write %v: %w", data, err))
}
return nil
} |
In cases we have something like
if y != "" {
x = y
}
where both x and y are strings, and x was not set before,
it makes no sense to have a condition, as such code is
equivalent to mere
x = y
Simplify such cases by removing "if".
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
In case getIntelRdtData() returns an error, d is set to nil. In case the error returned is of NotFoundError type (which happens if resctlr mount is not found in /proc/self/mountinfo), the function proceeds to call d.join(), resulting in a nil deref and a panic. In practice, this never happens in runc because of the checks in intelrdt() function in libcontainer/configs/validate, which raises an error in case any of the parameters are set in config but the IntelRTD itself is not available (that includes checking that the mount point is there). Nevertheless, the code is wrong, and can result in nil dereference if some external users uses Apply on a system without resctrl mount. Fix this by removing the exclusion. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Initially, this was copied over from libcontainer/cgroups, where it made sense as for cgroup v1 we have multiple controllers and mount points. Here, we only have a single mount, so there's no need for the whole type. Replace all that with a simple error (which is currently internal since the only user is our own test case). Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
For errors that only have a string and an underlying error, using fmt.Errorf with %w to wrap an error is sufficient. In this particular case, the code is simplified, and now we have unwrappable errors as a bonus (same could be achieved by adding (*LastCmdError).Unwrap() method, but that's adding more code). Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
... the stack, so every caller will automatically benefit from it. The only change that it causes is the user in libcontainer/process_linux.go will get a better error message. [v2: typo fix] Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
These are not used anywhere outside of the package (I have also checked the only external user of the package (github.com/google/cadvisor). No changes other than changing the case. The following identifiers are now private: * IntelRdtTasks * NewLastCmdError * NewStats Brought to you by gorename. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
|
Out of curiosity, I checked the impact of this PR on runc binary size [kir@kir-rhat runc]$ size runc-v*
text data bss dec hex filename
7531045 3885612 223416 11640073 b19d09 runc-v1.0.0-38-ge5ccc4b9
7528725 3884100 223416 11636241 b18e11 runc-v1.0.0-44-g0229a77aAlmost 4K savings out of nowhere. |
|
CI failure is a glitch; CI restarted |
|
LGTM. Thanks! |
Creatone
approved these changes
Jun 29, 2021
|
Thanks @kolyshkin! LGTM |
marquiz
approved these changes
Jul 5, 2021
AkihiroSuda
approved these changes
Jul 7, 2021
hqhq
approved these changes
Jul 10, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
A bunch of cleanups for the code in
libcontainer/intelrdt, mostly to simplify or remove the code, with occasional minor improvements and one nil dereference fix. See individual commit for details.Please review commit-by-commit.