Repository navigation
add bpftrace scripts to track performance and behavior of SOF functions in the kernel - #877
Conversation
|
added the 'kernel maintainer' team for feedback. |
There was a problem hiding this comment.
Can you rename the directory to something more specific, maybe sof-test/linux/tracing/ or sof-test/ktracing? We have a lot of "tracing" already and sof-test/tracing/ sounds definitely like firmware tracing.
Can you add a README with some pointers, context/purpose/intro/etc. or if too short for a README then some comments in this new .bt file? For now it's very... "raw".
|
Just added a readme and updated the directory structure to make it more clear what this is. |
it's better to squash all the commits together and do a git push --force to have a single commit, no? |
@nklayman has just used Github the way it was designed to be used, not the way we use it. Github tolerates but does not really support rewriting history and force pushes. Github is optimized for a single commit per pull request + fixup commits just like @nklayman did. For instance try the "View changes" button above: this time it works! Same for many other Github features. More info at zephyrproject-rtos/zephyr#39194 Note Github has an option to squash all commits right before merging. Not sure what is the final commit message then. |
|
I prefer to push individually and then squash into one commit before merging, either manually or through the Github UI. Otherwise, it's a lot harder to tell what's changed and see what code review comments refer to. I can do the single commit method if you'd prefer, but as long as we squash before merging the end result is the same, and the process to get there is a bit better. Edit: Github asks you to choose the final commit message/description when you squash and merge. |
|
As long as the goal is to have a single commit in the end, I totally agree the process is much better and the end result is the same. It is the way Github is designed to be used after all! The problem is purely about consistency: as Github PRs do not support reviewing multiple commits "natively", this means learning how to use Github TWICE, in two different ways. One for reviewing a single commit (in the end), and a second workflow for reviewing multiple commits (in the end). I don't mind at all but I've been the "local git guru" in many teams for many years so I'm not representative. I've seen many, many people struggling with ONE way to use git or Github. I've also seen some totally blank stares from people very experienced with one way when trying to explain them the other way. |
nklayman
changed the title
marc-hb
left a comment
marc-hb
left a comment
There was a problem hiding this comment.
Don't forget to run pylint now and then. It's not gospel but it's pretty good.
| "@avg[kretprobe:sof_resume, runtime] < 150000", | ||
| "@avg[kretprobe:sof_resume, system] < 200000", | ||
| "@avg[kretprobe:sof_suspend, runtime] < 50000", | ||
| "@avg[kretprobe:sof_suspend, system] < 1000" |
There was a problem hiding this comment.
Does @avg mean "average"? Averages hide intermittent bugs, they're not very interesting.
https://bravenewgeek.com/everything-you-know-about-latency-is-wrong/
There was a problem hiding this comment.
Yes, it means average. However, it's purely a demo (I just made the numbers up), so it doesn't matter much. I figured it'd be better to let those more familiar with SOF figure out the real metrics/conditions to use. I assume we'll end up with some sort of script to generate test cases with device-specific conditions and tests anyways. I've renamed the spec file to sample_spec.json to make it more clear that it's just a demo.
| # Check if line is outputting a variable from the script | ||
| # bpftrace output format is `@name[key1, key2, ...] = val` (keys are optional) | ||
| if re.match("@.*: .*", line): | ||
| key, val = line.split(": ", 1) |
There was a problem hiding this comment.
So an entire @name[key1, key2, ...] is a valid "key"? That's quite confusing. Could one of them not be called "key"?
There was a problem hiding this comment.
name would be the name of the map, which can be indexed by any number of keys. ie @max_time[ipc, stream] = 500 would be the map max_time indexed by two keys, in this case the value 500 is tied to keys ipc and stream. It's essentially a flattened nested map.
f2c6315 to
94434fd
Compare
|
@marc-hb all your comments should be addressed in the latest update. I switched it over to the one-condition-per-test format that you prefer, and applied the small changes. @plbossart I also squashed the commits together into two as you requested, one for the bpftrace scripts themselves and one for the metric evaluation part. Marking this "ready for review" now as it's at a good state for others to look at. The metric evaluation script will most likely need some changes to make it work in the CI, but that's probably a better task for the CI devs rather than myself. |
marc-hb
left a comment
marc-hb
left a comment
There was a problem hiding this comment.
I think the high-level design looks great: concise, scalable and extremely promising.
On the other hand I have a number of readability concerns and when trying it I found some usability issues.
I tried to run test.py as a regular, non-root user and I got this very clear and fatal looking error but then test.py hangs forever:
collecting ... ERROR: bpftrace currently only supports running as the root user.
Similarly, I tried to run as root but without the required CONFIG_ and again I got a very clear and fatal looking error and test.py hangs forever:
collecting ... modprobe: FATAL: Module kheaders not found in directory /lib/modules/9.9.9-rc9softest
BTW would it be possible to have a sample that does nothing at all and can be run on any Linux system with very minimal CONFIG_ dependency? For the sole purpose of testing the framework, the user interface, relative directory issues, etc.
Also, some sample output in the commit message or even in the README would be great.
marc-hb
left a comment
marc-hb
left a comment
There was a problem hiding this comment.
$ pylint kernel_tracing/metric_evaluation/test.py
************* Module test
kernel_tracing/metric_evaluation/test.py:15:4: C0103: Variable name "conditionWithVars" doesn't conform to snake_case naming style (invalid-name)
kernel_tracing/metric_evaluation/test.py:17:8: C0103: Variable name "conditionWithVars" doesn't conform to snake_case naming style (invalid-name)
|
@marc-hb Thanks for the review! Almost all your comments should be addressed now. The no-dependency test option is a bit tricky because the script can't really work without bpftrace. If you're okay with having that a dependency I could make a very simple case, otherwise I could try aliasing Also, I'm realising this script probably requires a readme of its own. I'll add one when I get the chance, probably this weekend. |
The dependency on bpftrace user space is not a problem because it's as simple as (I've not looked at the new version yet) |
marc-hb
left a comment
marc-hb
left a comment
There was a problem hiding this comment.
I have not tried to run it again but I find the entire code very clear and easy to read and none of this is running in production yet so bug fixes and incremental improvements can be done later: I recommend we merge this to make it easier for others to try.
@plbossart could you have another look, especially the .bt files that I'm not familiar with?
marc-hb
left a comment
marc-hb
left a comment
There was a problem hiding this comment.
Some review comments from a generous pytest expert friend who prefers to stay anonymous and not involved:
i don't know bpftrace but it looks a little bit like you might want to be using a parameterized fixture to run the test cases instead.
simple example usage: https://github.com/zephyrproject-rtos/west/blob/a343b7a7982355d0e5521ddcc69f700940d89e50/tests/test_manifest.py#L2885
with a parameterized fixture, you'll get finer-grained reporting and can customize pytest's notions of what each test is. as-is, it looks like you'll get a global pass/fail on a single function
I think we have parametrization already but maybe @pytest.mark.parametrize is more "pythonic", flexible and generally better way to do it. Again this can be a future improvement after merge.
I just remembered reading somewhere complaining that |
|
@marc-hb latest push includes a |
|
Nice try but: Also, not running as root still hangs: BTW only bpftrace should run as root, should be started as Again all this can be fixed later. |
331cb01 to
0472e88
Compare
Adds scripts to track sof_resume/suspend and ipc tx time, and count hda irq interrupt sources and calculate timestamp deltas per-source. Signed-off-by: Noah Klayman <noah.klayman@intel.com>
Takes a test specification from a JSON file, which contains a bpftrace script, shell commands, and test conditions. It runs the bpftrace script while executing shell commands, and collects all variables from bpftrace. These are then injected into the conditions which are evaluated with python's eval. Case execution and output formatting is handled by pytest. Signed-off-by: Noah Klayman <noah.klayman@intel.com>
|
BPF script changes have been moved to the first patch and |
marc-hb
left a comment
marc-hb
left a comment
There was a problem hiding this comment.
Great start, very clear and readable. None of this is used in production yet so bugs can always be fixed later.
Signed-off-by: Noah Klayman noah.klayman@intel.com
This PR adds some example bpftrace scripts to track performance and behavior of various SOF functions in the kernel. To run them, you'll need to install bpftrace, then run the script with
bpftrace [path to script]. If you want to use the hda_irq scripts, you'll need to be running this kernel pr.Example output from
suspend_resume_time.bt:Additionally, this PR includes a Python script to collect metrics and run conditions on them, see
kernel_tracing/metric_evaluation. It's intended to be used on the CI machines to prevent performance regressions.