Skip to content

Replace rdcycle instruction with rdtime#1550

Closed
marv wants to merge 1 commit into
abseil:masterfrom
marv:riscv64-replace-rdcycle-with-rdtime
Closed

Replace rdcycle instruction with rdtime#1550
marv wants to merge 1 commit into
abseil:masterfrom
marv:riscv64-replace-rdcycle-with-rdtime

Conversation

@marv
Copy link
Copy Markdown

@marv marv commented Oct 20, 2023

The rdcycle instruction causes SIGILL with RISCV_PMU_SBI=y in the kernel[1]

According to a PR for the highway project[2] which fixed the same problem there rdtime is the proper equivalent for the x86 rdtsc instruction on RISC-V

Using rdtime instead of rdcycle makes all tests pass on my VisionFive 2 board

[1] https://lists.infradead.org/pipermail/linux-riscv/2022-September/018862.html
[2] google/highway#961

@google-cla
Copy link
Copy Markdown

google-cla Bot commented Oct 20, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

The `rdcycle` instruction causes SIGILL with RISCV_PMU_SBI=y in the
kernel[1]

According to a PR for the highway project[2] which fixed the same
problem there `rdtime` is the proper equivalent for the x86 `rdtsc`
instruction on RISC-V

Using `rdtime` instead of `rdcycle` makes all tests pass on my
VisionFive 2 board

[1] https://lists.infradead.org/pipermail/linux-riscv/2022-September/018862.html
[2] google/highway#961

Signed-off-by: Marvin Schmidt <marv@exherbo.org>
@marv marv force-pushed the riscv64-replace-rdcycle-with-rdtime branch from ab3acbf to 771d1f2 Compare October 20, 2023 19:37
@derekmauro
Copy link
Copy Markdown
Member

@bbarenblat had proposed a similar (identical?) change internally a few months ago. I pointed out that it is a little bit more complicated because ABSL_INTERNAL_UNSCALED_CYCLECLOCK_FREQUENCY_IS_CPU_FREQUENCY would also need to change.

@aurel32
Copy link
Copy Markdown
Contributor

aurel32 commented Jan 19, 2024

Removing the ABSL_INTERNAL_UNSCALED_CYCLECLOCK_FREQUENCY_IS_CPU_FREQUENCY define for RISC-V seems easy to do. But then it seems we have to also update UnscaledCycleClock::Frequency. The frequency of the rdtime timer varies from board to board, and TTBOMK there is no easy way to get its frequency. In such condition, is it fine changing UnscaledCycleClock::Frequency to return 1.0, like it is done for the CPU when it is not possible to determine the frequency?

@aurel32
Copy link
Copy Markdown
Contributor

aurel32 commented Feb 29, 2024

Removing the ABSL_INTERNAL_UNSCALED_CYCLECLOCK_FREQUENCY_IS_CPU_FREQUENCY define for RISC-V seems easy to do. But then it seems we have to also update UnscaledCycleClock::Frequency. The frequency of the rdtime timer varies from board to board, and TTBOMK there is no easy way to get its frequency. In such condition, is it fine changing UnscaledCycleClock::Frequency to return 1.0, like it is done for the CPU when it is not possible to determine the frequency?

Given I got no answer I gave it a tried, and I have found it is not necessary to provide UnscaledCycleClock::Frequency. I have therefore opened a new PR: #1631

@derekmauro
Copy link
Copy Markdown
Member

This was addressed by #1644.

@derekmauro derekmauro closed this Apr 8, 2024
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