-
Notifications
You must be signed in to change notification settings - Fork 195
Use scx_bpf_cpu_curr() instead of deprecated scx_bpf_cpu_rq() #2734
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
base: main
Are you sure you want to change the base?
Conversation
htejun
left a comment
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.
Oh, wait, you need to port the compat macro too.
| static inline bool is_cpu_idle(s32 cpu) | ||
| { | ||
| struct rq *rq = scx_bpf_cpu_rq(cpu); | ||
| struct task_struct *p = __COMPAT_scx_bpf_cpu_curr(cpu); |
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.
We need to add __COMPAT_scx_bpf_cpu_curr() https://git.kernel.org/pub/scm/linux/kernel/git/tj/sched_ext.git/tree/tools/sched_ext/include/scx/compat.bpf.h?h=for-6.18#n238 to scheds/include/scx/compat.bpf.h or this won't build.
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.
Ah I thought these were getting pulled in from the kernel source (at some point)?
Do we update both compat.bpf.h and common.h separately for kernel and scx repo?
Edit: Nevermind, I misunderstood the process, I've added the two commits (definitions and Andrea's compat helper) here now, i.e. this PR compiles and runs self-sufficient if the kernel includes the mentioned series.
| return false; | ||
| } | ||
| return rq->curr->flags & PF_IDLE; | ||
| return !!(p->flags & PF_IDLE); |
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.
nit: we probably don't need !! here, since the result would be cast to bool anyway.
8d41e53 to
11bb34c
Compare
|
Overall LGTM. |
11bb34c to
9deff4b
Compare
arighi
left a comment
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.
Hm... I'm a bit confused.
We have:
BTF_ID_FLAGS(func, scx_bpf_cpu_curr, KF_RET_NULL | KF_RCU)
I thought the verifier would complain if we try to use scx_bpf_cpu_curr() outside of a bpf_rcu_read_lock/unlock() section, which is exactly what we're doing here. But the verifier is still happy and the kernel is (correctly) complaining:
[ 5.938735] =============================
[ 5.939878] WARNING: suspicious RCU usage
[ 5.941002] sched_ext: BPF scheduler "cosmos_1.0.2_gd0e71ca_x86_64_unknown_linux_gnu_debug" enabled
[ 5.943254] 6.17.0-rc1 #1-NixOS Not tainted
[ 5.944477] -----------------------------
[ 5.945636] kernel/sched/ext.c:6415 suspicious rcu_dereference_check() usage!
So we can fix this PR adding the proper bpf_rcu_read_lock/unlock() around __COMPAT_scx_bpf_cpu_curr(), but I'd like to understand why the verifier is happy.
@etsal do you have any idea? Thanks.
multics69
left a comment
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.
Thanks! It looks good to me.
eb700fc to
5cd7c5f
Compare
|
Hi @cloehle we have all the required pieces upstream now, so I think we should definitely merge this, if you have time can you update this PR and check if everything is still up to date? Thanks! |
5cd7c5f to
9de9b92
Compare
|
Andrea, the updated PR should be correct now, although for the compat layer to actually work (detect scx_bpf_cpu_curr()) we need the new |
@etsal was looking at updating the vmlinux files, maybe sync with him? |
Yeah I'll be regenerating the vmlinux.h to include up to 6.18, will push the diff today or tomorrow. |
|
Any update on this @etsal ? I tested the schedulers I've touched, although some of them never seemed to call scx_bpf_cpu_rq() / __COMPAT_scx_bpf_cpu_curr() in my testing! |
Sorry for the delay, updating right now, |
Hitting an issue with our scripts not properly generating vmlinux.h for the arm target on Fedora, I will just upload the rest of the archs for now. |
The new functions provide a safer way to access a rq or a remote
rq->curr respectively. They are both part of v6.18.
See kernel commits:
e0ca169638be ("sched_ext: Introduce scx_bpf_locked_rq()")
20b158094a1a ("sched_ext: Introduce scx_bpf_cpu_curr()")
Signed-off-by: Christian Loehle <[email protected]>
Introduce a compatibility helper that allows BPF schedulers to use
scx_bpf_cpu_curr() on older kernels.
See kernel commit
20b158094a1a ("sched_ext: Introduce scx_bpf_cpu_curr()")
Signed-off-by: Andrea Righi <[email protected]>
Signed-off-by: Christian Loehle <[email protected]>
9de9b92 to
e3e914b
Compare
|
Thank you Emil! |
|
#2734 (comment) still isn't addressed or replied. |
|
@mati865 Oops, sorry about that, I'll fix it right away. |
Use the new scx_bpf_cpu_curr() introduced in v6.18 as a safer way to access rq->curr instead of the deprecated scx_bpf_cpu_rq(). Signed-off-by: Christian Loehle <[email protected]>
Use the new scx_bpf_cpu_curr() introduced in v6.18 as a safer way to access rq->curr instead of the deprecated scx_bpf_cpu_rq(). Signed-off-by: Christian Loehle <[email protected]>
Use the new scx_bpf_cpu_curr() introduced in v6.18 as a safer way to access rq->curr instead of the deprecated scx_bpf_cpu_rq(). Signed-off-by: Christian Loehle <[email protected]>
Use the new scx_bpf_cpu_curr() introduced in v6.18 as a safer way to access rq->curr instead of the deprecated scx_bpf_cpu_rq(). Signed-off-by: Christian Loehle <[email protected]>
Use the new scx_bpf_cpu_curr() introduced in v6.18 as a safer way to access rq->curr instead of the deprecated scx_bpf_cpu_rq(). Signed-off-by: Christian Loehle <[email protected]>
e3e914b to
6a9363a
Compare
|
Gentle ping on this! |
arighi
left a comment
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.
For my stuff this looks good, I think we can merge it.
@multics69 @hodgesds what do you think about the changes to lavd and layered?
The new scx_bpf_cpu_curr() is queued for v6.18.
Use them instead of the soon deprecated scx_bpf_cpu_rq().
Error handling (on NULL) is kept as is for the schedulers
This needs the queued:
https://lore.kernel.org/lkml/[email protected]/