libcontainer/intelrdt: add support for Schemata field #4830

marquiz · 2025-08-01T07:30:57Z

Implement support for the linux.intelRdt.schemata field of the spec. This allows management of the "schemata" file in the resctrl group in a generic way.

Refs: opencontainers/runtime-spec#1230

marquiz · 2025-08-18T11:38:54Z

Updated:

rebased
go.mod: bumped runtime-spec to latest tip of the main branch
features.go: set linux.intelRdt.schemata: true

Signed-off-by: Markus Lehtonen <[email protected]>

marquiz · 2025-08-26T17:28:10Z

@rata @kolyshkin @AkihiroSuda @cyphar

rata

LGTM, thanks!

libcontainer/intelrdt/intelrdt_test.go

Signed-off-by: Markus Lehtonen <[email protected]>

kolyshkin · 2025-09-03T02:51:05Z

libcontainer/intelrdt/intelrdt.go

-		if l3CacheSchema == "" && memBwSchema != "" {
-			if err := writeFile(path, "schemata", memBwSchema); err != nil {
+		// Write a single joint schema string to schemata file
+		schemata := strings.Join(parts, "\n")


nit: perhaps you can use strings.Builder instead? Something like this:

var schemata strings.Builder for _, s := range []string{l3CacheSchema, memBwSchema, container.IntelRdt.Schemata...} { if s != "" { schemata.WriteString(s) schemata.WriteString("\n") } } if schemata.Len() > 0 { if err := writeFile(path, "schemata", schemata); err != nil { return err } }

+1. I thought about it but I wasn't sure it was worth it. If others thought about it too, let's do it :D

I actually realized that the implementation was not good. It's most robust to write each field in a separate writeFile() call. Case where intelRdt.Schemata tried to overwrite something specified in the other schemata fields failed as kernel rejected the "mega write". The code now handles that scenario.

Still, out of curiosity, was there some benefit of using strings.Builder() or just a style/readability thing?

Yeap, see the pkg documentation:

A Builder is used to efficiently build a string using Builder.Write methods. It minimizes memory copying. The zero value is ready to use. Do not copy a non-zero Builder.

libcontainer/intelrdt/stats.go

kolyshkin

Left a few comments, overall lgtm

marquiz · 2025-09-03T20:13:41Z

Update: code fixed and review comments addressed.

@kolyshkin @rata

EDIT: fixed unit tests. A separate commit so it's easier to review if that's acceptable

libcontainer/intelrdt/intelrdt.go

libcontainer/intelrdt/intelrdt_test.go

kolyshkin · 2025-09-04T22:55:56Z

libcontainer/intelrdt/intelrdt.go

-		// Write a single joint schema string to schemata file
-		if l3CacheSchema != "" && memBwSchema != "" {
-			if err := writeFile(path, "schemata", l3CacheSchema+"\n"+memBwSchema); err != nil {
+		if r.L3CacheSchema != "" {
+			if err := writeFile(path, "schemata", r.L3CacheSchema); err != nil {
 				return err
 			}
 		}
-
-		// Write only L3 cache schema string to schemata file
-		if l3CacheSchema != "" && memBwSchema == "" {
-			if err := writeFile(path, "schemata", l3CacheSchema); err != nil {
+		if r.MemBwSchema != "" {
+			if err := writeFile(path, "schemata", r.MemBwSchema); err != nil {
 				return err
 			}
 		}
-
-		// Write only memory bandwidth schema string to schemata file
-		if l3CacheSchema == "" && memBwSchema != "" {
-			if err := writeFile(path, "schemata", memBwSchema); err != nil {
+		if len(r.Schemata) > 0 {
+			if err := writeFile(path, "schemata", strings.Join(r.Schemata, "\n")); err != nil {


I wonder if it will be better to rewrite writeFile to accept multiple data:

func writeFile(dir, file, data... string) error { ... for _, d := range data { if d == "" { continue } err := f.WriteString(d) ... } ... }

and then use it like this here:

err := writeFile(path, "schemata", r.L3CacheSchema, r.MemBwSchema, r.Schemata...)

True. I did a bit more refactoring and ditched the whole writeFile() function. The impl is now inline inside Set()

@kolyshkin

libcontainer/intelrdt/intelrdt.go

libcontainer/configs/intelrdt.go

libcontainer/intelrdt/intelrdt_test.go

libcontainer/intelrdt/intelrdt.go

libcontainer/intelrdt/intelrdt_test.go

libcontainer/intelrdt/util_test.go

rata · 2025-09-05T10:53:16Z

libcontainer/intelrdt/intelrdt.go

+		path := filepath.Join(m.GetPath(), "schemata")
+		for _, line := range append([]string{r.L3CacheSchema, r.MemBwSchema}, r.Schemata...) {
+			if line != "" {
+				f, err := os.OpenFile(path, os.O_WRONLY|os.O_APPEND, 0o600)


Why did you change this from the previous iteration using the string builder thingy?

#4830 (comment)

It is also more robust to do one write per line. If we had L3CacheSchema: L3:0=... and then "L3:0=..." in the schemata fields as well, kernel would reject the write if done in one write.

What do you mean more robust? Why is it better to have "partial" data written instead of all/nothing (atomic)? We will return an error anyways. Is it simpler to debug or what?

If you do all in one write (like Write("L3:0=f\nL3:0:ff\n"). The write will fail. If you do two separate writes (like Write("L3:0=f\n"); Write("L3:0:ff\n")), it does not fail.

But it was just doing that: https://github.com/opencontainers/runc/pull/4830/files#diff-84ecbbd29a26d73ccc5bf64ce8ed27a1dcca405c3f605d51293b5b3a74dd8979L653

Why would it fail now?

It would fail if somebody put L3 in the memBwSchema field.

But there's the essential detail that's the gist of this PR. Add Schemata field. The schemata field is allowed to contain anything. A corner case, yes, but it could override the stuff in the l3CacheSchema field

Sorry, I'm missing something because from what you say, I believe not doing a write per line seems better.

If you write it one per line, then if schemata overrides the L3CacheSchema, then it will work just fine. However, if we write it all in one go, it will fail.

Failing seems way better, as if you wrote something on the L3CacheSchema, it's unlikely you want to overwrite it with the Schemata field (in that case you will leave it empty).

Am I missing something?

libcontainer/intelrdt/intelrdt.go

rata · 2025-09-15T10:13:07Z

libcontainer/intelrdt/intelrdt.go

+		path := filepath.Join(m.GetPath(), "schemata")
+		for _, line := range append([]string{r.L3CacheSchema, r.MemBwSchema}, r.Schemata...) {
+			if line != "" {
+				f, err := os.OpenFile(path, os.O_WRONLY|os.O_APPEND, 0o600)


Sorry, I'm missing something because from what you say, I believe not doing a write per line seems better.

If you write it one per line, then if schemata overrides the L3CacheSchema, then it will work just fine. However, if we write it all in one go, it will fail.

Failing seems way better, as if you wrote something on the L3CacheSchema, it's unlikely you want to overwrite it with the Schemata field (in that case you will leave it empty).

Am I missing something?

Implement support for the linux.intelRdt.schemata field of the spec. This allows management of the "schemata" file in the resctrl group in a generic way. Signed-off-by: Markus Lehtonen <[email protected]>

Signed-off-by: Markus Lehtonen <[email protected]>

marquiz · 2025-09-15T12:26:16Z

Sorry, I'm missing something because from what you say, I believe not doing a write per line seems better.

If you write it one per line, then if schemata overrides the L3CacheSchema, then it will work just fine. However, if we write it all in one go, it will fail.

Failing seems way better, as if you wrote something on the L3CacheSchema, it's unlikely you want to overwrite it with the Schemata field (in that case you will leave it empty).

@rata my thinking behind separate writes was that the schemata field could write whatever was specified in the other schema fields. Also, with blunt robustness in mind, as the spec does not specify how the different fields should be written, in one write (syscall), one write per field or one write per line...

But your standpoint is probably better justified. Especially with:

With one write(), the operation is atomic, failed writes do not leave the schemata in "undefined" state
This matches the already-existing crun implementation
Back to os.WriteFile(), simplified things :)

So I changed the implementation into one consolidated write, utilizing the strings.Builder()

rata · 2025-09-15T13:49:30Z

libcontainer/intelrdt/intelrdt.go

+		if schemata.Len() > 0 {
+			path := filepath.Join(m.GetPath(), "schemata")
+			if err := os.WriteFile(path, []byte(schemata.String()), 0o600); err != nil {
+				return newLastCmdError(fmt.Errorf("intelrdt: unable to write %q: %w", schemata.String(), err))


Sorry, this change is not 100% obvious to me.

Before we were not doing this, why do it now? Checking quickly, it seems like a different improvement, as the "info" file this ends up checking might have some more explanation on why it failed.

Am I reading this correctly?

A different commit doing this change, with its explanation, seems better IMHO. If that is too much of a hassle, then mention it in the commit you do this change. If this causes an issue in the future, it will seem like something not intentional when it really was intentional, that is very valuable information for future me :)

This is basically inlining the former writeFile() function (now dropped). I can create a separate commit about that if needed. @rata ?

Sorry, the comment is on the return line. Before we returned err, now we use this function. I was referring to that

marquiz force-pushed the devel/rdt-schemata-field branch 2 times, most recently from d4ca2a1 to 9c2639e Compare August 1, 2025 07:47

marquiz force-pushed the devel/rdt-schemata-field branch from 9c2639e to 9120253 Compare August 18, 2025 11:36

marquiz mentioned this pull request Aug 26, 2025

Support direct editing of the intelRdt config containerd/nri#215

Open

marquiz force-pushed the devel/rdt-schemata-field branch from 9120253 to 75e8dd9 Compare August 26, 2025 15:31

Update runtime-spec

74c5436

Signed-off-by: Markus Lehtonen <[email protected]>

marquiz force-pushed the devel/rdt-schemata-field branch from 75e8dd9 to a5d0f18 Compare August 26, 2025 16:55

rata approved these changes Aug 27, 2025

View reviewed changes

rata enabled auto-merge August 27, 2025 10:45

rata added the impact/changelog label Aug 27, 2025

rata reviewed Aug 28, 2025

View reviewed changes

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

libcontainer/intelrdt: refactor tests

3867f82

Signed-off-by: Markus Lehtonen <[email protected]>

auto-merge was automatically disabled August 28, 2025 10:43
Head branch was pushed to by a user without write access

marquiz force-pushed the devel/rdt-schemata-field branch from a5d0f18 to 8082dfe Compare August 28, 2025 10:43

kolyshkin reviewed Sep 3, 2025

View reviewed changes

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

kolyshkin requested changes Sep 3, 2025

View reviewed changes

kolyshkin added this to the 1.4.0-rc.1 milestone Sep 3, 2025

marquiz force-pushed the devel/rdt-schemata-field branch 3 times, most recently from 02bbcd9 to bd15875 Compare September 3, 2025 20:07

marquiz force-pushed the devel/rdt-schemata-field branch 2 times, most recently from 7430bc2 to f720d0e Compare September 3, 2025 20:50

rata requested changes Sep 4, 2025

View reviewed changes

libcontainer/intelrdt/intelrdt.go Show resolved Hide resolved

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

marquiz force-pushed the devel/rdt-schemata-field branch 3 times, most recently from eaadf2e to 0cf91bc Compare September 4, 2025 17:31

kolyshkin reviewed Sep 4, 2025

View reviewed changes

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

marquiz force-pushed the devel/rdt-schemata-field branch from 0cf91bc to 034fdfb Compare September 5, 2025 08:40

rata requested changes Sep 5, 2025

View reviewed changes

cyphar modified the milestones: 1.4.0-rc.1, 1.4.0-rc.2 Sep 6, 2025

marquiz force-pushed the devel/rdt-schemata-field branch 3 times, most recently from 03d5d45 to a3451ff Compare September 12, 2025 16:18

rata reviewed Sep 15, 2025

View reviewed changes

marquiz added 2 commits September 15, 2025 15:09

libcontainer/intelrdt: add support for Schemata field

4155321

Implement support for the linux.intelRdt.schemata field of the spec. This allows management of the "schemata" file in the resctrl group in a generic way. Signed-off-by: Markus Lehtonen <[email protected]>

events/intelrdt: report full schemata

7be025f

Signed-off-by: Markus Lehtonen <[email protected]>

marquiz force-pushed the devel/rdt-schemata-field branch from a3451ff to 7be025f Compare September 15, 2025 12:12

rata reviewed Sep 15, 2025

View reviewed changes

libcontainer/intelrdt: add support for Schemata field #4830

Are you sure you want to change the base?

libcontainer/intelrdt: add support for Schemata field #4830

Uh oh!

Conversation

marquiz commented Aug 1, 2025

Uh oh!

marquiz commented Aug 18, 2025

Uh oh!

marquiz commented Aug 26, 2025

Uh oh!

rata left a comment

Choose a reason for hiding this comment

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!

Uh oh!

kolyshkin left a comment

Choose a reason for hiding this comment

Uh oh!

marquiz commented Sep 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kolyshkin Sep 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marquiz commented Sep 15, 2025

Uh oh!

rata Sep 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Uh oh!

marquiz commented Sep 3, 2025 •

edited

Loading

kolyshkin Sep 4, 2025 •

edited

Loading

rata Sep 15, 2025 •

edited

Loading