specs: linux: add intel_rdt cgroup support in specs by xiaochenshen · Pull Request #267 · opencontainers/runtime-spec

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 xiaochen.shen@intel.com

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

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

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 <xiaochen.shen@intel.com>

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 <xiaochen.shen@intel.com>

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 <xiaochen.shen@intel.com>

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 <xiaochen.shen@intel.com>

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 ab5ee2aCompare December 15, 2015 08:16

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

hqhq reviewed Dec 22, 2015
View reviewed changes

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

xiaochenshen force-pushed the rdt-cat branch from 233311b to f5ca480Compare 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 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 e91f77fCompare January 7, 2016 09:28

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

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 <xiaochen.shen@intel.com>

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

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

Uh oh!

Conversation

xiaochenshen commented Dec 11, 2015

Uh oh!

wking commented Dec 11, 2015

Uh oh!

mrunalp commented Dec 18, 2015

Uh oh!

hqhq commented Dec 22, 2015

Uh oh!

xiaochenshen commented Dec 22, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hqhq commented Dec 22, 2015

Uh oh!

xiaochenshen commented Dec 22, 2015

Uh oh!

hqhq commented Dec 22, 2015

Uh oh!

mrunalp commented Dec 22, 2015

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!

Choose a reason for hiding this comment

Uh oh!

vishh commented Dec 22, 2015

Uh oh!

xiaochenshen commented Dec 23, 2015

Uh oh!

philips commented Dec 23, 2015

Uh oh!

xiaochenshen commented Jan 7, 2016

Uh oh!

vbatts commented Jan 7, 2016

Uh oh!

vbatts commented Jan 7, 2016

Uh oh!

xiaochenshen commented Jan 8, 2016

Uh oh!

xiaochenshen commented Feb 14, 2016

Uh oh!

hqhq commented Feb 14, 2016

Uh oh!

xiaochenshen commented Feb 14, 2016

Uh oh!

xiaochenshen commented Feb 14, 2016

Uh oh!

vbatts commented Mar 16, 2016

Uh oh!

xiaochenshen commented Mar 16, 2016

Uh oh!

tianon commented Nov 4, 2016

Uh oh!

xiaochenshen commented Nov 6, 2016

Uh oh!

xiaochenshen commented Nov 21, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants