Skip to content

Conversation

@hjelmn
Copy link
Member

@hjelmn hjelmn commented Jan 12, 2017

The linux timer code was multiplying the result of the x86 time stamp
counter by 1000000 before dividing by the cpu frequency. This can
cause us to overflow 64 bits if the time stamp counter grows larger
than ~ 1.8e13 (about 8400 seconds after boot). To fix the issue the
1000000 factor has been moved to avoid overflow.

Signed-off-by: Nathan Hjelm hjelmn@lanl.gov

@hjelmn hjelmn requested a review from bosilca January 12, 2017 00:20
@hjelmn hjelmn added this to the v1.10.6 milestone Jan 12, 2017
@hjelmn
Copy link
Member Author

hjelmn commented Jan 12, 2017

We may potentially loose some accuracy with the timer for cpus with non-integer clock speeds in MHz. Since this is unlikely we probably won't see any issues.

@hjelmn
Copy link
Member Author

hjelmn commented Jan 12, 2017

@hppritcha This is for 2.0.2 as well. Will open a PR tonight. If @bosilca can't get to it either you or @jsquyres can probably take a look. Pretty straightforward.

@bosilca
Copy link
Member

bosilca commented Jan 12, 2017

Good catch. Btw, why not changing the scale of opal_timer_linux_freq to completely avoid the overhead of the second integer division in the opal_timer_base_get_usec_sys_timer function ?

@hjelmn
Copy link
Member Author

hjelmn commented Jan 12, 2017

@bosilca Sure. Can do it that way. Was keeping the change simple but getting rid of the division is probably the better way to go. Will update the PR.

@hjelmn
Copy link
Member Author

hjelmn commented Jan 12, 2017

@bosilca Updated.

The linux timer code was multiplying the result of the x86 time stamp
counter by 1000000 before dividing by the cpu frequency. This can
cause us to overflow 64 bits if the time stamp counter grows larger
than ~ 1.8e13 (about 8400 seconds after boot). To fix the issue the
units of opal_timer_linux_freq have been changed to MHz.

Signed-off-by: Nathan Hjelm <hjelmn@lanl.gov>
@jsquyres jsquyres merged commit 938ab01 into open-mpi:master Jan 12, 2017
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.

3 participants