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 · 2022-05-06T01:36:41Z

@Creatone hey, can you refresh this one? I see that EnableCMT and EnableMBM appeared in runtime-spec, so we can use those now
.

Creatone · 2022-05-06T08:41:17Z

@kolyshkin sure, just give me a couple of days :)

Creatone · 2022-05-18T14:43:04Z

@kolyshkin PTAL

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

@@ -59,14 +59,14 @@ func getMonitoringStats(containerPath string, stats *Stats) error {
 	for _, file := range numaFiles {
 		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

@@ -579,8 +641,8 @@ func (m *Manager) GetStats() (*Stats, error) {
 		}
 	}

-	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?

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

Creatone force-pushed the resctrl-mon-group branch 2 times, most recently from 55151f9 to 7d19bd3 Compare May 18, 2022 14:27

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add mon groups for resctrl. #2523

Add mon groups for resctrl. #2523

Creatone commented Jul 21, 2020 •

edited

Loading

kolyshkin commented Jul 22, 2020

kolyshkin commented Jul 22, 2020

kolyshkin commented Jul 22, 2020

Creatone commented Jul 27, 2020

kolyshkin commented Jul 28, 2020

katarzyna-z commented Jul 31, 2020

Creatone commented Aug 3, 2020

Creatone commented Aug 19, 2020

Creatone commented Sep 1, 2020

kolyshkin commented May 6, 2022

Creatone commented May 6, 2022

Creatone commented May 18, 2022

kolyshkin left a comment

kolyshkin May 18, 2022

Creatone commented Dec 1, 2022

kolyshkin Aug 11, 2023

kolyshkin Aug 12, 2023

kolyshkin Aug 12, 2023

kolyshkin Aug 12, 2023

kolyshkin Aug 12, 2023

kolyshkin Aug 12, 2023

kolyshkin left a comment

Rouzip commented Nov 8, 2023

Creatone commented Nov 9, 2023

Rouzip Nov 10, 2023

Add mon groups for resctrl. #2523

Are you sure you want to change the base?

Add mon groups for resctrl. #2523

Conversation

Creatone commented Jul 21, 2020 • edited Loading

kolyshkin commented Jul 22, 2020

kolyshkin commented Jul 22, 2020

kolyshkin commented Jul 22, 2020

Creatone commented Jul 27, 2020

kolyshkin commented Jul 28, 2020

katarzyna-z commented Jul 31, 2020

Creatone commented Aug 3, 2020

Creatone commented Aug 19, 2020

Creatone commented Sep 1, 2020

kolyshkin commented May 6, 2022

Creatone commented May 6, 2022

Creatone commented May 18, 2022

kolyshkin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Creatone commented Dec 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kolyshkin left a comment

Choose a reason for hiding this comment

Rouzip commented Nov 8, 2023

Creatone commented Nov 9, 2023

Choose a reason for hiding this comment

Creatone commented Jul 21, 2020 •

edited

Loading