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

tests: test for prio inversion using msg_send_recv #9306

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

haukepetersen
Copy link
Contributor

Contribution description

#7445 also introduces priority inheritance for msg_send_receive calls. This PR adds a test showcasing priority inversion for msg_send_receive, cutting it from #7445 for simpler reviewing...

Issues/PRs references

Cut out from #7445 and similar to #7444

@haukepetersen haukepetersen added 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 labels Jun 7, 2018
Event 3: t1 - sending msg to t3 (msg_send_receive)
Event 4: t3 - received message
Event 5: t3 - sending reply
Event 6: t1 - received reply
Copy link
Contributor

Choose a reason for hiding this comment

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

I reckon this is starvation, rather than priority inversion. Reason is that the medium priority thread prevents the low thread from running even before the message is sent, rather than pre-empting it while it's handling the message. Priority inversion would involve the second one - i.e. indirect pre-emption of a higher priority thread.
An example displaying priority inversion with messages would involve the high thread sending the message to the low thread, and the low thread receiving it and doing some action before sending. But while that action is happening, the medium thread comes along and pre-empts the low thread.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, will fix.

puts("TEST OUTPUT:");

/* create threads */
for (unsigned i = 0; i < T_NUMOF; i++) {
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO separating out three "high", "medium" and "low" named threads was nicer for readability.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed.

return NULL;
}

static thread_task_func_t _handlers[] = { t1, t2, t3 };
Copy link
Contributor

Choose a reason for hiding this comment

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

This might fit better at the top, next to _names? Or at least the two together?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

its a tradeoff, having it here means that I was able to skip the forward declaration of the thread functions...

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah. yep

@danpetry
Copy link
Contributor

NB I've differentiated between "review changes" and "suggestions". I.e. between saying, "this should be changed", and just giving feedback, to be changed if desired. Is this clear in the way I've done it?

@haukepetersen
Copy link
Contributor Author

fixed style issues, looking into the starvation vs inversion problem next

@haukepetersen
Copy link
Contributor Author

changed the test slightly, should now actually test for prio inversion instead of prio starvation, right? In the end IMHO it does not really matter, as both versions break without any counter measures, and both versions are fixed with priority inheritance enabled, so they should actually test for very similar behavior.

@danpetry
Copy link
Contributor

danpetry commented Jun 14, 2018

Great, thanks very much. Would you please be able to change the README to match? Then squash?
After that, I have no more comments. What do you think about keeping this and #7444 open until #7445 is merged? Then merge immediately after? Or some other related plan?

Test for showing priority inversion when using msg_send_receive

If this tests succeeds, you should see 6 events appearing in order.
The expected output should look like this:
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please change the readme to reflect the recent changes in the test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

of course

@haukepetersen
Copy link
Contributor Author

fixed the README and squashed.

@haukepetersen
Copy link
Contributor Author

What do you think about keeping this and #7444 open until #7445 is merged? Then merge immediately after? Or some other related plan?

Sounds like a plan.

@haukepetersen
Copy link
Contributor Author

fixed board blacklisting and squashed.

@haukepetersen
Copy link
Contributor Author

fixed one more little pep issue in the testrunner script

@stale
Copy link

stale bot commented Aug 10, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

@stale stale bot added the State: stale State: The issue / PR has no activity for >185 days label Aug 10, 2019
@haukepetersen haukepetersen added the State: don't stale State: Tell state-bot to ignore this issue label Aug 12, 2019
@stale stale bot removed the State: stale State: The issue / PR has no activity for >185 days label Aug 12, 2019
@MrKevinWeiss MrKevinWeiss added this to the Release 2021.07 milestone Jun 22, 2021
@MrKevinWeiss MrKevinWeiss removed this from the Release 2021.07 milestone Jul 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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 State: don't stale State: Tell state-bot to ignore this issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants