-
Notifications
You must be signed in to change notification settings - Fork 183
scx_mitosis: Use RAII style for releasing resources #2798
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
Much of the logic was pretty confusing with many gotos and various release_cpumask/group/rcu lock, etc. This creates a few macros for common types and uses the gcc/clang __attribute__(cleanup, ...) to do the proper destruction of the resource once the variable goes out of scope.
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.
This is beautiful. Is it worth putting some of these in a common header?
Can we copy the guard() macros from the kernel and use them? They're pretty much the same things and it'd be nice to have some commonality. |
I wasn't aware of the guard() macros, let me look those up |
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.
I think this is much easier to understand than gotos.
cpu = bpf_cpumask_any_distribute(cast_mask(tctx->cpumask)); | ||
else | ||
cpu = prev_cpu; | ||
return bpf_cpumask_any_distribute(cast_mask(tctx->cpumask)); |
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] I know the tctx->cpumask
part of the condition above this line is redundant with the check before it & probably just there for the verifier. But it still looks weird to see the check in that order bc the short circuit implications of:
!bpf_cpumask_test_cpu(prev_cpu, cast_mask(tctx->cpumask)) && tctx->cpumask
vs
tctx->cpumask && !bpf_cpumask_test_cpu(prev_cpu, cast_mask(tctx->cpumask))
s32 cpu; | ||
DECLARE_IDLE_CPUMASK(idle_smtmask) = scx_bpf_get_idle_smtmask(); | ||
|
||
if (!(task_cpumask = (struct cpumask *)tctx->cpumask) || |
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.
Know this is outside the diff, but the bpf_cpumask is larger than the task_cpumask. I think we'll be fine using it as a pointer as long as we never e.g. copy size of bpf_cpumask into a cpumask
https://elixir.bootlin.com/linux/v6.16.8/source/kernel/bpf/cpumask.c#L25
Much of the logic was pretty confusing with many gotos and various release_cpumask/group/rcu lock, etc. This creates a few macros for common types and uses the gcc/clang attribute(cleanup, ...) to do the proper destruction of the resource once the variable goes out of scope.