Replace mtime counter with realtime timer #49
Conversation
clint.c
Outdated
| uint64_t time_delta = | ||
| clint->mtimecmp[hart->mhartid] - semu_timer_gettime(&hart->time); | ||
| if (time_delta & 0x8000000000000000 || time_delta == 0) |
There was a problem hiding this comment.
Is it better to cast time_delta to a int64_t instead of unsigned integer? By doing this, the comparison in the if statement can be reduced and more intuitive.
There was a problem hiding this comment.
You are correct, casting time_delta can be more intutive. Below is the refined implementation
uint64_t time_delta =
clint->mtimecmp[hart->mhartid] - semu_timer_gettime(&hart->time);
if ((int32_t)time_delta <= 0)
...
|
I believe using a real-time counter is a better approach for simulating the timer device. However, I didn't notice any differences between the "Before" and "Now" sections in your text. Also, the RCU issue mentioned above is caused by too frequent timer interrupt triggers. Since RVVM is a multi-threaded RISC-V simulator, it's okay to directly simulate the timer as you implemented. However, semu is currently a single-threaded simulator, so SEMU becomes slower as the number of simulated harts grows. Therefore, the frequency at |
Sorry I need to take back what I said earlier. The real-time counter may not always be a better implementation. After measuring the boot-up time with SMP=4, the original version takes around 23 seconds, while this commit takes 39 seconds. I think this is because the real-time counter generates more interrupts then the original version. Does the real-time counter (RTC) provide any features that can't be neglected? |
Initially, my intention was to enable the actual clock for the RDTIME (0xC01/0xC81) pseudo-instruction, which originally retrieves the monotonic-incremented
I acknowledge the observation that SEMU slows down as the number of simulated harts increases. While reducing the frequency in
I believe implementing multi-threaded simulation for SEMU could provide a better solution to the RCU warning issue, is that make sense? |
|
Properly rebase the latest $ git remote add upstream https://github.com/sysprog21/semu
$ git fetch upstream
$ git rebase upstream/master
$ git push --force origin masterNext time, create a dedicated branch rather than |
Thank you for the instructions. I understand and will follow these steps and will also make sure to create a dedicated branch for future PR. |
There was a problem hiding this comment.
Improve the git commit message. You should explain the rationale.
See https://cbea.ms/git-commit/
clint.c
Outdated
| uint64_t time_delta = | ||
| clint->mtimecmp[hart->mhartid] - semu_timer_gettime(&hart->time); | ||
| if ((int32_t) time_delta <= 0) |
There was a problem hiding this comment.
I think using int32_t might trigger potential errors since this is a real-time counter. We can't predict when the clint_update_interrupts function will be executed. The time_delta could be a very small negative value, which would normally trigger the timer interrupt. However, if we cast it into a 32-bit integer, the value could incorrectly become positive. For example, if time_delta is 0xFFFFFFFF00000001, which is -4294967295, casting it to int32_t will result in the value becoming 1.
Therefore, I suggest using int64_t instead.
There was a problem hiding this comment.
Yes, you're absolutely right. When the larger number exceeds the small one by more than 0x100000000, it will incorrectly cast a negative number to a positive one. Thanks for pointing that out!
chiangkd
force-pushed
the
master
branch
2 times, most recently
from
be007bb to
82eb0b5
Compare
There was a problem hiding this comment.
Read https://cbea.ms/git-commit/ carefully and refine your git commit messages.
Wrap the body at 72 characters
Avoid using backticks in git commit messages and comments to ensure compatibility with terminal emulators that may not render the backtick character properly.
Instead, use the pair ' or " (quotation marks).
Have you tried |
Yes, Execute with Execute with |
How about |
Execute with Execute with Use
|
Based on the experiments, empirical evidence suggests utilizing |
Replace 'mtime' counter by retrieving time of the specified clock ID. Additionally, a Real-Time counter (RTC) mechanism is implemented to prevent 'mtime' overflow. By changing 'mtime' to use the retrieved timer, real timestamps are visible in the booting log due to 'CONFIG_PRINTK_TIME' being enable by default. However, SEMU experiences a slowdown as the number of simulated harts increases. This is due to the increased number of timer interrupts and 'clock_gettime' system calls in this commit. Another issue addressed is RCU CPU stall warning[1], which occurs when the system waits more than a grace period (typically 21 seconds). [1]: https://docs.kernel.org/RCU/stallwarn.html
|
Thank @chiangkd for contributing! |
Replace
mtimecounter by retrieving time of the specified clock ID. Also, Real time counter (RTC) mechanism is implemented to preventmtimeoverflow.Before:
Now:
Revised boot log to display actual timer during boot flow.
Reference to RVVM:https://github.com/LekKit/RVVM/blob/staging/src/rvtimer.c
However, another issue accopanies this PR
RCU issues a CPU stall wrning when it waits more than a grace period, which is normally 21 seconds.
Detail here: https://docs.kernel.org/RCU/stallwarn.html