| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20241023101948.GH16066@noisy.programming.kicks-ass.net>
Date: Wed, 23 Oct 2024 12:19:48 +0200
From: Peter Zijlstra <peterz@...radead.org>
To: Christian Loehle <christian.loehle@....com>
Cc: mpe@...erman.id.au, Thomas Gleixner <tglx@...utronix.de>,
x86@...nel.org, Vincent Guittot <vincent.guittot@...aro.org>,
Ingo Molnar <mingo@...hat.com>,
Dietmar Eggemann <dietmar.eggemann@....com>,
Pierre Gondois <pierre.gondois@....com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] sched: Make ASYM_PACKING compile-time arch config
On Thu, Oct 17, 2024 at 10:46:49PM +0100, Christian Loehle wrote:
> Only x86 and Power7 set SD_ASYM_PACKING at boot-time depending on the
> system. All other platforms don't make use of asym-packing anyway,
> so introduce auxiliary ARCH_HAS_SCHED_ASYM_PACKING and guard all the
> related logic behind that so it isn't compiled when not needed.
>
> On arm64 this reduces
> size kernel/sched/fair.o
> text data bss dec hex filename
> 74120 4097 88 78305 131e1 kernel/sched/fair.o
> to
> size kernel/sched/fair.o
> text data bss dec hex filename
> 72896 4065 88 77049 12cf9 kernel/sched/fair.o
>
> Most of that is on the load-balance hot-path, in particular
> need_active_balance() reduces from 141 to 84 instructions.
>
> hackbench -pTl 20000 on a rk3399 goes from
> 58.4664 to 57.6056 (-1.5%), mean over 20 iterations.
*sigh* more ifdef is the very last thing we need :/ What's the error on
that measurement? Is it statistically relevant etc.
> @@ -9186,12 +9179,14 @@ enum group_type {
> * a task on SMT with busy sibling to another CPU on idle core.
> */
> group_smt_balance,
> +#ifdef CONFIG_ARCH_HAS_SCHED_ASYM_PACKING
> /*
> * SD_ASYM_PACKING only: One local CPU with higher capacity is available,
> * and the task should be migrated to it instead of running on the
> * current CPU.
> */
> group_asym_packing,
> +#endif
Do we really need to remove the value from the enum !?
> /*
> * The tasks' affinity constraints previously prevented the scheduler
> * from balancing the load across the system.
> @@ -9876,7 +9871,9 @@ struct sg_lb_stats {
> unsigned int idle_cpus; /* Nr of idle CPUs in the group */
> unsigned int group_weight;
> enum group_type group_type;
> +#ifdef CONFIG_ARCH_HAS_SCHED_ASYM_PACKING
> unsigned int group_asym_packing; /* Tasks should be moved to preferred CPU */
> +#endif
just leave it be, who cares if it goes unused?
> unsigned int group_smt_balance; /* Task on busy SMT be moved */
> unsigned long group_misfit_task_load; /* A CPU has a task too big for its capacity */
> #ifdef CONFIG_NUMA_BALANCING
> @@ -10136,8 +10133,10 @@ group_type group_classify(unsigned int imbalance_pct,
> if (sg_imbalanced(group))
> return group_imbalanced;
>
> +#ifdef CONFIG_ARCH_HAS_SCHED_ASYM_PACKING
> if (sgs->group_asym_packing)
> return group_asym_packing;
> +#endif
Add a helper that returns false such that the compiler can DCE it?
> @@ -10360,10 +10402,12 @@ static inline void update_sg_lb_stats(struct lb_env *env,
>
> sgs->group_weight = group->group_weight;
>
> +#ifdef CONFIG_ARCH_HAS_SCHED_ASYM_PACKING
> /* Check if dst CPU is idle and preferred to this group */
> if (!local_group && env->idle && sgs->sum_h_nr_running &&
> sched_group_asym(env, sgs, group))
> sgs->group_asym_packing = 1;
> +#endif
Just make sure sched_group_asym() is unconditionally false and the
compiler will DCE it, no?
>
> /* Check for loaded SMT group to be balanced to dst CPU */
> if (!local_group && smt_balance(env, sgs, group))
> @@ -10436,9 +10480,11 @@ static bool update_sd_pick_busiest(struct lb_env *env,
> */
> return false;
>
> +#ifdef CONFIG_ARCH_HAS_SCHED_ASYM_PACKING
> case group_asym_packing:
> /* Prefer to move from lowest priority CPU's work */
> return sched_asym_prefer(sds->busiest->asym_prefer_cpu, sg->asym_prefer_cpu);
> +#endif
Just leave it be, it'll never get selected.
> case group_misfit_task:
> /*
> @@ -10691,7 +10737,9 @@ static bool update_pick_idlest(struct sched_group *idlest,
> break;
>
> case group_imbalanced:
> +#ifdef CONFIG_ARCH_HAS_SCHED_ASYM_PACKING
> case group_asym_packing:
> +#endif
idem
> case group_smt_balance:
> /* Those types are not used in the slow wakeup path */
> return false;
> @@ -10823,7 +10871,9 @@ sched_balance_find_dst_group(struct sched_domain *sd, struct task_struct *p, int
> break;
>
> case group_imbalanced:
> +#ifdef CONFIG_ARCH_HAS_SCHED_ASYM_PACKING
> case group_asym_packing:
> +#endif
and again.
> case group_smt_balance:
> /* Those type are not used in the slow wakeup path */
> return NULL;
> @@ -11058,7 +11108,7 @@ static inline void calculate_imbalance(struct lb_env *env, struct sd_lb_stats *s
> return;
> }
>
> - if (busiest->group_type == group_asym_packing) {
> + if (check_asym_packing(busiest)) {
> /*
> * In case of asym capacity, we will try to migrate all load to
> * the preferred CPU.
> @@ -11265,7 +11315,7 @@ static struct sched_group *sched_balance_find_src_group(struct lb_env *env)
> goto out_balanced;
>
> /* ASYM feature bypasses nice load balance check */
> - if (busiest->group_type == group_asym_packing)
> + if (check_asym_packing(busiest))
> goto force_balance;
>
> /*
> diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
> index b1c3588a8f00..51d49700f643 100644
> --- a/kernel/sched/sched.h
> +++ b/kernel/sched/sched.h
> @@ -937,11 +937,17 @@ static inline long se_weight(struct sched_entity *se)
> return scale_load_down(se->load.weight);
> }
>
> -
> +#ifdef CONFIG_ARCH_HAS_SCHED_ASYM_PACKING
> static inline bool sched_asym_prefer(int a, int b)
> {
> return arch_asym_cpu_priority(a) > arch_asym_cpu_priority(b);
> }
> +#else
> +static inline bool sched_asym_prefer(int a, int b)
> +{
> + return false;
> +}
> +#endif
Or you can write:
static inline bool sched_asym_prefer(int a, int b)
{
if (!IS_ENABLED(CONFIG_ARCH_HAS_SCHED_ASYM_PACKING))
return false;
return arch_asym_cpu_priority(a) > arch_asym_cpu_priority(b);
}
Anyway, ifdef bad, less is more. DCE good.
Powered by blists - more mailing lists