fix(bpf): Fix issue(s) introduced with bpf refactor #1407
Conversation
dave-tucker
force-pushed
the
fix-1402
branch
2 times, most recently
from
ecb4e9a
to
d7b6775
Compare
dave-tucker
changed the title
|
@sthaha @vimalk78 thanks for your review and comments. I took a pass over the code and ensured that everything that needed to be zero'd, was zero'd. Force pushed and updated commit message - I'd appreciate you both taking another look. |
dave-tucker
changed the title
|
@dave-tucker , thanks a lot for cleaning up the code quite a bit 👼 ! |
| @@ -203,7 +204,8 @@ int kepler_sched_switch_trace(struct sched_switch_info *ctx) | |||
| u64 pid_tgid, cgroup_id, cur_ts; | |||
| pid_t cur_pid; | |||
|
|
|||
| struct process_metrics_t *cur_pid_metrics, *prev_pid_metrics, buf; | |||
| struct process_metrics_t *cur_pid_metrics, *prev_pid_metrics; | |||
| struct process_metrics_t buf; | |||
|
|
|||
| if (SAMPLE_RATE > 0) { | |||
There was a problem hiding this comment.
SAMPLE_RATE seems changed from 1 to 5. means once in every 6 task switch we are updating maps, earlier it would be every other task switch. Update in CHANGELOG.md
There was a problem hiding this comment.
may be i missed this in earlier PR, but we were using SEC("tracepoint/sched/sched_switch") some time ago, and switched to SEC("kprobe/finish_task_switch") in this commit , and now we are back to using tracepoint, because API is stable. We can confirm the reason for using kprobes with @rootfs , and capture the reason in CHANGELOG.md
There was a problem hiding this comment.
re: SAMPLE_RATE it's 5 in the code - but can be changed back to 0 if you prefer.
Regardless, at program load time it gets patched here to whatever the user has configured where the default of 0 remains unchanged.
There was a problem hiding this comment.
and re: tracepoints, I got confirmation from one of our kernel devs that the tracepoint and kprobe/finish_task_switch are close enough together that it makes no difference to power measurements.
there was nothing I can see in the patch you linked that indicates that kprobes would be required for any reason, but we can let @rootfs confirm.
|
@dave-tucker , could you please rebase this PR? There are some open comments as well which you may want to address. |
This fixes a few issues identified with the bpf code format and refactor. 1. Zero initialize all variables 2. Use the bpf_perf_event_read_value helper exlusively In addition, add some logging around eBPF array resizing. Fixes: sustainable-computing-io#1402 sustainable-computing-io#1411 Signed-off-by: Dave Tucker <dave@dtucker.co.uk>
Signed-off-by: Dave Tucker <dave@dtucker.co.uk>
There was a problem hiding this comment.
tested, works fine. `/lgtm
1583: hash name processes flags 0x0
key 4B value 112B max_entries 32768 memlock 6034304B
btf_id 812
pids kepler(750518)
1584: hash name pid_time flags 0x0
key 4B value 8B max_entries 32768 memlock 2624352B
btf_id 812
pids kepler(750518)
|
@dave-tucker for 5.4 kernel, bpf_perf_event_read_value has an issue (discussed here) |
This fixes a few issues identified with the bpf code format and refactor.
processesandpid_timemaps. Without this patch entries won't be created in theprocessesmap for pids that are >number of cpu coreswhich fundamentally breaks power reporting.In addition, add some logging around eBPF array resizing.
Fixes: #1402 #1411