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.

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

config-linux: RFC 2119 wording for intelRdt #787

Merged
merged 1 commit into from
May 9, 2017

Conversation

wking
Copy link
Contributor

@wking wking commented May 1, 2017

So we can compliance-test runtimes for these settings.

Also remove the tutorial, since the kernel docs should provide sufficient documentation on that front. The kernel can be patched if they do not, and we do not include tutorials for other config-linux settings in this spec.

See also opencontainers/runc#433 and opencontainers/runc#1279.

CC @xiaochenshen.

defined 'subset' of L3 cache which may be overlapping with other 'subsets'.
The different subsets are identified by class of service (CLOS) and each CLOS
has a capacity bitmask (CBM).
If `intelRdt` is not set, the runtime MUST NOT manipulate any `resctrl` psuedo-filesystems.
Copy link
Contributor

Choose a reason for hiding this comment

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

+ If `intelRdt` is not set, the runtime MUST NOT manipulate any `resctrl` psuedo-filesystems.

How about using these words? There should not be more than one resctrl filesystems.
+ If `intelRdt` is not set, the runtime MUST NOT manipulate the `resctrl` psuedo-filesystem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There should not be more than one resctrl filesystems.

But there might be more than one (just like proc). I expect you'd only actually have multiple resctrl filesystems mounted when you were setting up nested containers (e.g. docker-in-docker), but the “any” wording isn't much more complicated and avoids confusion in the multiple-mount corner case.

Copy link
Contributor

Choose a reason for hiding this comment

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

@wking I got it. It makes sense in nested containers case.

Copy link
Contributor

@xiaochenshen xiaochenshen left a comment

Choose a reason for hiding this comment

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

LGTM. Please also find some comments inline.

config-linux.md Outdated

The following parameters can be specified for the container:
Consider a two-socket machine with two L3 caches where the default CBM is 0xfffff and the max CBM length is 20 bits.
This configuration assigns 4/5 of L3 cache id 0 and the whole L3 cache id 1 for the container:
Copy link
Contributor

@xiaochenshen xiaochenshen May 2, 2017

Choose a reason for hiding this comment

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

Since the inline tutorial will be removed, I would like to improve the original wording to prevent confusion to new users of Intel RDT, and provide a more sophisticated example.

I will submit a new PR for it.

+Tasks inside the container only have access to the "upper" 80% of L3 cache id 0 and the "lower" 50% L3 cache id 1:

"linux": {
	"intelRdt": {
		"l3CacheSchema": "L3:0=ffff0;1=3ff"
	}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

… provide a more sophisticated example…

I've pulled in your suggested example with edef71fcbd2c02. How does that look?

Copy link
Contributor

Choose a reason for hiding this comment

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

@wking Thanks. It looks good to me.

@wking wking force-pushed the intel-rdt-style branch from edef71f to cbd2c02 Compare May 2, 2017 17:43
wking added a commit to wking/opencontainer-runtime-spec that referenced this pull request May 2, 2017
So we can compliance-test runtimes for these settings.

Also remove the tutorial, since the kernel docs should provide
sufficient documentation on that front.  The kernel can be patched if
they do not, and we do not include tutorials for other config-linux
settings in this spec.

The updated example was recommended by Xiaochen to compensate for the
removed inline tutorial [1].

[1]: opencontainers#787 (comment)

Signed-off-by: W. Trevor King <[email protected]>
@hqhq
Copy link
Contributor

hqhq commented May 9, 2017

LGTM

Approved with PullApprove

So we can compliance-test runtimes for these settings.

Also remove the tutorial, since the kernel docs should provide
sufficient documentation on that front.  The kernel can be patched if
they do not, and we do not include tutorials for other config-linux
settings in this spec.

The updated example was recommended by Xiaochen to compensate for the
removed inline tutorial [1].

[1]: opencontainers#787 (comment)

Signed-off-by: W. Trevor King <[email protected]>
@wking wking force-pushed the intel-rdt-style branch from cbd2c02 to b11ade4 Compare May 9, 2017 16:30
@wking
Copy link
Contributor Author

wking commented May 9, 2017

I fixed a “MUST not” → “MUST NOT” and rebased onto master with cbd2c02b11ade4.

@crosbymichael
Copy link
Member

crosbymichael commented May 9, 2017

LGTM

Approved with PullApprove

1 similar comment
@mrunalp
Copy link
Contributor

mrunalp commented May 9, 2017

LGTM

Approved with PullApprove

@mrunalp mrunalp merged commit 9b4b6d7 into opencontainers:master May 9, 2017
@wking wking deleted the intel-rdt-style branch May 9, 2017 23:49
@vbatts vbatts mentioned this pull request Jul 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants