Intel RDT: update mkdir behaviour to match runtime-spec changes by ipuustin · Pull Request #3832 · opencontainers/runc

ipuustin · 2023-04-18T10:29:14Z

There are two proposed clarifications to the OCI spec here: opencontainers/runtime-spec#1196

The first item specifies that the L3CacheSchema line filtering needs to happen only when both MemBw and L3Cache schemas are specified.

The second item specifies that the subdirectory needs to be deleted. Runc already does that, but the clarification adds for directory removal only if the directory was created by us.

ipuustin · 2023-04-18T12:33:38Z

@marquiz

ipuustin · 2023-05-02T06:29:27Z

Rebased.

ipuustin · 2023-05-09T11:15:31Z

Rebased.

AkihiroSuda · 2023-05-12T00:17:42Z

The spec PR is not merged yet:

Clarify Intel RDT configuration runtime-spec#1196

kolyshkin · 2023-05-18T22:50:42Z

So, this one and #3833 shares some common changes. Which one do you want to go first?

kolyshkin · 2023-05-18T22:53:08Z

libcontainer/intelrdt/intelrdt.go

+	if m.config.IntelRdt != nil && m.config.IntelRdt.ClosID == "" && m.directoryCreated {
 		m.mu.Lock()
 		defer m.mu.Unlock()
 		if err := os.RemoveAll(m.GetPath()); err != nil {


not really about this PR, but this can benefit from cgroups.RemovePath, as here we only want to remove directories, not files.

kolyshkin · 2023-05-18T22:55:38Z

libcontainer/intelrdt/intelrdt.go

+	// If both l3CacheSchema and memBwSchema are set and
+	// l3CacheSchema contains a line beginning with "MB:", the
+	// value written to schemata file MUST be the non-"MB:"
+	// line(s) from l3CacheSchema and the line from memBWSchema.
+
+	validLines := make([]string, 0)
+
+	// Split the l3CacheSchema to lines.
+	lines := strings.Split(l3CacheSchema, "\n")
+
+	// Remove the "MB:" lines.
+	for _, line := range lines {
+		if strings.HasPrefix(line, "MB:") {
+			continue
+		}
+		validLines = append(validLines, line)
+	}
+
+	return strings.Join(validLines, "\n") + "\n" + memBwSchema


This is not very efficient in terms of resources. We split the strings and then we combine them, creating two slices in the process. I'm not sure if there's a better solution (except than a slight optimization of reusing the same slice -- see https://github.com/golang/go/wiki/SliceTricks#filtering-without-allocating).

kolyshkin · 2023-05-18T22:59:15Z

libcontainer/intelrdt/intelrdt.go

+	// If both l3CacheSchema and memBwSchema are set and
+	// l3CacheSchema contains a line beginning with "MB:", the
+	// value written to schemata file MUST be the non-"MB:"
+	// line(s) from l3CacheSchema and the line from memBWSchema.


So, can you elaborate on that requirement? In particular, why we allow a user to include MB: lines into l3CacheSchema? Would it be better to offload this requirement to a user of runc (whoever creates the OCI spec)? I mean, runc is a low level tool and should, theoretically, be as simple as possible.

My other question is -- what happens if we write both values as they are (i.e. without filtering)? Is this a security risk? Or is it just a convenience feature that allows memBwSchema to overwrite whatever is in l3CacheSchema?

One other thought. Let's say that we have

memBwSchema = "MB:0=70;1=20" l3CacheSchema = "MB:0=80;1=10\nL3:0=f0;1=f"

Does it make a difference for the kernel, if we write

MB:0=80;1=10 L3:0=f0;1=f MB:0=70;1=20

or just

L3:0=f0;1=f MB:0=70;1=20

?

(I mean, except the additional line parsing done in the kernel)

To answer your other question: I tried this out now and having two MB values for the same domain in the same write won't work:

[root@wolfpass ipuustin]# uname -r 6.0.10-100.fc35.x86_64 [root@wolfpass ipuustin]# cat schemata-1.txt MB:0=80;1=90 L3:0=ff;1=ff [root@wolfpass ipuustin]# cat schemata-2.txt MB:0=80;1=90 L3:0=ff;1=ff MB:0=60;1=70 [root@wolfpass ipuustin]# cat schemata-1.txt > /sys/fs/resctrl/foo/schemata [root@wolfpass ipuustin]# cat /sys/fs/resctrl/foo/schemata MB:0= 80;1= 90 L3:0=0ff;1=0ff [root@wolfpass ipuustin]# cat schemata-2.txt > /sys/fs/resctrl/foo/schemata cat: write error: Invalid argument [root@wolfpass ipuustin]# cat /sys/fs/resctrl/info/last_cmd_status Duplicate domain 0

So we know that there is nobody trying to set overlapping MB: values in both memBwSchema and l3CacheSchema because that would now trigger an error in runc. There might be someone setting non-overlapping MB: values and not getting those filtered out however.

cyphar · 2023-10-25T14:36:32Z

Moving to 1.3.0, since the spec change isn't out and we want a 1.2.0 release soon.

cyphar · 2025-08-27T22:22:32Z

My impression is that the spec PR removed the MB filtering aspects, so this is no longer necessary. But I'll just move it to 1.5.0-rc1 for now.

marquiz · 2025-08-28T07:15:49Z

My impression is that the spec PR removed the MB filtering aspects, so this is no longer necessary. But I'll just move it to 1.5.0-rc1 for now.

Yes, you're right. But the "only-rmdir-if-we-did-mkdir" part is still relevat (pending the spec change).

ipuustin · 2025-11-19T19:12:59Z

Ok, rebased and kept the "only-rmdir-if-we-did-mkdir" part.

libcontainer/intelrdt/intelrdt.go

cyphar · 2026-03-05T01:27:27Z

@AkihiroSuda It's been ~2 years since you approved this PR, did you want to take another look at it before I merge it?

There is one proposed clarification to the OCI spec: the subdirectory needs to be deleted. Runc already does that, but the clarification adds for directory removal only if the directory was created by us. Signed-off-by: Ismo Puustinen <ismo.puustinen@intel.com> Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>

cyphar · 2026-03-06T07:43:30Z

I'm going to hit merge since @AkihiroSuda seems to have done a fleeting review recently and I resolved his comment. We can always fix this up later.

AkihiroSuda · 2026-03-06T07:53:42Z

libcontainer/intelrdt/intelrdt.go

+	// to clean it up afterwards. Make a note to the manager.
+	if _, err := os.Stat(path); err != nil {
+		if errors.Is(err, os.ErrNotExist) {
+			m.directoryCreated = true


nit: this should be set after mkdir?

Or the variable could be named like directoryDidNotExist

Hmmm, probably. This could've be done more simply by basing it on os.ErrExist as well. That's what I get for merging without waiting for your review... 🙇

ipuustin force-pushed the intel-rdt-updates branch from c486145 to d34c05a Compare May 2, 2023 06:29

ipuustin force-pushed the intel-rdt-updates branch from d34c05a to 1641f82 Compare May 9, 2023 11:15

AkihiroSuda approved these changes May 12, 2023

View reviewed changes

AkihiroSuda added this to the 1.3.0 milestone May 12, 2023

kolyshkin reviewed May 18, 2023

View reviewed changes

AkihiroSuda modified the milestones: 1.3.0, 1.2.0 Jun 12, 2023

cyphar modified the milestones: 1.2.0, 1.3.0 Oct 25, 2023

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

ipuustin force-pushed the intel-rdt-updates branch from 1641f82 to 72a35e9 Compare November 19, 2025 19:08

AkihiroSuda reviewed Nov 20, 2025

View reviewed changes

libcontainer/intelrdt/intelrdt.go Show resolved Hide resolved

cyphar force-pushed the intel-rdt-updates branch from 72a35e9 to 28cc492 Compare March 5, 2026 01:26

cyphar requested a review from kolyshkin March 5, 2026 01:26

cyphar force-pushed the intel-rdt-updates branch from 28cc492 to e2baa3a Compare March 5, 2026 01:28

cyphar approved these changes Mar 5, 2026

View reviewed changes

cyphar changed the title ~~Intel RDT: updates according to proposed spec changes.~~ Intel RDT: minor update to match runtime-spec changes Mar 6, 2026

cyphar changed the title ~~Intel RDT: minor update to match runtime-spec changes~~ Intel RDT: update mkdir behaviour to match runtime-spec changes Mar 6, 2026

cyphar merged commit bf547fb into opencontainers:main Mar 6, 2026
42 checks passed

AkihiroSuda reviewed Mar 6, 2026

View reviewed changes

cyphar mentioned this pull request Mar 6, 2026

libct: intelrdt: improve directory cleanup logic #5155

Merged

Conversation

ipuustin commented Apr 18, 2023

Uh oh!

ipuustin commented Apr 18, 2023

Uh oh!

ipuustin commented May 2, 2023

Uh oh!

ipuustin commented May 9, 2023

Uh oh!

AkihiroSuda commented May 12, 2023

Uh oh!

kolyshkin commented May 18, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kolyshkin May 18, 2023 • 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!

cyphar commented Oct 25, 2023

Uh oh!

cyphar commented Aug 27, 2025

Uh oh!

marquiz commented Aug 28, 2025

Uh oh!

ipuustin commented Nov 19, 2025

Uh oh!

Uh oh!

cyphar commented Mar 5, 2026

Uh oh!

cyphar commented Mar 6, 2026

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

kolyshkin May 18, 2023 •

edited

Loading