scx_rusty: use arena-based CPU masks #1293
Conversation
| /* XXX HACK hardcoded MAX_CPUS / 8 from src/bpf/intf.h, right now this header is a mess */ | ||
| struct scx_cpumask { | ||
| union sdt_id tid; | ||
| u8 mask[64]; |
There was a problem hiding this comment.
Maybe use u64's? BPF is a lot more efficient with 64bit operations. With smaller types, it'd be generating a lot of masking and shifting instructions.
There was a problem hiding this comment.
For sure, cpumask_t also uses an array of longs for the mask so this would be consistent with the kernel bitmasks.
| cast_kern(data); | ||
|
|
||
| mask = (scx_cpumask_t)data->payload; | ||
| mask->tid = data->tid; |
There was a problem hiding this comment.
It's fine for now but I wonder whether allocation ID linking can be hidden inside sdt_alloc() down the line. It's a bit weird because sdt originally meant scx data for task and the tid is unique task ID. The names and explicit tid handling make sense for task data allocations but get confusing when generalized like this. Something to clean up down the line.
There was a problem hiding this comment.
The tid is definitely a misnomer, because what we really care about is the idx of the allocation within the radix tree.
The reason the tid is embedded in the struct is to avoid having a separate KV map of arena pointers to idx'es. The sdt_task allocator must be able to match a struct proc to the arena allocation we've made for it, and that map also holds the TID in the value. For cpumasks we don't need that kind of map if we embed the tid in the struct.
The type-specific code in cpumask.h doesn't do anything with the TID, it just stores it. We can make a wrapper struct for the cpumasks to remove the tid from the actual struct we give out during allocations and completely hide tids from outside the allocator code.
There was a problem hiding this comment.
Yeah, later, we can create an allocation wrapper which puts the ID after the actual allocation (to not disturb alignment requirement) so that the detail can be hidden from allocator users.
|
|
||
|
|
||
| __weak | ||
| int scxmask_set_cpu(u32 cpu, scx_cpumask_t __arg_arena mask) |
There was a problem hiding this comment.
There are three prefixes in use - scx_cpumask, scx_mask and scxmask. If this can be generalized in terms of number of bits (which we do need), maybe use scx_bitmap? In kernel, cpumasks are just wrapping bitmaps too.
There was a problem hiding this comment.
I will do a rename pass to scx_bitmap, I used scxmask for function prefixes for brevity as opposed to scx_cpumask but that may have been more confusing than helpful.
| int i; | ||
|
|
||
| bpf_for(i, 0, nr_cpu_ids) { | ||
| if (!scxmask_test_cpu(i, big) && bpf_cpumask_test_cpu(i, small)) |
There was a problem hiding this comment.
This can be done word by word, right? ~mask[i] & mask[j]
There was a problem hiding this comment.
These are different mask types and one of them is a kptr that we cannot dereference directly (there may be a way to do that with a kernel patch though? IIRC we can direclty read some fields from task_struct. Looking into it rn)
There was a problem hiding this comment.
Ah, can't we convert them and then just operate on the scx bitmasks?
There was a problem hiding this comment.
We can, I will adjust these accordingly.
| int i; | ||
|
|
||
| bpf_for(i, 0, nr_cpu_ids) { | ||
| if (scxmask_test_cpu(i, scx) && bpf_cpumask_test_cpu(i, cpu)) |
There was a problem hiding this comment.
(Same as above) These are comparisons between a const struct cpumask * and an scxmask so we can't access the cpumask's bitmap directly AFAICT.
| int i; | ||
|
|
||
| bpf_for(i, 0, nr_cpu_ids) { | ||
| if (scxmask_test_cpu(i, scx) && bpf_cpumask_test_cpu(i, cpu)) |
The scx_rusty scheduler currently represents domains and tasks with arena-based structs, but cannot embed cpumasks into these fields as they are represented as kptrs. Introduce a cpumask type that is implemented as a BPF library and so can be embedded/pointed to by arena structs. Also add wrappers around kfuncs that take struct bpf_cpumask * arguments to transparently translate between our new internal cpumask type and the bpf_cpumask type expected by the calls.
Note: This code currently has really suboptimal bpf_cpumask to scxmask conversions that I am in the process of fixing. We'll need a kfunc for that but the rest of the code compiles and verifies just fine so it's a matter of solving this one isolated issue.