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

KM Multi-threaded stress tests - Code clean-up, duplicate code refactoring #2467

Merged
merged 1 commit into from
May 16, 2023
Merged

KM Multi-threaded stress tests - Code clean-up, duplicate code refactoring #2467

merged 1 commit into from
May 16, 2023

Conversation

dv-msft
Copy link
Collaborator

@dv-msft dv-msft commented May 12, 2023

Description

Code clean-up and common code refactoring of the tests in the 'Kernel mode Multi-threaded stress tests' suite.

Testing

clean-up of existing test collateral.

Documentation

No doc changes.

Fixes #2463

@codecov
Copy link

codecov bot commented May 12, 2023

Codecov Report

Merging #2467 (56e1938) into main (174757e) will decrease coverage by 0.06%.
The diff coverage is n/a.

❗ Current head 56e1938 differs from pull request most recent head e59e7ee. Consider uploading reports for the commit e59e7ee to get more accurate results

@@            Coverage Diff             @@
##             main    #2467      +/-   ##
==========================================
- Coverage   84.02%   83.97%   -0.06%     
==========================================
  Files         155      155              
  Lines       28868    28868              
==========================================
- Hits        24257    24242      -15     
- Misses       4611     4626      +15     

see 4 files with indirect coverage changes

@dv-msft dv-msft changed the title (DRAFT PR - NOT FOR REVIEW) KM Multi-threaded stress tests - Code clean-up, duplicate code refactoring KM Multi-threaded stress tests - Code clean-up, duplicate code refactoring May 12, 2023
@dv-msft dv-msft marked this pull request as ready for review May 12, 2023 05:04
@dthaler dthaler added the bug Something isn't working label May 12, 2023
tests/stress/km/stress_tests_km.cpp Outdated Show resolved Hide resolved
[&](uint32_t local_restart_delay, uint32_t local_thread_lifetime) {
// Delay the start of this thread for a bit to allow the ebpf programs to attach successfully. There's a
// window where if the extension is unloading/unloaded, an incoming attach might fail.
std::this_thread::sleep_for(std::chrono::seconds(3));
Copy link
Collaborator

Choose a reason for hiding this comment

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

This implies the test is non-deterministic and if the CPU is pegged then the test can fail, is that correct? If so, there's probably a better solution in the future than introducing an arbitrary delay.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, the test itself wouldn't fail. It would just carry on with the start of the 'extension restart' thread delayed even further. Given that the minimum test runtime is 1 minute, IMO it's more than enough for the restart thread to start running eventually. In any case, system load affecting thread scheduling would affect the test threads as well, so their delays would balance out.

FWIW, I did think of synchronizing this thread with the test threads but thought it was overkill given the 'test only' nature of this code. But I can certainly file an issue to revisit that synchronization if it is seen to cause issues during actual testing.

tests/stress/km/stress_tests_km.cpp Outdated Show resolved Hide resolved
tests/stress/km/stress_tests_km.cpp Outdated Show resolved Hide resolved
tests/stress/readme.md Outdated Show resolved Hide resolved
@Alan-Jowett Alan-Jowett added this pull request to the merge queue May 15, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks May 15, 2023
@dv-msft dv-msft added this pull request to the merge queue May 15, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks May 15, 2023
@dv-msft dv-msft added this pull request to the merge queue May 16, 2023
Merged via the queue into microsoft:main with commit 574d41b May 16, 2023
@dv-msft dv-msft deleted the km-stress-cleanup branch May 16, 2023 02:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

code clean-up - kernel mode multi-threaded stress tests
3 participants