libcontainer/intelrdt: fix CMT feature check

[xiaochenshen]

Intel RDT sub-features can be selectively disabled or enabled by kernel
command line. See "rdt=" option details in kernel document:
https://www.kernel.org/doc/Documentation/admin-guide/kernel-parameters.txt

But Cache Monitoring Technology (CMT) feature is not correctly checked
in init() and getCMTNumaNodeStats() now. If CMT is disabled by kernel
command line (e.g., rdt=!cmt,mbmtotal,mbmlocal,l3cat,mba) while hardware
supports CMT, we may get following error when getting Intel RDT stats:
runc run c1
runc events c1
ERRO[0005] container_linux.go:200: getting container's Intel RDT stats
caused: open /sys/fs/resctrl/c1/mon_data/mon_L3_00/llc_occupancy: no
such file or directory

Fix CMT feature check in init() and GetStats() call paths.

Signed-off-by: Xiaochen Shen [email protected]

[xiaochenshen]

@Creatone @kolyshkin @AkihiroSuda
Could you help review this PR? Thank you!

[kolyshkin]

LGTM

[kolyshkin]

@xiaochenshen thanks!

Can you work on moving the initialization out of func init() and into some function that will be called when required? You can wrap it all into sync.Once so the initialization will only happens once.

Let me know if you need a more detailed explanation.

[kolyshkin]

Can you work on moving the initialization out of func init() and into some function that will be called when required? You can wrap it all into sync.Once so the initialization will only happens once.

@xiaochenshen I mean something like this: #2646

[xiaochenshen]

Can you work on moving the initialization out of func init() and into some function that will be called when required? You can wrap it all into sync.Once so the initialization will only happens once.

@xiaochenshen I mean something like this: #2646

@kolyshkin Thank you for code review!

Actually, the original implementation of initialization was wrapped in sync.Once. But in #1589 (review), I was suggested to use init() instead.

From the description in #2646 , this benefit of change is a speedup to operations, am I right?
I have no preference. I will make the change if you confirm this is necessary. Thank you!

[xiaochenshen]

Can you work on moving the initialization out of func init() and into some function that will be called when required? You can wrap it all into sync.Once so the initialization will only happens once.

@kolyshkin I have added 2 more commits in this PR:
dea6fff libcontainer/intelrdt: rename CAT and MBA enabled flags
ac27050 libcontainer/intelrdt: rm init() from intelrdt.go

Commit dea6fff only cleans code, no functional change.
Commit ac27050 wraps Intel RDT initialization into sync.Once.

[xiaochenshen]

@kolyshkin
Rebased the code. Addressed two issues above.

[xiaochenshen]

@Creatone @AkihiroSuda
Could you help review this PR at your convenience? Thank you!

[Creatone]

LGTM

[kolyshkin]

I favor the sync.Once() approach, as I feel that reading/parsing a /proc file (albeit small) for everyone who merely imports the package is a tad too much. But I'd appreciate the @cyphar input on the matter.

[xiaochenshen]

I favor the sync.Once() approach, as I feel that reading/parsing a /proc file (albeit small) for everyone who merely imports the package is a tad too much. But I'd appreciate the @cyphar input on the matter.

@cyphar Could you help review and comment this change from init() to sync.Once() in 606c5ad at your convenience?
Thank you!

[xiaochenshen]

Rebased the code.
Change log:

• Make use of intelrdt.NewManager() which introduced in PR libcontainer/intelrdt: IntelRdtManager refactoring #2653, no functional change.

But golang-lint an CI failed due to #2661, waiting for #2661 to be merged.

[xiaochenshen]

@kolyshkin mentioned: I favor the sync.Once() approach, as I feel that reading/parsing a /proc file (albeit small) for everyone who merely imports the package is a tad too much. But I'd appreciate the @cyphar input on the matter.

@cyphar Could you help review and comment this change from init() to sync.Once() in 606c5ad at your convenience?
Thank you!

@cyphar PTAL, thank you!
I have rebased the code. Could you help review and comment the change from init() to sync.Once() in ec13651 at your convenience?

[xiaochenshen]

@kolyshkin mentioned: I favor the sync.Once() approach, as I feel that reading/parsing a /proc file (albeit small) for everyone who merely imports the package is a tad too much. But I'd appreciate the @cyphar input on the matter.

@cyphar
Could you help review and comment the change from init() to sync.Once() in ec13651 at your convenience?
Thank you!

@AkihiroSuda @crosbymichael @hqhq
Could you help review this PR at your convenience?
Thank you!

[xiaochenshen]

@kolyshkin
What do you think if I could split this PR into two parts? Thank you!
(1) Fix bug in CMT feature check (changes in first commit).
(2) Open a follow-up PR to track the improvement from init() to sync.Once() and code clean (changes in second and third commits).

This PR has opened for a long time. But I think the bug in part (1) should be fixed as soon as possible.

[kolyshkin]

What do you think if I could split this PR into two parts?

Makes sense, please do, thanks!

[xiaochenshen]

@kolyshkin
Opened two follow-up PRs: #2677 and #2678
Leave only bug fixing in this PR (keep code unchanged).

@kolyshkin @AkihiroSuda @Creatone
Could you help review this PR at your convenience? Thank you!

[kolyshkin]

LGTM, thanks!

[Creatone]

LGTM

[xiaochenshen]

@crosbymichael @AkihiroSuda
Could you help review this PR at your convenience? Thank you!