| Message ID | 10396de8046ada347d681eb84ea4dc6ec27e1742.1607593250.git.viresh.kumar@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: topology: Avoid the static_branch_{enable|disable} dance | expand |
On Thursday 10 Dec 2020 at 15:12:25 (+0530), Viresh Kumar wrote: > Avoid the static_branch_enable() and static_branch_disable() dance by > redoing the code in a different way. We will be fully invariant here > only if amu_fie_cpus is set with all present CPUs, use that instead of > yet another call to topology_scale_freq_invariant(). > > This also avoids running rest of the routine if we enabled the static > branch, followed by a disable. > > Also make the first call to topology_scale_freq_invariant() just when we > need it, instead of at the top of the routine. This makes it further > clear on why we need it, i.e. just around enabling AMUs use here. > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > --- > arch/arm64/kernel/topology.c | 21 +++++++++------------ > 1 file changed, 9 insertions(+), 12 deletions(-) > > diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c > index 7f7d8de325b6..6dedc6ee91cf 100644 > --- a/arch/arm64/kernel/topology.c > +++ b/arch/arm64/kernel/topology.c > @@ -221,7 +221,7 @@ static DEFINE_STATIC_KEY_FALSE(amu_fie_key); > > static int __init init_amu_fie(void) > { > - bool invariance_status = topology_scale_freq_invariant(); > + bool invariance_status; > cpumask_var_t valid_cpus; > int ret = 0; > int cpu; > @@ -255,18 +255,15 @@ static int __init init_amu_fie(void) > cpumask_equal(valid_cpus, cpu_present_mask)) > cpumask_copy(amu_fie_cpus, cpu_present_mask); > > - if (!cpumask_empty(amu_fie_cpus)) { > - pr_info("CPUs[%*pbl]: counters will be used for FIE.", > - cpumask_pr_args(amu_fie_cpus)); > - static_branch_enable(&amu_fie_key); > - } This check verifies if there are *any* CPUs for which AMUs can be used for FIE (!empty mask) -> enable static key. > + /* Disallow partial use of counters for frequency invariance */ > + if (!cpumask_equal(amu_fie_cpus, cpu_present_mask)) > + goto free_valid_mask; > The replacement verifies if *all* present CPUs support AMUs for FIE and only then it enables the static key. Ionela. > - /* > - * If the system is not fully invariant after AMU init, disable > - * partial use of counters for frequency invariance. > - */ > - if (!topology_scale_freq_invariant()) > - static_branch_disable(&amu_fie_key); > + pr_info("CPUs[%*pbl]: counters will be used for FIE.", > + cpumask_pr_args(amu_fie_cpus)); > + > + invariance_status = topology_scale_freq_invariant(); > + static_branch_enable(&amu_fie_key); > > /* > * Task scheduler behavior depends on frequency invariance support, > -- > 2.25.0.rc1.19.g042ed3e048af >
On 10-12-20, 11:09, Ionela Voinescu wrote: > On Thursday 10 Dec 2020 at 15:12:25 (+0530), Viresh Kumar wrote: > > Avoid the static_branch_enable() and static_branch_disable() dance by > > redoing the code in a different way. We will be fully invariant here > > only if amu_fie_cpus is set with all present CPUs, use that instead of > > yet another call to topology_scale_freq_invariant(). > > > > This also avoids running rest of the routine if we enabled the static > > branch, followed by a disable. > > > > Also make the first call to topology_scale_freq_invariant() just when we > > need it, instead of at the top of the routine. This makes it further > > clear on why we need it, i.e. just around enabling AMUs use here. > > > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > > --- > > arch/arm64/kernel/topology.c | 21 +++++++++------------ > > 1 file changed, 9 insertions(+), 12 deletions(-) > > > > diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c > > index 7f7d8de325b6..6dedc6ee91cf 100644 > > --- a/arch/arm64/kernel/topology.c > > +++ b/arch/arm64/kernel/topology.c > > @@ -221,7 +221,7 @@ static DEFINE_STATIC_KEY_FALSE(amu_fie_key); > > > > static int __init init_amu_fie(void) > > { > > - bool invariance_status = topology_scale_freq_invariant(); > > + bool invariance_status; > > cpumask_var_t valid_cpus; > > int ret = 0; > > int cpu; > > @@ -255,18 +255,15 @@ static int __init init_amu_fie(void) > > cpumask_equal(valid_cpus, cpu_present_mask)) > > cpumask_copy(amu_fie_cpus, cpu_present_mask); > > > > - if (!cpumask_empty(amu_fie_cpus)) { > > - pr_info("CPUs[%*pbl]: counters will be used for FIE.", > > - cpumask_pr_args(amu_fie_cpus)); > > - static_branch_enable(&amu_fie_key); > > - } > > This check verifies if there are *any* CPUs for which AMUs can be used for > FIE (!empty mask) -> enable static key. > > > + /* Disallow partial use of counters for frequency invariance */ > > + if (!cpumask_equal(amu_fie_cpus, cpu_present_mask)) > > + goto free_valid_mask; > > > > The replacement verifies if *all* present CPUs support AMUs for FIE and > only then it enables the static key. > > > - /* > > - * If the system is not fully invariant after AMU init, disable > > - * partial use of counters for frequency invariance. > > - */ > > - if (!topology_scale_freq_invariant()) I mis-read something here, as shared in the other thread, so yeah I need to think again about this patch. > > - static_branch_disable(&amu_fie_key); > > + pr_info("CPUs[%*pbl]: counters will be used for FIE.", > > + cpumask_pr_args(amu_fie_cpus)); > > + > > + invariance_status = topology_scale_freq_invariant(); > > + static_branch_enable(&amu_fie_key); > > > > /* > > * Task scheduler behavior depends on frequency invariance support, > > -- > > 2.25.0.rc1.19.g042ed3e048af > >
diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c index 7f7d8de325b6..6dedc6ee91cf 100644 --- a/arch/arm64/kernel/topology.c +++ b/arch/arm64/kernel/topology.c @@ -221,7 +221,7 @@ static DEFINE_STATIC_KEY_FALSE(amu_fie_key); static int __init init_amu_fie(void) { - bool invariance_status = topology_scale_freq_invariant(); + bool invariance_status; cpumask_var_t valid_cpus; int ret = 0; int cpu; @@ -255,18 +255,15 @@ static int __init init_amu_fie(void) cpumask_equal(valid_cpus, cpu_present_mask)) cpumask_copy(amu_fie_cpus, cpu_present_mask); - if (!cpumask_empty(amu_fie_cpus)) { - pr_info("CPUs[%*pbl]: counters will be used for FIE.", - cpumask_pr_args(amu_fie_cpus)); - static_branch_enable(&amu_fie_key); - } + /* Disallow partial use of counters for frequency invariance */ + if (!cpumask_equal(amu_fie_cpus, cpu_present_mask)) + goto free_valid_mask; - /* - * If the system is not fully invariant after AMU init, disable - * partial use of counters for frequency invariance. - */ - if (!topology_scale_freq_invariant()) - static_branch_disable(&amu_fie_key); + pr_info("CPUs[%*pbl]: counters will be used for FIE.", + cpumask_pr_args(amu_fie_cpus)); + + invariance_status = topology_scale_freq_invariant(); + static_branch_enable(&amu_fie_key); /* * Task scheduler behavior depends on frequency invariance support,
Avoid the static_branch_enable() and static_branch_disable() dance by redoing the code in a different way. We will be fully invariant here only if amu_fie_cpus is set with all present CPUs, use that instead of yet another call to topology_scale_freq_invariant(). This also avoids running rest of the routine if we enabled the static branch, followed by a disable. Also make the first call to topology_scale_freq_invariant() just when we need it, instead of at the top of the routine. This makes it further clear on why we need it, i.e. just around enabling AMUs use here. Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> --- arch/arm64/kernel/topology.c | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-)