i#7157: Inject syscall templates dynamically in scheduler #7277

abhinav92003 · 2025-02-14T01:45:33Z

Adds support to the drmemtrace scheduler for injecting system call trace templates dynamically during scheduling. This obviates the need to create a separate statically-injected trace with system call trace templates.

Reuses context switch trace injection code to the extent possible.

Adds a new analyzer flag -sched_syscall_file and new scheduler_options_t fields to allow specifying the system call trace template file.

Adds a mock system call template file generated using the burst_syscall_inject test (slightly modified to use sysnums that match the checked-in test trace).

Adds various unit tests: a new scheduler unit test that verifies dynamic syscall trace injection and its effect on scheduling, analyzer tests that use -sched_syscall_file for dynamic injection in core-sharded and non-core-sharded modes, and an invariant checker test on the added mock system call template file.

Issue: #7157

Adds support to the drmemtrace scheduler for injecting system call trace templates dynamically during scheduling. This obviates the need to create a separate statically-injected trace with system call trace templates. Reuses context switch trace injection code to the extent possible. Adds a new analyzer flag -sched_syscall_file and new scheduler_options_t fields to allow specifying the system call trace template file. Issue: #7157

abhinav92003 · 2025-02-14T15:02:20Z

clients/drcachesim/tests/scheduler_unit_tests.cpp

+               "Av0is1ii1,Cv0is1ii1,Aiis2iii2,Ciis2iii2,Aiis1ii1,Ciis1ii1,Aiis2iii2,"
+               "Ciis2iii2,Aiis1ii1,Ciis1ii1");
+        assert(sched_as_string[1] ==
+               "Bv0is1ii1iis2iii2iis1ii1iis2iii2iis1ii1____________________________"


@derekbruening: Should core 2 have stolen either A or C from core 1?

There is a migration threshold that is almost certainly what is preventing that: it's assumed the cost of A migrating and having cold caches is higher than waiting to run on core0.

derekbruening · 2025-02-14T16:15:04Z

clients/drcachesim/analyzer.cpp

@@ -324,13 +324,21 @@ analyzer_tmpl_t<RecordType, ReaderType>::init_scheduler_common(
        sched_ops = sched_type_t::make_scheduler_parallel_options(verbosity_);
        sched_ops.replay_as_traced_istream = options.replay_as_traced_istream;
        sched_ops.read_inputs_in_init = options.read_inputs_in_init;
+        sched_ops.kernel_syscall_trace_path = options.kernel_syscall_trace_path;


analyzer.h docs for init_scheduler() say:

// For core-sharded, all of "options" is used; otherwise, only the // read_inputs_in_init field is preserved.

Please add that kernel_syscall_trace_path is preserved.

Also, please update to match HEAD while at it:

Looks like that should also include replay_as_traced_istream for the current code.

Plus a comment saying the same rules apply should be on the other init_scheduler and init_scheduler_common I would think.

derekbruening · 2025-02-14T16:16:21Z

clients/drcachesim/analyzer.cpp

@@ -324,13 +324,21 @@ analyzer_tmpl_t<RecordType, ReaderType>::init_scheduler_common(
        sched_ops = sched_type_t::make_scheduler_parallel_options(verbosity_);
        sched_ops.replay_as_traced_istream = options.replay_as_traced_istream;
        sched_ops.read_inputs_in_init = options.read_inputs_in_init;
+        sched_ops.kernel_syscall_trace_path = options.kernel_syscall_trace_path;
+        sched_ops.kernel_syscall_reader = std::move(options.kernel_syscall_reader);


These 2 reader fields are not set by analyzer nor promised to be propagated so no reason to copy here (while not copying every other field). (Maybe worth a comment here repeating that we only preserve certain fields.)

derekbruening · 2025-02-14T16:17:26Z

clients/drcachesim/analyzer.cpp

        if (worker_count_ <= 0)
            worker_count_ = std::thread::hardware_concurrency();
        output_count = worker_count_;
    } else {
        sched_ops = sched_type_t::make_scheduler_serial_options(verbosity_);
        sched_ops.replay_as_traced_istream = options.replay_as_traced_istream;
        sched_ops.read_inputs_in_init = options.read_inputs_in_init;
+        sched_ops.kernel_syscall_trace_path = options.kernel_syscall_trace_path;


At this point it seems worth sharing these preservation lines by joining the two parallel and serial in one else at line 323 and only splitting for the calls to make_scheduler_*_options and setting worker_count.

derekbruening · 2025-02-14T17:42:15Z

clients/drcachesim/common/options.cpp

+    "sequences.  The file can contain multiple sequences each with regular trace "
+    "headers and the sequence proper bracketed by "
+    "TRACE_MARKER_TYPE_CONTEXT_SWITCH_START and TRACE_MARKER_TYPE_CONTEXT_SWITCH_END "
+    "markers.");


I don't see any actual change here: revert?

derekbruening · 2025-02-14T17:44:29Z

clients/drcachesim/scheduler/scheduler.h

+         * indicating the system call it corresponds to. Sequences for
+         * multiple system calls are concatenated into a single file.
+         * Each sequence should be in the regular offline drmemtrace format.
+         * The sequence is inserted into the output stream after the


s/The/Each/
s/after the/after any/

derekbruening · 2025-02-14T18:15:36Z

clients/drcachesim/scheduler/scheduler_impl.cpp

                return sched_type_t::STATUS_ERROR_INVALID_PARAMETER;
            }
        }
-        if (switch_type != sched_type_t::SWITCH_INVALID)


Was this check lost?

derekbruening · 2025-02-14T18:20:14Z

clients/drcachesim/scheduler/scheduler_impl.cpp

+    trace_marker_type_t marker_type;
+    uintptr_t marker_value;
+    // Good to queue the injected records at this point, because we now surely will
+    // be done with TRACE_MARKER_TYPE_SYSCALL.


Can this be moved inside process_marker()? It feels out of place here. If for some reason it has to be here: please make a new helper function as this containing next_record() function is already long.

derekbruening · 2025-02-14T18:23:57Z

clients/drcachesim/tests/syscall_insertion.templatex

+Basic counts tool results:
+Total counts:
+      [1-9][0-9][0-9][0-9][0-9][0-9] total \(fetched\) instructions
+        5971 total unique \(fetched\) instructions


If we hardcoded this number, should we hardcode the others as well as they don't vary? Or you're just trying to have fewer values to change if we update the data file? Ditto below.

derekbruening · 2025-02-14T18:30:53Z

clients/drcachesim/tests/scheduler_unit_tests.cpp

+                        sched_as_string[i] += ',';
+                    sched_as_string[i] +=
+                        'A' + static_cast<char>(memref.instr.tid - TID_BASE);
+                }


Lines 6118-here plus some below look identical to test_kernel_switch_sequences() -- could we share this code via a new helper that lets each test customize the middle of the loop?

derekbruening · 2025-02-14T18:33:40Z

clients/drcachesim/tests/scheduler_unit_tests.cpp

+               "Av0is1ii1,Cv0is1ii1,Aiis2iii2,Ciis2iii2,Aiis1ii1,Ciis1ii1,Aiis2iii2,"
+               "Ciis2iii2,Aiis1ii1,Ciis1ii1");
+        assert(sched_as_string[1] ==
+               "Bv0is1ii1iis2iii2iis1ii1iis2iii2iis1ii1____________________________"


There is a migration threshold that is almost certainly what is preventing that: it's assumed the cost of A migrating and having cold caches is higher than waiting to run on core0.

abhinav92003 added 14 commits February 13, 2025 20:45

Fix format error

0aa2f04

Fix format error

e3aa741

Fix uninit error on Windows

c71e4c5

Fix Windows var declaration error

2e21613

Fix Windows lossy conversion warning

562d403

Cleanup

efe9774

Fix Windows uninit warning

b8b27e2

Fix Windows uninit warning

5ace127

Fix Windows uninit warning

a97ee6b

Add more tests for syscall trace injection

06dec36

Fix Windows uninit warning

166c816

Add in_sequence check

94fea86

Fix comment

7344de4

abhinav92003 requested a review from derekbruening February 14, 2025 07:11

abhinav92003 commented Feb 14, 2025

View reviewed changes

Merge branch 'master' into i7157-dyn-syscall-inj

6ac114d

derekbruening approved these changes Feb 14, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

i#7157: Inject syscall templates dynamically in scheduler #7277

i#7157: Inject syscall templates dynamically in scheduler #7277

abhinav92003 commented Feb 14, 2025 •

edited

Loading

abhinav92003 Feb 14, 2025

derekbruening Feb 14, 2025

derekbruening Feb 14, 2025

derekbruening Feb 14, 2025

derekbruening Feb 14, 2025

derekbruening Feb 14, 2025

derekbruening Feb 14, 2025

derekbruening Feb 14, 2025

derekbruening Feb 14, 2025

derekbruening Feb 14, 2025

derekbruening Feb 14, 2025

derekbruening Feb 14, 2025

i#7157: Inject syscall templates dynamically in scheduler #7277

Are you sure you want to change the base?

i#7157: Inject syscall templates dynamically in scheduler #7277

Conversation

abhinav92003 commented Feb 14, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abhinav92003 commented Feb 14, 2025 •

edited

Loading