Scale frequency to suppress RCU CPU stall warning #67
Conversation
Mes0903
force-pushed
the
scale-frequency
branch
from
67b96a5 to
7c0fccd
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Mes0903
force-pushed
the
scale-frequency
branch
from
7c0fccd to
9395054
Compare
| #define SEMU_BOOT_TARGET_TIME 10 | ||
| #endif | ||
|
|
||
| bool boot_complete = false; |
There was a problem hiding this comment.
I suggest moving boot_complete variable into vm_t for a more conceptually accurate design.
There was a problem hiding this comment.
If we move boot_complete into vm_t, all existing functions for semu_timer_t would need an additional vm_t parameter. For example, semu_timer_get would change to:
semu_timer_get(vm_t *vm, semu_timer_t *timer)This change would indirectly require the areas that call this function to also pass in a vm_tparameter. For instance, since semu_timer_get is called within aclint_mtimer_update_interrupts, the API of aclint_mtimer_update_interrupts would also need to be updated to include vm_t.
As this pattern continues, the API changes would proliferate significantly. Perhaps we could introduce a static bool pointer pointing to boot_complete and assign its value during semu_timer_init. This way, we would only need to modify the parameters of semu_timer_init.
|
I noticed that the time during boot process was far from the target time on macOS. I think this may cause by the hypothesis of " |
Mes0903
force-pushed
the
scale-frequency
branch
2 times, most recently
from
bdf4a62 to
cb265ae
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Mes0903
force-pushed
the
scale-frequency
branch
from
cb265ae to
ee5506e
Compare
This comment was marked as outdated.
This comment was marked as outdated.
|
Based on the output, I think it clearly that the assumption of " |
Mes0903
force-pushed
the
scale-frequency
branch
from
4fae085 to
e814185
Compare
Mes0903
force-pushed
the
scale-frequency
branch
from
e814185 to
16e0fc7
Compare
There was a problem hiding this comment.
Refine the git commit messages.
Change from
According to the Linux kernel documentation:
https://docs.kernel.org/RCU/stallwarn.html#config-rcu-cpu-stall-timeout
to
According to Using RCU’s CPU Stall Detector [1]
...
[1] https://docs.kernel.org/RCU/stallwarn.html#config-rcu-cpu-stall-timeout
Append Close #51 at the end of git commit messages.
Since the emulator currently operates using sequential emulation, the execution time for the boot process is relatively long, which can result in the generation of RCU CPU stall warnings. To address this issue, there are several potential solutions: 1. Scale the frequency to slow down emulator time during the boot process, thereby eliminating RCU CPU stall warnings. 2. During the boot process, avoid using 'clock_gettime' to update ticks and instead manage the tick increment relationship manually. 3. Implement multi-threaded emulation to accelerate the emulator's execution speed. For the third point, while implementing multi-threaded emulation can significantly accelerate the emulator's execution speed, it cannot guarantee that this issue will not reappear as the number of cores increases in the future. Therefore, a better approach is to use methods 1 and 2 to allow the emulator to set an expected time for completing the boot process. The advantages and disadvantages of the scale method are as follows: Advantages: - Simple implementation - Effectively sets the expected boot process completion time - Results have strong interpretability - Emulator time can be easily mapped back to real time Disadvantages: - Slower execution speed The advantages and disadvantages of the increment ticks method are as follows: Advantages: - Faster execution speed - Effectively sets the expected boot process completion time Disadvantages: - More complex implementation - Some results are difficult to interpret - Emulator time is difficult to map back to real time Based on practical tests, the second method provides limited acceleration but introduces some significant drawbacks, such as difficulty in interpreting results and the complexity of managing the increment relationship. Therefore, this commit opts for the scale frequency method to address this issue. This commit divides time into emulator time and real time. During the boot process, the timer uses scale frequency to slow down the growth of emulator time, eliminating RCU CPU stall warnings. After the boot process is complete, the growth of emulator time aligns with real time. To configure the scale frequency parameter, three pieces of information are required: 1. The expected completion time of the boot process 2. A reference point for estimating the boot process completion time 3. The relationship between the reference point and the number of SMPs According to Using RCU’s CPU Stall Detector [1], the grace period for RCU CPU stalls is typically set to 21 seconds. By dividing this value by two as the expected completion time, we can provide a sufficient buffer to reduce the impact of errors and avoid RCU CPU stall warnings. Using 'gprof' for basic statistical analysis, it was found that 'semu_timer_clocksource' accounts for approximately 10% of the boot process execution time. Since the logic within 'semu_timer_clocksource' is relatively simple, its execution time can be assumed to be nearly equal to 'clock_gettime'. Furthermore, by adding a counter to 'semu_timer_clocksource', it was observed that each time the number of SMPs increases by 1, the execution count of 'semu_timer_clocksource' increases by approximately '2 * 10^8' With this information, we can estimate the boot process completion time as 'sec_per_call * SMPs * 2 * 10^8 * (100% / 10%)' seconds, and thereby calculate the scale frequency parameter. For instance, if the estimated time is 200 seconds and the target time is 10 seconds, the scaling factor would be '10 / 200'. Close sysprog21#51 [1] https://docs.kernel.org/RCU/stallwarn.html#config-rcu-cpu-stall-timeout
Mes0903
force-pushed
the
scale-frequency
branch
from
16e0fc7 to
1f9c6a8
Compare
|
Running on macOS + Apple M1, I got the following: |
Mes0903
force-pushed
the
scale-frequency
branch
from
fbf1f0c to
fb3bf51
Compare
Code Review Agent Run #6023d9Actionable Suggestions - 10
Additional Suggestions - 4
Review Details
AI Code Review powered by |
Changelist by BitoThis pull request implements the following key changes.
|
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
| BASEDIR="profile-2" | ||
| LOGDIR="logs-${TAG}" # Directory containing the log files | ||
| OUTFILE="$BASEDIR/results_summary-${TAG}.txt" |
There was a problem hiding this comment.
Consider making the base directory path configurable through an environment variable or command line argument instead of hardcoding BASEDIR="profile-2". This would make the script more flexible and reusable across different environments.
Code suggestion
Check the AI-generated fix before applying
- BASEDIR="profile-2"
- LOGDIR="logs-${TAG}"
- OUTFILE="$BASEDIR/results_summary-${TAG}.txt"
+ BASEDIR="${BASE_DIR:-profile-2}"
+ LOGDIR="logs-${TAG}"
+ OUTFILE="$BASEDIR/results_summary-${TAG}.txt"
Code Review Run #6023d9
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
| @@ -19,6 +20,14 @@ | |||
| #endif | |||
| #endif | |||
|
|
|||
| bool boot_complete = false; | |||
There was a problem hiding this comment.
Consider making boot_complete static since it appears to be an internal state variable that tracks boot completion status. Global variables should generally be avoided unless cross-module access is required.
Code suggestion
Check the AI-generated fix before applying
| bool boot_complete = false; | |
| static bool boot_complete = false; |
Code Review Run #6023d9
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
| #if defined(HAVE_POSIX_TIMER) | ||
| struct timespec ts; | ||
| clock_gettime(CLOCKID, &ts); | ||
| return (uint64_t) ts.tv_sec * 1e9 + (uint64_t) ts.tv_nsec; |
There was a problem hiding this comment.
The floating-point literal 1e9 used in nanosecond calculations might cause precision issues with integer operations. Consider using the integer constant 1000000000ULL instead.
Code suggestion
Check the AI-generated fix before applying
- return (uint64_t) ts.tv_sec * 1e9 + (uint64_t) ts.tv_nsec;
+ return (uint64_t) ts.tv_sec * 1000000000ULL + (uint64_t) ts.tv_nsec;
@@ -72 +72 @@
- return (uint64_t) now_sec * 1e9;
+ return (uint64_t) now_sec * 1000000000ULL;
Code Review Run #6023d9
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
| #else | ||
| /* Fallback to non-HRT calls time(0) in seconds => convert to ns. */ | ||
| time_t now_sec = time(0); | ||
| return (uint64_t) now_sec * 1e9; |
There was a problem hiding this comment.
Consider using time() with error checking since it can return -1 on failure. Additionally, the nanosecond conversion using floating point 1e9 could lead to precision issues with large timestamps.
Code suggestion
Check the AI-generated fix before applying
| return (uint64_t) now_sec * 1e9; | |
| if (now_sec == -1) { | |
| fprintf(stderr, "Error getting time\n"); | |
| return 0; | |
| } | |
| return (uint64_t) now_sec * 1000000000ULL; |
Code Review Run #6023d9
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
|
|
||
| exit(0); | ||
| } | ||
| return (uint64_t) ((int64_t) real_ticks - offset); |
There was a problem hiding this comment.
Consider removing the exit(0) call after switching to real-time timer. This abrupt program termination could prevent proper cleanup and resource deallocation. Instead, consider using a return value or status flag to indicate the switch completion.
Code suggestion
Check the AI-generated fix before applying
| exit(0); | |
| } | |
| return (uint64_t) ((int64_t) real_ticks - offset); | |
| } | |
| return (uint64_t) ((int64_t) real_ticks - offset); |
Code Review Run #6023d9
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
|
|
||
| timer->begin = mult_frac(host_time_ns(), timer->freq, 1e9); | ||
| boot_ticks = timer->begin; | ||
| ticks_increment = (SEMU_BOOT_TARGET_TIME * CLOCK_FREQ) / (2.0e8 * n_harts); |
There was a problem hiding this comment.
Consider defining the magic number 2.0e8 as a named constant to improve code maintainability and readability. The constant could be named something like SEMU_TIMER_SCALE_FACTOR to better document its purpose.
Code suggestion
Check the AI-generated fix before applying
-static double ticks_increment;
+#define SEMU_TIMER_SCALE_FACTOR 2.0e8
+static double ticks_increment;
@@ -151,1 +151,1 @@
- ticks_increment = (SEMU_BOOT_TARGET_TIME * CLOCK_FREQ) / (2.0e8 * n_harts);
+ ticks_increment = (SEMU_BOOT_TARGET_TIME * CLOCK_FREQ) / (SEMU_TIMER_SCALE_FACTOR * n_harts);
Code Review Run #6023d9
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
| #!/usr/bin/env bash | ||
|
|
||
| # Create a directory to store logs (optional) | ||
| mkdir -p logs | ||
|
|
||
| for N in $(seq 1 32); do | ||
| echo "=============================================" | ||
| echo "Starting experiment with SMP=$N" | ||
| echo "=============================================" | ||
|
|
||
| echo "Building and running 'make check SMP=$N'..." | ||
| make check SMP=$N 2>&1 | tee "logs/emulator_SMP_${N}.log" | ||
|
|
||
| echo "Done with SMP=$N. Logs saved:" | ||
| echo " - logs/emulator_SMP_${N}.log" | ||
| echo | ||
| done | ||
|
|
||
| echo "All experiments complete!" |
There was a problem hiding this comment.
Consider adding error handling for the make check command. If the command fails, it might be helpful to capture the exit status and handle failures appropriately. Could consider adding set -e at the beginning of the script to fail fast on any error.
Code suggestion
Check the AI-generated fix before applying
| #!/usr/bin/env bash | |
| # Create a directory to store logs (optional) | |
| mkdir -p logs | |
| for N in $(seq 1 32); do | |
| echo "=============================================" | |
| echo "Starting experiment with SMP=$N" | |
| echo "=============================================" | |
| echo "Building and running 'make check SMP=$N'..." | |
| make check SMP=$N 2>&1 | tee "logs/emulator_SMP_${N}.log" | |
| echo "Done with SMP=$N. Logs saved:" | |
| echo " - logs/emulator_SMP_${N}.log" | |
| echo | |
| done | |
| echo "All experiments complete!" | |
| #!/usr/bin/env bash | |
| set -e | |
| # Create a directory to store logs (optional) | |
| mkdir -p logs | |
| failed_runs=() | |
| for N in $(seq 1 32); do | |
| echo "=============================================" | |
| echo "Starting experiment with SMP=$N" | |
| echo "=============================================" | |
| echo "Building and running 'make check SMP=$N'..." | |
| if ! make check SMP=$N 2>&1 | tee "logs/emulator_SMP_${N}.log"; then | |
| failed_runs+=("$N") | |
| fi | |
| echo "Done with SMP=$N. Logs saved:" | |
| echo " - logs/emulator_SMP_${N}.log" | |
| echo | |
| done | |
| if [ ${#failed_runs[@]} -eq 0 ]; then | |
| echo "All experiments completed successfully!" | |
| else | |
| echo "Experiments failed for SMP values: ${failed_runs[*]}" | |
| exit 1 | |
| fi |
Code Review Run #6023d9
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
|
The current issue lies in the fact that the bogoMIPS measurement may have a significant discrepancy compared to the actual value. I suspect that this discrepancy arises because the instruction cache (i-cache) is in a warm state during the bogoMIPS measurement, whereas in real scenarios, this is not always the case. However, I have yet to come up with an effective method to measure and verify this hypothesis. Regardless, if we continue to use the scale frequency method, the complexity of the program will undoubtedly increase. We would need to refine the measurement approach or implement additional corrections during the boot process to better align the values with real-world conditions. Fundamentally, this complexity arises because the scale frequency method requires three key pieces of information to be implemented:
Among these, the third piece of information is highly consistent across different environments, approximately equal to For example, in an M2 Pro Max environment with SMP set to 32, the relationship between the values is illustrated in the figure below:
The red dashed line represents the value measured by bogoMIPS, while the blue line represents the actual value. The significant gap between the two results in the persistent occurrence of RCU CPU stall warnings. I have re-evaluated the increment method. Since it only requires the number of
As expected, this method effectively suppresses RCU CPU stall warnings across various environments. Initially, we chose the scale frequency method because:
However, after a series of modifications, these advantages no longer hold. Additionally, the implementation of the scale frequency method has become more complex than the increment method. Furthermore, the increment method eliminates the need to call Therefore, I think we should switch to the increment method to resolve the RCU CPU stall warning issue. |
Since the emulator currently operates using sequential emulation, the execution time for the boot process is relatively long, which can result in the generation of RCU CPU stall warnings.
To address this issue, there are several potential solutions:
clock_gettimeto update ticks and instead manage the tick increment relationship manually.For the third point, while implementing multi-threaded emulation can significantly accelerate the emulator's execution speed, it cannot guarantee that this issue will not reappear as the number of cores increases in the future. Therefore, a better approach is to use methods 1 and 2 to allow the emulator to set an expected time for completing the boot process.
The advantages and disadvantages of the scale method are as follows:
Advantages:
Disadvantages:
The advantages and disadvantages of the increment ticks method are as follows:
Advantages:
Disadvantages:
Based on practical tests, the second method provides limited acceleration but introduces some significant drawbacks, such as difficulty in interpreting results and the complexity of managing the increment relationship. Therefore, this commit opts for the scale frequency method to address this issue.
This commit divides time into emulator time and real time. During the boot process, the timer uses scale frequency to slow down the growth of emulator time, eliminating RCU CPU stall warnings. After the boot process is complete, the growth of emulator time aligns with real time.
To configure the scale frequency parameter, three pieces of information are required:
According to the Linux kernel documentation:
https://docs.kernel.org/RCU/stallwarn.html#config-rcu-cpu-stall-timeout
The grace period for RCU CPU stalls is typically set to 21 seconds. By dividing this value by two as the expected completion time, we can provide a sufficient buffer to reduce the impact of errors and avoid RCU CPU stall warnings.
Using gprof for basic statistical analysis, it was found that
semu_timer_clocksourceaccounts for approximately 10% of the boot process execution time. Since the logic withinsemu_timer_clocksourceis relatively simple, its execution time can be assumed to be nearly equal toclock_gettime.Furthermore, by adding a counter to$2 \times 10^8$ (see the table below).
semu_timer_clocksource, it was observed that each time the number of SMPs increases by 1, the execution count ofsemu_timer_clocksourceincreases by approximatelyWith this information, we can estimate the boot process completion time as
seconds, and thereby calculate the scale frequency parameter. For instance, if the estimated time is 200 seconds and the target time is 10 seconds, the scaling factor would be
10 / 200.To calculate the proportion of
semu_timer_clocksourceusing gprof, simply add the-pgoption during compilation. Then, terminate the emulator's operation immediately after the first switch to U mode (when the boot process is complete). This approach allows for a rough estimation of the proportion occupied bysemu_timer_clocksource.Below is the gprof testing output:
CPU: 13th Gen Intel(R) Core(TM) i7-13700
CPU: Intel(R) Core(TM) i7-1065G7 CPU @ 1.30GHz
And by adding a counter to
semu_timer_clocksource, it becomes possible to calculate the relationship between SMPs and the number of timessemu_timer_clocksourceis called.Below is the testing output(13th Gen Intel(R) Core(TM) i7-13700):
semu_timer_total_ticksSummary by Bito
Implementation of frequency scaling solution to address RCU CPU stall warnings during boot process. The solution divides time handling between emulator and real time, using scaled frequency during boot and switching to real-time after completion. Targets 10-second boot time with comprehensive testing infrastructure across SMP configurations.Unit tests added: False
Estimated effort to review (1-5, lower is better): 3