Skip to content
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

lisa._assets.kmodules.lisa: Don't rely on rq->cpu_capacity_orig anymore #2145

Conversation

deggeman
Copy link
Contributor

The kernel commit 7bc263840bc3 ("sched/topology: Consolidate and clean up access to a CPU's max compute") removed rq->cpu_capacity_orig so we can't use it anymore to retrieve the original CPU capacity nor for setting the FREQ_INVARIANCE kernel feature in LISA.

Replace rq->cpu_capacity_orig with arch_scale_cpu_capacity(cpu) in Lisa trace module's rq_cpu_orig_capacity() and remove the HAS_MEMBER(struct, rq, cpu_capacity_orig) check from the FREQ_INVARIANCE test.

Since arch_scale_cpu_capacity(cpu) exists in the kernel for a long time we don't have to take care of activating this only for newer Linux kernel versions.

@deggeman
Copy link
Contributor Author

Related to PWRSOFT-5115

@deggeman
Copy link
Contributor Author

deggeman commented Nov 28, 2023

The kernel commit 7bc263840bc3 ("sched/topology: Consolidate and clean
up access to a CPU's max compute") removed rq->cpu_capacity_orig so we
can't use it anymore to retrieve the original CPU capacity nor for
setting the FREQ_INVARIANCE kernel feature in LISA.

Replace rq->cpu_capacity_orig with arch_scale_cpu_capacity(cpu) in Lisa
trace module's rq_cpu_orig_capacity() and remove the HAS_MEMBER(struct,
rq, cpu_capacity_orig) check from the FREQ_INVARIANCE test.

Pull request: #2145

@credp
Copy link
Contributor

credp commented Nov 28, 2023

We probably need to figure out how to make this work for pre and post-patch kernels in order to complete this.

@deggeman
Copy link
Contributor Author

Fortunately not in this case:

I use arch_scale_cpu_capacity(cpu) now instead as accessing rq->cpu_capacity_orig directly.

Since arch_scale_cpu_capacity(cpu) exists in the kernel for a long time we don't have to take care of activating this only for newer Linux kernel versions.

@@ -218,7 +218,7 @@ static inline int rq_cpu_orig_capacity(const struct rq *rq)
{
return
# if HAS_KERNEL_FEATURE(FREQ_INVARIANCE)
rq->cpu_capacity_orig;
arch_scale_cpu_capacity(cpu_of(rq))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using arch_scale_cpu_capacity() is problematic, as this is not an exported function (or at least not always). Instead it's a macro that can point at anything, which means that instead of being able to check for the availability of a single exported function, we need to check for the availability of all its dependencies. That would be bad enough but on top of that, the implementation depends on the architecture.

So AFAIK we only have two choices:

  1. We implement getters for this sort of info in the scheduler code as non-inline functions, ideally exported
  2. We copy-paste the inner implementation for arm64 like we have for rq_cpu_current_capacity() (sched_helpers.h) and guard with appropriate checks for the dependencies of that implementations, while crossing fingers that a) no-one will try to use it on an arch that does it differently and b) that the upstream kernel will not suddenly change the meaning of the variables/functions it depends on.

Since arch_scale_cpu_capacity(cpu) exists in the kernel for a long time we don't have to take care of activating this only for newer Linux kernel versions.

Yes but the macro being there for internal use is sadly not enough, we also need all the symbols it relies on to be available, which will break with Android kernels.

Also unfortunately since we need to be able to change the CPU capacities at runtime, we cannot just read the value from e.g. sysfs or debugfs and store it locally inside the module, it needs to be read every time the event is emitted and reading sysfs like that is not something we want to do on a hot path.

The kernel commit 7bc263840bc3 ("sched/topology: Consolidate and clean
up access to a CPU's max compute") removed rq->cpu_capacity_orig so we
can't use it anymore to retrieve the original CPU capacity nor for
setting the FREQ_INVARIANCE kernel feature in LISA.

Split the micro-architectural part of the Invariance from the
FREQ_INVARIANCE into its own kernel feature UARCH_INVARIANCE.

Use a copy of the Arm64 (+ Arm, Riscv) kernel implementation (per-cpu
cpu_scale variable in drivers/base/arch_topology.c) also in the lisa
trace module code.

Tested on Juno-r0 with 'lisa-load-kmod --conf target_conf.yml' by
evaluating the 'capacity_orig' field of the 'lisa__sched_cpu_capacity'
trace event.

Signed-off-by: Dietmar Eggemann <[email protected]>
@deggeman deggeman force-pushed the fix_rqinv_test_load_correctness branch from 3eaa854 to 39d9b13 Compare December 19, 2023 10:21
@deggeman
Copy link
Contributor Author

I updated the fix by implementing choice 1.

@douglas-raillard-arm douglas-raillard-arm merged commit 32322b3 into ARM-software:main Dec 19, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants