scx_cosmos: use PMU library for perf event handling#3220
scx_cosmos: use PMU library for perf event handling#3220arighi merged 2 commits intosched-ext:mainfrom
Conversation
arighi
left a comment
arighi
left a comment
There was a problem hiding this comment.
Thanks @etsal for working at this! I haven't looked at the changes in details, but something is not working for me with this changes (I just tested it on my AMD laptop, where 0xc3 == branch misses) and I get this:
- before:
23:43:00 [INFO] scheduler options: ./target/release/scx_cosmos -d -c 0 -p 0 -s 20000 -e 0xc3 -E 10 --stats 1
23:43:00 [INFO] scheduler flags: 0x36
23:43:01 [INFO] Setting up performance counters for 24 CPUs...
23:43:01 [INFO] Performance counters configured successfully for all CPUs
[scx_cosmos] CPUs 0.0% [deadline] ev_dispatches=227
[scx_cosmos] CPUs 0.0% [deadline] ev_dispatches=986
[scx_cosmos] CPUs 0.0% [deadline] ev_dispatches=494
...
- after:
23:41:57 [INFO] scheduler options: ./target/release/scx_cosmos -d -c 0 -p 0 -s 20000 -e 0xc3 -E 10 --stats 1
23:41:57 [INFO] scheduler flags: 0x36
23:41:57 [INFO] Setting up performance counters for 24 CPUs...
23:41:57 [INFO] Performance counters configured successfully for all CPUs
[scx_cosmos] CPUs 0.0% [deadline] ev_dispatches=0
[scx_cosmos] CPUs 0.0% [deadline] ev_dispatches=0
[scx_cosmos] CPUs 0.0% [deadline] ev_dispatches=0
...
It looks like the stats are not read correctly? I'll investigate more tomorrow morning.
|
Hi Andrea, sorry about that! I will do some debugging too and update accordingly. |
|
Ok, should work now: There were two errors, a) no setup/teardown for per-task counters on task_init/task_exit, and b) the code was reading the PMU after it was cleared each time we computed the delta, always returning 0. Both are fixed. |
arighi
left a comment
arighi
left a comment
There was a problem hiding this comment.
I left a small comment, but overall looks good now, thanks!
| * Return true if the task is triggering too many PMU events. | ||
| */ | ||
| static inline bool is_event_heavy(const struct task_ctx *tctx) | ||
| static inline bool is_event_heavy(struct task_struct *p) |
There was a problem hiding this comment.
I'd prefer to pass tctx to this function to potentially save some unnecessary calls to try_lookup_task_ctx(). The compiler might optimize this away, but passing tctx explicitly guarantees we don't incur the extra lookup. And if the verifier isn't happy we an still check if tctx == NULL inside the function and return.
There was a problem hiding this comment.
Sorry for the late reply! Sounds good, will update.
arighi
force-pushed
the
cosmos-pmu-lib
branch
from
ce170e8 to
df21501
Compare
|
Hi @etsal I've udpated your branch with the changes that I suggested, can you check if it still looks good? In that case we can merge this. Thanks! |
|
I did some tests with this and it looks good, so let's merge it. |
2 participants
The perf_event tracking logic in scx_cosmos and that of the pmu library are relatively similar and can be merged. Expand the PMU library API, and use the newly exposed functionality in scx_cosmos to handle PMU events.