Skip to content

Manage ticks to suppress RCU CPU stall warning #75

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

Merged
merged 1 commit into from
Feb 16, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,16 @@ DTC ?= dtc
E :=
S := $E $E

# During boot process, he emulator manually manages the growth of ticks to
# suppress RCU CPU stall warnings. Thus, we need an target time to set the
# increment of ticks. 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.
# [1] docs.kernel.org/RCU/stallwarn.html#config-rcu-cpu-stall-timeout
CFLAGS += -D SEMU_BOOT_TARGET_TIME=10

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider making boot time configurable

Consider making the boot target time configurable via environment variable instead of hardcoding it in the Makefile. This would allow users to override the value without modifying the Makefile. Could use something like: SEMU_BOOT_TARGET_TIME ?= 10

Code suggestion
Check the AI-generated fix before applying
Suggested change
CFLAGS += -D SEMU_BOOT_TARGET_TIME=10
SEMU_BOOT_TARGET_TIME ?= 10
CFLAGS += -D SEMU_BOOT_TARGET_TIME=$(SEMU_BOOT_TARGET_TIME)

Code Review Run #9f603b


Is this a valid issue, or was it incorrectly flagged by the Agent?

  • it was incorrectly flagged


SMP ?= 1
.PHONY: riscv-harts.dtsi
riscv-harts.dtsi:
Expand Down
2 changes: 1 addition & 1 deletion main.c
Original file line number Diff line number Diff line change
Expand Up @@ -678,7 +678,7 @@ static int semu_init(emu_state_t *emu, int argc, char **argv)
virtio_rng_init();
#endif
/* Set up ACLINT */
semu_timer_init(&emu->mtimer.mtime, CLOCK_FREQ);
semu_timer_init(&emu->mtimer.mtime, CLOCK_FREQ, hart_count);
emu->mtimer.mtimecmp = calloc(vm->n_hart, sizeof(uint64_t));
emu->mswi.msip = calloc(vm->n_hart, sizeof(uint32_t));
emu->sswi.ssip = calloc(vm->n_hart, sizeof(uint32_t));
Expand Down
8 changes: 8 additions & 0 deletions riscv.c
Original file line number Diff line number Diff line change
Expand Up @@ -382,6 +382,14 @@ static void op_sret(hart_t *vm)
vm->s_mode = vm->sstatus_spp;
vm->sstatus_sie = vm->sstatus_spie;

/* After the booting process is complete, initrd will be loaded. At this
* point, the sytstem will switch to U mode for the first time. Therefore,
* by checking whether the switch to U mode has already occurred, we can
* determine if the boot process has been completed.
*/
if (!boot_complete && !vm->s_mode)
boot_complete = true;

/* Reset stack */
vm->sstatus_spp = false;
vm->sstatus_spie = true;
Expand Down
99 changes: 81 additions & 18 deletions utils.c
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
#include <stdbool.h>
#include <time.h>

#include "utils.h"
Expand All @@ -19,6 +20,10 @@
#endif
#endif

bool boot_complete = false;
static double ticks_increment;
static double boot_ticks;
Comment on lines +24 to +25

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider initializing static timing variables

Consider initializing boot_ticks and ticks_increment with meaningful default values to avoid potential undefined behavior when these variables are first accessed.

Code suggestion
Check the AI-generated fix before applying
Suggested change
static double ticks_increment;
static double boot_ticks;
static double ticks_increment = 0.0;
static double boot_ticks = 0.0;

Code Review Run #9f603b


Is this a valid issue, or was it incorrectly flagged by the Agent?

  • it was incorrectly flagged

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider consistent integer type usage

The boot_ticks variable is declared as double but cast to uint64_t without checking for potential overflow or precision loss. Consider using uint64_t type consistently.

Code suggestion
Check the AI-generated fix before applying
Suggested change
static double boot_ticks;
static uint64_t boot_ticks;

Code Review Run #9f603b


Is this a valid issue, or was it incorrectly flagged by the Agent?

  • it was incorrectly flagged


/* Calculate "x * n / d" without unnecessary overflow or loss of precision.
*
* Reference:
Expand All @@ -32,35 +37,93 @@ static inline uint64_t mult_frac(uint64_t x, uint64_t n, uint64_t d)
return q * n + r * n / d;
}

void semu_timer_init(semu_timer_t *timer, uint64_t freq)
{
timer->freq = freq;
semu_timer_rebase(timer, 0);
}

static uint64_t semu_timer_clocksource(uint64_t freq)
/* High-precision time measurement:
* - POSIX systems: clock_gettime() for nanosecond precision
* - macOS: mach_absolute_time() with timebase conversion
* - Other platforms: time(0) with conversion to nanoseconds as fallback
*
* The platform-specific timing logic is now clearly separated: POSIX and macOS
* implementations provide high-precision measurements, while the fallback path
* uses time(0) for a coarser but portable approach.
*/
static inline uint64_t host_time_ns()
{
#if defined(HAVE_POSIX_TIMER)
struct timespec t;
clock_gettime(CLOCKID, &t);
return t.tv_sec * freq + mult_frac(t.tv_nsec, freq, 1e9);
struct timespec ts;
clock_gettime(CLOCKID, &ts);
return (uint64_t) ts.tv_sec * 1e9 + (uint64_t) ts.tv_nsec;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider using integer constant for nanoseconds

Consider using UINT64_C(1000000000) instead of 1e9 for explicit integer constant. Using floating point literals for integer calculations could lead to precision issues.

Code suggestion
Check the AI-generated fix before applying
Suggested change
return (uint64_t) ts.tv_sec * 1e9 + (uint64_t) ts.tv_nsec;
return (uint64_t) ts.tv_sec * UINT64_C(1000000000) + (uint64_t) ts.tv_nsec;

Code Review Run #9f603b


Is this a valid issue, or was it incorrectly flagged by the Agent?

  • it was incorrectly flagged


#elif defined(HAVE_MACH_TIMER)
static mach_timebase_info_data_t t;
if (t.denom == 0)
(void) mach_timebase_info(&t);
return mult_frac(mult_frac(mach_absolute_time(), t.numer, t.denom), freq,
1e9);
static mach_timebase_info_data_t ts = {0};
if (ts.denom == 0)
(void) mach_timebase_info(&ts);

uint64_t now = mach_absolute_time();
/* convert to nanoseconds: (now * t.numer / t.denom) */
return mult_frac(now, ts.numer, (uint64_t) ts.denom);

#else
return time(0) * freq;
/* Fallback to non-HRT calls time(0) in seconds => convert to ns. */
time_t now_sec = time(0);
return (uint64_t) now_sec * 1e9;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use consistent integer constant in fallback

Similar to above, consider using UINT64_C(1000000000) instead of 1e9 in the fallback path for consistency and to avoid potential floating point precision issues.

Code suggestion
Check the AI-generated fix before applying
Suggested change
return (uint64_t) now_sec * 1e9;
return (uint64_t) now_sec * UINT64_C(1000000000);

Code Review Run #9f603b


Is this a valid issue, or was it incorrectly flagged by the Agent?

  • it was incorrectly flagged

#endif
}

/* The function that returns the "emulator time" in ticks.
*
* Before the boot process is completed, the emulator manually manages the
* growth of ticks to suppress RCU CPU stall warnings. After the boot process is
* completed, the emulator switches back to the real-time timer, using an offset
* bridging to ensure that the ticks of both timers remain consistent.
*/
static uint64_t semu_timer_clocksource(semu_timer_t *timer)
{
/* After boot process complete, the timer will switch to real time. Thus,
* there is an offset between the real time and the emulator time.
*
* After switching to real time, the correct way to update time is to
* calculate the increment of time. Then add it to the emulator time.
*/
static int64_t offset = 0;
static bool first_switch = true;
Comment on lines +87 to +88

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider making static variables thread-safe

Consider making offset and first_switch variables thread-safe since they are static variables accessed from multiple threads. Using atomic variables would prevent potential race conditions.

Code suggestion
Check the AI-generated fix before applying
Suggested change
static int64_t offset = 0;
static bool first_switch = true;
static atomic_int64_t offset = ATOMIC_VAR_INIT(0);
static atomic_bool first_switch = ATOMIC_VAR_INIT(true);

Code Review Run #9f603b


Is this a valid issue, or was it incorrectly flagged by the Agent?

  • it was incorrectly flagged


if (!boot_complete) {
boot_ticks += ticks_increment;
return (uint64_t) boot_ticks;
}

uint64_t real_ticks = mult_frac(host_time_ns(), timer->freq, 1e9);
if (first_switch) {
first_switch = false;

/* Calculate the offset between the real time and the emulator time */
offset = (int64_t) (real_ticks - boot_ticks);
}
return (uint64_t) ((int64_t) real_ticks - offset);
}

void semu_timer_init(semu_timer_t *timer, uint64_t freq, int n_harts)
{
timer->freq = freq;
timer->begin = mult_frac(host_time_ns(), timer->freq, 1e9);
boot_ticks = timer->begin; /* Initialize the fake ticks for boot process */

/* According to statistics, the number of times 'semu_timer_clocksource'
* called is approximately 'SMP count * 2.15 * 1e8'. By the time the boot
* process is completed, the emulator will have a total of 'boot seconds *
* frequency' ticks. Therefore, each time, '(boot seconds * frequency) /
* (2.15 * 1e8 * SMP count)' ticks need to be added.
*/
ticks_increment =
(SEMU_BOOT_TARGET_TIME * CLOCK_FREQ) / (2.15 * 1e8 * n_harts);
}

uint64_t semu_timer_get(semu_timer_t *timer)
{
return semu_timer_clocksource(timer->freq) - timer->begin;
return semu_timer_clocksource(timer) - timer->begin;
}

void semu_timer_rebase(semu_timer_t *timer, uint64_t time)
{
timer->begin = semu_timer_clocksource(timer->freq) - time;
timer->begin = semu_timer_clocksource(timer) - time;
}
12 changes: 11 additions & 1 deletion utils.h
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,23 @@
#include <stddef.h>
#include <stdint.h>

/* To suppress RCU CPU stall warnings, the emulator provides a fake timer to
* the Guest OS during the boot process. After the boot process is complete, the
* emulator will switch to real-time timer.
*
* Since the Guest OS transitions to U mode for the first time when it loads the
* initial user-mode process, we use this transition to determine whether the
* boot process has completed.
*/
extern bool boot_complete;

/* TIMER */
typedef struct {
uint64_t begin;
uint64_t freq;
} semu_timer_t;

void semu_timer_init(semu_timer_t *timer, uint64_t freq);
void semu_timer_init(semu_timer_t *timer, uint64_t freq, int n_harts);
uint64_t semu_timer_get(semu_timer_t *timer);
void semu_timer_rebase(semu_timer_t *timer, uint64_t time);

Expand Down
Loading