-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
libcontainer: intelrdt: use init() to avoid race condition #1589
libcontainer: intelrdt: use init() to avoid race condition #1589
Conversation
libcontainer/intelrdt/intelrdt.go
Outdated
return true | ||
} | ||
// We only need to set the flag once | ||
once.Do(func() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we only need to do this once, why not just do it in an init
and set the flag when the program starts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for code review.
Intel RDT/CAT is a hardware-dependent feature. We wish add zero/minimal overhead to the hardware platform which doesn't support this feature. So there are several orthogonal call paths in libcontainer to check the capability:
- In loadFactory() --> libcontainer.New().
- In LinuxFactory.Create() and Load().
- In ConfigValidator.Validate()
- In libcontainer and intelrdt test cases.
Taking loadFactory() --> libcontainer.New() for example. It calls IsIntelRdtEnabled() to check the capability. If not support, libcontainer.New() will not add intelRdtManager as a factory constructor option. In subsequent container creating, running and destroying life cycle, intelRdtManager handles Intel RDT operations. If intelRdtManager is nil, we will not touch Intel RDT related functionality.
From my understanding, keeping IsIntelRdtEnabled() as a "utility" function is no harm in this case. We could check the capability in outside package without adding intelRdtManager hooks. Please correct me if I am wrong.
Thank you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you understood my question. I was asking why don't we set the isIntelRdtEnabled
flag inside an init
rather than setting it when you do the check (and then just making IsEnabled
check that flag and not touch it). Something like
package intelrdt
// ... rest of package ...
func init() {
// code to check if it's enabled
isIntelRdtEnabled = true // or whatever
}
func IsEnabled() {
return isIntelRdtEnabled
}
So you don't need to do the sync.Do
stuff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the follow-up PR of opencontainers#1279 to fix remaining issues: Use init() to avoid race condition in IsIntelRdtEnabled(). Add also rename some variables and functions. Signed-off-by: Xiaochen Shen <xiaochen.shen@intel.com>
f621206
to
88d22fd
Compare
This is the follow-up PR of #1279 to fix remaining issues:
Use init() to avoid race condition in IsIntelRdtEnabled().
Add also rename some variables and functions.
Signed-off-by: Xiaochen Shen xiaochen.shen@intel.com