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

Simplify ticks, as the value is a constant #12

Merged
merged 1 commit into from
Jun 27, 2017

Conversation

justincormack
Copy link
Contributor

See for example in the Musl libc source code https://git.musl-libc.org/cgit/musl/tree/src/conf/sysconf.c#n29

This removes another cgo dependency in containerd.

Signed-off-by: Justin Cormack justin.cormack@docker.com

@justincormack
Copy link
Contributor Author

Hmm, looks like a Prom API change has broken the CI...

@crosbymichael
Copy link
Member

Can you rebase this one so it picks up your fix?

@hqhq
Copy link
Contributor

hqhq commented Jun 27, 2017

Wouldn't this cause portable issue? If it's about removing cgo dependency, maybe we can use genconf command, getconf CLK_TCK can also return this value.

@crosbymichael
Copy link
Member

I've never need it not 100 but it is possible. @justincormack should we add the cgo build tag for this file and then have a hard coded 100 in a !cgo file?

See for example in the Musl libc source code https://git.musl-libc.org/cgit/musl/tree/src/conf/sysconf.c#n29

This removes another cgo dependency in `containerd`.

Signed-off-by: Justin Cormack <justin.cormack@docker.com>
@codecov-io
Copy link

Codecov Report

Merging #12 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #12   +/-   ##
=======================================
  Coverage   36.69%   36.69%           
=======================================
  Files          21       21           
  Lines        1398     1398           
=======================================
  Hits          513      513           
  Misses        786      786           
  Partials       99       99
Impacted Files Coverage Δ
ticks.go 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2c98220...ba3e9ce. Read the comment docs.

@justincormack
Copy link
Contributor Author

justincormack commented Jun 27, 2017

The value is a constant in Linux, there is no point in "dynamically" looking it up. This is cgroups code so only runs on Linux. To quote Linus:

https://groups.google.com/forum/#!msg/fa.linux.kernel/JndVy0RgHHI/Nu7nkRfZ-c0J

CLK_TCK is 100 on x86. As it has always been. User land should never
care about whatever random value the kernel happens to use for the
actual timer tick at that particular moment. Especially since the kernel
internal timer tick may well be variable some day.

The fact that libproc believes that HZ can change is their problem.
I've told people over and over that user-level HZ is a constant (and, on
x86, that constant is 100), and that won't change.

So in current 2.5.x times() still counts at 100Hz, and /proc files that
export clock_t still show the same 100Hz rate.

The fact that the kernel internally counts at some different rate should
be totally invisible to user programs (except they get better latency
for stuff like select() and other timeouts).

            Linus

(tick rate internally in the kernel can now be variable, so this API would be useless anyway). This is just an artefact of the ABI. It does not vary by architecture, as I said above it is hard coded in libc too.

@dqminh
Copy link
Member

dqminh commented Jun 27, 2017

Its still a changeable constant though ? With sysconf which reads the ELF aux value, process will always get the correct value no matter what. I think its saner to look up even if its probably always be 100 in practice.

@dqminh
Copy link
Member

dqminh commented Jun 27, 2017

Also for some arch like alpha or ia64, this is not 100 iirc. Though we probably not care about them now with this library.

@justincormack
Copy link
Contributor Author

justincormack commented Jun 27, 2017

No, it is a constant constant. It is the same on every architecture. The ELF aux value is only used on Glibc with dynamically linked programs, Musl libc always sets it to 100, and as you can see in
https://github.com/torvalds/linux/blob/master/fs/binfmt_elf.c#L246 it is set to CLOCKS_PER_SEC which is defines as USER_HZ which is 100 on all architectures except Alpha which is totally obsolete, and ia64 which is also obsolete, where it was configurable. It is set in https://github.com/torvalds/linux/blob/master/include/asm-generic/param.h which is never going to vary again, its really a constant.

@dqminh
Copy link
Member

dqminh commented Jun 27, 2017

LGTM

By "changeable constant" i meant someone can go in and change the value and recompile 😄 , then the program compiled on another kernel will have incorrect value. Not sure if we care though, i think this change is really ok in practice.

@crosbymichael
Copy link
Member

LGTM

@crosbymichael crosbymichael merged commit 177db07 into containerd:master Jun 27, 2017
tklauser added a commit to tklauser/go-sysconf that referenced this pull request Feb 1, 2021
CLK_TCK is always 100 on Linux. For example, musl hard-codes this
https://git.musl-libc.org/cgit/musl/tree/src/conf/sysconf.c#n30 and
glibc looks it up from the ELF auxv, but the kernel sets it to 100 on
all architectures (except alpha and ia64, both of which are obsolete and
not supported by Go). Also see
containerd/cgroups#12
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