Add mon groups for resctrl. #2523

Creatone · 2020-07-21T08:21:04Z

On a system with RDT control features additional directories can be
created in the root directory that specify different amounts of each
resource. The root and these additional top level
directories are referred to as "CTRL_MON" groups.
And this is already implemented in runc.

On a system with RDT monitoring the root directory and other top level
directories contain a directory named "mon_groups" in which additional
directories can be created to monitor subsets of tasks in the CTRL_MON
group that is their ancestor.
And that PR implements this behavior.

More info:
https://www.kernel.org/doc/Documentation/x86/intel_rdt_ui.txt

#2519

Signed-off-by: Paweł Szulik [email protected]

kolyshkin · 2020-07-22T19:11:25Z

there's too much external dependencies for a couple of assert.EqualError calls in a test case. Can you please redo it without using stretchr/testify? I am not against using it in general, but for a small case like this it seems excessive.

kolyshkin · 2020-07-22T19:11:43Z

sorry, didn't mean to close this one

kolyshkin · 2020-07-22T19:12:36Z

A commit message with some details about what is being done, links to docs etc might be helpful, too.

Creatone · 2020-07-27T13:13:34Z

@kolyshkin

kolyshkin · 2020-07-28T00:47:32Z

A commit message with some details about what is being done, links to docs etc might be helpful, too.

This ^^^ is still valid.

libcontainer/intelrdt/intelrdt.go

libcontainer/intelrdt/intelrdt_test.go

katarzyna-z · 2020-07-31T07:04:51Z

CI build failed with following error "FAIL - commit subject exceeds 90 characters".
@Creatone Could you fix this?

Creatone · 2020-08-03T16:01:13Z

@kolyshkin @katarzyna-z

Creatone · 2020-08-19T08:16:49Z

@crosbymichael @mrunalp @dqminh @hqhq @cyphar @AkihiroSuda @kolyshkin

libcontainer/intelrdt/intelrdt.go

Creatone · 2020-09-01T07:11:06Z

Is it possible to review and merge this PR?
I want to fix bugs with Resctrl metrics in the cAdvisor project and I just need this one.

libcontainer/intelrdt/intelrdt.go

libcontainer/intelrdt/intelrdt_test.go

kolyshkin

I found some old review comments that I forgot to submit.

Also, this needs rebasing. Can you take a look @Creatone?

go.mod

libcontainer/configs/intelrdt.go

kolyshkin · 2022-05-18T20:38:25Z

libcontainer/intelrdt/monitoring.go

 		if file.IsDir() {
 			numaPath := filepath.Join(containerPath, "mon_data", file.Name())
-			if IsMBMEnabled() {
+			if IsMBMEnabled() && enableMBM {


Nit: perhaps reverse the order of checks, since checking a boolean is faster than calling a function.

libcontainer/intelrdt/monitoring.go

libcontainer/specconv/spec_linux.go

Creatone · 2022-12-01T21:17:41Z

@kolyshkin I force-pushed my changes, are you ok with that? PTAL :)

More info: https://www.kernel.org/doc/Documentation/x86/intel_rdt_ui.txt Signed-off-by: Paweł Szulik <[email protected]>

libcontainer/specconv/spec_linux.go

Signed-off-by: Paweł Szulik <[email protected]>

kolyshkin · 2023-08-11T23:59:28Z

libcontainer/intelrdt/intelrdt.go


-	if IsMBMEnabled() || IsCMTEnabled() {
-		err = getMonitoringStats(containerPath, stats)
+	if (IsCMTEnabled() && m.config.IntelRdt.EnableCMT) || (IsMBMEnabled() && m.config.IntelRdt.EnableMBM) {


Shouldn't this be if m.monitoringGroup?

kolyshkin · 2023-08-12T00:00:45Z

libcontainer/intelrdt/intelrdt.go

 		l3CacheSchema := container.IntelRdt.L3CacheSchema
 		memBwSchema := container.IntelRdt.MemBwSchema

+		// Write a single joint schema string to schemata file


This comment is either misplaced or misleading.

kolyshkin · 2023-08-12T00:01:37Z

libcontainer/intelrdt/intelrdt.go

+		if l3CacheSchema != "" || memBwSchema != "" {
+			// Schema can be set only in Control Group.
+			if m.monitoringGroup {


These two ifs can be combined into one.

kolyshkin · 2023-08-12T00:01:51Z

libcontainer/intelrdt/intelrdt.go

+		if l3CacheSchema != "" || memBwSchema != "" {
+			// Schema can be set only in Control Group.
+			if m.monitoringGroup {
+				return fmt.Errorf("couldn't set IntelRdt l3CacheSchema or memBwSchema for the monitoring group")


nit: errors.New.

kolyshkin · 2023-08-12T00:02:16Z

libcontainer/intelrdt/intelrdt.go

+	if config.IntelRdt.L3CacheSchema != "" || config.IntelRdt.MemBwSchema != "" || config.IntelRdt.ClosID != "" {
+		monitoringGroup = false
+	} else if config.IntelRdt.EnableCMT || config.IntelRdt.EnableMBM {
+		monitoringGroup = true


@Creatone am reading this right, that if ClosID or L3CacheSchema or MemBwSchema is set, the settings for EnableCMT and EnableMBM are silently ignored?

If yes, such configuration should probably be rejected during validation.

kolyshkin · 2023-08-12T00:03:12Z

libcontainer/intelrdt/intelrdt.go

-		path := m.GetPath()
+


No need to add an empty line here.

kolyshkin

Left a few more comments. Also, please squash the commits.

Rouzip · 2023-11-08T05:22:15Z

Hi, @Creatone . Thanks for your efforts on this pr. I noticed that you have not updated this pr anymore recently. Do you mind if I help you continue working on this pr?

Creatone · 2023-11-09T09:19:32Z

@Rouzip I'll try to address @kolyshkin comments this week.

Rouzip · 2023-11-10T09:43:10Z

libcontainer/configs/intelrdt.go

Add rmid field for special monitor group name in the future?

marquiz · 2025-06-03T10:16:27Z

An effort to clarify and simplify the usage of resctrl monitoring features:
opencontainers/runtime-spec#1287

I'd suggest to put a hold on this PR.

Creatone force-pushed the resctrl-mon-group branch 3 times, most recently from 7c629c8 to 62ccdb1 Compare July 22, 2020 12:19

kolyshkin closed this Jul 22, 2020

kolyshkin reopened this Jul 22, 2020

Creatone force-pushed the resctrl-mon-group branch 3 times, most recently from b6266be to 2b63c34 Compare July 27, 2020 13:07

kolyshkin reviewed Jul 28, 2020

View reviewed changes

libcontainer/intelrdt/intelrdt.go Outdated Show resolved Hide resolved

kolyshkin reviewed Jul 28, 2020

View reviewed changes

libcontainer/intelrdt/intelrdt_test.go Outdated Show resolved Hide resolved

kolyshkin reviewed Jul 28, 2020

View reviewed changes

libcontainer/intelrdt/intelrdt_test.go Outdated Show resolved Hide resolved

Creatone force-pushed the resctrl-mon-group branch from 2b63c34 to 3e3bcae Compare July 29, 2020 07:38

Creatone force-pushed the resctrl-mon-group branch from 3e3bcae to 711c104 Compare August 3, 2020 15:26

Creatone force-pushed the resctrl-mon-group branch from 711c104 to 507ecec Compare August 4, 2020 11:43

crosbymichael reviewed Aug 19, 2020

View reviewed changes

libcontainer/intelrdt/intelrdt.go Outdated Show resolved Hide resolved

Creatone force-pushed the resctrl-mon-group branch 3 times, most recently from 397e410 to 2852ea3 Compare August 25, 2020 12:29

AkihiroSuda reviewed Sep 9, 2020

View reviewed changes

libcontainer/intelrdt/intelrdt.go Outdated Show resolved Hide resolved

AkihiroSuda reviewed Sep 9, 2020

View reviewed changes

libcontainer/intelrdt/intelrdt_test.go Outdated Show resolved Hide resolved

AkihiroSuda previously approved these changes Sep 10, 2020

View reviewed changes

kolyshkin requested changes Dec 1, 2022

View reviewed changes

Creatone force-pushed the resctrl-mon-group branch 5 times, most recently from 6e254b4 to e5ce002 Compare December 1, 2022 20:52

libcontainer/intelrdt: add basic "MON" groups support.

775a366

More info: https://www.kernel.org/doc/Documentation/x86/intel_rdt_ui.txt Signed-off-by: Paweł Szulik <[email protected]>

Creatone force-pushed the resctrl-mon-group branch from e5ce002 to 775a366 Compare April 17, 2023 15:11

kolyshkin reviewed Apr 17, 2023

View reviewed changes

libcontainer/specconv/spec_linux.go Outdated Show resolved Hide resolved

Address the review comments.

e6f2971

Signed-off-by: Paweł Szulik <[email protected]>

AkihiroSuda requested a review from kolyshkin June 12, 2023 08:56

kolyshkin reviewed Aug 11, 2023

View reviewed changes

kolyshkin reviewed Aug 12, 2023

View reviewed changes

libcontainer/intelrdt/intelrdt.go

Comment on lines -640 to +702

path := m.GetPath()

Copy link

Contributor

kolyshkin Aug 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to add an empty line here.

kolyshkin requested changes Aug 12, 2023

View reviewed changes

AkihiroSuda mentioned this pull request Nov 6, 2023

Blockers for v1.2.0 #4114

Closed

21 tasks

Rouzip reviewed Nov 10, 2023

View reviewed changes

libcontainer/configs/intelrdt.go

Copy link

Rouzip Nov 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add rmid field for special monitor group name in the future?

cyphar modified the milestones: 1.2.0, 1.3.0 Oct 21, 2024

cyphar modified the milestones: 1.3.0, 1.4.0-rc.1 Mar 1, 2025

cyphar modified the milestones: 1.4.0-rc.1, 1.5.0-rc.1 Aug 27, 2025

Uh oh!

Add mon groups for resctrl. #2523

Are you sure you want to change the base?

Add mon groups for resctrl. #2523

Uh oh!

Conversation

Creatone commented Jul 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kolyshkin commented Jul 22, 2020

Uh oh!

kolyshkin commented Jul 22, 2020

Uh oh!

kolyshkin commented Jul 22, 2020

Uh oh!

Creatone commented Jul 27, 2020

Uh oh!

kolyshkin commented Jul 28, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

katarzyna-z commented Jul 31, 2020

Uh oh!

Creatone commented Aug 3, 2020

Uh oh!

Creatone commented Aug 19, 2020

Uh oh!

Uh oh!

Creatone commented Sep 1, 2020

Uh oh!

Uh oh!

Uh oh!

kolyshkin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Creatone commented Dec 1, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kolyshkin left a comment

Choose a reason for hiding this comment

Uh oh!

Rouzip commented Nov 8, 2023

Uh oh!

Creatone commented Nov 9, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marquiz commented Jun 3, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

Creatone commented Jul 21, 2020 •

edited

Loading