[PATCH] lib/smp_processor_id: Use is_percpu_thread() instead of nr_cpus_allowed

[PATCH] lib/smp_processor_id: Use is_percpu_thread() instead of nr_cpus_allowed

[Yejune Deng]

Use is_percpu_thread() instead of 'current->nr_cpus_allowed == 1',
is_percpu_thread() is includes boths SMP and SP. It also more readable.
The comments are no needed.

Signed-off-by: Yejune Deng <yejunedeng@gmail.com>
---
 lib/smp_processor_id.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/lib/smp_processor_id.c b/lib/smp_processor_id.c
index 1c1dbd3..046ac62 100644
--- a/lib/smp_processor_id.c
+++ b/lib/smp_processor_id.c
@@ -19,11 +19,7 @@ unsigned int check_preemption_disabled(const char *what1, const char *what2)
 	if (irqs_disabled())
 		goto out;
 
-	/*
-	 * Kernel threads bound to a single CPU can safely use
-	 * smp_processor_id():
-	 */
-	if (current->nr_cpus_allowed == 1)
+	if (is_percpu_thread())
 		goto out;
 
 #ifdef CONFIG_SMP
-- 
2.7.4

Re: [PATCH] lib/smp_processor_id: Use is_percpu_thread() instead of nr_cpus_allowed

[Valentin Schneider]

On 08/05/21 15:25, Yejune Deng wrote:
> Use is_percpu_thread() instead of 'current->nr_cpus_allowed == 1',
> is_percpu_thread() is includes boths SMP and SP. It also more readable.
> The comments are no needed.
>
> Signed-off-by: Yejune Deng <yejunedeng@gmail.com>
> ---
>  lib/smp_processor_id.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/lib/smp_processor_id.c b/lib/smp_processor_id.c
> index 1c1dbd3..046ac62 100644
> --- a/lib/smp_processor_id.c
> +++ b/lib/smp_processor_id.c
> @@ -19,11 +19,7 @@ unsigned int check_preemption_disabled(const char *what1, const char *what2)
>       if (irqs_disabled())
>               goto out;
>
> -	/*
> -	 * Kernel threads bound to a single CPU can safely use
> -	 * smp_processor_id():
> -	 */
> -	if (current->nr_cpus_allowed == 1)
> +	if (is_percpu_thread())
>               goto out;

That's not entirely equivalent as this adds (p->flags & PF_NO_SETAFFINITY)
to the condition. Per-CPU kthreads will have it set, so that at least
matches the existing comment, but it's not an innocent change.

I'm thinking this might actually be something we want, as not having
this flag + having preemption enabled means this could race with
sched_setaffinity(), and it would be nice to detect this regardless of the
current affinity.

I ran this on my Juno and the idle thread gets in the way as it only has

  PF_KTHREAD | PF_IDLE

So you'd need something like the below (which is purely for instrumentation
/ assertion purposes).

---
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 4a0668acd876..2a6fdf3e7061 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -7445,7 +7445,7 @@ void init_idle(struct task_struct *idle, int cpu)
 
 	idle->state = TASK_RUNNING;
 	idle->se.exec_start = sched_clock();
-	idle->flags |= PF_IDLE;
+	idle->flags |= PF_IDLE | PF_NO_SETAFFINITY;
 
 	scs_task_reset(idle);
 	kasan_unpoison_task_stack(idle);

[PATCH v2] lib/smp_processor_id: Use is_percpu_thread() check affinity

[Yejune Deng]

Use is_percpu_thread() instead of 'current->nr_cpus_allowed == 1',
and add PF_NO_SETAFFINITY flag in init_idle().This would be nice to
the sched_setaffinity(), and it also more readable.

v1->v2:
  - add PF_NO_SETAFFINITY flag in init_idle().

Signed-off-by: Yejune Deng <yejunedeng@gmail.com>
---
 kernel/sched/core.c    | 2 +-
 lib/smp_processor_id.c | 6 +-----
 2 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 9143163..82e21ea 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -7446,7 +7446,7 @@ void init_idle(struct task_struct *idle, int cpu)
 
 	idle->state = TASK_RUNNING;
 	idle->se.exec_start = sched_clock();
-	idle->flags |= PF_IDLE;
+	idle->flags |= PF_IDLE | PF_NO_SETAFFINITY;
 
 	scs_task_reset(idle);
 	kasan_unpoison_task_stack(idle);
diff --git a/lib/smp_processor_id.c b/lib/smp_processor_id.c
index 1c1dbd3..046ac62 100644
--- a/lib/smp_processor_id.c
+++ b/lib/smp_processor_id.c
@@ -19,11 +19,7 @@ unsigned int check_preemption_disabled(const char *what1, const char *what2)
 	if (irqs_disabled())
 		goto out;
 
-	/*
-	 * Kernel threads bound to a single CPU can safely use
-	 * smp_processor_id():
-	 */
-	if (current->nr_cpus_allowed == 1)
+	if (is_percpu_thread())
 		goto out;
 
 #ifdef CONFIG_SMP
-- 
2.7.4

Re: [PATCH v2] lib/smp_processor_id: Use is_percpu_thread() check affinity

[Valentin Schneider]

On 10/05/21 21:43, Yejune Deng wrote:
> Use is_percpu_thread() instead of 'current->nr_cpus_allowed == 1',
> and add PF_NO_SETAFFINITY flag in init_idle().This would be nice to
> the sched_setaffinity(), and it also more readable.
>
> v1->v2:
>   - add PF_NO_SETAFFINITY flag in init_idle().
>
> Signed-off-by: Yejune Deng <yejunedeng@gmail.com>
> ---
>  kernel/sched/core.c    | 2 +-
>  lib/smp_processor_id.c | 6 +-----
>  2 files changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 9143163..82e21ea 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -7446,7 +7446,7 @@ void init_idle(struct task_struct *idle, int cpu)
>
>       idle->state = TASK_RUNNING;
>       idle->se.exec_start = sched_clock();
> -	idle->flags |= PF_IDLE;
> +	idle->flags |= PF_IDLE | PF_NO_SETAFFINITY;

Thanks for spinning a v2!

Peter reminded me that it's not the only flag the idle task is missing, so
I went and wrote something to make the idle task really look like a run of
the mill pcpu kthread. I'll bundle your v1 with it.