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

unscaledcycleclock: remove RISC-V support #1644

Closed
wants to merge 1 commit into from

Conversation

aurel32
Copy link
Contributor

@aurel32 aurel32 commented Mar 19, 2024

Starting with Linux 6.6 [1], RDCYCLE is a privileged instruction on RISC-V and can't be used directly from userland. There is a sysctl option to change that as a transition period, but it will eventually disappear.

The RDTIME instruction is another less accurate alternative, however its frequency varies from board to board, and there is currently now way to get its frequency from userland [2].

Therefore this patch just removes the code for unscaledcycleclock on RISC-V. Without processor specific implementation, abseil relies on std::chrono::steady_clock::now().time_since_epoch() which is basically a wrapper around clock_gettime (CLOCK_MONOTONIC), which in turns use __vdso_clock_gettime(). On RISC-V this VDSO is just a wrapper around RDTIME correctly scaled to use nanoseconds units.

This fixes the testsuite on riscv64, tested on a VisionFive 2 board.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=cc4c07c89aada16229084eeb93895c95b7eabaa3
[2] #1631

Starting with Linux 6.6 [1], RDCYCLE is a privileged instruction on
RISC-V and can't be used directly from userland. There is a sysctl
option to change that as a transition period, but it will eventually
disappear.

The RDTIME instruction is another less accurate alternative, however its
frequency varies from board to board, and there is currently now way to
get its frequency from userland [2].

Therefore this patch just removes the code for unscaledcycleclock on
RISC-V. Without processor specific implementation, abseil relies on
std::chrono::steady_clock::now().time_since_epoch() which is basically a
wrapper around clock_gettime (CLOCK_MONOTONIC), which in turns use
__vdso_clock_gettime(). On RISC-V this VDSO is just a wrapper around
RDTIME correctly scaled to use nanoseconds units.

This fixes the testsuite on riscv64, tested on a VisionFive 2 board.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=cc4c07c89aada16229084eeb93895c95b7eabaa3
[2] abseil#1631
@derekmauro
Copy link
Member

We have internal clients that are using this code. It can't simply be removed.

In the other pull request I asked if you could build with -DABSL_USE_UNSCALED_CYCLECLOCK=0. Does that solve your problem?

@derekmauro
Copy link
Member

I'm also going to have to track down the callers of this and see what they want to do.

@aurel32
Copy link
Contributor Author

aurel32 commented Mar 20, 2024

We have internal clients that are using this code. It can't simply be removed.

Ok. Note that in addition to the 6.6 kernel issue, the use of rdcycle means it can't be use on multicore CPUs, unless pinning the process on a single core, as the controrary to rdtime, the rdcycle counters are not synchronised between cores and just "jumps" if the process is rescheduled on another core.

In the other pull request I asked if you could build with -DABSL_USE_UNSCALED_CYCLECLOCK=0. Does that solve your problem?

I guess you mean at the cmake level. It doesn't work and cmake outputs:

CMake Warning:
  Manually-specified variables were not used by the project:

    ABSL_USE_UNSCALED_CYCLECLOCK

@derekmauro
Copy link
Member

I've decided to accept this change and keep the RISC-V code as an internal-only patch. I've also filed an internal bug (b/330872512) for the team that is using it to figure out what they want to do with it. This should be merged soon.

Re: ABSL_USE_UNSCALED_CYCLECLOCK - I was talking about a preprocessor define. There is no corresponding CMake option.

@aurel32
Copy link
Contributor Author

aurel32 commented Mar 23, 2024

Thanks a lot @derekmauro

netkex pushed a commit to netkex/abseil-cpp that referenced this pull request Apr 3, 2024
Imported from GitHub PR abseil#1644

Starting with Linux 6.6 [1], RDCYCLE is a privileged instruction on RISC-V and can't be used directly from userland. There is a sysctl option to change that as a transition period, but it will eventually disappear.

The RDTIME instruction is another less accurate alternative, however its frequency varies from board to board, and there is currently now way to get its frequency from userland [2].

Therefore this patch just removes the code for unscaledcycleclock on RISC-V. Without processor specific implementation, abseil relies on std::chrono::steady_clock::now().time_since_epoch() which is basically a wrapper around clock_gettime (CLOCK_MONOTONIC), which in turns use __vdso_clock_gettime(). On RISC-V this VDSO is just a wrapper around RDTIME correctly scaled to use nanoseconds units.

This fixes the testsuite on riscv64, tested on a VisionFive 2 board.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=cc4c07c89aada16229084eeb93895c95b7eabaa3
[2] abseil#1631
Merge 43356a2 into 76f8011

Merging this change closes abseil#1644

COPYBARA_INTEGRATE_REVIEW=abseil#1644 from aurel32:rv64-no-unscaledcycleclock 43356a2
PiperOrigin-RevId: 618286262
Change-Id: Ie4120a727e7d0bb185df6e06ea145c780ebe6652
@jrtc27
Copy link

jrtc27 commented Sep 13, 2024

RDCYCLE works just fine on FreeBSD; if you want to avoid RDCYCLE, and don't want to use RDTIME, on Linux then at least please instead change the #ifdefs to include non-Linux rather than penalise every OS out there.

@jrtc27
Copy link

jrtc27 commented Sep 13, 2024

That or leave it in there and add defined(__riscv) && defined(__linux__) to ABSL_USE_UNSCALED_CYCLECLOCK_DEFAULT, which seems to exist entirely to support this kind of situation?

@aurel32
Copy link
Contributor Author

aurel32 commented Sep 13, 2024

RDCYCLE works just fine on FreeBSD; if you want to avoid RDCYCLE, and don't want to use RDTIME, on Linux then at least please instead change the #ifdefs to include non-Linux rather than penalise every OS out there.

Do you mean that RDCYCLE is accessible from userland on FreeBSD? Or do you mean it works on FreeBSD the way it is used in abseil-cpp?

The other reason not to use RDCYCLE is that it is not monotonic if the task is moved between cores, causing the testsuite to fail: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1025221

@jrtc27
Copy link

jrtc27 commented Sep 13, 2024

It is accessible from userland on FreeBSD. With the caveat of course that you be careful about core migration. For RDTIME there's an (ugly) sysctl available to get the frequency, too. Of course we do have a vDSO implementation of clock_gettime too that uses RDTIME under the hood so it's not awful to lack a faster implementation, I just don't like the idea of deleting code just because it doesn't work on only Linux.

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