layered: cleanup skip-preempt #1843
Conversation
| if (rq && (rq->curr->sched_class != ext_sched_class) && | ||
| (rq->curr->sched_class != idle_sched_class)) { | ||
| if (!(cpuc = lookup_cpu_ctx(-1))) | ||
| if (rq && (rq->curr->sched_class != ext_sched_class) && |
There was a problem hiding this comment.
Can skip the enable_skip_preempt variable and roll the outer loop into the inner one by writing (ext_sched_class_addr != NULL), but it's just for terseness.
There was a problem hiding this comment.
ahh that makes sense, thx.
There was a problem hiding this comment.
Still works wrt/ doing the right thing when kallsyms is present, does the C look about right (I think it is/that one can cast null like that fine)?
There was a problem hiding this comment.
The C does look right, I assume the verifier has no issue with using the addr as a scalar right? Other than that yeah, the address is in the BSS so should be init'ed to 0, and will always be non-zero when both symbols are read.
| struct rq *rq = NULL; | ||
| s32 sib; | ||
| struct sched_class *ext_sched_class, *idle_sched_class; | ||
| struct sched_class *ext_sched_class = NULL, *idle_sched_class = NULL; |
There was a problem hiding this comment.
I don't think you need this. The variables are unconditionally set below, right?
scheds/rust/scx_layered/src/main.rs
Outdated
| skel.maps.rodata_data.ext_sched_class_addr = ext_sched_class_addr.unwrap(); | ||
| skel.maps.rodata_data.idle_sched_class_addr = idle_sched_class_addr.unwrap(); | ||
| } else { | ||
| warn!("skip_preempt is not supported, ignoring"); |
There was a problem hiding this comment.
Maybe mention that the addresses couldn't be read from /proc/kallsyms so that users can hunt down what's not working more easily?
There was a problem hiding this comment.
I'll add this, but also like, do we think these symbols could be exported kernel side such that we don't need to do this in the future or no?
There was a problem hiding this comment.
It will probably remain this way.
There was a problem hiding this comment.
will make this into a helper in common then, thanks!
There was a problem hiding this comment.
Oh, let's keep it in layered for now. We can move it to utils when there are other users.
|
Followup to this: If we can check the current running task for the rq instead of the rq itself, #1844 solves this problem a bit more idiomatically. |
Just to make sure I follow -- more/less, store acquired/released status on cpuc, toggle it with those two callbacks, and use that instead of checking the rq? |
Exactly, it requires the use of the two extra scx methods. As long as there is a kfunc for grabbing the current running task for a cpu, that should work (not sure whether that exists given that it sounds inherently racy, i.e. the function would be returning a task that may be switched out immediately after being returned). |
3 participants
Have layered only attempt to skip preemption's that it could not accomplish (i.e. due to tasks with RT sched class) when kallsyms addresses for idle sched class and ext sched class are present.
At the present, layered would exit if those addresses were unobtainable.
still works when kallsyms present for both the skip and not-skip cases:
