Skip to content

tests/kernel: fifo_timeout: Remove wake-up order checking #10047

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

Merged
merged 1 commit into from
Oct 2, 2018

Conversation

rgundi
Copy link
Contributor

@rgundi rgundi commented Sep 18, 2018

There is no guarantee of wake-up order when multiple threads
are woken up on the same tick. Hence, modified the tests
accordingly.

Fixes #8159.

Signed-off-by: Rajavardhan Gundi [email protected]

@@ -1,2 +1,3 @@
CONFIG_ZTEST=y
CONFIG_IRQ_OFFLOAD=y
CONFIG_BOOT_DELAY=1000
Copy link
Contributor

Choose a reason for hiding this comment

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

@rgundi: Is there any reason for introducing boot delay?

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. The issue #8159 cannot be reproduced without this delay. It's a very timing dependent issue.

Copy link
Member

Choose a reason for hiding this comment

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

you should remove it though, we do not set this for tests by default.

Copy link
Collaborator

@pizi-nordic pizi-nordic left a comment

Choose a reason for hiding this comment

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

I think that this PR throws out the baby with the bathwater, as it removes order check completely.

IMHO order check should be still there, but it has to take under account presence of tick granulation. For example we can record start time of each thread and check if the timeouts are in specified bounds (not earlier than _ms_to_ticks() and not later than _ms_to_ticks()+1).

@rgundi
Copy link
Contributor Author

rgundi commented Sep 19, 2018

I can't see a better method as the timeouts get affected by system loading. If the interrupts get locked for whatever reason, the timeouts may go out of bounds (later than _ms_to_ticks()+1). So, all we can do is to ensure the timeouts do not happen earlier than _ms_to_ticks().

@rgundi
Copy link
Contributor Author

rgundi commented Sep 20, 2018

@pizi-nordic .. can you please comment?

@pizi-nordic
Copy link
Collaborator

pizi-nordic commented Sep 21, 2018

I agree that there is no upper limit on loaded system.
However during the test we are controlling system state (configuration, number of threads and so on).
So we can assume upper bound. What about the following checks:

  1. Make sure that thread does not waited shorter than expected,
  2. Make sure that thread was woken up not later than expected_time + X (X = 1-2 ticks should be sufficient).

@rgundi
Copy link
Contributor Author

rgundi commented Sep 24, 2018

@pizi-nordic Agree. Have updated the test. Please check.

@codecov-io
Copy link

codecov-io commented Sep 24, 2018

Codecov Report

Merging #10047 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #10047   +/-   ##
=======================================
  Coverage   53.05%   53.05%           
=======================================
  Files         214      214           
  Lines       26203    26203           
  Branches     5646     5646           
=======================================
  Hits        13903    13903           
  Misses      10028    10028           
  Partials     2272     2272

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4dfd311...1733655. Read the comment docs.

}
}

if (data->timeout > test_data[j].timeout) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

IMHO this check is not the one we want.
Whit this code, kernel might wake-up thread earlier than expected and the test will not notice that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is already taken care in "is_timeout_in_range()" function. That function checks for the lower limit while the new code above checks for the upper limit.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok

@@ -1,2 +1,3 @@
CONFIG_ZTEST=y
CONFIG_IRQ_OFFLOAD=y
CONFIG_BOOT_DELAY=1000
Copy link
Member

Choose a reason for hiding this comment

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

you should remove it though, we do not set this for tests by default.

There is no guarantee of wake-up order when multiple threads
are woken up on the same tick. Hence, modified the tests
accordingly.

Fixes zephyrproject-rtos#8159.

Signed-off-by: Rajavardhan Gundi <[email protected]>
@rgundi
Copy link
Contributor Author

rgundi commented Oct 1, 2018

@nashif : Done.

@nashif nashif merged commit fa77e40 into zephyrproject-rtos:master Oct 2, 2018
@rgundi rgundi deleted the fifo_timeout branch October 3, 2018 05:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants