Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Clarify Intel RDT configuration #1196

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Clarify Intel RDT configuration #1196

wants to merge 2 commits into from

Conversation

ipuustin
Copy link

@ipuustin ipuustin commented Apr 6, 2023 •
edited

First change: make it explicit when l3CacheSchema should have lines with MB: prefix filtered out. Previously it was unclear if this should be done always or only if memBwSchema did exist.

Second change: clarify when the subdirectory needs to be removed after the container is finished (not sure if the config spec is the best place for this?).

@utam0k
Copy link
Member

utam0k commented Apr 6, 2023

Does this change require a change in runc behavior?

@ipuustin
Copy link
Author

ipuustin commented Apr 6, 2023

Yes it does... I don't think runc filters the MB:-lines at all -- it just concatenates the values. Also for directory removal, runc removes the directory if and only if ClosID is not set. It doesn't take into account whether or not runc has created the directory to begin with.

@AkihiroSuda AkihiroSuda added this to the v1.2.0 milestone Apr 12, 2023
@utam0k
Copy link
Member

utam0k commented Apr 12, 2023

I have looked at the major container runtime implementations, and only runc is affected. It would be good to have @AkihiroSuda, @kolyshkin, and @cyphar review this.

@giuseppe
Just to be sure, crun doesn't implement Intel RDT, does it?

@giuseppe
Copy link
Member

@giuseppe
Just to be sure, crun doesn't implement Intel RDT, does it?

correct, it is not implemented in crun

@utam0k
Copy link
Member

utam0k commented Apr 13, 2023

@giuseppe Thanks for your quick response 🙏

This was referenced Apr 17, 2023
Merged
Open
@ipuustin
Copy link
Author

I have looked at the major container runtime implementations, and only runc is affected. It would be good to have @AkihiroSuda, @kolyshkin, and @cyphar review this.

@giuseppe Just to be sure, crun doesn't implement Intel RDT, does it?

Thanks for looking at this! I implemented the required runc changes now (in a linked PR).

@AkihiroSuda AkihiroSuda modified the milestones: v1.2.0, v1.1.1 Apr 20, 2023
kolyshkin
kolyshkin approved these changes May 18, 2023
View reviewed changes
Copy link
Contributor

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

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

LGTM

@kolyshkin
Copy link
Contributor

First change: make it explicit when l3CacheSchema should have lines with MB: prefix filtered out. Previously it was unclear if this should be done always or only if memBwSchema did exist.

This is kind of weird. Runtime is not a tool to groom configs. Why don't we just require that if both l3CacheSchema and memBwSchema exist, the first one should not contain lines with MB: prefix? This would be way easier to implement in config validation (strings.HasPrefix(s, "MB:") || strings.Contains(s, "\nMB:")) than the filtering.

@kolyshkin
Copy link
Contributor

Alternatively, if the kernel is OK with getting multiple MB: strings, we can just write both l3CacheSchema and memBwSchema and the latter will naturally overwrite the former.

kolyshkin
kolyshkin requested changes May 18, 2023
View reviewed changes
Copy link
Contributor

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

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

have a question about the requirement of filtering out MB: entries from l3CacheSchema.

@ipuustin
Copy link
Author

TBH I'm not sure why this is written the way it is now, but I think this may be due to originally L3 cache being the only supported resource (see opencontainers/runc#433). The reason for l3CacheSchema containing MB: strings is probably for backwards compatibility during the transitioning from l3CacheSchema to having both l3CacheSchema and memBwSchema.

My target with this PR was to clarify the spec from the implementation point of view, but I fully agree that removing the filtering would make things simpler. I would however lean more towards your first option (erroring out if there are conflicting values). Having the "silent override" would probably confuse users and cause hard-to-debug bugs.

Erroring out in case of conflict would be a breaking change though -- I don't know if there are any workloads which currently set MB: strings from both variables. @marquiz since the previous wording is from your PR, do you have any insight or opinion into this?

@marquiz
Copy link
Contributor

marquiz commented May 24, 2023

Alternatively, if the kernel is OK with getting multiple MB: strings, we can just write both l3CacheSchema and memBwSchema and the latter will naturally overwrite the former.

I think this should actually be true

@ipuustin
config-linux: clarify the "MB:"-line filtering in RDT.
da3b517 
The thinking is that the runtimes should not do the filtering of values,
but instead just apply the values in order. This way the possible
MB-lines in l3CacheSchema will become overwritten by memBwSchema values
(if the domains overlap).

Note that we can't just concatenate the values because kernel will error
out if the same domain is attempted to be set multiple times within one
write() call.

Signed-off-by: Ismo Puustinen <[email protected]>
@ipuustin
config-linux: clarify when the RDT sub-directory should be removed.
a4fca5d 
Signed-off-by: Ismo Puustinen <[email protected]>
@ipuustin ipuustin requested a review from utam0k as a code owner June 2, 2023 11:48
@ipuustin
Copy link
Author

ipuustin commented Jun 2, 2023

@kolyshkin @marquiz I now updated the PR to say that the schemata changes should be applied in order. This is in line with the -"natural override" proposal and also maybe what the user has in mind. This will be a breaking change however, because now we don't filter anything out. If the MB domains set in l3CacheSchema are different than the domains in memBwSchema, both will be set.

@marquiz
Copy link
Contributor

marquiz commented Sep 28, 2023

I now updated the PR to say that the schemata changes should be applied in order. This is in line with the -"natural override" proposal and also maybe what the user has in mind. This will be a breaking change however, because now we don't filter anything out. If the MB domains set in l3CacheSchema are different than the domains in memBwSchema, both will be set.

@ipuustin looking at this now, I'm inclined against a breaking change in the spec. Even though I don't think any runtime actually implements the filtering of data. But LGTM for the additions about directory removal

Related: with a "leave the old mess behind" mentality, I submitted PR #1230.

@AkihiroSuda
Copy link
Member

@kolyshkin PTAL

@AkihiroSuda AkihiroSuda modified the milestones: v1.1.1, v1.1.2 Dec 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AkihiroSuda AkihiroSuda Awaiting requested review from AkihiroSuda AkihiroSuda is a code owner

@crosbymichael crosbymichael Awaiting requested review from crosbymichael crosbymichael is a code owner

@cyphar cyphar Awaiting requested review from cyphar cyphar is a code owner

@dqminh dqminh Awaiting requested review from dqminh dqminh is a code owner

@giuseppe giuseppe Awaiting requested review from giuseppe giuseppe is a code owner

@hqhq hqhq Awaiting requested review from hqhq hqhq is a code owner

@mrunalp mrunalp Awaiting requested review from mrunalp mrunalp is a code owner

@thaJeztah thaJeztah Awaiting requested review from thaJeztah thaJeztah is a code owner

@tianon tianon Awaiting requested review from tianon tianon is a code owner

@vbatts vbatts Awaiting requested review from vbatts

@utam0k utam0k Awaiting requested review from utam0k utam0k is a code owner

@kolyshkin kolyshkin Awaiting requested review from kolyshkin kolyshkin is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v1.2.1 (tentative)
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@ipuustin @utam0k @giuseppe @kolyshkin @marquiz @AkihiroSuda