-
Notifications
You must be signed in to change notification settings - Fork 669
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
image-index: Document Linux cpuinfo flags (on x86) for platform.features #631
Conversation
image-index.md
Outdated
This OPTIONAL property specifies an array of strings, each specifying a mandatory CPU feature (for example `sse4` or `aes`). | ||
This OPTIONAL property specifies an array of strings, each specifying a mandatory CPU feature. | ||
|
||
When `architecture` is 386 or amd64 Image indexes SHOULD use, and implementations SHOULD understand, values [supported by Linux][cpufeatures.h] with the `X86_FEATURE_` prefix removed and the remainder lowercased (e.g. `fpu` for `X86_FEATURE_FPU` and `vme` for `X86_FEATURE_VME`). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should not be defined in terms of linux, but instead in terms of amd64
/x86
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should not be defined in terms of linux, but instead in terms of amd64/x86.
Isn't that what I'm doing? As I said in the initial post, I'm happy to link to something other than a Linux header if we find an OS-agnostic registry, but I'm currently not aware of one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops, forgot to click submit
image-index.md
Outdated
This OPTIONAL property specifies an array of strings, each specifying a mandatory CPU feature. | ||
|
||
When `architecture` is 386 or amd64 Image indexes SHOULD use, and implementations SHOULD understand, values [supported by Linux][cpufeatures.h] with the `X86_FEATURE_` prefix removed and the remainder lowercased (e.g. `fpu` for `X86_FEATURE_FPU` and `vme` for `X86_FEATURE_VME`). | ||
On Linux, the features supported by host CPUs can be found in the `flags` entries in the `cpuinfo` file provided by the [proc filesystem][proc.5]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perhaps "on Linux on these architectures"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
image-index.md
Outdated
This OPTIONAL property specifies an array of strings, each specifying a mandatory CPU feature (for example `sse4` or `aes`). | ||
This OPTIONAL property specifies an array of strings, each specifying a mandatory CPU feature. | ||
|
||
When `architecture` is 386 or amd64 Image indexes SHOULD use, and implementations SHOULD understand, values [supported by Linux][cpufeatures.h] with the `X86_FEATURE_` prefix removed and the remainder lowercased (e.g. `fpu` for `X86_FEATURE_FPU` and `vme` for `X86_FEATURE_VME`). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"When architecture
is set to 386
or amd64
, image indexes SHOULD use"
don't we also need to clarify that os == linux ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't we also need to clarify that os == linux ?
The features are architecture-specific, and are OS-independent. It's just our current definition of strings that is tied to Linux, and I will happily change that to use an OS-agnostic registry of CPU features if we can find one (as discussed in the initial post).
image-index.md
Outdated
When `architecture` is 386 or amd64 Image indexes SHOULD use, and implementations SHOULD understand, values [supported by Linux][cpufeatures.h] with the `X86_FEATURE_` prefix removed and the remainder lowercased (e.g. `fpu` for `X86_FEATURE_FPU` and `vme` for `X86_FEATURE_VME`). | ||
On Linux, the features supported by host CPUs can be found in the `flags` entries in the `cpuinfo` file provided by the [proc filesystem][proc.5]. | ||
|
||
When `architecture` is neither 386 nor amd64, values are implementation-defined. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unset or set to any other value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unset or set to any other value
I think “neither 386 nor amd64” already covers architecture
being unset or set to another value, and can't come up with clearer wording that incorporates your phrase. Can you just paste out your whole recommended sentence in case I'm missing something obvious?
ee92880
to
dc5e4a0
Compare
On Mon, Apr 03, 2017 at 10:34:28AM -0700, Jonathan Boulle wrote:
@wking should/can we close this in favour of #632?
If that's easier for maintainers, I'm happy to do that [1]. However,
I like the wording here more than what's currently in #632, so if we
close this I'll advocate for:
* A cpufeatures.h reference (which allows maintainers interested in
non-Linux OSes to still discover the feature slugs they should be
supporting).
* Making ‘features’ implementation-defined (instead of unspecified) on
non-x86-ish architectures, so users can figure out what their
tooling is trying to do, even if that tooling doesn't register their
values with the image spec.
Putting that discussion in #632 along with its other threads starts to
feel crowded to me, but if it won't bog down #632 I'm happy to move
the conversation over there.
[1]: #632 (comment)
|
I removed duplicating part from my PR. |
image-index.md
Outdated
When `architecture` is 386 or amd64 Image indexes SHOULD use, and implementations SHOULD understand, values [supported by Linux][cpufeatures.h] with the `X86_FEATURE_` prefix removed and the remainder lowercased (e.g. `fpu` for `X86_FEATURE_FPU` and `vme` for `X86_FEATURE_VME`). | ||
On Linux on these architectures, the features supported by host CPUs can be found in the `flags` entries in the `cpuinfo` file provided by the [proc filesystem][proc.5]. | ||
|
||
When `architecture` is neither 386 nor amd64, values are implementation-defined. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
diff --git a/image-index.md b/image-index.md
index 977aa80..c1c9778 100644
--- a/image-index.md
+++ b/image-index.md
@@ -69,10 +69,10 @@ For the media type(s) that this document is compatible with, see the [matrix][ma
This OPTIONAL property specifies an array of strings, each specifying a mandatory CPU feature.
- When `architecture` is 386 or amd64 Image indexes SHOULD use, and implementations SHOULD understand, values [supported by Linux][cpufeatures.h] with the `X86_FEATURE_` pref
ix removed and the remainder lowercased (e.g. `fpu` for `X86_FEATURE_FPU` and `vme` for `X86_FEATURE_VME`).
+ When `architecture` is `386` or `amd64`, image indexes SHOULD use, and implementations SHOULD understand, values [supported by Linux][cpufeatures.h] with the `X86_FEATURE_`
prefix removed and the remainder lowercased (e.g. `fpu` for `X86_FEATURE_FPU` and `vme` for `X86_FEATURE_VME`).
On Linux on these architectures, the features supported by host CPUs can be found in the `flags` entries in the `cpuinfo` file provided by the [proc filesystem][proc.5].
- When `architecture` is neither 386 nor amd64, values are implementation-defined.
+ When `architecture` is neither `386` nor `amd64`, it SHOULD be submitted to this specification for standardization.
- **`annotations`** *string-string map*
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AkihiroSuda, in dc5e4a0 → e1045bf, I've picked up your backticks, “Image” → “image”, and commas. I've also picked up your “SHOULD be submitted”, but kept my “implementation-defined” for the reasons I gave here.
@@ -112,5 +117,7 @@ For the media type(s) that this document is compatible with, see the [matrix][ma | |||
} | |||
``` | |||
|
|||
[cpufeatures.h]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/x86/include/asm/cpufeatures.h |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For L108 (GitHub doesn't allow me to directly comment there)
- os.features -> features
- sse4 -> sse4_2 (please grep and update other files as well)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed “os.features” → “features” in dc5e4a0 → e1045bf. Also replaced the old sse4
instances, although I replaced them with fpu
since I was using fpu
and vme
since those are the first two entries in cpufeatures.h
. I don't really care what features we use as examples though; if there's a strong preference for sse4_2
, I can switch to that instead.
cc @xiaochenshen (author of some Xeon-specific runtime spec: opencontainers/runtime-spec@73a6002) |
dc5e4a0
to
e1045bf
Compare
How about rather using |
Sounds good to me. More explanation of For more information about Intel RDT and CAT: |
So is it possible to have Also, this sort of resource allocation seems like more of a runtime concern than an image concern. An image that requires CAT is hard to imagine. Maybe it's going to be the only container on my host, in which case I think I should be able to run it even if my processor doesn't support CAT (with a runtime config that doesn't call for CAT). |
cat_l3 depends on rdt_a. If the flag cat_l3 is available in /proc/cpuinfo, rdt_a should be also available in /proc/cpuinfo. Logically: cat = L3 cache allocation (cat_l3) or L2 cache allocation (cat_l2, in some Intel Atom CPU based platforms in future)
I agree with you that the resource allocation is more of a runtime concern. It looks like hardware-assisted cgroup for resource (e.g., L3 cache) constraints. You can refer to opencontainers/runc#1279 In following cases, L3 cache is entirely shared. It is equivalent to the case which we don't have CAT feature. |
On Tue, Apr 11, 2017 at 10:49:55AM -0700, Xiaochen Shen wrote:
cat_l3 depends on rdt_a. If the flag cat_l3 is available in
/proc/cpuinfo, rdt_a should be also available in /proc/cpuinfo.
So if you were setting platform.features for this, you'd only include
cat_l3 to stay DRY.
I agree with you that the resource allocation is more of a runtime
concern.
So can we find some features that are more likely to be image
concerns? It seems like the most defensible would be “feature $FEAT
marks support for instruction $INST, and this image contains an
executable compiled to use $INST with no fallback”. And ideally this
would be backed up by an actual image that someone is using. If we
can't find such an image, we may want to consider reserving this field
for now.
The ‘fpu’ feature doesn't quite meet those criteria, because there are
likely to be software workarounds if the CPU doesn't support floating
point operations. But if you're doing anything that requires a lot of
floating point calculations, you *really* want hardware support for
them, so it's not that far off. And while broad support for ‘fpu’ [1]
makes the example less interesting, I think an image designed for lots
of floating point operations should still list ‘fpu’ in
platform.features. I'm not familiar enough with ‘vme’ to know, but
expect it falls into the same boat as ‘fpu’.
[1]: #631 (comment)
|
I think we should not take too much effort for deciding example values, but how about: "feature I see some reasons to deploy such VMs as OCI images:
For these use cases, checking whether |
e1045bf
to
e6136dc
Compare
Based on Akihiro Suda's proposal [1] and the subsequent discussion. Using the Linux kernel to define the values is very convenient on Linux, but will likely be a pain for folks on other OSes. If a comprehensive, OS-agnostic list is found, we could use that instead, but would want to provide a mapping from that list of values to the Linux cpuinfo slugs for convenience. Providing a mapping from the OS-agnostic list to the cpuinfo analog on non-Linux OSes would also be convenient. Until then, folks wondering what a given feature slug means but unwilling to dig through the Linux source may find the Stack Overflow post at [2] useful. This definition is not only Linux-centric, it is x86-centric. The x86 Linux implementation writes 'flags' out here [3], but arm uses 'Features' [4]. While we could suggest values for other architectures, this commit restricts itself to the x86 family (using their GOARCH names [5], as the runtime spec recommends [6]), because that's the scope Stephen is currently claiming [7]. Extending the field to other architectures can happen in follow-up work. The vmx example is based on: On Mon, Apr 24, 2017 at 01:53:43AM -0700, Akihiro Suda wrote [8]: > I think we should not take too much effort for deciding example > values, but how about: > > "feature `vmx` marks support for instruction > `VMXON,VMXOFF,VMLAUNCH,VMRESUME,VMCALL,VMPTRLD,VMPTRST,VMCLEAR,VMREAD > and VMWRITE`, and this image contains an executable compiled to > use `VM... instructions` with no fallback”. > > I see some reasons to deploy such VMs as OCI images: > > - Android emulator (x86) > - Forked versions of KVM, that cannot be installed via apt-get > - `docker run`-nable Unikernel demo https://github.com/Unikernel-Systems/DockerConEU2015-demo > - ... > > For these use cases, checking whether `vmx` supported is useful, > because `vmx` is typically missing on EC2. [1]: opencontainers#622 (comment) [2]: http://unix.stackexchange.com/questions/43539/what-do-the-flags-in-proc-cpuinfo-mean [3]: https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git/tree/arch/x86/kernel/cpu/proc.c?id=refs/tags/v4.10.4#n96 [4]: https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git/tree/arch/arm/kernel/setup.c?id=refs/tags/v4.10.4#n1234 [5]: https://golang.org/doc/install/source#environment [6]: https://github.com/opencontainers/runtime-spec/blob/v1.0.0-rc5/config.md#platform [7]: opencontainers#622 (comment) [8]: opencontainers#631 (comment) Signed-off-by: W. Trevor King <[email protected]>
On Mon, Apr 24, 2017 at 01:53:43AM -0700, Akihiro Suda wrote:
- `docker run`-nable Unikernel demo
https://github.com/Unikernel-Systems/DockerConEU2015-demo
|
image-index.md
Outdated
"os.features": [ | ||
"sse4" | ||
"features": [ | ||
"fpu" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fpu -> vmx
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SGTM (IANAM) |
Based on Akihiro Suda's proposal [1] and the subsequent discussion. Using the Linux kernel to define the values is very convenient on Linux, but will likely be a pain for folks on other OSes. If a comprehensive, OS-agnostic list is found, we could use that instead, but would want to provide a mapping from that list of values to the Linux cpuinfo slugs for convenience. Providing a mapping from the OS-agnostic list to the cpuinfo analog on non-Linux OSes would also be convenient. Until then, folks wondering what a given feature slug means but unwilling to dig through the Linux source may find the Stack Overflow post at [2] useful. This definition is not only Linux-centric, it is x86-centric. The x86 Linux implementation writes 'flags' out here [3], but arm uses 'Features' [4]. While we could suggest values for other architectures, this commit restricts itself to the x86 family (using their GOARCH names [5], as the runtime spec recommends [6]), because that's the scope Stephen is currently claiming [7]. Extending the field to other architectures can happen in follow-up work. The vmx example is based on: On Mon, Apr 24, 2017 at 01:53:43AM -0700, Akihiro Suda wrote [8]: > I think we should not take too much effort for deciding example > values, but how about: > > "feature `vmx` marks support for instruction > `VMXON,VMXOFF,VMLAUNCH,VMRESUME,VMCALL,VMPTRLD,VMPTRST,VMCLEAR,VMREAD > and VMWRITE`, and this image contains an executable compiled to > use `VM... instructions` with no fallback”. > > I see some reasons to deploy such VMs as OCI images: > > - Android emulator (x86) > - Forked versions of KVM, that cannot be installed via apt-get > - `docker run`-nable Unikernel demo https://github.com/Unikernel-Systems/DockerConEU2015-demo > - ... > > For these use cases, checking whether `vmx` supported is useful, > because `vmx` is typically missing on EC2. [1]: opencontainers#622 (comment) [2]: http://unix.stackexchange.com/questions/43539/what-do-the-flags-in-proc-cpuinfo-mean [3]: https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git/tree/arch/x86/kernel/cpu/proc.c?id=refs/tags/v4.10.4#n96 [4]: https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git/tree/arch/arm/kernel/setup.c?id=refs/tags/v4.10.4#n1234 [5]: https://golang.org/doc/install/source#environment [6]: https://github.com/opencontainers/runtime-spec/blob/v1.0.0-rc5/config.md#platform [7]: opencontainers#622 (comment) [8]: opencontainers#631 (comment) Signed-off-by: W. Trevor King <[email protected]>
e6136dc
to
ca8c1fd
Compare
is this ready for review? |
On Wed, May 03, 2017 at 12:41:33PM -0700, v1.0.0.batts wrote:
is this ready for review?
Is there a threshold for that? ;). I'm not aware of any outstanding
change requests, at any rate. If anybody has something they'd like me
to change that I've forgotten about, please remind me :).
|
I'm not sure it is a good idea to specify CPU features for images and then match them with host features, mostly for practical reasons. Every new Intel architecture revision introduces new features. On my system I currently have about 60+ features reported. The Linux kernel needs to detect these new feature and report them. Some of this detection/reporting code is not necessarily backported to older kernels. So, you may have a new CPU but your vendor kernel does not report the features it supports, preventing you from running some images although your hardware supports them. In VMs it's even harder because your Hypervisor also needs to detect the features, so you need a matching between Hypervisor, Guest OS kernel and images flags (and you Hypervisor may needs special support for some features). How do you determine which flags to set on your image? Compiler flags are typically different to And this is just the "simple" Intel/x86/linux case... My suggestion would be to either not have |
On Thu, May 04, 2017 at 01:11:00AM -0700, Rolf Neugebauer wrote:
Some of this detection/reporting code is not necessarily backported
to older kernels. So, you may have a new CPU but your vendor kernel
does not report the features it supports, preventing you from
running some images although your hardware supports them.
That's a fair point, but as it stands in ca8c1fd there is no language
about what image-spec implementations should *do* with these values.
This PR is only defining what values are valid, and (sort of) what
those values mean. So if you need a particular instruction (and your
binary has no auto-detection or fallback), you set the appropriate
flag in ‘features’. If a particular host is interested in your image
and the CPU supports that instruction, you can use it regardless of
whether that host's kernel reports the feature or not. I'm open to
adding a caveat to this effect to the current wording. How does:
On Linux on these architectures, the features supported by host CPUs
can be found in the `flags` entries in the `cpuinfo` file provided
by the [proc filesystem][proc.5], although older kernels may not
report all features supported by newer CPUs.
sound to you?
How do you determine which flags to set on your image?
You could get an upper limit by scanning the binaries for instructions:
$ objdump -d /bin/busybox | sed -n 's/^.*\t.*\t\([^ ]*\) .*/\1/p' | sort | uniq
and then mapping from instruction to feature flag (e.g. AESENC → aes).
But that's not going to pick up cases where the binary includes a
fallback for an unsupported instruction.
But I expect that in most cases, features entries will be added because:
1. The software authors declare required features, and the image
authors (who may also be the software authors) carry those
dev-declared features into their image.
2. A user tries to run the image on a CPU that does not support a
required instruction, they report the failure to the image authors,
and the image authors add the missing feature listing when updating
their image.
And then you still have no idea which features are actually used by
the compiler.
This is the main issue which separates the ‘features’ field from
compiler options. Unless you dig enough to figure this out, you may
have to rely on the second approach (patching features as required
instructions are reported).
My suggestion would be to either not have `features` field at all or
use it purely advisory, more like a hint, so that a warning can be
printed, or, if a image crashes with an illegal instruction
exception, one can report an error and say, "hey, maybe it's because
some of these features don't match".
The current wording allows this advisory approach. If you feel it
does not, what would you suggest I change to allow the advisory
approach?
|
@wking I've been on both sides of the scenarios you are hypothesizing. You either have to compile for both machines, update the old machines or compile for the lowest common denominator. The combinatoric explosion is unrealistic to handle in such a simple field. In reality, a full solution for a scenario where this crosses the cost-benefit threshold would be highly specialized to the hardware and deployment of that hardware. For applications where this is valuable, careful labeling would be much more fruitful and can handle the myriad possibility that is possible to encounter. After looking into this problem closely, my current recommendation is to completely remove or deprecate the field. It was my suggestion, long ago, in the creation of the manifest list, and I apologize to the community for ever conceiving of the idea. Rather than wasting valuable man-years in trying to specify this and deal with confusion, the much wiser approach here is to cut losses and remove it. In that regard, I move that we close this PR, in favor of removing the field. @opencontainers/image-spec-maintainers |
I have submitted to #672. |
Sounds good to me :). This is @AkihiroSuda's plan 2, which I'm happy with. |
Based on @AkihiroSuda's proposal and the subsequent discussion (#622).
Using the Linux kernel to define the values is very convenient on Linux, but will likely be a pain for folks on other OSes. If a comprehensive, OS-agnostic list is found, we could use that instead, but would want to provide a mapping from that list of values to the Linux cpuinfo slugs for convenience. Providing a mapping from the OS-agnostic list to the cpuinfo analog on non-Linux OSes would also be convenient. Until then, folks wondering what a given feature slug means but unwilling to dig through the Linux source may find this Stack Overflow post useful.
This definition is not only Linux-centric, it is x86-centric. The x86 Linux implementation writes
flags
out here, but arm usesFeatures
. While we could suggest values for other architectures, this commit restricts itself to the x86 family (using theirGOARCH
names, as the runtime spec recommends), because that's the scope @stevvooe is currently claiming. Extending the field to other architectures can happen in follow-up work.