specs: linux: add intel_rdt cgroup support in specs #267

xiaochenshen · 2015-12-11T19:48:40Z

Add support for the intel_rdt cgroup resource in Linux-specific
configuration to support config.json.

The intel_rdt cgroup subsystem will be available in Linux 4.6 (or later).

Signed-off-by: Xiaochen Shen [email protected]

This is the prerequisite of this runc proposal: opencontainers/runc#433
For more information about intel_rdt cgroup, please refer to: opencontainers/runc#433

wking · 2015-12-11T19:59:29Z

On Fri, Dec 11, 2015 at 11:48:42AM -0800, Xiaochen Shen wrote:

Add support for the intel_rdt cgroup resource in Linux-specific
runtime configuration to support runtime.json.

There has been discussion of factoring cgroup handling out of this
spec 1. I'm not sure where that discussion stands (I'm still
personally in favor of using hooks for cgroups), but it's probably
worth getting some clarity on the current OCI consensus before pushing
forward with this.

If we do want to keep a thin wrapper around all the kernel cgroup APIs
in the specs, than the suggested changes look like they cover the new
controller's only resource file 2.

 Subject: removal of cgroups from the OCI Linux spec
 Date: Wed, 28 Oct 2015 17:01:59 +0000
 Message-ID: <CAD2oYtO1RMCcUp52w-xXemzDTs+J6t4hS5Mm4mX+uBnVONGDfA@mail.gmail.com>

mrunalp · 2015-12-18T19:31:24Z

LGTM

hqhq · 2015-12-22T06:23:25Z

Needs rebase and fit current resource value types.

xiaochenshen · 2015-12-22T08:03:04Z

I have git-rebased against latest master branch. New commit: 3f61bc5

hqhq · 2015-12-22T08:13:53Z

runtime_config_linux.go

@@ -216,6 +222,8 @@ type Resources struct {
 	HugepageLimits []HugepageLimit `json:"hugepageLimits,omitempty"`
 	// Network restriction configuration
 	Network *Network `json:"network,omitempty"`
+	// IntelRdt restriction configuration
+	IntelRdt IntelRdt `json:"intelRdt,omitempty"`


I think it's better to use pointer to be consistent with other fields, you can see from #233 why we are doing this.

Thanks. Changed the types of IntelRdt and L3Cbm to pointer according to #233.
Commit: 233311b

hqhq · 2015-12-22T08:57:09Z

Sorry didn't mention it before, can you add more description about intel_rdt cgroup in runtime-config-linux.md like the other subsystems?

xiaochenshen · 2015-12-22T09:57:03Z

@hqhq Added intel_rdt cgroup description in runtime-config-linux.md in commit f5ca480

hqhq · 2015-12-22T10:55:28Z

I'm not sure if an on going patchset is proper for specs reference, or maybe we should wait it merge to kernel upstream before add it to specs? Other than that, the whole change LGTM.

Let's wait for one more maintainer's input before merge this. Thanks for your work @xiaochenshen !
ping @vishh @crosbymichael @LK4D4 @vbatts @dqminh

mrunalp · 2015-12-22T17:04:52Z

I did not realize that this wasn't merged yet. In that case, we should wait for it to be merged to the kernel first.

vishh · 2015-12-22T19:19:41Z

runtime_config_linux.go

+// IntelRdt for Linux cgroup 'intel_rdt' resource management
+type IntelRdt struct {
+	// L3 cache capacity bitmask (CBM) for container
+	L3Cbm *string `json:"l3Cbm,omitempty"`


Why is it a string and not an integer?

On Tue, Dec 22, 2015 at 11:19:43AM -0800, Vish Kannan wrote:

// L3 cache capacity bitmask (CBM) for container

L3Cbm *string json:"l3Cbm,omitempty"

Why is it a string and not an integer?

So folks can use hex [1](which JSON doesn't support [2])? That's
probably not a good enough reason, since convenient hand-editing is a
lower priority for us than convenient machine readability. A rule for
handling traditionally-hex values should go into a best-practices doc
(#273) if/when we decide to do that.

@vishh @wking Thank you for pointing this out.
In this cgroup context, intel_rdt.l3_cbm represents the L3 cache capacity bitmask. In kernel, by nature, read/write with 'hexadecimal string' is the best choice for user-friendly and user-readable. Kernel handles it as 'u64' via 'write_u64' interface inside for 'machine readability'.

In oci/specs, currently I choose the type 'string' instead of 'uint64' for some reasons:

JSON doesn't natively support hexadecimal, as @wking mentioned. For me it is really an important reason. If JSON supports hexadecimal or something more readable for bitmask, 'uint64' type inside with '0x' prefix is perfect, we don't need discuss this issue here any more.

User-readability VS machine-readability. JSON doesn't support hexadecimal means user-readability issue even though kernel treats hexadecimal (0x-prefix), octal (0-prefix) and decimal integers without discrimination. For example (l3Cbm: 0xff0, bits 4-11 are set):
(1) Type string:

"intel_rdt": { "l3Cbm": "0xff0" }

(2) Type uint64:

"intel_rdt": { "l3Cbm": 4080 }

(3) If JSON supports hexadecimal, with type uint64 looks like:

"intel_rdt": { "l3Cbm": 0xff0 }

Case (3) should be perfect, but it doesn't work. Case (2), '4080' is very confusing as a bitmask value for end user. As a compromise, we use case (1) now.

Anyway, I will not always stick to my point if we have a better solution. Actually, when I was implementing opencontainers/runc#433, I found the string/integer conversion back-and-forth is really chatty due to JSON's limitation on non-decimal integer support.

On Tue, Dec 22, 2015 at 09:22:58PM -0800, Xiaochen Shen wrote:

Case (2), '4080' is very confusing as a bitmask value for end user.

The counter-argument here is that the target audience for the
config-file isn't end users [1,2,3], which can always insert some
tooling in between themselves and the JSON config. For example, it
wouldn't be too bad to write a parser that converts a JSON-extension
form like:

"intel_rdt": {
"l3Cbm": 0xff0
}

to official JSON. And it's even easier (from an implementation
perspective, if not from a usability perspective) to do:

$ python -c 'print(hex(4080))'
0xff0

So my preference is “use integer types for integer values, and deal
with the lack of JSON hex support using other tools”. But I don't
feel strongly enough about that put up a fight if the consensus is
that we want to use JSON strings for integers that are more intuitive
as hex.

Subject: Re: removal of cgroups from the OCI Linux spec Date: Thu, 29 Oct 2015 20:15:13 -0700 Message-ID: <CACfVidd8sV14rG6S2DX1mJ35ij1-Y7sAxir4p-U9EscCeqCMwA@mail.gmail.com>

wking added a note 18 hours ago

The counter-argument here is that the target audience for the
config-file isn't end users [1,2,3], which can always insert some
tooling in between themselves and the JSON config. For example, it
wouldn't be too bad to write a parser that converts a JSON-extension
form like:

@wking It makes sense.If JSON or similar config-file is not always UI-like thing,
integer is better than string even for 'bitmask' in the case.

Actually, I tried to use type 'uint64' instead of 'string' in the v1 patch of
opencontainers/runc#447, the code looks better, and the JSON config file looks like:

"intel_rdt": { "l3Cbm": 4080 }

@Others, please tell me if you have different opinions.
Otherwise, I will change the type of IntelRdt.L3Cbm from *string to *uint64 later.

xiaochenshen added
@Others, please tell me if you have different opinions.
Otherwise, I will change the type of IntelRdt.L3Cbm from *string to *uint64 later.

Ping @mrunalp @vishh @hqhq @philips

+1 for *uint64.

I have changed the type of 'IntelRdt.L3Cbm' from *string to *uint64 in commit e91f77f

vishh · 2015-12-22T19:21:17Z

+1 for holding on to this PR until this feature is merged and becomes stable upstream.
In general, for new experimental features, the spec should be extensible and not require updating the Spec.

xiaochenshen · 2015-12-23T03:25:07Z

@mrunalp @vishh Sure, we would wait for the kernel patch 'upstreamed' in case of any changes in the data structures or APIs

philips · 2015-12-23T03:58:33Z

This is another example of the spec being of questionable valuable with regards to encapsulating the entire cgroups API. I created a tag of "platforms/linux/blocked-upstream" to track these.

This patch is not necessary if this pull request is merged: opencontainers/runtime-spec#267 Signed-off-by: Xiaochen Shen <[email protected]>

xiaochenshen · 2016-01-07T09:56:12Z

I have submitted latest commit e91f77f to address the comments of the data type discussion. But I found 2/3 of Travis CI build is failed (go: 1.4.3, 1.3.3), but 1 is passed (go: 1.5.1).

Who knows why?

vbatts · 2016-01-07T19:15:00Z

hmmm. perhaps something in the upstream vet changed that is broken on go1.3 and go1.4?
https://travis-ci.org/opencontainers/specs/jobs/100780130#L117

vbatts · 2016-01-07T20:37:09Z

@xiaochenshen i retriggered the tests, and they ran fine.

xiaochenshen · 2016-01-08T09:23:17Z

@vbatts Thanks.

xiaochenshen · 2016-02-14T05:28:11Z

The latest commit 38df066 rebased e91f77f for unified config file according to #284.

hqhq · 2016-02-14T06:19:36Z

@xiaochenshen Thanks for your PR, since it's blocked by Linux upstream, you don't need to rebase now as we won't merge until it's landed to Linux upstream.

But you can rebase your PR in runC, we can merge that without specs change.

This patch is not necessary if this pull request is merged: opencontainers/runtime-spec#267 Signed-off-by: Xiaochen Shen <[email protected]>

Add support for the intel_rdt cgroup resource in Linux-specific configuration to support config.json. The intel_rdt cgroup subsystem will be available in Linux 4.6 (or later). Signed-off-by: Xiaochen Shen <[email protected]>

xiaochenshen · 2016-02-14T08:40:45Z

Fixed typo in config-linux.md. The latest commit: c3b1cf7

xiaochenshen · 2016-02-14T08:42:34Z

@hqhq I plan to update both specs and runc and make them synced.

This patch is not necessary if this pull request is merged: opencontainers/runtime-spec#267 Signed-off-by: Xiaochen Shen <[email protected]>

vbatts · 2016-03-16T17:35:10Z

Is this still waiting on upstream linux 4.6? (given that 4.5 was only released this week...)

xiaochenshen · 2016-03-16T23:40:03Z

@vbatts Yes. Kernel patch is still pending. The interface/API of the kernel patch may change per community's comments. I will track the change and modify this PR accordingly.

tianon · 2016-11-04T22:02:40Z

Can we get a status update here? 🙏

xiaochenshen · 2016-11-06T16:32:21Z

@tianon This PR depends on Intel RDT kernel patch. But the kernel interface has changed from cgroup to "resource control" interface (cgroup-like). Currently, the kernel patch has finished code reviewed and will be likely to merge in 4.10 window.

Accordingly, this PR will be obsoleted by new PR which based on the new kernel interface soon. The implementation is working in progress now. Please find more information from opencontainers/runc#433

xiaochenshen · 2016-11-21T16:31:27Z

This PR could be closed for the new PR #630 is open.

xiaochenshen force-pushed the rdt-cat branch from 7a7f846 to ab5ee2a Compare December 15, 2015 08:16

xiaochenshen force-pushed the rdt-cat branch from ab5ee2a to 3f61bc5 Compare December 22, 2015 07:58

hqhq reviewed Dec 22, 2015
View reviewed changes

xiaochenshen force-pushed the rdt-cat branch from 3f61bc5 to 233311b Compare December 22, 2015 08:24

xiaochenshen force-pushed the rdt-cat branch from 233311b to f5ca480 Compare December 22, 2015 09:51

vishh reviewed Dec 22, 2015
View reviewed changes

philips added platforms/linux platforms/linux/blocked-upstream labels Dec 23, 2015

xiaochenshen added a commit to xiaochenshen/runc that referenced this pull request Dec 24, 2015

Godeps: specs: update specs for intel_rdt cgroup

c63855c

This patch is not necessary if this pull request is merged: opencontainers/runtime-spec#267 Signed-off-by: Xiaochen Shen <[email protected]>

xiaochenshen added a commit to xiaochenshen/runc that referenced this pull request Dec 24, 2015

Godeps: specs: update specs for intel_rdt cgroup

b328258

This patch is not necessary if this pull request is merged: opencontainers/runtime-spec#267 Signed-off-by: Xiaochen Shen <[email protected]>

xiaochenshen mentioned this pull request Dec 24, 2015

libcontainer: cgroups: add intel_rdt support in runc opencontainers/runc#447

Closed

wking mentioned this pull request Dec 28, 2015

config: Single, unified config file #284

Merged

wking mentioned this pull request Jan 5, 2016

style: Collect established conventions in a discoverable location #287

Merged

xiaochenshen force-pushed the rdt-cat branch 3 times, most recently from 455219f to e91f77f Compare January 7, 2016 09:28

xiaochenshen force-pushed the rdt-cat branch from e91f77f to 38df066 Compare February 14, 2016 05:19

xiaochenshen added a commit to xiaochenshen/runc that referenced this pull request Feb 14, 2016

Godeps: specs: update specs for intel_rdt cgroup

7ad50b1

This patch is not necessary if this pull request is merged: opencontainers/runtime-spec#267 Signed-off-by: Xiaochen Shen <[email protected]>

specs: linux: add intel_rdt cgroup support in specs

c3b1cf7

Add support for the intel_rdt cgroup resource in Linux-specific configuration to support config.json. The intel_rdt cgroup subsystem will be available in Linux 4.6 (or later). Signed-off-by: Xiaochen Shen <[email protected]>

xiaochenshen force-pushed the rdt-cat branch from 38df066 to c3b1cf7 Compare February 14, 2016 08:20

xiaochenshen added a commit to xiaochenshen/runc that referenced this pull request Feb 14, 2016

Godeps: specs: update specs for intel_rdt cgroup

273cbbb

This patch is not necessary if this pull request is merged: opencontainers/runtime-spec#267 Signed-off-by: Xiaochen Shen <[email protected]>

wking mentioned this pull request Mar 18, 2016

Support Lifecycle Hooks #20

Closed

vbatts mentioned this pull request Apr 8, 2016

Anchor examples #348

Merged

xiaochenshen mentioned this pull request Jun 16, 2016

Proposal: Intel RDT/CAT support for OCI/runc and Docker opencontainers/runc#433

Closed

wking mentioned this pull request Jul 29, 2016

Add Runtime CLI Spec #513

Closed

xiaochenshen mentioned this pull request Nov 21, 2016

specs-go/config: add Intel RDT/CAT Linux support #630

Merged

hqhq closed this Nov 22, 2016

xiaochenshen deleted the rdt-cat branch October 16, 2017 09:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

specs: linux: add intel_rdt cgroup support in specs #267

specs: linux: add intel_rdt cgroup support in specs #267

xiaochenshen commented Dec 11, 2015

wking commented Dec 11, 2015

mrunalp commented Dec 18, 2015

hqhq commented Dec 22, 2015

xiaochenshen commented Dec 22, 2015

hqhq Dec 22, 2015

xiaochenshen Dec 22, 2015

hqhq commented Dec 22, 2015

xiaochenshen commented Dec 22, 2015

hqhq commented Dec 22, 2015

mrunalp commented Dec 22, 2015

vishh Dec 22, 2015

wking Dec 22, 2015

xiaochenshen Dec 23, 2015

wking Dec 23, 2015

xiaochenshen Dec 24, 2015

xiaochenshen Dec 30, 2015

vishh Jan 4, 2016

xiaochenshen Jan 7, 2016

vishh commented Dec 22, 2015

xiaochenshen commented Dec 23, 2015

philips commented Dec 23, 2015

xiaochenshen commented Jan 7, 2016

vbatts commented Jan 7, 2016

vbatts commented Jan 7, 2016

xiaochenshen commented Jan 8, 2016

xiaochenshen commented Feb 14, 2016

hqhq commented Feb 14, 2016

xiaochenshen commented Feb 14, 2016

xiaochenshen commented Feb 14, 2016

vbatts commented Mar 16, 2016

xiaochenshen commented Mar 16, 2016

tianon commented Nov 4, 2016

xiaochenshen commented Nov 6, 2016

xiaochenshen commented Nov 21, 2016

specs: linux: add intel_rdt cgroup support in specs #267

specs: linux: add intel_rdt cgroup support in specs #267

Conversation

xiaochenshen commented Dec 11, 2015

wking commented Dec 11, 2015

mrunalp commented Dec 18, 2015

hqhq commented Dec 22, 2015

xiaochenshen commented Dec 22, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hqhq commented Dec 22, 2015

xiaochenshen commented Dec 22, 2015

hqhq commented Dec 22, 2015

mrunalp commented Dec 22, 2015

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vishh commented Dec 22, 2015

xiaochenshen commented Dec 23, 2015

philips commented Dec 23, 2015

xiaochenshen commented Jan 7, 2016

vbatts commented Jan 7, 2016

vbatts commented Jan 7, 2016

xiaochenshen commented Jan 8, 2016

xiaochenshen commented Feb 14, 2016

hqhq commented Feb 14, 2016

xiaochenshen commented Feb 14, 2016

xiaochenshen commented Feb 14, 2016

vbatts commented Mar 16, 2016

xiaochenshen commented Mar 16, 2016

tianon commented Nov 4, 2016

xiaochenshen commented Nov 6, 2016

xiaochenshen commented Nov 21, 2016