RFC: config-linux: add an initial spec of RDMA related field #930

mitake · 2017-10-17T05:54:20Z

This commit adds an initial spec of RDMA related field for Linux
configuration. They will be used for controlling rdmacg of the Linux
kernel.

Signed-off-by: Hitoshi Mitake mitake.hitoshi@lab.ntt.co.jp

mitake · 2017-10-17T05:56:15Z

/cc @cyphar this is the spec side PR of opencontainers/runc#1612. The main motivation is multiplexing RDMA network interface safely by protecting RDMA resources from buggy applications. Our current target is deep learning framework e.g. TensorFlow and MXNet.

Should I update other files e.g. https://github.com/opencontainers/runtime-spec/blob/master/schema/config-linux.json ?

TomSweeneyRedHat · 2017-10-17T11:29:40Z

config-linux.md

@@ -637,6 +637,30 @@ The following parameters can be specified to set up seccomp:
    "mountLabel": "system_u:object_r:svirt_sandbox_file_t:s0:c715,c811"
 ```

+### <a name="configLinuxRdmaLimits" />RDMA resource limits
+
+**`rdma_limits`** (array of objects, OPTIONAL) represents the `rdma` controller which allows to limit the


suggest: "which allows to limit the" to "which allows the limiting of the"

wking · 2017-10-17T15:22:27Z

config-linux.md

+
+**`rdma_limits`** (array of objects, OPTIONAL) represents the `rdma` controller which allows to limit the
+resources related to network interfaces which support RDMA (e.g. HCA handles).
+For more information, see the RDMA section of [the kernel cgroups documentation][cgroup-v2].


To show that RDMA is not v2-specific, we may want to link both the v1 and v2 docs.

mitake · 2017-10-19T05:50:40Z

Updated based on the comment, could you take a look?

wking

This looks reasonable to me. I've left a few copy-edit suggestions inline, but I'm fine with this PR even if none of those happen.

@crosbymichael may suggest using camelCase for consistency with the rest of the Linux spec (more discussion on this here). But I'll let a maintainer champion that f they're interested, and am fine with this landing either way.

wking · 2017-10-19T18:48:46Z

config-linux.md

+### <a name="configLinuxRdmaLimits" />RDMA resource limits
+
+**`rdma_limits`** (array of objects, OPTIONAL) represents the `rdma` controller which allows the limiting of the
+resources related to network interfaces which support RDMA (e.g. HCA handles).


This doesn't follow the one sentence per line policy.

Also maybe rephrase to the more compact:

rdma_limits (array of objects, OPTIONAL) limits resources for network interfaces which support RDMA (e.g. HCA handles).

wking · 2017-10-19T18:51:37Z

config-linux.md

+
+**`rdma_limits`** (array of objects, OPTIONAL) represents the `rdma` controller which allows the limiting of the
+resources related to network interfaces which support RDMA (e.g. HCA handles).
+For more information, see the RDMA section of [the kernel cgroups documentation][cgroup-v1-rdma].


If you're just linking the v1 docs, you don't need “the RDMA section of”, because the whole page is about RDMA. How about:

For more information, see the [the kernel RMDA controller documentation][cgroup-v1-rdma].

or:

For more information, see the the kernel RMDA controller documentation for [v1][cgroup-v1-rdma] and [v2][cgroup-v2] cgroups.

wking · 2017-10-19T18:54:24Z

config-linux.md

+
+Each entry has the following structure:
+
+* **`interface_name`** *(string, REQUIRED)* - a name of an interface


config_linux.md isn't consistent on this, but config.md is pretty consistent in not wrapping the type/requirement parenthetical in *. That would make this line:

* **`interface_name`** (string, REQUIRED) - a name of an interface

Because of the current config-linux.md inconsistency, I'd be fine with you using wrapping * or not in this commit, although you should probably be internally consistent. Can you drop them here and below, or keep them here and in the rdma_limits entry above?

wking · 2017-10-19T18:58:28Z

config-linux.md

+Each entry has the following structure:
+
+* **`interface_name`** *(string, REQUIRED)* - a name of an interface
+* **`hca_handle_limit`** *(int64, REQUIRED)* - limit a number of HCA handles which can be used by a container (-1 means max)


Maybe follow the kernel wording and use:

* **`hca_handle_limit`** (int64, REQUIRED) - Maximum number of HCA Handles (use `-1` to request the maximum available).

I think talking about “maximum number of” is less ambiguous than “limit a number of”, although I can't put my finger on why, so I'm also fine leaving this as it stands.

wking · 2017-10-19T19:05:58Z

Also, this should be tagged 1.Y.0 or 1.1.0 or some such, because it's adding functionality and therefore is not a patch-level change.

mitake · 2017-10-26T05:39:18Z

@wking updated for your comments, could you take a look?

wking

This still looks good to me. I've left one very minor copy-edit suggestion inline, but I'm fine with this PR even if that suggestion isn't addressed.

With the Markdown looking good, this PR could grow JSON Schema and Go entries, but I'm also fine leaving that to follow-up work (although it would be good to have them landed before we cut the minor bump containing these Markdown changes).

The Travis failure looks like a network hiccup.

wking · 2017-10-26T16:50:16Z

config-linux.md

+
+Each entry has the following structure:
+
+* **`interface_name`** (string, REQUIRED) - a name of an interface


nit: The spec isn't currently consistent on whether single-phrase list entries should end in a period, or whether the post-hyphen word should be capitalized, so I'm fine with whatever you want there. However, your following two lines are capitalized post-hyphen and end with a period, while this one is neither. Whichever way you prefer, it would be good to have this particular list be internally consistent.

This commit adds an initial spec of RDMA related field for Linux configuration. They will be used for controlling rdmacg of the Linux kernel. Signed-off-by: Hitoshi Mitake <mitake.hitoshi@lab.ntt.co.jp>

mitake · 2017-11-06T05:23:20Z

@wking updated based on your latest comment, could you take a look?

paravmellanox · 2017-12-07T19:52:15Z

I noticed this RFC now during meeting.
One reference implementation is available at
paravmellanox/cgroups@469ac1c

There are few minor differences between two PR specifically with respect the size as int64 and int32

wking · 2017-12-07T19:59:29Z

Ah, I'd forgotten about this one. Cross-linking #942, which is moving in the same direction.

wking · 2017-12-07T20:03:26Z

config-linux.md

+
+* **`interface_name`** (string, REQUIRED) - A name of an interface.
+* **`hca_handle_limit`** (int64, REQUIRED) - Maximum number of HCA Handles (use `-1` to request the maximum available).
+* **`hca_object_limit`** (int64, REQUIRED) - Maximum number of HCA Objects (use `-1` to request the maximum available).


From the kernel docs, I think hca_handle_limit and hca_object_limit should both be OPTIONAL, possibly with a requirement that at least one of them be set (this is the same suggestion I made for #942 here).

wking · 2017-12-07T20:04:06Z

config-linux.md

+#### Example
+
+```json
+    "rdma_limits": [


You'll want to drop the leading indent from this block to catch up with #937.

mitake · 2017-12-07T20:14:14Z

I'm closing this because @paravmellanox has the good alternative and reference implementation.

mitake changed the title ~~config-linux: add an initial spec of RDMA related field~~ RFC: config-linux: add an initial spec of RDMA related field Oct 17, 2017

TomSweeneyRedHat reviewed Oct 17, 2017

View reviewed changes

wking reviewed Oct 17, 2017

View reviewed changes

mitake force-pushed the rdmacg branch from 2ac25e3 to 8baaf5c Compare October 19, 2017 05:50

wking reviewed Oct 19, 2017

View reviewed changes

mitake force-pushed the rdmacg branch from 8baaf5c to 10001e7 Compare October 26, 2017 05:38

wking reviewed Oct 26, 2017

View reviewed changes

config-linux: add an initial spec of RDMA related field

f2516eb

This commit adds an initial spec of RDMA related field for Linux configuration. They will be used for controlling rdmacg of the Linux kernel. Signed-off-by: Hitoshi Mitake <mitake.hitoshi@lab.ntt.co.jp>

mitake force-pushed the rdmacg branch from 10001e7 to f2516eb Compare November 6, 2017 05:23

wking reviewed Dec 7, 2017

View reviewed changes

mitake closed this Dec 7, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RFC: config-linux: add an initial spec of RDMA related field #930

RFC: config-linux: add an initial spec of RDMA related field #930

mitake commented Oct 17, 2017

mitake commented Oct 17, 2017

TomSweeneyRedHat Oct 17, 2017

wking Oct 17, 2017

mitake commented Oct 19, 2017

wking left a comment

wking Oct 19, 2017

wking Oct 19, 2017

wking Oct 19, 2017

wking Oct 19, 2017

wking commented Oct 19, 2017

mitake commented Oct 26, 2017

wking left a comment

wking Oct 26, 2017

mitake commented Nov 6, 2017

paravmellanox commented Dec 7, 2017

wking commented Dec 7, 2017

wking Dec 7, 2017

wking Dec 7, 2017

mitake commented Dec 7, 2017


		Each entry has the following structure:

		* `interface_name` (string, REQUIRED) - a name of an interface


		Each entry has the following structure:

		* `interface_name` (string, REQUIRED) - a name of an interface

RFC: config-linux: add an initial spec of RDMA related field #930

RFC: config-linux: add an initial spec of RDMA related field #930

Conversation

mitake commented Oct 17, 2017

mitake commented Oct 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mitake commented Oct 19, 2017

wking left a 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

wking commented Oct 19, 2017

mitake commented Oct 26, 2017

wking left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mitake commented Nov 6, 2017

paravmellanox commented Dec 7, 2017

wking commented Dec 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mitake commented Dec 7, 2017