[PATCH 0/3] Take the scheduling domain into account in numa balancing

[PATCH 0/3] Take the scheduling domain into account in numa balancing

[Chuyi Zhou]

This patchset tries to adjust the logic of handling isolate cpus in numa balancing.

patch#1: Clean up for task_numa_migrate().

patch#2: Skips the isolate cpus when gathering numa status and finding
idle cpus in update_numa_stats().

patch#3: Ensure that we do not select an isolated CPU in
task_numa_find_cpu(), even if it is present in the task's CPU mask.

Chuyi Zhou (3):
  sched/fair: Remove unused task_numa_migrate return value
  sched/fair: Ignore isolated cpus in update_numa_stat
  sched/fair: Ensure select housekeeping cpus in task_numa_find_cpu

 kernel/sched/fair.c | 26 +++++++++++++++++---------
 1 file changed, 17 insertions(+), 9 deletions(-)

-- 
2.20.1

[PATCH 1/3] sched/fair: Remove unused task_numa_migrate return value

[Chuyi Zhou]

The return value of task_numa_migrate is unused, remove it.

Signed-off-by: Chuyi Zhou <zhouchuyi@bytedance.com>
---
 kernel/sched/fair.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index d5127d9beaea..f544012b9320 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -2486,7 +2486,7 @@ static void task_numa_find_cpu(struct task_numa_env *env,
 	}
 }
 
-static int task_numa_migrate(struct task_struct *p)
+static void task_numa_migrate(struct task_struct *p)
 {
 	struct task_numa_env env = {
 		.p = p,
@@ -2531,7 +2531,7 @@ static int task_numa_migrate(struct task_struct *p)
 	 */
 	if (unlikely(!sd)) {
 		sched_setnuma(p, task_node(p));
-		return -EINVAL;
+		return;
 	}
 
 	env.dst_nid = p->numa_preferred_nid;
@@ -2600,7 +2600,7 @@ static int task_numa_migrate(struct task_struct *p)
 	/* No better CPU than the current one was found. */
 	if (env.best_cpu == -1) {
 		trace_sched_stick_numa(p, env.src_cpu, NULL, -1);
-		return -EAGAIN;
+		return;
 	}
 
 	best_rq = cpu_rq(env.best_cpu);
@@ -2609,7 +2609,7 @@ static int task_numa_migrate(struct task_struct *p)
 		WRITE_ONCE(best_rq->numa_migrate_on, 0);
 		if (ret != 0)
 			trace_sched_stick_numa(p, env.src_cpu, NULL, env.best_cpu);
-		return ret;
+		return;
 	}
 
 	ret = migrate_swap(p, env.best_task, env.best_cpu, env.src_cpu);
@@ -2618,7 +2618,6 @@ static int task_numa_migrate(struct task_struct *p)
 	if (ret != 0)
 		trace_sched_stick_numa(p, env.src_cpu, env.best_task, env.best_cpu);
 	put_task_struct(env.best_task);
-	return ret;
 }
 
 /* Attempt to migrate a task to a CPU on the preferred node. */
-- 
2.20.1

Re: [PATCH 1/3] sched/fair: Remove unused task_numa_migrate return value

[K Prateek Nayak]

Hello Chuyi,

On 12/16/2024 5:53 PM, Chuyi Zhou wrote:
> The return value of task_numa_migrate is unused, remove it.
> 
> Signed-off-by: Chuyi Zhou <zhouchuyi@bytedance.com>

Feel free to include:

Reviewed-by: K Prateek Nayak <kprateek.nayak@amd.com>

If anyone cares for the history, initial NUMA Balancing implementation
used the return value of task_numa_migrate() to retry NUMA Balancing in
commit 6b9a7460b6ba ("sched/numa: Retry migration of tasks to CPU on a
preferred node") however in the same series[1], Mel also included an
optimization from Rik which retried NUMA Balancing periodically
irrespective the return value from task_numa_migrate() in commit
2739d3eef3a9 ("sched/numa: Retry task_numa_migrate() periodically")

[1] https://lore.kernel.org/all/1381141781-10992-34-git-send-email-mgorman@suse.de/

-- 
Thanks and Regards,
Prateek

> ---
>   kernel/sched/fair.c | 9 ++++-----
>   1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index d5127d9beaea..f544012b9320 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -2486,7 +2486,7 @@ static void task_numa_find_cpu(struct task_numa_env *env,
>   	}
>   }
>   
> -static int task_numa_migrate(struct task_struct *p)
> +static void task_numa_migrate(struct task_struct *p)
>   {
>   	struct task_numa_env env = {
>   		.p = p,
> @@ -2531,7 +2531,7 @@ static int task_numa_migrate(struct task_struct *p)
>   	 */
>   	if (unlikely(!sd)) {
>   		sched_setnuma(p, task_node(p));
> -		return -EINVAL;
> +		return;
>   	}
>   
>   	env.dst_nid = p->numa_preferred_nid;
> @@ -2600,7 +2600,7 @@ static int task_numa_migrate(struct task_struct *p)
>   	/* No better CPU than the current one was found. */
>   	if (env.best_cpu == -1) {
>   		trace_sched_stick_numa(p, env.src_cpu, NULL, -1);
> -		return -EAGAIN;
> +		return;
>   	}
>   
>   	best_rq = cpu_rq(env.best_cpu);
> @@ -2609,7 +2609,7 @@ static int task_numa_migrate(struct task_struct *p)
>   		WRITE_ONCE(best_rq->numa_migrate_on, 0);
>   		if (ret != 0)
>   			trace_sched_stick_numa(p, env.src_cpu, NULL, env.best_cpu);
> -		return ret;
> +		return;
>   	}
>   
>   	ret = migrate_swap(p, env.best_task, env.best_cpu, env.src_cpu);
> @@ -2618,7 +2618,6 @@ static int task_numa_migrate(struct task_struct *p)
>   	if (ret != 0)
>   		trace_sched_stick_numa(p, env.src_cpu, env.best_task, env.best_cpu);
>   	put_task_struct(env.best_task);
> -	return ret;
>   }
>   
>   /* Attempt to migrate a task to a CPU on the preferred node. */

Re: [PATCH 1/3] sched/fair: Remove unused task_numa_migrate return value

[Chuyi Zhou]

Hello Prateek,

在 2024/12/18 14:29, K Prateek Nayak 写道:
> Hello Chuyi,
> 
> On 12/16/2024 5:53 PM, Chuyi Zhou wrote:
>> The return value of task_numa_migrate is unused, remove it.
>>
>> Signed-off-by: Chuyi Zhou <zhouchuyi@bytedance.com>
> 
> Feel free to include:
> 
> Reviewed-by: K Prateek Nayak <kprateek.nayak@amd.com>
> 
> If anyone cares for the history, initial NUMA Balancing implementation
> used the return value of task_numa_migrate() to retry NUMA Balancing in
> commit 6b9a7460b6ba ("sched/numa: Retry migration of tasks to CPU on a
> preferred node") however in the same series[1], Mel also included an
> optimization from Rik which retried NUMA Balancing periodically
> irrespective the return value from task_numa_migrate() in commit
> 2739d3eef3a9 ("sched/numa: Retry task_numa_migrate() periodically")
> 
> [1] https://lore.kernel.org/all/1381141781-10992-34-git-send-email- 
> mgorman@suse.de/
> 

Thank you for providing the historical information! I will incorporate 
it into the next version.

[PATCH 2/3] sched/fair: Ignore isolated cpus in update_numa_stat

[Chuyi Zhou]

Now update_numa_stats() iterates each cpu in a node to gather load
information for the node and attempts to find the idle cpu as a candidate
best_cpu within the node.

In update_numa_stats() we should take into account the scheduling domain.
This is because the "isolcpus" kernel command line option and cpuset iso-
late partitions can remove CPUs from load balance. Similar to task wakeup
and periodic load balancing, we should not involve isolated CPUs in NUMA
balancing. When gathering load information for nodes, we need to ignore the
load of isolated CPUs. This change also avoids selecting an isolated CPU
as the idle_cpu.

Signed-off-by: Chuyi Zhou <zhouchuyi@bytedance.com>
---
 kernel/sched/fair.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index f544012b9320..a0139659fe7a 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -2125,6 +2125,11 @@ static void update_numa_stats(struct task_numa_env *env,
 	for_each_cpu(cpu, cpumask_of_node(nid)) {
 		struct rq *rq = cpu_rq(cpu);
 
+		/* skip isolated cpus' load */
+		if (!rcu_dereference(rq->sd))
+			continue;
+
+		ns->weight++;
 		ns->load += cpu_load(rq);
 		ns->runnable += cpu_runnable(rq);
 		ns->util += cpu_util_cfs(cpu);
@@ -2144,8 +2149,6 @@ static void update_numa_stats(struct task_numa_env *env,
 	}
 	rcu_read_unlock();
 
-	ns->weight = cpumask_weight(cpumask_of_node(nid));
-
 	ns->node_type = numa_classify(env->imbalance_pct, ns);
 
 	if (idle_core >= 0)
-- 
2.20.1

Re: [PATCH 2/3] sched/fair: Ignore isolated cpus in update_numa_stat

[K Prateek Nayak]

Hello Chuyi,

On 12/16/2024 5:53 PM, Chuyi Zhou wrote:
> [..snip..] 
> @@ -2125,6 +2125,11 @@ static void update_numa_stats(struct task_numa_env *env,
>   	for_each_cpu(cpu, cpumask_of_node(nid)) {

Looking at sched_init_domains(), we only build sched domains only for
active CPUs in housekeeping_cpumask(HK_TYPE_DOMAIN) so similar to the
question on Patch 3, can we get away with just modifying this outer loop
to:

	for_each_cpu_and(cpu, cpumask_of_node(nid), housekeeping_cpumask(HK_TYPE_DOMAIN)) {
		...
	}

Thoughts?

-- 
Thanks and Regards,
Prateek

>   		struct rq *rq = cpu_rq(cpu);
>   
> +		/* skip isolated cpus' load */
> +		if (!rcu_dereference(rq->sd))
> +			continue;
> +
> +		ns->weight++;
>   		ns->load += cpu_load(rq);
>   		ns->runnable += cpu_runnable(rq);
>   		ns->util += cpu_util_cfs(cpu);
> @@ -2144,8 +2149,6 @@ static void update_numa_stats(struct task_numa_env *env,
>   	}
>   	rcu_read_unlock();
>   
> -	ns->weight = cpumask_weight(cpumask_of_node(nid));
> -
>   	ns->node_type = numa_classify(env->imbalance_pct, ns);
>   
>   	if (idle_core >= 0)

Re: [External] Re: [PATCH 2/3] sched/fair: Ignore isolated cpus in update_numa_stat

[Chuyi Zhou]

Hello Prateek

在 2024/12/18 14:26, K Prateek Nayak 写道:
> Hello Chuyi,
> 
> On 12/16/2024 5:53 PM, Chuyi Zhou wrote:
>> [..snip..] @@ -2125,6 +2125,11 @@ static void update_numa_stats(struct 
>> task_numa_env *env,
>>       for_each_cpu(cpu, cpumask_of_node(nid)) {
> 
> Looking at sched_init_domains(), we only build sched domains only for
> active CPUs in housekeeping_cpumask(HK_TYPE_DOMAIN) so similar to the
> question on Patch 3, can we get away with just modifying this outer loop
> to:
> 
>      for_each_cpu_and(cpu, cpumask_of_node(nid), 
> housekeeping_cpumask(HK_TYPE_DOMAIN)) {
>          ...
>      }
> 
> Thoughts?
> 

We now have two ways of using isolated CPUs.

One is the isolcpus= kernel command line. 'isolcpus=0-7' would
exclude 0-7 cpus from housekeeping_cpumask without further restrictions 
on tasks' cpumasks. A typical case that could lead to errors is when a 
task's CPU mask covers all the CPUs in the system, causing the task to 
potentially be migrated to or woken up on CPUs 0-7. Commit 23d04d8 
resolves a similar issue in task wakeup.


The other is the isolated cpuset partition:
  mkdir blue
  echo 5-8 > blue/cpuset.cpus
  echo "isolated" > blue/cpuset.cpus.partition

The 5-8 cpus now is also a isolated domain, but 5-8 would not be 
excluded from housekeeping_cpumask. The tasks' cpumask in blue cgroup 
would be restricted to 5-8.

I thinks in update_numa_stats(), only using 
housekeeping_cpumask(HK_TYPE_DOMAIN) is not enough because it cannot 
skip those isolated cpuset partitions.

Thanks.

[PATCH 3/3] sched/fair: Ensure select housekeeping cpus in task_numa_find_cpu

[Chuyi Zhou]

Now in task_numa_find_cpu(), we only skip CPUs that are not in the task's
cpumask, which could result in migrating the task to an isolated domain if
the task's cpumask includes isolated CPUs. This is because cpuset
configured partitions are always reflected in each member task's cpumask.
However, for isolcpus= kernel command line option, the isolated CPUs are
simply omitted from sched_domains without further restrictions on tasks'
cpumasks.

This change replaces the set of CPUs allowed to migrate the task from
p->cpus_ptr by the intersection of p->cpus_ptr and
housekeeping_cpumask(HK_TYPE_DOMAIN).

Signed-off-by: Chuyi Zhou <zhouchuyi@bytedance.com>
---
 kernel/sched/fair.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index a0139659fe7a..05782b563609 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -2081,6 +2081,12 @@ numa_type numa_classify(unsigned int imbalance_pct,
 	return node_fully_busy;
 }
 
+static inline bool numa_migrate_test_cpu(struct task_struct *p, int cpu)
+{
+	return cpumask_test_cpu(cpu, p->cpus_ptr) &&
+			housekeeping_cpu(cpu, HK_TYPE_DOMAIN);
+}
+
 #ifdef CONFIG_SCHED_SMT
 /* Forward declarations of select_idle_sibling helpers */
 static inline bool test_idle_cores(int cpu);
@@ -2168,7 +2174,7 @@ static void task_numa_assign(struct task_numa_env *env,
 		/* Find alternative idle CPU. */
 		for_each_cpu_wrap(cpu, cpumask_of_node(env->dst_nid), start + 1) {
 			if (cpu == env->best_cpu || !idle_cpu(cpu) ||
-			    !cpumask_test_cpu(cpu, env->p->cpus_ptr)) {
+			    !numa_migrate_test_cpu(env->p, cpu)) {
 				continue;
 			}
 
@@ -2480,7 +2486,7 @@ static void task_numa_find_cpu(struct task_numa_env *env,
 
 	for_each_cpu(cpu, cpumask_of_node(env->dst_nid)) {
 		/* Skip this CPU if the source task cannot migrate */
-		if (!cpumask_test_cpu(cpu, env->p->cpus_ptr))
+		if (!numa_migrate_test_cpu(env->p, cpu))
 			continue;
 
 		env->dst_cpu = cpu;
-- 
2.20.1

Re: [PATCH 3/3] sched/fair: Ensure select housekeeping cpus in task_numa_find_cpu

[K Prateek Nayak]

Hello Chuyi,

On 12/16/2024 5:53 PM, Chuyi Zhou wrote:
> [..snip..]
> @@ -2081,6 +2081,12 @@ numa_type numa_classify(unsigned int imbalance_pct,
>   	return node_fully_busy;
>   }
>   
> +static inline bool numa_migrate_test_cpu(struct task_struct *p, int cpu)
> +{
> +	return cpumask_test_cpu(cpu, p->cpus_ptr) &&
> +			housekeeping_cpu(cpu, HK_TYPE_DOMAIN);
> +}
> +
>   #ifdef CONFIG_SCHED_SMT
>   /* Forward declarations of select_idle_sibling helpers */
>   static inline bool test_idle_cores(int cpu);
> @@ -2168,7 +2174,7 @@ static void task_numa_assign(struct task_numa_env *env,
>   		/* Find alternative idle CPU. */
>   		for_each_cpu_wrap(cpu, cpumask_of_node(env->dst_nid), start + 1) {

Can we just do:

	for_each_cpu_and(cpu, cpumask_of_node(env->dst_nid), housekeeping_cpumask(HK_TYPE_DOMAIN)) {
		...
	}

and avoid adding numa_migrate_test_cpu(). Thoughts?

>   			if (cpu == env->best_cpu || !idle_cpu(cpu) ||
> -			    !cpumask_test_cpu(cpu, env->p->cpus_ptr)) {
> +			    !numa_migrate_test_cpu(env->p, cpu)) {
>   				continue;
>   			}
>   
> @@ -2480,7 +2486,7 @@ static void task_numa_find_cpu(struct task_numa_env *env,
>   
>   	for_each_cpu(cpu, cpumask_of_node(env->dst_nid)) {

Same modifications can be made for this outer loop.

-- 
Thanks and Regards,
Prateek

>   		/* Skip this CPU if the source task cannot migrate */
> -		if (!cpumask_test_cpu(cpu, env->p->cpus_ptr))
> +		if (!numa_migrate_test_cpu(env->p, cpu))
>   			continue;
>   
>   		env->dst_cpu = cpu;

Re: [PATCH 3/3] sched/fair: Ensure select housekeeping cpus in task_numa_find_cpu

[Chuyi Zhou]

在 2024/12/18 14:21, K Prateek Nayak 写道:
> Hello Chuyi,
> 
> On 12/16/2024 5:53 PM, Chuyi Zhou wrote:
>> [..snip..]
>> @@ -2081,6 +2081,12 @@ numa_type numa_classify(unsigned int 
>> imbalance_pct,
>>       return node_fully_busy;
>>   }
>> +static inline bool numa_migrate_test_cpu(struct task_struct *p, int cpu)
>> +{
>> +    return cpumask_test_cpu(cpu, p->cpus_ptr) &&
>> +            housekeeping_cpu(cpu, HK_TYPE_DOMAIN);
>> +}
>> +
>>   #ifdef CONFIG_SCHED_SMT
>>   /* Forward declarations of select_idle_sibling helpers */
>>   static inline bool test_idle_cores(int cpu);
>> @@ -2168,7 +2174,7 @@ static void task_numa_assign(struct 
>> task_numa_env *env,
>>           /* Find alternative idle CPU. */
>>           for_each_cpu_wrap(cpu, cpumask_of_node(env->dst_nid), start 
>> + 1) {
> 
> Can we just do:
> 
>      for_each_cpu_and(cpu, cpumask_of_node(env->dst_nid), 
> housekeeping_cpumask(HK_TYPE_DOMAIN)) {
>          ...
>      }
> 
> and avoid adding numa_migrate_test_cpu(). Thoughts?

Make sense, but now there doesn't seem to be an API like 
for_each_cpu_wrap_and().

Do you think the following is better?

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 855df103f4dd..4792ef672738 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -2167,9 +2167,9 @@ static void task_numa_assign(struct task_numa_env 
*env,
                 int start = env->dst_cpu;

                 /* Find alternative idle CPU. */
-               for_each_cpu_wrap(cpu, cpumask_of_node(env->dst_nid), 
start + 1) {
+               for_each_cpu_and(cpu, cpumask_of_node(env->dst_nid), 
housekeeping_cpumask(HK_TYPE_DOMAIN)) {
                         if (cpu == env->best_cpu || !idle_cpu(cpu) ||
-                           !cpumask_test_cpu(cpu, env->p->cpus_ptr)) {
+                               cpu == start || !cpumask_test_cpu(cpu, 
env->p->cpus_ptr)) {
                                 continue;
                         }


Thanks.


> 
>>               if (cpu == env->best_cpu || !idle_cpu(cpu) ||
>> -                !cpumask_test_cpu(cpu, env->p->cpus_ptr)) {
>> +                !numa_migrate_test_cpu(env->p, cpu)) {
>>                   continue;
>>               }
>> @@ -2480,7 +2486,7 @@ static void task_numa_find_cpu(struct 
>> task_numa_env *env,
>>       for_each_cpu(cpu, cpumask_of_node(env->dst_nid)) {
> 
> Same modifications can be made for this outer loop.
> 

Re: [PATCH 3/3] sched/fair: Ensure select housekeeping cpus in task_numa_find_cpu

[K Prateek Nayak]

Hello Chuyi,

On 12/23/2024 6:28 PM, Chuyi Zhou wrote:
> 
> 
> 在 2024/12/18 14:21, K Prateek Nayak 写道:
>> Hello Chuyi,
>>
>> On 12/16/2024 5:53 PM, Chuyi Zhou wrote:
>>> [..snip..]
>>> @@ -2081,6 +2081,12 @@ numa_type numa_classify(unsigned int imbalance_pct,
>>>       return node_fully_busy;
>>>   }
>>> +static inline bool numa_migrate_test_cpu(struct task_struct *p, int cpu)
>>> +{
>>> +    return cpumask_test_cpu(cpu, p->cpus_ptr) &&
>>> +            housekeeping_cpu(cpu, HK_TYPE_DOMAIN);
>>> +}
>>> +
>>>   #ifdef CONFIG_SCHED_SMT
>>>   /* Forward declarations of select_idle_sibling helpers */
>>>   static inline bool test_idle_cores(int cpu);
>>> @@ -2168,7 +2174,7 @@ static void task_numa_assign(struct task_numa_env *env,
>>>           /* Find alternative idle CPU. */
>>>           for_each_cpu_wrap(cpu, cpumask_of_node(env->dst_nid), start + 1) {
>>
>> Can we just do:
>>
>>      for_each_cpu_and(cpu, cpumask_of_node(env->dst_nid), housekeeping_cpumask(HK_TYPE_DOMAIN)) {
>>          ...
>>      }
>>
>> and avoid adding numa_migrate_test_cpu(). Thoughts?
> 
> Make sense, but now there doesn't seem to be an API like for_each_cpu_wrap_and().
> 
> Do you think the following is better?
> 
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 855df103f4dd..4792ef672738 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -2167,9 +2167,9 @@ static void task_numa_assign(struct task_numa_env *env,
>                  int start = env->dst_cpu;
> 
>                  /* Find alternative idle CPU. */
> -               for_each_cpu_wrap(cpu, cpumask_of_node(env->dst_nid), start + 1) {
> +               for_each_cpu_and(cpu, cpumask_of_node(env->dst_nid), housekeeping_cpumask(HK_TYPE_DOMAIN)) {
>                          if (cpu == env->best_cpu || !idle_cpu(cpu) ||

"start" is set to "env->dst_cpu" is already taken care here with the
first comparison.

> -                           !cpumask_test_cpu(cpu, env->p->cpus_ptr)) {
> +                               cpu == start || !cpumask_test_cpu(cpu, env->p->cpus_ptr)) {
>                                  continue;
>                          }
> 

I think the for_each_cpu_wrap() was used to reduce contention for xchg
operation below. Perhaps we can have a per-cpu temporary mask (like
load_balance_mask) if we want to reduce the xchg contention and break
this into cpumask_and() + for_each_cpu_wrap() steps. I'm not sure if
any of the existing masks (load_balance_mask, select_rq_mask,
should_we_balance_tmpmask) can be safely reused. Otherwise, perhaps we
can make a case for for_each_cpu_and_wrap() with this use case.

> 
> Thanks.
> 
> 
>>
>>>               if (cpu == env->best_cpu || !idle_cpu(cpu) ||
>>> -                !cpumask_test_cpu(cpu, env->p->cpus_ptr)) {
>>> +                !numa_migrate_test_cpu(env->p, cpu)) {
>>>                   continue;
>>>               }
>>> @@ -2480,7 +2486,7 @@ static void task_numa_find_cpu(struct task_numa_env *env,
>>>       for_each_cpu(cpu, cpumask_of_node(env->dst_nid)) {
>>
>> Same modifications can be made for this outer loop.
>>
> 

-- 
Thanks and Regards,
Prateek

Re: [PATCH 3/3] sched/fair: Ensure select housekeeping cpus in task_numa_find_cpu

[Chuyi Zhou]

Hello,

在 2024/12/27 12:40, K Prateek Nayak 写道:
> Hello Chuyi,
> 
> On 12/23/2024 6:28 PM, Chuyi Zhou wrote:
>>
>>
>> 在 2024/12/18 14:21, K Prateek Nayak 写道:
>>> Hello Chuyi,
>>>
>>> On 12/16/2024 5:53 PM, Chuyi Zhou wrote:
>>>> [..snip..]
>>>> @@ -2081,6 +2081,12 @@ numa_type numa_classify(unsigned int 
>>>> imbalance_pct,
>>>>       return node_fully_busy;
>>>>   }
>>>> +static inline bool numa_migrate_test_cpu(struct task_struct *p, int 
>>>> cpu)
>>>> +{
>>>> +    return cpumask_test_cpu(cpu, p->cpus_ptr) &&
>>>> +            housekeeping_cpu(cpu, HK_TYPE_DOMAIN);
>>>> +}
>>>> +
>>>>   #ifdef CONFIG_SCHED_SMT
>>>>   /* Forward declarations of select_idle_sibling helpers */
>>>>   static inline bool test_idle_cores(int cpu);
>>>> @@ -2168,7 +2174,7 @@ static void task_numa_assign(struct 
>>>> task_numa_env *env,
>>>>           /* Find alternative idle CPU. */
>>>>           for_each_cpu_wrap(cpu, cpumask_of_node(env->dst_nid), 
>>>> start + 1) {
>>>
>>> Can we just do:
>>>
>>>      for_each_cpu_and(cpu, cpumask_of_node(env->dst_nid), 
>>> housekeeping_cpumask(HK_TYPE_DOMAIN)) {
>>>          ...
>>>      }
>>>
>>> and avoid adding numa_migrate_test_cpu(). Thoughts?
>>
>> Make sense, but now there doesn't seem to be an API like 
>> for_each_cpu_wrap_and().
>>
>> Do you think the following is better?
>>
>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
>> index 855df103f4dd..4792ef672738 100644
>> --- a/kernel/sched/fair.c
>> +++ b/kernel/sched/fair.c
>> @@ -2167,9 +2167,9 @@ static void task_numa_assign(struct 
>> task_numa_env *env,
>>                  int start = env->dst_cpu;
>>
>>                  /* Find alternative idle CPU. */
>> -               for_each_cpu_wrap(cpu, cpumask_of_node(env->dst_nid), 
>> start + 1) {
>> +               for_each_cpu_and(cpu, cpumask_of_node(env->dst_nid), 
>> housekeeping_cpumask(HK_TYPE_DOMAIN)) {
>>                          if (cpu == env->best_cpu || !idle_cpu(cpu) ||
> 
> "start" is set to "env->dst_cpu" is already taken care here with the
> first comparison.
> 
>> -                           !cpumask_test_cpu(cpu, env->p->cpus_ptr)) {
>> +                               cpu == start || !cpumask_test_cpu(cpu, 
>> env->p->cpus_ptr)) {
>>                                  continue;
>>                          }
>>
> 
> I think the for_each_cpu_wrap() was used to reduce contention for xchg
> operation below. Perhaps we can have a per-cpu temporary mask (like
> load_balance_mask) if we want to reduce the xchg contention and break
> this into cpumask_and() + for_each_cpu_wrap() steps. I'm not sure if
> any of the existing masks (load_balance_mask, select_rq_mask,
> should_we_balance_tmpmask) can be safely reused. Otherwise, perhaps we
> can make a case for for_each_cpu_and_wrap() with this use case.
> 


for_each_cpu_and_wrap() is a good idea, but it might be slightly 
off-topic for this subject. Perhaps we should stick with this 
implementation for now and see what others think about v2.


Thanks.

Re: [PATCH 3/3] sched/fair: Ensure select housekeeping cpus in task_numa_find_cpu

[K Prateek Nayak]

Hello Chuyi,

On 12/27/2024 1:29 PM, Chuyi Zhou wrote:
> [..snip..]
>>
>> I think the for_each_cpu_wrap() was used to reduce contention for xchg
>> operation below. Perhaps we can have a per-cpu temporary mask (like
>> load_balance_mask) if we want to reduce the xchg contention and break
>> this into cpumask_and() + for_each_cpu_wrap() steps. I'm not sure if
>> any of the existing masks (load_balance_mask, select_rq_mask,
>> should_we_balance_tmpmask) can be safely reused. Otherwise, perhaps we
>> can make a case for for_each_cpu_and_wrap() with this use case.
>>
> 
> 
> for_each_cpu_and_wrap() is a good idea, but it might be slightly off-topic for this subject. Perhaps we should stick with this implementation for now and see what others think about v2.

Sure thing! No strong feeling from my side :)

> 
> 
> Thanks.
> 

-- 
Thanks and Regards,
Prateek