-
Notifications
You must be signed in to change notification settings - Fork 571
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
i#7091: Add -multi_indir to drmemtrace #7242
base: master
Are you sure you want to change the base?
Conversation
Adds a new option -multi_indir to the drmemtrace framework. This allows combining multiple workload traces in core-sharded mode, interleaving threads from all the workloads together. Updates the documentation. Adds a test that runs 3 copies of the checked-in threadsig trace. Issue: #7091
return false; | ||
} | ||
workloads.emplace_back(path, regions); | ||
// Currently these modifiers apply to every workload. |
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.
This won't work if the thread_ids and shard_ids across workloads are not mutually exclusive.
Also, the shard_ids are just indices right? So there will definitely be overlap?
Maybe it's better to explicit not support modifiers for multi-workload for now?
Let's atleast add a TODO.
if (!op_infile.get_value().empty()) | ||
++offline_requests; | ||
if (offline_requests > 1) { | ||
this->error_string_ = "Usage error: only one of -indir, -multi_indir, or " |
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.
Could a user want to specify a mix of indir and infile? Do we want to support that?
op_ipc_name.get_value().empty()) { | ||
this->error_string_ = | ||
"Usage error: -ipc_name or -indir or -infile is required\nUsage:\n" + | ||
if (op_indir.get_value().empty() && op_multi_indir.get_value().empty() && |
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.
This could be simplified to else if(op_ipc_name.get_value().empty())
dir.in_kfiles_map_, dir.kcoredir_, dir.kallsymsdir_, | ||
std::move(dir.syscall_template_file_reader_), | ||
op_pt2ir_best_effort.get_value()); | ||
std::string error = raw2trace.do_conversion(); |
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.
Add a log at the appropriate verbosity to demarcate logs from raw2trace conversion of each.
@@ -513,10 +535,15 @@ analyzer_multi_tmpl_t<RecordType, ReaderType>::analyzer_multi_tmpl_t() | |||
return; | |||
} | |||
} | |||
if (!op_multi_indir.get_value().empty() && !op_core_sharded.get_value()) { |
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.
This should preferably be an unfront error, because the raw2trace conversion takes a non-trivial amount of time (though completing raw2trace successfully may still be useful even if we're going to error out later, but it's usually a disappointment to the user if an obvious config error happens so late).
"") | ||
set(tool.multi_indir_rawtemp ON) # no preprocessor | ||
|
||
# Test -multi_indir with 3 copies of our sample dir. |
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.
This second one looks precisely the same as the first one. Error?
Schedule stats tool results: | ||
Total counts: | ||
3 cores | ||
24 threads: .*W2.T1257.* |
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.
Would it be worth checking that T1257 appears three times, one for each workload?
.*W{0,1,2}.T1257.*W{0,1,2}.T1257.*W{0,1,2}.T1257.*
instead?
Would help ensure that the same tid across multiple workloads is indeed treated differently.
And maybe use #cores != #workloads.
@@ -139,8 +139,7 @@ analyzer_multi_t::create_invariant_checker() | |||
// reader(s) for seeking. For now we only read them for this test. | |||
// TODO i#5843: Share this code with scheduler_t or pass in for all | |||
// tools from here for fast skipping in serial and per-cpu modes. | |||
std::string tracedir = | |||
raw2trace_directory_t::tracedir_from_rawdir(op_indir.get_value()); | |||
std::string tracedir = get_input_dir(); |
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.
tracedir_from_rawdir seems to handle certain cases that get_input_dir doesn't. E.g., if op_indir is specified, it is returned directly, whereas previously tracedir_from_rawdir would ensure it ends in '/trace'. Is this a bug?
{ | ||
std::string trace_dir; | ||
if (!op_indir.get_value().empty()) | ||
trace_dir = op_indir.get_value(); |
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.
Add early returns.
Adds a new option -multi_indir to the drmemtrace framework. This allows combining multiple workload traces in core-sharded mode, interleaving threads from all the workloads together.
Updates the documentation.
Adds a test that runs 3 copies of the checked-in threadsig trace.
Issue: #7091