Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

tests/thread_float: improve and add script #16901

Merged
merged 1 commit into from
Nov 10, 2021
Merged

tests/thread_float: improve and add script #16901

merged 1 commit into from
Nov 10, 2021

Conversation

maribu
Copy link
Member

@maribu maribu commented Sep 28, 2021 •
edited
Loading

Contribution description

  • Perform the same computation over and over again. If the results differ, context switches have an impact on the calculation (e.g. when the FPU internally uses more bits than a float, but that bits are not saved / restored on context switch)
  • Give the three threads the names "t1", "t2", and "t3" and print them on console, instead of the process ID. This makes interpretation of the output easier, as the process IDs depend e.g. on whether a given platforms requires an idle thread or not.
  • Do not use the thread ID in the calculation, but the number at the end of the thread name. This will result in the number printed only depending on the precision of the (software) FPU and the printf() implementation, and not on which threads are created in which order (including the idle thread)
  • Add a script to support running make test

Testing procedure

The test should still work, but now have easier to read output.

Issues/PRs references

#16896

@maribu maribu added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: tests Area: tests and testing framework Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer labels Sep 28, 2021
@benpicco
Copy link
Contributor

Could you also provide a tests/01-run.py while you are at it?

@maribu maribu requested a review from miri64 as a code owner September 28, 2021 15:58
@maribu
Copy link
Member Author

maribu commented Sep 28, 2021

Could you also provide a tests/01-run.py while you are at it?

Done. I'm not sure if it makes sense to also match the output after the period (since that will greatly depend on the precision of the (soft) FPU and the printf() implementation). In any case, I'm not a decent Python programmer anyway, so I better not try to extend the test and leave this to someone more qualified, if this is considered to be beneficial.

@miri64
Copy link
Member

miri64 commented Sep 28, 2021

I'm not sure if it makes sense to also match the output after the period (since that will greatly depend on the precision of the (soft) FPU and the printf() implementation). In any case, I'm not a decent Python programmer anyway, so I better not try to extend the test and leave this to someone more qualified, if this is considered to be beneficial.

Please remember, that periods have special meaning in regular expressions ;-).

miri64
miri64 reviewed Sep 28, 2021
View reviewed changes
@benpicco benpicco added the CI: run tests If set, CI server will run tests on hardware for the labeled PR label Sep 28, 2021
@maribu
Copy link
Member Author

maribu commented Sep 28, 2021

Btw.: I think I just realized what the test might be about: Whether the FPU context is properly saved and restored on context switching. Which apparently is not the case for my STM32F767ZI, the threads end up with a (slightly) different result each time.

It is a pity that there is no README or anything pointing out the reasoning of the test.

maribu
maribu commented Sep 28, 2021
View reviewed changes
@benpicco benpicco requested a review from vincent-d September 28, 2021 17:11
@maribu maribu changed the title tests/thread_float: improve console output tests/thread_float: improve and add script Sep 28, 2021
@github-actions github-actions bot added the Area: doc Area: Documentation label Sep 28, 2021
@maribu
Copy link
Member Author

maribu commented Sep 28, 2021

I think I'm now happy with the test. May I squash?

miri64
miri64 reviewed Sep 29, 2021
View reviewed changes
Copy link
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

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

Will provide suggestions for that.

Here we go.

@maribu maribu force-pushed the tests/thread_float branch 2 times, most recently from 5ca22e2 to eb87273 Compare September 29, 2021 09:38
@maribu maribu added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Sep 29, 2021
@maribu maribu 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 labels Oct 22, 2021
@maribu maribu force-pushed the tests/thread_float branch from 5ecd434 to b63b44e Compare November 1, 2021 12:28
@maribu maribu 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 labels Nov 1, 2021
@maribu
Copy link
Member Author

maribu commented Nov 1, 2021

The last CI run shows the expected failure on native which indicates that context switching on native does not reliably save and restore the FPU state.

I assume blacklisting native for now to get the test in is fine? To he honest, I'm not sure how to fix the issue on native. I think it is exposing a bug in glibc. But since the makecontext() / swapcontext() is obsolete since quite some time, I guess the enthusiasm to fix them is limited.

@maribu maribu force-pushed the tests/thread_float branch from b63b44e to 5e9e3fd Compare November 4, 2021 08:22
@maribu maribu 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 labels Nov 4, 2021
@maribu
Copy link
Member Author

maribu commented Nov 4, 2021

All green now (after disabling the test run for native on the CI ...)

@fjmolinas
Copy link
Contributor

All green now (after disabling the test run for native on the CI ...)

I'm OK with merging as is, but if there is a possible bug then I think an issue should be opened as well.

@maribu
Copy link
Member Author

maribu commented Nov 9, 2021

I'm OK with merging as is, but if there is a possible bug then I think an issue should be opened as well.

I cannot reproduce the issue with the current riotbuild. Maybe the issue has been solved by switching to the newest Ubuntu TLS release? I'll revert the blacklisting. Let's see, what happens.

@maribu maribu force-pushed the tests/thread_float branch from 5e9e3fd to 57b35c8 Compare November 9, 2021 13:06
@maribu
Copy link
Member Author

maribu commented Nov 9, 2021

All green with testing on native reenabled. I guess chances are good that the issue on native is fixed with updating the docker container to the latest Ubuntu LTS.

@fjmolinas
Copy link
Contributor

All green with testing on native reenabled. I guess chances are good that the issue on native is fixed with updating the docker container to the latest Ubuntu LTS.

For me it fails locally still... can we be sure the last run was not just lucky?

@maribu
Copy link
Member Author

maribu commented Nov 9, 2021

can we be sure the last run was not just lucky?

Not really. Let me disable the test again. But this time, we skip compile tests :-) I'll open the issue.

@maribu @aabadie
tests/thread_float: improve and add test script
f62b662 
- Perform the same computation over and over again. If the results
  differ, context switches have an impact on the calculation (e.g.
  when the FPU internally uses more bits than a float, but that bits
  are not saved / restored on context switch)
- Give the three threads the names "t1", "t2", and "t3" and print them
  on console, instead of the process ID. This makes interpretation of
  the output easier, as the process IDs depend e.g. on whether a given
  platforms requires an idle thread or not.
- Do not use the thread ID in the calculation, but the number at the
  end of the thread name. This will result in the number printed only
  depending on the precision of the (software) FPU and the printf()
  implementation, and not on which threads are created in which order
  (including the idle thread)
- Add a script to support running `make test`

Update tests/thread_float/tests/01-run.py

Co-authored-by: Alexandre Abadie <alexandre.abadie@inria.fr>
@maribu maribu force-pushed the tests/thread_float branch from 57b35c8 to f62b662 Compare November 9, 2021 18:58
@maribu maribu added CI: skip compile test If set, CI server will run only non-compile jobs, but no compile jobs or their dependent jobs 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 labels Nov 9, 2021
@maribu maribu mentioned this pull request Nov 9, 2021
Open
@maribu
Copy link
Member Author

maribu commented Nov 9, 2021

Issue opened at: #17170

fjmolinas
fjmolinas approved these changes Nov 10, 2021
View reviewed changes
Copy link
Contributor

@fjmolinas fjmolinas left a comment

Choose a reason for hiding this comment

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

ACK!, I agree with the current status where a potentially flaky native test is blacklisted.

@fjmolinas fjmolinas merged commit 2479467 into RIOT-OS:master Nov 10, 2021
@fjmolinas
Copy link
Contributor

Thanks for the rework @maribu!

@yarrick yarrick mentioned this pull request Nov 11, 2021
Merged
@fjmolinas fjmolinas added this to the Release 2022.01 milestone Nov 18, 2021
@maribu maribu deleted the tests/thread_float branch January 23, 2022 16:38
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@miri64 miri64 miri64 approved these changes

@aabadie aabadie aabadie left review comments

@fjmolinas fjmolinas fjmolinas approved these changes

@leandrolanzieri leandrolanzieri Awaiting requested review from leandrolanzieri leandrolanzieri is a code owner

@MichelRottleuthner MichelRottleuthner Awaiting requested review from MichelRottleuthner MichelRottleuthner is a code owner

@smlng smlng Awaiting requested review from smlng

@vincent-d vincent-d Awaiting requested review from vincent-d

Assignees
No one assigned
Labels
Area: doc Area: Documentation 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 CI: skip compile test If set, CI server will run only non-compile jobs, but no compile jobs or their dependent jobs Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Milestone
Release 2022.01
Development

Successfully merging this pull request may close these issues.
5 participants
@maribu @benpicco @miri64 @fjmolinas @aabadie