Skip to content

tests/thread_race: add test for race conditions#8897

Merged
kYc0o merged 1 commit intoRIOT-OS:masterfrom
ZetaR60:RIOT_atmega_race
May 4, 2018
Merged

tests/thread_race: add test for race conditions#8897
kYc0o merged 1 commit intoRIOT-OS:masterfrom
ZetaR60:RIOT_atmega_race

Conversation

@ZetaR60
Copy link
Contributor

@ZetaR60 ZetaR60 commented Apr 7, 2018

This adds a test for race conditions in context switching, especially for AVR_CONTEXT_SWAP_TRIGGER on the ATmega.

Tests for problem in #8896

This was referenced Apr 7, 2018
Closed
Merged
@ZetaR60
Copy link
Contributor Author

ZetaR60 commented Apr 9, 2018

Fixed for #8904

kYc0o
kYc0o requested changes Apr 11, 2018
View reviewed changes
Copy link
Contributor

@kYc0o kYc0o left a comment

Choose a reason for hiding this comment

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

I found some things. I think this PR should be made dependent on #8904 .

tests/thread_race/main.c Outdated
/* Volatile so it is not messed with by optimizations */
volatile uint8_t i;

for (i=0; i<255; i++);
Copy link
Contributor

@kYc0o kYc0o Apr 11, 2018

Choose a reason for hiding this comment

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

Here and below (for the if-else statements), please follow coding conventions. You may also run uncrustify on this file to correct it automatically.

Copy link
Contributor Author

@ZetaR60 ZetaR60 Apr 11, 2018

Choose a reason for hiding this comment

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

Sorry, still new to RIOT conventions. Uncrustified.

tests/thread_race/main.c Outdated

int main(void)
{
puts("Context swap race condition test application\n");
Copy link
Contributor

@kYc0o kYc0o Apr 11, 2018

Choose a reason for hiding this comment

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

Here and below, no need of \n in the puts() function.

Copy link
Contributor Author

@ZetaR60 ZetaR60 Apr 11, 2018

Choose a reason for hiding this comment

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

Removed.

tests/thread_race/main.c Outdated
#include "thread.h"

/* board.h for AVR_CONTEXT_SWAP_TRIGGER */
#include "board.h"
Copy link
Contributor

@kYc0o kYc0o Apr 11, 2018

Choose a reason for hiding this comment

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

Here and below, I don't like the non-generic nature on this tests, and why AVR should be treated in such special way... I see more convenient to have a generic test which works for all platforms without #ifdefs.

If this is due to #8896, please make this PR dependent on #8904, on which AVR_CONTEXT_SWAP_TRIGGER doesn't exist anymore.

Copy link
Contributor Author

@ZetaR60 ZetaR60 Apr 11, 2018

Choose a reason for hiding this comment

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

I was having some false negatives with thread_yield_higher, probably due to the slower clock on mega-xplained. AVR_CONTEXT_SWAP_TRIGGER is now removed.

tests/thread_race/main.c
#endif

/* Delay so we are not testing for race conditions also */
_spin();
Copy link
Contributor

@kYc0o kYc0o Apr 11, 2018

Choose a reason for hiding this comment

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

Can you explain a bit better this spin? If I understood correctly, this test is actually used to test race conditions isn't it?

Copy link
Contributor Author

@ZetaR60 ZetaR60 Apr 11, 2018

Choose a reason for hiding this comment

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

The first two checks are verifying that context swaps are working properly. Without these checks a more general problem with context swaps would be reported as a race condition. I added to the outputted text to clarify this.

@ZetaR60
Copy link
Contributor Author

ZetaR60 commented Apr 27, 2018 •
edited
Loading

Squashed with requested changes incorporated.

EDIT: Oops; wasn't supposed to squash. Unsquashed from backup. Sorry.

kYc0o
kYc0o approved these changes May 3, 2018
View reviewed changes
Copy link
Contributor

@kYc0o kYc0o left a comment

Choose a reason for hiding this comment

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

Looks good. Please squash.

@kYc0o kYc0o added Platform: AVR Platform: This PR/issue effects AVR-based platforms quality defect labels May 3, 2018
@kYc0o kYc0o added this to the Release 2018.07 milestone May 3, 2018
@ZetaR60 ZetaR60 force-pushed the RIOT_atmega_race branch from bcc73fc to edd27ab Compare May 3, 2018 21:39
@ZetaR60
Copy link
Contributor Author

ZetaR60 commented May 3, 2018

@kYc0o Squashed. Thanks!

kYc0o
kYc0o approved these changes May 4, 2018
View reviewed changes
Copy link
Contributor

@kYc0o kYc0o left a comment

Choose a reason for hiding this comment

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

Sorry, forgot a change.

tests/thread_race/Makefile
include ../Makefile.tests_common

DISABLE_MODULE += auto_init

Copy link
Contributor

@kYc0o kYc0o May 4, 2018

Choose a reason for hiding this comment

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

I forgot this part. I think this test should be run by the CI on hardware. Can you please add:

TEST_ON_CI_WHITELIST += all

just here?

Copy link
Contributor Author

@ZetaR60 ZetaR60 May 4, 2018

Choose a reason for hiding this comment

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

Added support for CI hardware testing.

@kYc0o kYc0o added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: run tests If set, CI server will run tests on hardware for the labeled PR labels May 4, 2018
@kYc0o
Copy link
Contributor

kYc0o commented May 4, 2018

Thanks @ZetaR60 ! Let's see what Murdock has to say.

@ZetaR60 ZetaR60 force-pushed the RIOT_atmega_race branch from 9d59466 to 8d40e3c Compare May 4, 2018 16:53
@kYc0o kYc0o added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: run tests If set, CI server will run tests on hardware for the labeled PR labels May 4, 2018
@kYc0o
Copy link
Contributor

kYc0o commented May 4, 2018

Removed tests on hardware because unrelated errors block this PR. However the test added by this PR passed.

@kYc0o
Copy link
Contributor

kYc0o commented May 4, 2018

Let's go!

@kYc0o kYc0o merged commit 5519f06 into RIOT-OS:master May 4, 2018
@miri64 miri64 added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: tests Area: tests and testing framework and removed quality defect labels Oct 1, 2018
This was referenced Sep 9, 2019
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kYc0o kYc0o kYc0o approved these changes

Assignees

No one assigned

Labels

Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: AVR Platform: This PR/issue effects AVR-based platforms Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation

Projects

None yet

Milestone

Release 2018.07

Development

Successfully merging this pull request may close these issues.

3 participants

@ZetaR60 @kYc0o @miri64