From 7d422e958f4cfd5a6b9cc92995703a57b7ee02a1 Mon Sep 17 00:00:00 2001 From: Derek Bruening Date: Mon, 18 Dec 2023 13:19:14 -0500 Subject: [PATCH] Review requests: dup type error and test; rename field; more comments and checks --- clients/drcachesim/scheduler/scheduler.cpp | 13 +++-- clients/drcachesim/scheduler/scheduler.h | 7 ++- .../drcachesim/tests/scheduler_unit_tests.cpp | 48 +++++++++++++++++-- 3 files changed, 59 insertions(+), 9 deletions(-) diff --git a/clients/drcachesim/scheduler/scheduler.cpp b/clients/drcachesim/scheduler/scheduler.cpp index 8e59118916f..dd97477945f 100644 --- a/clients/drcachesim/scheduler/scheduler.cpp +++ b/clients/drcachesim/scheduler/scheduler.cpp @@ -1183,6 +1183,7 @@ scheduler_tmpl_t::read_switch_sequences() } reader = std::move(options_.kernel_switch_reader); reader_end = std::move(options_.kernel_switch_reader_end); + // We own calling init() as it can block. if (!reader->init()) { error_string_ += "Failed to init kernel switch reader"; return STATUS_ERROR_INVALID_PARAMETER; @@ -1201,6 +1202,10 @@ scheduler_tmpl_t::read_switch_sequences() if (record_type_is_marker(record, marker_type, marker_value) && marker_type == TRACE_MARKER_TYPE_CONTEXT_SWITCH_START) { switch_type = static_cast(marker_value); + if (!switch_sequence_[switch_type].empty()) { + error_string_ += "Duplicate context switch sequence type found"; + return STATUS_ERROR_INVALID_PARAMETER; + } } if (switch_type != SWITCH_INVALID) switch_sequence_[switch_type].push_back(record); @@ -1375,7 +1380,7 @@ scheduler_tmpl_t::is_record_synthetic(output_ordinal_t o int index = outputs_[output].cur_input; if (index < 0) return false; - if (outputs_[output].in_switch_code) + if (outputs_[output].in_context_switch_code) return true; return inputs_[index].reader->is_record_synthetic(); } @@ -2400,8 +2405,8 @@ scheduler_tmpl_t::next_record(output_ordinal_t output, } } if (outputs_[output].hit_switch_code_end) { - // We have to delay so the end marker is still in_switch_code. - outputs_[output].in_switch_code = false; + // We have to delay so the end marker is still in_context_switch_code. + outputs_[output].in_context_switch_code = false; outputs_[output].hit_switch_code_end = false; // We're now back "on the clock". if (options_.quantum_unit == QUANTUM_TIME) @@ -2426,7 +2431,7 @@ scheduler_tmpl_t::next_record(output_ordinal_t output, marker_type == TRACE_MARKER_TYPE_CONTEXT_SWITCH_START)) { outputs_[output].in_kernel_code = true; if (marker_type == TRACE_MARKER_TYPE_CONTEXT_SWITCH_START) - outputs_[output].in_switch_code = true; + outputs_[output].in_context_switch_code = true; } else if (record_type_is_marker(record, marker_type, marker_value) && (marker_type == TRACE_MARKER_TYPE_SYSCALL_TRACE_END || marker_type == TRACE_MARKER_TYPE_CONTEXT_SWITCH_END)) { diff --git a/clients/drcachesim/scheduler/scheduler.h b/clients/drcachesim/scheduler/scheduler.h index efb3db20049..b1fa587d133 100644 --- a/clients/drcachesim/scheduler/scheduler.h +++ b/clients/drcachesim/scheduler/scheduler.h @@ -562,8 +562,11 @@ template class scheduler_tmpl_t { * #block_time_scale. */ uint64_t block_time_max = 25000000; + // XXX: Should we share the file-to-reader code currently in the scheduler + // with the analyzer and only then need reader interfaces and not pass paths + // to the scheduler? /** - * Input file containing canned sequences of kernel context switch code. + * Input file containing template sequences of kernel context switch code. * Each sequence must start with a #TRACE_MARKER_TYPE_CONTEXT_SWITCH_START * marker and end with #TRACE_MARKER_TYPE_CONTEXT_SWITCH_END. * The values of each marker must hold a #switch_type_t enum value @@ -1179,7 +1182,7 @@ template class scheduler_tmpl_t { bool waiting = false; // Waiting or idling. bool active = true; bool in_kernel_code = false; - bool in_switch_code = false; + bool in_context_switch_code = false; bool hit_switch_code_end = false; // Used for time-based quanta. uint64_t cur_time = 0; diff --git a/clients/drcachesim/tests/scheduler_unit_tests.cpp b/clients/drcachesim/tests/scheduler_unit_tests.cpp index 158bfc9c6fd..9f42346c2bd 100644 --- a/clients/drcachesim/tests/scheduler_unit_tests.cpp +++ b/clients/drcachesim/tests/scheduler_unit_tests.cpp @@ -3564,7 +3564,6 @@ test_kernel_switch_sequences() auto switch_reader = std::unique_ptr(new mock_reader_t(switch_sequence)); auto switch_reader_end = std::unique_ptr(new mock_reader_t()); - static constexpr int NUM_WORKLOADS = 3; static constexpr int NUM_INPUTS_PER_WORKLOAD = 3; static constexpr int NUM_OUTPUTS = 2; @@ -3676,7 +3675,7 @@ test_kernel_switch_sequences() else if (memref.marker.marker_value == scheduler_t::SWITCH_THREAD) sched_as_string[i] += 't'; else - sched_as_string[i] += '?'; + assert(false && "unknown context switch type"); break; default: sched_as_string[i] += '?'; break; } @@ -3742,7 +3741,50 @@ test_kernel_switch_sequences() // The 3-instr quantum should not count the 2 switch instrs. check_ref(refs[0], idx, TID_BASE + 4, TRACE_TYPE_INSTR) && check_ref(refs[0], idx, TID_BASE + 4, TRACE_TYPE_INSTR) && - check_ref(refs[0], idx, TID_BASE + 4, TRACE_TYPE_INSTR) && true; + check_ref(refs[0], idx, TID_BASE + 4, TRACE_TYPE_INSTR); + + { + // Test a bad input sequence. + std::vector bad_switch_sequence = { + /* clang-format off */ + make_header(TRACE_ENTRY_VERSION), + make_thread(TID_IN_SWITCHES), + make_pid(TID_IN_SWITCHES), + make_marker(TRACE_MARKER_TYPE_CONTEXT_SWITCH_START, scheduler_t::SWITCH_PROCESS), + make_instr(PROCESS_SWITCH_PC_START), + make_marker(TRACE_MARKER_TYPE_CONTEXT_SWITCH_END, scheduler_t::SWITCH_PROCESS), + make_footer(), + make_header(TRACE_ENTRY_VERSION), + make_thread(TID_IN_SWITCHES), + make_pid(TID_IN_SWITCHES), + // Error: duplicate type. + make_marker(TRACE_MARKER_TYPE_CONTEXT_SWITCH_START, scheduler_t::SWITCH_PROCESS), + make_instr(PROCESS_SWITCH_PC_START), + make_marker(TRACE_MARKER_TYPE_CONTEXT_SWITCH_END, scheduler_t::SWITCH_PROCESS), + make_footer(), + /* clang-format on */ + }; + auto bad_switch_reader = + std::unique_ptr(new mock_reader_t(bad_switch_sequence)); + auto switch_reader_end = std::unique_ptr(new mock_reader_t()); + std::vector sched_inputs; + std::vector readers; + std::vector inputs; + inputs.push_back(make_header(TRACE_ENTRY_VERSION)); + readers.emplace_back(std::unique_ptr(new mock_reader_t(inputs)), + std::unique_ptr(new mock_reader_t()), + TID_BASE); + sched_inputs.emplace_back(std::move(readers)); + scheduler_t::scheduler_options_t sched_ops(scheduler_t::MAP_TO_ANY_OUTPUT, + scheduler_t::DEPENDENCY_TIMESTAMPS, + scheduler_t::SCHEDULER_DEFAULTS); + sched_ops.kernel_switch_reader = std::move(bad_switch_reader); + sched_ops.kernel_switch_reader_end = std::move(switch_reader_end); + scheduler_t scheduler; + if (scheduler.init(sched_inputs, NUM_OUTPUTS, std::move(sched_ops)) != + scheduler_t::STATUS_ERROR_INVALID_PARAMETER) + assert(false); + } } int