Skip to content

Fix yield test and remove few compile warnings #14

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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Fix yield test and remove few compile warnings #14

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8922abc
Remove warnings about unused function arguments
robots Mar 19, 2024
0c37f4c
Fix yield test not cleaning threads
robots Mar 19, 2024
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
2 changes: 2 additions & 0 deletions src/common/bench_interrupt_latency_test.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ __weak void bench_exit_timer_isr(void)
*/
Copy link
Collaborator

Choose a reason for hiding this comment

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

For the commit message: you give a more detailed description of the issue on the PR message - could you please add that info to the commit message? It's better to have more information in it.

static void irq_latency_isr(void *arg)
{
ARG_UNUSED(arg);
bench_isr_cycles = bench_timer_cycles_get();

diff_cycles = bench_timer_cycles_diff(bench_trigger_cycles,
Expand Down Expand Up @@ -126,6 +127,7 @@ static void bench_thread_low(void *args)
*/
void bench_interrupt_latency_test(void *arg)
{
ARG_UNUSED(arg);
uint32_t i;

bench_stats_reset(&latency_times);
Expand Down
2 changes: 1 addition & 1 deletion src/common/bench_malloc_free_test.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,4 +82,4 @@ int main(void)

return 0;
}
#endif
#endif
1 change: 1 addition & 0 deletions src/common/bench_message_queue_test.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,7 @@ static void gather_receive_context_switch_stats(int priority, int iteration)
*/
void bench_message_queue_init(void *arg)
{
ARG_UNUSED(arg);
#if RTOS_HAS_MESSAGE_QUEUE
int i;

Expand Down
2 changes: 2 additions & 0 deletions src/common/bench_mutex_lock_unlock_test.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,6 +255,7 @@ static void bench_pend_low(void *args)
*/
static void bench_pend_high(void *args)
{
ARG_UNUSED(args);
/* Step 4 */

helper_start = bench_timing_counter_get();
Expand Down Expand Up @@ -373,6 +374,7 @@ static void gather_pend_inheritance_stats(int priority, uint32_t iteration)
*/
void bench_mutex_lock_unlock_test(void *arg)
{
ARG_UNUSED(arg);
uint32_t i;

bench_mutex_create(MUTEX_ID);
Expand Down
1 change: 1 addition & 0 deletions src/common/bench_sem_context_switch_test.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ void bench_sem_context_switch_low_prio_give(int priority, int iteration)
*/
void bench_sem_context_switch_init(void *arg)
{
ARG_UNUSED(arg);
int i;

bench_timing_init();
Expand Down
1 change: 1 addition & 0 deletions src/common/bench_sem_signal_release_test.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ void bench_sem_signal_release()
*/
void bench_sem_signal_release_init(void *param)
{
ARG_UNUSED(param);
bench_timing_init();

bench_sem_create(0, 0, ITERATIONS);
Expand Down
3 changes: 3 additions & 0 deletions src/common/bench_thread_switch_yield_test.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,7 @@ static void gather_set1_stats(int priority, uint32_t iteration)
*/
void bench_thread_yield(void *arg)
{
ARG_UNUSED(arg);
uint32_t i;

bench_timing_init();
Expand All @@ -164,6 +165,7 @@ void bench_thread_yield(void *arg)

for (i = 1; i <= ITERATIONS; i++) {
gather_set1_stats(MAIN_PRIORITY, i);
bench_collect_resources();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this right? The ancillary thread should never run, so it never adds itself to the "remove list".

Copy link
Collaborator

Choose a reason for hiding this comment

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

As @edersondisouza pointed out, the ancillary thread should never run. If it is actually running then the test is not working as intended and further investigation would be needed. The presence of bench_collect_resources() would then be hiding a problem.

}

bench_stats_report_line("Yield (no context switch)", &time_to_yield);
Expand All @@ -172,6 +174,7 @@ void bench_thread_yield(void *arg)

for (i = 1; i < ITERATIONS; i++) {
gather_set2_stats(MAIN_PRIORITY, i);
bench_collect_resources();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Even here, IIUIC, the ancillary thread (bench_set2_helper) doesn't ever get to the bench_thread_exit(), it's aborted before - am I missing something?

Copy link
Author

Choose a reason for hiding this comment

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

My debugging shows that bench_thread_exit is called 1000times (or ITERATIONS times). Which effectively leads to buffer overflow. (I used different FreeRTOS_config.h than the one provided)

I have followed how other tests were organized - gather_xx_stats, bench_collect_resources. When this same template was applied on the failing test, it started to work. I have not analyzed how gather_set[12]_stats works for the yield test.

The test framework should not assume what bench does on the inside.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You indicated that a different configuration file was used. Can you identify the key differences?

Copy link
Contributor

@maiomai maiomai May 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The issue occurs when configUSE_TIME_SLICING is defined 1 in FreeRTOSConfig.h . The bench_set2_helper() will run into bench_thread_exit() after the time slice of bench_thread_yield() is used up.

Copy link
Author

Choose a reason for hiding this comment

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

Nice catch, I didn't notice this option, as it was not in my config file and defaults to being enabled by default. I didnt have enough time to get back to testing this.

}

bench_timing_stop();
Expand Down
1 change: 1 addition & 0 deletions src/common/bench_thread_test.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -280,6 +280,7 @@ static void gather_set1_stats(int priority, uint32_t iteration)
*/
void bench_basic_thread_ops(void *arg)
{
ARG_UNUSED(arg);
uint32_t i;

bench_timing_init();
Expand Down