Re: [PATCH 0/9] kthread detection cleanup

From: Valentin Schneider <valentin.schneider@arm.com>
To: Mark Rutland <mark.rutland@arm.com>, linux-kernel@vger.kernel.org
Cc: ak@linux.intel.com, akpm@linux-foundation.org,
	bigeasy@linutronix.de, bp@suse.de, catalin.marinas@arm.com,
	davem@davemloft.net, hch@lst.de, kan.liang@intel.com,
	mingo@kernel.org, peterz@infradead.org, riel@surriel.com,
	will@kernel.org, Mel Gorman <mgorman@techsingularity.net>
Subject: Re: [PATCH 0/9] kthread detection cleanup
Date: Wed, 14 Aug 2019 14:43:17 +0100	[thread overview]
Message-ID: <c189d01c-3a46-40e2-cee1-e021504cec6e@arm.com> (raw)
In-Reply-To: <20190814104131.20190-1-mark.rutland@arm.com>

On 14/08/2019 11:41, Mark Rutland wrote:
> A reasonable amount of kernel code looks at task_struct::mm to determine
> whether a thread is a kthread or a real user task. This isn't quite right,
> since kthreads can have a non-NULL mm when calling use_mm().
> 
> The correct way to check whether a task is a kthread is to check whether
> PF_KTHREAD is set in task_struct::flags, but doing so is a bit unwieldy.
> 
> To make this a bit nicer, this series adds a new is_kthread(tsk) helper,
> converts existing code to make use of it, and fixes up a number of erroneous
> checks of current->mm.  Hopefully this will push people to use the right
> approach in future.
> 
> I'm sure there are other instances in the kernel tree where we don't check this
> correctly. In this series I'm just trying to fix the instances I'm reasonably
> confident are incorrect.
> 

I've only found one extra location in kernel/sched/ that seems like it
wants to use is_kthread(), included below. I can send it out separately
after this gets merged.

-----8<-----

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 4c4a0494c39b..0956972f6ea7 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -1964,11 +1964,10 @@ static int select_fallback_rq(int cpu, struct task_struct *p)
 out:
 	if (state != cpuset) {
 		/*
-		 * Don't tell them about moving exiting tasks or
-		 * kernel threads (both mm NULL), since they never
-		 * leave kernel.
+		 * Don't tell them about moving exiting tasks (NULL mm) or
+		 * kernel threads since they never leave kernel.
 		 */
-		if (p->mm && printk_ratelimit()) {
+		if ((!is_kthread(p) && p->mm) && printk_ratelimit()) {
 			printk_deferred("process %d (%s) no longer affine to cpu%d\n",
 					task_pid_nr(p), p->comm, cpu);
 		}
----->8-----

Regarding sched/fair.c, I am quite convinced all of the p->mm checks are
genuine (as in they don't care if it's a kthread or not) - those are all
about NUMA balancing. Maybe Rik/Mel can correct me if I'm wrong.

Cheers,
Valentin
