-
Notifications
You must be signed in to change notification settings - Fork 7
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
Add a test to ensure the report_stats hook is actually called internally #18
Open
doctorlai-msrc
wants to merge
10
commits into
main
Choose a base branch
from
zhihua/report_stats
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
4be42cc
Add a test to make sure the report_stats is actually called
f6c4e33
Add comment
4221023
Clang-format -i
7fc65d6
Merge branch 'main' into zhihua/report_stats
doctorlai-msrc f546a0e
Merge branch 'main' into zhihua/report_stats
fd9bca2
Changed to a simple local test
b63fb7c
Fix comment
59f95e0
Merge branch 'main' into zhihua/report_stats
doctorlai-msrc 62bdb35
Merge branch 'main' into zhihua/report_stats
doctorlai-msrc 030ba6a
Merge branch 'main' into zhihua/report_stats
doctorlai-msrc File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
# Copyright (c) Microsoft Corporation. All rights reserved. | ||
## report stats hook tests | ||
set(REPORT_STATS_TESTS ${TESTS_FUNCTIONAL}/report_stats) | ||
file(GLOB REPORT_STATS_TESTS_SOURCES ${REPORT_STATS_TESTS}/*.c) | ||
set(JBPF_TESTS ${JBPF_TESTS} PARENT_SCOPE) | ||
# Loop through each test file and create an executable | ||
foreach(TEST_FILE ${REPORT_STATS_TESTS_SOURCES}) | ||
# Get the filename without the path | ||
get_filename_component(TEST_NAME ${TEST_FILE} NAME_WE) | ||
|
||
# Create an executable target for the test | ||
add_executable(${TEST_NAME} ${TEST_FILE} ${TESTS_COMMON}/jbpf_test_lib.c ${JBPF_HASHMAP_MGMT_SOURCES}) | ||
|
||
# Link the necessary libraries | ||
target_link_libraries(${TEST_NAME} PUBLIC jbpf::core_lib jbpf::logger_lib jbpf::mem_mgmt_lib) | ||
|
||
# Set the include directories | ||
target_include_directories(${TEST_NAME} PUBLIC ${JBPF_LIB_HEADER_FILES} ${TEST_HEADER_FILES} ${JBPF_HASHMAP_MGMT_HEADER_FILES}) | ||
|
||
# Add the test to the list of tests to be executed | ||
add_test(NAME REPORT_STATS_TESTS/${TEST_NAME} COMMAND ${TEST_NAME}) | ||
|
||
# Test coverage | ||
list(APPEND JBPF_TESTS REPORT_STATS_TESTS/${TEST_NAME}) | ||
add_clang_format_check(${TEST_NAME} ${TEST_FILE}) | ||
set(JBPF_TESTS ${JBPF_TESTS} PARENT_SCOPE) | ||
endforeach() |
122 changes: 122 additions & 0 deletions
122
jbpf_tests/functional/report_stats/report_stats_hook_test.c
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,122 @@ | ||
/* | ||
* This test does the following: | ||
* 1. It loads a single codelet (C1). | ||
* 2. The codelet has a single hook (report_stats) that is called by the agent internally at the interval of | ||
* MAINTENANCE_CHECK_INTERVAL (see jbpf_perf.c). | ||
* 3. Then we check if the codelet (attached to report_stats hook) has been actually called by the agent. If it has been | ||
* called, we should get the output from the codelet. | ||
* 4. It uses the LCM IPC API to unload the codelet. | ||
*/ | ||
|
||
#include <assert.h> | ||
#include <semaphore.h> | ||
#include <fcntl.h> | ||
#include <sys/wait.h> | ||
#include <signal.h> | ||
|
||
#include "jbpf.h" | ||
#include "jbpf_agent_common.h" | ||
#include "jbpf_utils.h" | ||
|
||
// Contains the struct and hook definitions | ||
#include "jbpf_test_def.h" | ||
|
||
pid_t cpid = -1; | ||
|
||
sem_t sem; | ||
|
||
jbpf_io_stream_id_t stream_id_c1 = { | ||
.id = {0x00, 0x11, 0x22, 0x33, 0x44, 0x55, 0x66, 0x77, 0x88, 0x99, 0xAA, 0xBB, 0xCC, 0xDD, 0xEE, 0xFF}}; | ||
|
||
static void | ||
io_channel_check_output(jbpf_io_stream_id_t* stream_id, void** bufs, int num_bufs, void* ctx) | ||
{ | ||
int count = 0; | ||
for (int i = 0; i < num_bufs; i++) { | ||
if (memcmp(stream_id, &stream_id_c1, sizeof(stream_id_c1)) == 0) { | ||
// Output from C1. Check that the counter has the expected value | ||
count++; | ||
} else { | ||
// Unexpected output. Fail the test | ||
assert(false); | ||
} | ||
} | ||
|
||
// This means the report_stats hook has been called | ||
if (count > 0) { | ||
sem_post(&sem); | ||
} else { | ||
// we don't get any output from the codelet, meaning the report_stats hook has not been called | ||
assert(false); | ||
} | ||
} | ||
|
||
int | ||
main(int argc, char** argv) | ||
{ | ||
struct jbpf_codeletset_load_req codeletset_req_c1 = {0}; | ||
struct jbpf_codeletset_unload_req codeletset_unload_req_c1 = {0}; | ||
const char* jbpf_path = getenv("JBPF_PATH"); | ||
struct jbpf_config config = {0}; | ||
|
||
jbpf_set_default_config_options(&config); | ||
sem_init(&sem, 0, 0); | ||
|
||
config.lcm_ipc_config.has_lcm_ipc_thread = false; | ||
|
||
assert(jbpf_init(&config) == 0); | ||
|
||
// The thread will be calling hooks, so we need to register it | ||
jbpf_register_thread(); | ||
|
||
// Register a callback to handle the buffers sent from the codelets | ||
jbpf_register_io_output_cb(io_channel_check_output); | ||
|
||
// Load the codeletset with codelet C1 in hook "test1" | ||
|
||
// The name of the codeletset | ||
strcpy(codeletset_req_c1.codeletset_id.name, "simple_output_codeletset"); | ||
|
||
// We have only one codelet in this codeletset | ||
codeletset_req_c1.num_codelet_descriptors = 1; | ||
|
||
// The codelet has just one output channel and no shared maps | ||
codeletset_req_c1.codelet_descriptor[0].num_in_io_channel = 0; | ||
codeletset_req_c1.codelet_descriptor[0].num_out_io_channel = 1; | ||
|
||
// The name of the output map that corresponds to the output channel of the codelet | ||
strcpy(codeletset_req_c1.codelet_descriptor[0].out_io_channel[0].name, "output_map"); | ||
// Link the map to a stream id | ||
memcpy(&codeletset_req_c1.codelet_descriptor[0].out_io_channel[0].stream_id, &stream_id_c1, JBPF_STREAM_ID_LEN); | ||
// The output channel of the codelet does not have a serializer | ||
codeletset_req_c1.codelet_descriptor[0].out_io_channel[0].has_serde = false; | ||
codeletset_req_c1.codelet_descriptor[0].num_linked_maps = 0; | ||
|
||
// The path of the codelet | ||
assert(jbpf_path != NULL); | ||
snprintf( | ||
codeletset_req_c1.codelet_descriptor[0].codelet_path, | ||
JBPF_PATH_LEN, | ||
"%s/jbpf_tests/test_files/codelets/simple_output/simple_output.o", | ||
jbpf_path); | ||
strcpy(codeletset_req_c1.codelet_descriptor[0].codelet_name, "simple_output"); | ||
strcpy(codeletset_req_c1.codelet_descriptor[0].hook_name, "report_stats"); | ||
|
||
// Load the codeletset | ||
assert(jbpf_codeletset_load(&codeletset_req_c1, NULL) == JBPF_CODELET_LOAD_SUCCESS); | ||
|
||
// Wait for all the output checks to finish | ||
sem_wait(&sem); | ||
|
||
// Unload the codeletsets | ||
strcpy(codeletset_unload_req_c1.codeletset_id.name, "simple_output_codeletset"); | ||
assert(jbpf_codeletset_unload(&codeletset_unload_req_c1, NULL) == JBPF_CODELET_UNLOAD_SUCCESS); | ||
|
||
// Stop | ||
jbpf_stop(); | ||
|
||
sem_destroy(&sem); | ||
|
||
printf("Test completed successfully.\n"); | ||
return 0; | ||
} |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't this test fail due to timing issues? What if the output thread is called right before the maintenance thread is called? Wouldn't
io_channel_check_output()
fail in this case although everything might be working fine?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We use sem_post(lcm_ipc_sem) to make sure the codelet is loaded only after maintenance thread is created. By default, a dummy output_handler is registered, before
io_channel_check_output
is registered.I've ran the test 500 times, all passing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But how can we guarantee that the maintenance thread is called? What if it is delayed for a long time (e.g., 1s), because it runs in a very resource constrained environment? Can we somehow make this test deterministic and avoid relying on the timer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is a problem. As the test use the semphore to ensure the maintenance thread is called see line 47 and 109.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The semaphore of lines 47 and 109 ensures that the test will only terminate when the data is received. But what if the maintenance thread is delayed for a long time as I described above? Doesn't that mean that when the
io_channel_check_output()
function is called, execution might go to line 50? This would lead to a test failure, not because there is any issue, but just because the maintenance thread was late to be called. Or am I missing something?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding is that won't happen, as shown here and here the maintenance thread will be ready when
_jbpf_handle_out_bufs
is called. Correct me if I am wrong.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The thread will be ready, but there is no guarantee that it will be called before
_jbpf_handle_out_bufs()
. For example if both threads are running on the same CPU core and there is high contention, it could be that maintenance thread runs later than the io thread. As such, there could be scenarios where this test could fail simply due to high contention.I would suggest to revisit this test, to somehow not rely on the synchronization order of the two threads.