linux-kernel - Re: [PATCH] sche/fair: static cpumasks for load balance

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <e4523195-b271-0b11-e297-5ca25cc23504@kylinos.cn>
Date:   Thu, 26 May 2022 16:32:54 +0800
From:   huangbing <huangbing@...inos.cn>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     mingo@...hat.com, juri.lelli@...hat.com,
        vincent.guittot@...aro.org, dietmar.eggemann@....com,
        rostedt@...dmis.org, bsegall@...gle.com, mgorman@...e.de,
        bristot@...hat.com, brauner@...nel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] sche/fair: static cpumasks for load balance



On 5/26/22 01:04, Peter Zijlstra wrote:
> On Mon, May 23, 2022 at 05:42:16PM +0800, Bing Huang wrote:
>> The both cpu mask load_balance_mask and select_idle_mask just only used
>> in fair.c, but allocation in core.c in CONFIG_CPUMASK_OFFSTACK=y case,
>> and global via declare per cpu variations. More or less, it looks wired.
>>
>> Signed-off-by: Bing Huang <huangbing@...inos.cn>
>> ---
>>   kernel/sched/core.c  | 13 +++----------
>>   kernel/sched/fair.c  | 18 ++++++++++++++++--
>>   kernel/sched/sched.h |  4 ++++
>>   3 files changed, 23 insertions(+), 12 deletions(-)
>>
>> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
>> index d58c0389eb23..64934c5e236d 100644
>> --- a/kernel/sched/core.c
>> +++ b/kernel/sched/core.c
>> @@ -9448,9 +9448,6 @@ LIST_HEAD(task_groups);
>>   static struct kmem_cache *task_group_cache __read_mostly;
>>   #endif
>>   
>> -DECLARE_PER_CPU(cpumask_var_t, load_balance_mask);
>> -DECLARE_PER_CPU(cpumask_var_t, select_idle_mask);
>> -
>>   void __init sched_init(void)
>>   {
>>   	unsigned long ptr = 0;
>> @@ -9494,14 +9491,10 @@ void __init sched_init(void)
>>   
>>   #endif /* CONFIG_RT_GROUP_SCHED */
>>   	}
>> +
>>   #ifdef CONFIG_CPUMASK_OFFSTACK
>> -	for_each_possible_cpu(i) {
>> -		per_cpu(load_balance_mask, i) = (cpumask_var_t)kzalloc_node(
>> -			cpumask_size(), GFP_KERNEL, cpu_to_node(i));
>> -		per_cpu(select_idle_mask, i) = (cpumask_var_t)kzalloc_node(
>> -			cpumask_size(), GFP_KERNEL, cpu_to_node(i));
>> -	}
>> -#endif /* CONFIG_CPUMASK_OFFSTACK */
>> +	init_working_cpumask();
>> +#endif
> We already have init_sched_fair_class(), can't you use that?

OK, thanks for your review. I got it.

In addition, I notice that load_balance_mask and select_idle_mask are 
defined
in CONFIG_SMP=y case in fair.c, so could be they moved inside 
CONFIG_SMP, or just outside ?

>
>>   
>>   	init_rt_bandwidth(&def_rt_bandwidth, global_rt_period(), global_rt_runtime());
>>   
>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
>> index a68482d66535..3a4b0dd1beb5 100644
>> --- a/kernel/sched/fair.c
>> +++ b/kernel/sched/fair.c
>> @@ -5831,8 +5831,8 @@ static void dequeue_task_fair(struct rq *rq, struct task_struct *p, int flags)
>>   #ifdef CONFIG_SMP
>>   
>>   /* Working cpumask for: load_balance, load_balance_newidle. */
>> -DEFINE_PER_CPU(cpumask_var_t, load_balance_mask);
>> -DEFINE_PER_CPU(cpumask_var_t, select_idle_mask);
>> +static DEFINE_PER_CPU(cpumask_var_t, load_balance_mask);
>> +static DEFINE_PER_CPU(cpumask_var_t, select_idle_mask);
>>   
>>   #ifdef CONFIG_NO_HZ_COMMON
>>   
>> @@ -7052,6 +7052,20 @@ balance_fair(struct rq *rq, struct task_struct *prev, struct rq_flags *rf)
>>   }
>>   #endif /* CONFIG_SMP */
>>   
>> +#ifdef CONFIG_CPUMASK_OFFSTACK
>> +void __init init_working_cpumask(void)
>> +{
>> +	int i;
>> +
>> +	for_each_possible_cpu(i) {
>> +		per_cpu(load_balance_mask, i) = (cpumask_var_t)kzalloc_node(
>> +				cpumask_size(), GFP_KERNEL, cpu_to_node(i));
>> +		per_cpu(select_idle_mask, i) = (cpumask_var_t)kzalloc_node(
>> +				cpumask_size(), GFP_KERNEL, cpu_to_node(i));
>> +	}
>> +}
>> +#endif
> If you stick that #ifdef inside the function, you can remove it
> everwhere else. Less #ifdef is more better.
I will remove it, move them inside init_sched_fair_class().