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

Enable icount mode for failure-prone platforms #22904

Merged
merged 14 commits into from
May 14, 2020
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
9 changes: 9 additions & 0 deletions boards/Kconfig
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,15 @@ endchoice
rsource "shields/*/Kconfig.shield"

menu "Board Options"
config QEMU_ICOUNT
wentongwu marked this conversation as resolved.
Show resolved Hide resolved
bool "QEMU icount mode"
depends on QEMU_TARGET
default y if !NETWORKING
help
Enable QEMU virtual instruction counter. The virtual cpu will
execute one instruction every 2^N ns of virtual time. This will
give deterministic execution times from the guest point of view.

# There might not be any board options, hence the optional source
osource "$(BOARD_DIR)/Kconfig"
endmenu
4 changes: 3 additions & 1 deletion boards/arm/mps2_an385/board.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,9 @@ set(QEMU_FLAGS_${ARCH}
-machine mps2-an385
-nographic
-vga none
-icount shift=7,align=off,sleep=off -rtc clock=vm
)

if(CONFIG_QEMU_ICOUNT)
list(APPEND QEMU_EXTRA_FLAGS -icount shift=7,align=off,sleep=off -rtc clock=vm)
endif()
board_set_debugger_ifnset(qemu)
4 changes: 3 additions & 1 deletion boards/arm/mps2_an521/board.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,9 @@ set(QEMU_FLAGS_${ARCH}
-nographic
-m 16
-vga none
-icount shift=7,align=off,sleep=off -rtc clock=vm
)

if(CONFIG_QEMU_ICOUNT)
list(APPEND QEMU_EXTRA_FLAGS -icount shift=7,align=off,sleep=off -rtc clock=vm)
endif()
board_set_debugger_ifnset(qemu)
3 changes: 3 additions & 0 deletions boards/arm/qemu_cortex_a53/board.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -11,4 +11,7 @@ set(QEMU_FLAGS_${ARCH}
-machine virt
)

if(CONFIG_QEMU_ICOUNT)
list(APPEND QEMU_EXTRA_FLAGS -icount shift=4,align=off,sleep=off -rtc clock=vm)
endif()
board_set_debugger_ifnset(qemu)
3 changes: 3 additions & 0 deletions boards/arm/qemu_cortex_m0/board.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -13,4 +13,7 @@ set(QEMU_FLAGS_${ARCH}
-vga none
)

if(CONFIG_QEMU_ICOUNT)
list(APPEND QEMU_EXTRA_FLAGS -icount shift=6,align=off,sleep=off -rtc clock=vm)
endif()
board_set_debugger_ifnset(qemu)
3 changes: 3 additions & 0 deletions boards/arm/qemu_cortex_m3/board.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -10,4 +10,7 @@ set(QEMU_FLAGS_${ARCH}
-vga none
)

if(CONFIG_QEMU_ICOUNT)
list(APPEND QEMU_EXTRA_FLAGS -icount shift=6,align=off,sleep=off -rtc clock=vm)
endif()
board_set_debugger_ifnset(qemu)
1 change: 1 addition & 0 deletions boards/arm/qemu_cortex_r5/Kconfig.board
Original file line number Diff line number Diff line change
Expand Up @@ -4,3 +4,4 @@
config BOARD_QEMU_CORTEX_R5
bool "Cortex-R5 Emulation (QEMU)"
depends on SOC_XILINX_ZYNQMP_RPU
select QEMU_TARGET
5 changes: 4 additions & 1 deletion boards/arm/qemu_cortex_r5/board.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,13 @@ set(QEMU_CPU_TYPE_${ARCH} cortex-r5)
set(QEMU_FLAGS_${ARCH}
-nographic
-machine arm-generic-fdt
-icount shift=3,align=off,sleep=off -rtc clock=vm
-dtb ${ZEPHYR_BASE}/boards/${ARCH}/${BOARD}/fdt-single_arch-zcu102-arm.dtb
)

if(CONFIG_QEMU_ICOUNT)
list(APPEND QEMU_EXTRA_FLAGS -icount shift=3,align=off,sleep=off -rtc clock=vm)
endif()

set(QEMU_KERNEL_OPTION
"-device;loader,file=\$<TARGET_FILE:\${logical_target_for_zephyr_elf}>,cpu-num=4"
"-device;loader,addr=0xff5e023c,data=0x80008fde,data-len=4"
Expand Down
4 changes: 4 additions & 0 deletions boards/riscv/hifive1/board.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,10 @@ set(QEMU_FLAGS_${ARCH}
-machine sifive_e
)

if(CONFIG_QEMU_ICOUNT)
list(APPEND QEMU_EXTRA_FLAGS -icount shift=6,align=off,sleep=off -rtc clock=vm)
endif()

board_set_debugger_ifnset(qemu)
board_set_flasher_ifnset(hifive1)
board_finalize_runner_args(hifive1)
3 changes: 3 additions & 0 deletions boards/riscv/qemu_riscv32/board.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -10,4 +10,7 @@ set(QEMU_FLAGS_${ARCH}
-machine sifive_e
)

if(CONFIG_QEMU_ICOUNT)
list(APPEND QEMU_EXTRA_FLAGS -icount shift=6,align=off,sleep=off -rtc clock=vm)
endif()
board_set_debugger_ifnset(qemu)
3 changes: 3 additions & 0 deletions boards/riscv/qemu_riscv64/board.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -10,4 +10,7 @@ set(QEMU_FLAGS_${ARCH}
-machine sifive_e
)

if(CONFIG_QEMU_ICOUNT)
list(APPEND QEMU_EXTRA_FLAGS -icount shift=6,align=off,sleep=off -rtc clock=vm)
endif()
board_set_debugger_ifnset(qemu)
3 changes: 3 additions & 0 deletions boards/x86/qemu_x86/board.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,9 @@ if(CONFIG_X86_64)
set(QEMU_CPU_TYPE_${ARCH} qemu64,+x2apic)
else()
set(QEMU_CPU_TYPE_${ARCH} qemu32,+nx,+pae)
if(CONFIG_QEMU_ICOUNT)
list(APPEND QEMU_EXTRA_FLAGS -icount shift=5,align=off,sleep=off -rtc clock=vm)
endif()
endif()

set(QEMU_FLAGS_${ARCH}
Expand Down
3 changes: 3 additions & 0 deletions boards/xtensa/qemu_xtensa/board.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@ set(QEMU_FLAGS_${ARCH}
-machine sim -semihosting -nographic -cpu sample_controller
)

if(CONFIG_QEMU_ICOUNT)
list(APPEND QEMU_EXTRA_FLAGS -icount shift=6,align=off,sleep=off -rtc clock=vm)
endif()
# TODO: Support debug
# board_set_debugger_ifnset(qemu)
# debugserver: QEMU_EXTRA_FLAGS += -s -S
Expand Down
16 changes: 0 additions & 16 deletions drivers/timer/Kconfig
Original file line number Diff line number Diff line change
Expand Up @@ -305,20 +305,4 @@ config TICKLESS_CAPABLE
z_clock_announce() (really, not to produce an interrupt at
all) until the specified expiration.

config QEMU_TICKLESS_WORKAROUND
bool "Disable tickless on qemu due to asynchrony bug"
depends on QEMU_TARGET && TICKLESS_KERNEL
help
Qemu (without -icount) has trouble keeping time when the
host process needs to timeshare. The host OS will routinely
schedule out a process at timescales equivalent to the guest
tick rate. With traditional ticks delivered regularly by
the hardware, that's mostly OK as it looks like a late
interrupt. But in tickless mode, the driver needs some CPU
in order to schedule the tick in the first place. If that
gets delayed across a tick boundary, time gets wonky. This
tunable is a hint to the driver to disable tickless
accounting on qemu. Use it only on tests that are known to
have problems.

endmenu
5 changes: 2 additions & 3 deletions drivers/timer/arm_arch_timer.c
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,7 @@ static void arm_arch_timer_compare_isr(void *arg)

last_cycle += delta_ticks * CYC_PER_TICK;

if (!IS_ENABLED(CONFIG_TICKLESS_KERNEL) ||
IS_ENABLED(CONFIG_QEMU_TICKLESS_WORKAROUND)) {
if (!IS_ENABLED(CONFIG_TICKLESS_KERNEL)) {
u64_t next_cycle = last_cycle + CYC_PER_TICK;

if ((s64_t)(next_cycle - curr_cycle) < MIN_DELAY) {
Expand Down Expand Up @@ -62,7 +61,7 @@ void z_clock_set_timeout(s32_t ticks, bool idle)
{
ARG_UNUSED(idle);

#if defined(CONFIG_TICKLESS_KERNEL) && !defined(CONFIG_QEMU_TICKLESS_WORKAROUND)
#if defined(CONFIG_TICKLESS_KERNEL)

if (idle) {
return;
Expand Down
5 changes: 2 additions & 3 deletions drivers/timer/hpet.c
Original file line number Diff line number Diff line change
Expand Up @@ -59,8 +59,7 @@ static void hpet_isr(void *arg)

last_count += dticks * cyc_per_tick;

if (!IS_ENABLED(CONFIG_TICKLESS_KERNEL) ||
IS_ENABLED(CONFIG_QEMU_TICKLESS_WORKAROUND)) {
if (!IS_ENABLED(CONFIG_TICKLESS_KERNEL)) {
u32_t next = last_count + cyc_per_tick;

if ((s32_t)(next - now) < MIN_DELAY) {
Expand Down Expand Up @@ -128,7 +127,7 @@ void z_clock_set_timeout(s32_t ticks, bool idle)
{
ARG_UNUSED(idle);

#if defined(CONFIG_TICKLESS_KERNEL) && !defined(CONFIG_QEMU_TICKLESS_WORKAROUND)
#if defined(CONFIG_TICKLESS_KERNEL)
if (ticks == K_TICKS_FOREVER && idle) {
GENERAL_CONF_REG &= ~GCONF_ENABLE;
return;
Expand Down
5 changes: 2 additions & 3 deletions drivers/timer/riscv_machine_timer.c
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,7 @@
#define MAX_TICKS ((MAX_CYC - CYC_PER_TICK) / CYC_PER_TICK)
#define MIN_DELAY 1000

#define TICKLESS (IS_ENABLED(CONFIG_TICKLESS_KERNEL) && \
!IS_ENABLED(CONFIG_QEMU_TICKLESS_WORKAROUND))
#define TICKLESS IS_ENABLED(CONFIG_TICKLESS_KERNEL)

static struct k_spinlock lock;
static u64_t last_count;
Expand Down Expand Up @@ -93,7 +92,7 @@ void z_clock_set_timeout(s32_t ticks, bool idle)
{
ARG_UNUSED(idle);

#if defined(CONFIG_TICKLESS_KERNEL) && !defined(CONFIG_QEMU_TICKLESS_WORKAROUND)
#if defined(CONFIG_TICKLESS_KERNEL)
/* RISCV has no idle handler yet, so if we try to spin on the
* logic below to reset the comparator, we'll always bump it
* forward to the "next tick" due to MIN_DELAY handling and
Expand Down
5 changes: 2 additions & 3 deletions drivers/timer/xtensa_sys_timer.c
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,7 @@ static void ccompare_isr(void *arg)

last_count += dticks * CYC_PER_TICK;

if (!IS_ENABLED(CONFIG_TICKLESS_KERNEL) ||
IS_ENABLED(CONFIG_QEMU_TICKLESS_WORKAROUND)) {
if (!IS_ENABLED(CONFIG_TICKLESS_KERNEL)) {
u32_t next = last_count + CYC_PER_TICK;

if ((s32_t)(next - curr) < MIN_DELAY) {
Expand All @@ -70,7 +69,7 @@ void z_clock_set_timeout(s32_t ticks, bool idle)
{
ARG_UNUSED(idle);

#if defined(CONFIG_TICKLESS_KERNEL) && !defined(CONFIG_QEMU_TICKLESS_WORKAROUND)
#if defined(CONFIG_TICKLESS_KERNEL)
ticks = ticks == K_TICKS_FOREVER ? MAX_TICKS : ticks;
ticks = MAX(MIN(ticks - 1, (s32_t)MAX_TICKS), 0);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,5 @@ CONFIG_THREAD_MONITOR=y
CONFIG_INIT_STACKS=y
CONFIG_POLL=y
CONFIG_SCHED_SCALABLE=y
CONFIG_QEMU_TICKLESS_WORKAROUND=y
# The Zephyr CMSIS v2 emulation assumes that ticks are ms, currently
CONFIG_SYS_CLOCK_TICKS_PER_SEC=1000
64 changes: 58 additions & 6 deletions scripts/sanity_chk/sanitylib.py
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,11 @@
except ImportError:
print("Install tabulate python module with pip to use --device-testing option.")

try:
import psutil
except ImportError:
print("Install psutil python module with pip to use --qemu-testing option.")

ZEPHYR_BASE = os.getenv("ZEPHYR_BASE")
if not ZEPHYR_BASE:
sys.exit("$ZEPHYR_BASE environment variable undefined")
Expand Down Expand Up @@ -708,6 +713,18 @@ def __init__(self, instance, type_str):

self.pid_fn = os.path.join(instance.build_dir, "qemu.pid")

@staticmethod
wentongwu marked this conversation as resolved.
Show resolved Hide resolved
def _get_cpu_time(pid):
"""get process CPU time.

The guest virtual time in QEMU icount mode isn't host time and
it's maintained by counting guest instructions, so we use QEMU
process exection time to mostly simulate the time of guest OS.
"""
proc = psutil.Process(pid)
cpu_time = proc.cpu_times()
return cpu_time.user + cpu_time.system

@staticmethod
def _thread(handler, timeout, outdir, logfile, fifo_fn, pid_fn, results, harness):
fifo_in = fifo_fn + ".in"
Expand Down Expand Up @@ -737,13 +754,30 @@ def _thread(handler, timeout, outdir, logfile, fifo_fn, pid_fn, results, harness

line = ""
timeout_extended = False

pid = 0
if os.path.exists(pid_fn):
pid = int(open(pid_fn).read())

while True:
this_timeout = int((timeout_time - time.time()) * 1000)
if this_timeout < 0 or not p.poll(this_timeout):
if pid and this_timeout > 0:
wentongwu marked this conversation as resolved.
Show resolved Hide resolved
#there is possibility we polled nothing because
#of host not scheduled QEMU process enough CPU
#time during p.poll(this_timeout)
cpu_time = QEMUHandler._get_cpu_time(pid)
if cpu_time < timeout and not out_state:
timeout_time = time.time() + (timeout - cpu_time)
continue

if not out_state:
out_state = "timeout"
break

if pid == 0 and os.path.exists(pid_fn):
pid = int(open(pid_fn).read())

try:
c = in_fp.read(1).decode("utf-8")
except UnicodeDecodeError:
Expand Down Expand Up @@ -800,10 +834,7 @@ def _thread(handler, timeout, outdir, logfile, fifo_fn, pid_fn, results, harness
log_out_fp.close()
out_fp.close()
in_fp.close()
if os.path.exists(pid_fn):
pid = int(open(pid_fn).read())
os.unlink(pid_fn)

if pid:
try:
if pid:
os.kill(pid, signal.SIGTERM)
Expand Down Expand Up @@ -848,8 +879,29 @@ def handle(self):

with subprocess.Popen(command, stdout=subprocess.PIPE, stderr=subprocess.PIPE, cwd=self.build_dir) as proc:
logger.debug("Spawning QEMUHandler Thread for %s" % self.name)
proc.wait()
self.returncode = proc.returncode
try:
proc.wait(self.timeout)
except subprocess.TimeoutExpired:
#sometimes QEMU can't handle SIGTERM signal correctly
#in that case kill -9 QEMU process directly and leave
#sanitycheck judge testing result by console output
if os.path.exists(self.pid_fn):
qemu_pid = int(open(self.pid_fn).read())
try:
os.kill(qemu_pid, signal.SIGKILL)
except ProcessLookupError:
pass
proc.wait()
self.returncode = 0
else:
proc.terminate()
proc.kill()
self.returncode = proc.returncode
else:
self.returncode = proc.returncode

if os.path.exists(self.pid_fn):
os.unlink(self.pid_fn)

if self.returncode != 0:
self.set_state("failed", 0)
Expand Down
1 change: 0 additions & 1 deletion tests/kernel/common/prj.conf
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ CONFIG_ZTEST=y
CONFIG_PRINTK=y
CONFIG_LOG=y
CONFIG_POLL=y
CONFIG_QEMU_TICKLESS_WORKAROUND=y
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this remove this workaround for all tests that use it? If not, it should, and we should remove the whole feature. It only exists because of timer nondeterminism.

If not, and some things still seem to need it, it implies we have some more work to do somewhere.

Copy link
Contributor Author

@wentongwu wentongwu Feb 19, 2020

Choose a reason for hiding this comment

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

@andyross In this PR, I remove CONFIG_QEMU_TICKLESS_WORKAROUND in all tests cases, but just three Qemu platforms enabled icount so far, so maybe other Qemu platforms still need it.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, then if it needs to stay we probably want to force it off at the kconfig level somehow so tests can say "give me the qemu tick workaround if the platform needs it" but the platform layer somewhere can say "this configuration is running under qemu with -icount, don't hack the timer driver".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will change the patch, thanks

CONFIG_BOOT_DELAY=500
CONFIG_IRQ_OFFLOAD=y
CONFIG_TEST_USERSPACE=y
Expand Down
1 change: 0 additions & 1 deletion tests/kernel/sched/schedule_api/prj.conf
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ CONFIG_ZTEST=y
CONFIG_IRQ_OFFLOAD=y
CONFIG_NUM_PREEMPT_PRIORITIES=30
#CONFIG_SCHED_SCALABLE=y
CONFIG_QEMU_TICKLESS_WORKAROUND=y
CONFIG_MAX_THREAD_BYTES=4
CONFIG_TEST_USERSPACE=y
CONFIG_MP_NUM_CPUS=1
1 change: 0 additions & 1 deletion tests/kernel/sched/schedule_api/prj_multiq.conf
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,5 @@ CONFIG_ZTEST=y
CONFIG_IRQ_OFFLOAD=y
CONFIG_TEST_USERSPACE=y
CONFIG_SCHED_MULTIQ=y
CONFIG_QEMU_TICKLESS_WORKAROUND=y
CONFIG_MAX_THREAD_BYTES=4
CONFIG_MP_NUM_CPUS=1
3 changes: 1 addition & 2 deletions tests/kernel/sleep/prj.conf
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
CONFIG_IRQ_OFFLOAD=y
CONFIG_ZTEST=y
CONFIG_QEMU_TICKLESS_WORKAROUND=y
CONFIG_TEST_USERSPACE=y
CONFIG_MP_NUM_CPUS=1
CONFIG_MP_NUM_CPUS=1
1 change: 0 additions & 1 deletion tests/kernel/timer/timer_api/prj.conf
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
CONFIG_ZTEST=y
CONFIG_QEMU_TICKLESS_WORKAROUND=y
CONFIG_TEST_USERSPACE=y

CONFIG_MP_NUM_CPUS=1
Expand Down
4 changes: 2 additions & 2 deletions tests/kernel/workq/critical/src/main.c
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,8 @@
#include <linker/sections.h>
#include <ztest.h>

#define NUM_MILLISECONDS 5000
#define TEST_TIMEOUT 20000
#define NUM_MILLISECONDS 50
#define TEST_TIMEOUT 200

#ifdef CONFIG_COVERAGE
#define OFFLOAD_WORKQUEUE_STACK_SIZE 4096
Expand Down
1 change: 0 additions & 1 deletion tests/kernel/workq/work_queue/prj.conf
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
CONFIG_ZTEST=y
CONFIG_QEMU_TICKLESS_WORKAROUND=y
CONFIG_POLL=y

# Not a single test case here is SMP-safe. Save the cycles needed for
Expand Down
2 changes: 1 addition & 1 deletion tests/lib/heap/testcase.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -6,4 +6,4 @@
tests:
lib.heap:
tags: heap
platform_exclude: m2gl025_miv
platform_exclude: m2gl025_miv qemu_riscv32
Loading