While working on the deferred stacktrace code, Peter Zijlstra told
me to use task->flags & PF_KTHREAD instead of checking task->mm for NULL.
This seemed reasonable, but while working on it, as there were several
places that check if the task is a kernel thread and other places that
check if the task is a user space thread I found it a bit confusing
when looking at both:

	if (task->flags & PF_KTHREAD)
and
	if (!(task->flags & PF_KTHREAD))

Where I mixed them up sometimes, and checked for a user space thread when I
really wanted to check for a kernel thread. I found these mistakes before
sending out my patches, but going back and reviewing the code, I always had
to stop and spend a few unnecessary seconds making sure the check was
testing that flag correctly.

To make this a bit more obvious, I introduced two helper functions:

	is_user_thread(task)
	is_kernel_thread(task)

which simply test the flag for you. Thus, seeing:

	if (is_user_thread(task))
or
	if (is_kernel_thread(task))

it was very obvious to which test you wanted to make.

I then created a coccinelle script to change all the checks throughout the
kernel to use one of these macros.

      $ cat kthread.cocci
      @@
      identifier task;
      @@
      -     !(task->flags & PF_KTHREAD)
      +     is_user_thread(task)
      @@
      identifier task;
      @@
      -     (task->flags & PF_KTHREAD) == 0
      +     is_user_thread(task)
      @@
      identifier task;
      @@
      -     (task->flags & PF_KTHREAD) != 0
      +     is_kernel_thread(task)
      @@
      identifier task;
      @@
      -     task->flags & PF_KTHREAD
      +     is_kernel_thread(task)
    
      $ spatch --dir --include-headers kthread.cocci . > /tmp/t.patch
      $ patch -p1 < /tmp/t.patch

Make sure to undo the conversion of the helper functions themselves!
        
      $ git show include/linux/sched.h | patch -p1 -R

It did modify the tools/sched_ext code, and I'm not sure if that's OK
or not. Does it still use the sched.h header? If so, it should be fine.
But if it is an issue, I can undo the changes to tools as well.


Steven Rostedt (2):
      kthread: Add is_user_thread() and is_kernel_thread() helper functions
      treewide: Have the task->flags & PF_KTHREAD check use the helper functions

----
 arch/arm/mm/init.c                         |  2 +-
 arch/arm64/include/asm/uaccess.h           |  2 +-
 arch/arm64/kernel/process.c                |  2 +-
 arch/arm64/kernel/proton-pack.c            |  2 +-
 arch/mips/kernel/process.c                 |  2 +-
 arch/powerpc/kernel/process.c              |  2 +-
 arch/powerpc/kernel/stacktrace.c           |  2 +-
 arch/x86/kernel/fpu/core.c                 |  2 +-
 arch/x86/kernel/process.c                  |  2 +-
 block/blk-cgroup.c                         |  2 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h |  2 +-
 drivers/md/dm-vdo/logger.c                 |  2 +-
 drivers/tty/sysrq.c                        |  2 +-
 fs/bcachefs/clock.c                        |  2 +-
 fs/bcachefs/journal_reclaim.c              |  2 +-
 fs/bcachefs/move.c                         |  6 +++---
 fs/exec.c                                  |  2 +-
 fs/file_table.c                            |  2 +-
 fs/namespace.c                             |  2 +-
 fs/proc/array.c                            |  4 ++--
 fs/proc/base.c                             |  6 +++---
 include/linux/sched.h                      | 10 ++++++++++
 io_uring/io_uring.c                        |  2 +-
 kernel/cgroup/cgroup.c                     |  6 +++---
 kernel/cgroup/freezer.c                    |  4 ++--
 kernel/events/core.c                       |  2 +-
 kernel/exit.c                              |  2 +-
 kernel/fork.c                              |  6 +++---
 kernel/freezer.c                           |  4 ++--
 kernel/futex/pi.c                          |  2 +-
 kernel/kthread.c                           | 12 ++++++------
 kernel/livepatch/transition.c              |  2 +-
 kernel/power/process.c                     |  2 +-
 kernel/sched/core.c                        |  6 +++---
 kernel/sched/idle.c                        |  2 +-
 kernel/sched/sched.h                       |  4 ++--
 kernel/signal.c                            |  4 ++--
 kernel/stacktrace.c                        |  2 +-
 lib/is_single_threaded.c                   |  2 +-
 mm/memcontrol.c                            |  2 +-
 mm/oom_kill.c                              |  4 ++--
 mm/page_alloc.c                            |  2 +-
 mm/vmscan.c                                |  2 +-
 security/keys/request_key.c                |  2 +-
 security/smack/smack_access.c              |  2 +-
 security/smack/smack_lsm.c                 |  4 ++--
 security/tomoyo/network.c                  |  2 +-
 security/yama/yama_lsm.c                   |  2 +-
 tools/sched_ext/scx_central.bpf.c          |  2 +-
 tools/sched_ext/scx_flatcg.bpf.c           |  2 +-
 tools/sched_ext/scx_qmap.bpf.c             |  2 +-
 51 files changed, 82 insertions(+), 72 deletions(-)

On Fri, 25 Apr 2025 16:41:20 -0400 Steven Rostedt <rostedt@goodmis.org> wrote:

> While working on the deferred stacktrace code, Peter Zijlstra told
> me to use task->flags & PF_KTHREAD instead of checking task->mm for NULL.
> This seemed reasonable, but while working on it, as there were several
> places that check if the task is a kernel thread and other places that
> check if the task is a user space thread I found it a bit confusing
> when looking at both:
> 
> 	if (task->flags & PF_KTHREAD)
> and
> 	if (!(task->flags & PF_KTHREAD))
> 
> Where I mixed them up sometimes, and checked for a user space thread when I
> really wanted to check for a kernel thread. I found these mistakes before
> sending out my patches, but going back and reviewing the code, I always had
> to stop and spend a few unnecessary seconds making sure the check was
> testing that flag correctly.
> 
> To make this a bit more obvious, I introduced two helper functions:
> 
> 	is_user_thread(task)
> 	is_kernel_thread(task)
> 
> which simply test the flag for you. Thus, seeing:
> 
> 	if (is_user_thread(task))
> or
> 	if (is_kernel_thread(task))
> 
> it was very obvious to which test you wanted to make.

Seems sensible.  Please consider renaming PF_KTHREAD in order to break
missed conversion sites.

On Fri, 25 Apr 2025 16:14:49 -0700
Andrew Morton <akpm@linux-foundation.org> wrote:

> Seems sensible.  Please consider renaming PF_KTHREAD in order to break
> missed conversion sites.

It's not wrong to use the thread. I just find using these helper
functions a bit easier to review code. There's also some places that
have special tests where it can't use the flag:

kernel/sched/core.c:    if (!curr->mm || (curr->flags & (PF_EXITING | PF_KTHREAD)) ||
kernel/sched/fair.c:    if (!curr->mm || (curr->flags & (PF_EXITING | PF_KTHREAD)) || work->next != work)
kernel/trace/bpf_trace.c:                    current->flags & (PF_KTHREAD | PF_EXITING)))
kernel/trace/bpf_trace.c:       if (unlikely(task->flags & (PF_KTHREAD | PF_EXITING)))

Maybe we can have a: is_user_exiting_or_kthread() ?

Note, for coccinelle patches, I would wait till the end of the merge
window, run the scripts on what's in Linus's tree, run my tests, and
then submit. This way it catches most of the conversions with the least
amount of conflicts.

-- Steve

* Steven Rostedt <rostedt@goodmis.org> wrote:

> On Fri, 25 Apr 2025 16:14:49 -0700
> Andrew Morton <akpm@linux-foundation.org> wrote:
> 
> > Seems sensible.  Please consider renaming PF_KTHREAD in order to break
> > missed conversion sites.
> 
> It's not wrong to use the thread. I just find using these helper
> functions a bit easier to review code. There's also some places that
> have special tests where it can't use the flag:
> 
> kernel/sched/core.c:    if (!curr->mm || (curr->flags & (PF_EXITING | PF_KTHREAD)) ||
> kernel/sched/fair.c:    if (!curr->mm || (curr->flags & (PF_EXITING | PF_KTHREAD)) || work->next != work)
> kernel/trace/bpf_trace.c:                    current->flags & (PF_KTHREAD | PF_EXITING)))
> kernel/trace/bpf_trace.c:       if (unlikely(task->flags & (PF_KTHREAD | PF_EXITING)))
> 
> Maybe we can have a: is_user_exiting_or_kthread() ?

No, we don't need is_user_exiting_or_kthread(). At all. Ever. In this 
universe. Or in any alternative universes. We don't even need 
is_user_exiting_or_kthread() in horror fiction novels written for 
kernel developers: there's really a limit to the level of horror that 
people are able to accept. Sheesh ...

This:

	if (task_kthread(task) || task_exiting(task))
		...

is a perfectly fine and readable C expression.

There's also *zero* reason for the whole is_*() complication your 
methods introduce. There's no need for 'is_' if we put a proper noun as 
a prefix before these methods, such as task_*(), and it also nicely 
organizes the namespace!

Let's not complicate trivial use of C logical operators unnecessarily, 
and let's not suck at namespaces more than we need to, 'kay?

( What's next, are we going to redefine 'if (x)' statements???
  ... wait a minute ... ;-)

The attached patch does the sane thing and introduces the following 
helpers for PF_ flags:

	/*
	 * Helpers for PF_ flags:
	 */
	#define task_vcpu(task)				((task)->flags & PF_VCPU)
	#define task_idle(task)				((task)->flags & PF_IDLE)
	#define task_exiting(task)			((task)->flags & PF_EXITING)
	#define task_postcoredump(task)			((task)->flags & PF_POSTCOREDUMP)
	#define task_io_worker(task)			((task)->flags & PF_IO_WORKER)
	#define task_wq_worker(task)			((task)->flags & PF_WQ_WORKER)
	#define task_forknoexec(task)			((task)->flags & PF_FORKNOEXEC)
	#define task_mce_process(task)			((task)->flags & PF_MCE_PROCESS)
	#define task_superpriv(task)			((task)->flags & PF_SUPERPRIV)
	#define task_dumpcore(task)			((task)->flags & PF_DUMPCORE)
	#define task_signaled(task)			((task)->flags & PF_SIGNALED)
	#define task_memalloc(task)			((task)->flags & PF_MEMALLOC)
	#define task_nproc_exceeded(task)		((task)->flags & PF_NPROC_EXCEEDED)
	#define task_used_math(task)			((task)->flags & PF_USED_MATH)
	#define task_user_worker(task)			((task)->flags & PF_USER_WORKER)
	#define task_nofreeze(task)			((task)->flags & PF_NOFREEZE)
	#define task_kcompactd(task)			((task)->flags & PF_KCOMPACTD)
	#define task_kswapd(task)			((task)->flags & PF_KSWAPD)
	#define task_memalloc_nofs(task)		((task)->flags & PF_MEMALLOC_NOFS)
	#define task_memalloc_noio(task)		((task)->flags & PF_MEMALLOC_NOIO)
	#define task_local_throttle(task)		((task)->flags & PF_LOCAL_THROTTLE)
	#define task_kthread(task)			((task)->flags & PF_KTHREAD)
	#define task_randomize(task)			((task)->flags & PF_RANDOMIZE)
	#define task_no_setaffinity(task)		((task)->flags & PF_NO_SETAFFINITY)
	#define task_mce_early(task)			((task)->flags & PF_MCE_EARLY)
	#define task_memalloc_pin(task)			((task)->flags & PF_MEMALLOC_PIN)
	#define task_block_ts(task)			((task)->flags & PF_BLOCK_TS)
	#define task_suspend_task(task)			((task)->flags & PF_SUSPEND_TASK)

These are easy names, precise lower-case variants of the PF_ flag 
names: if you know the PF flag's name, you know the helper's name as 
well.

And no, we don't need separate helpers for !task_kthread() et al: the C 
logical negation unary operator is perfectly readable when placed 
before a function call or a macro invocation, and a competent Linux 
kernel developer is expected to recognize it on sight:

	if (!task_kthread(task))
		...

Let's not infantilize the kernel source...

You can find this patch in my tree at:

	git://git.kernel.org/pub/scm/linux/kernel/git/mingo/tip.git WIP.sched/core

Note that I plan to add a few more patches as well, while we are 
touching this area:

 - tsk_used_math() is now overlapping and should be consolidated a bit. 
   And the used_math() indirection should go away as well.

 - I'll add conversion patches as well, at minimum for the top 10 flags 
   that cover 90%+ of the PF_ flag usage:

             nr of uses | flag
             -------------------------
		      7 | PF_USED_MATH
		      8 | PF_USER_WORKER
		     10   PF_NOFREEZE
		     15   PF_WQ_WORKER
		     18   PF_NO_SETAFFINITY
		     18   PF_VCPU
		     24   PF_RANDOMIZE
		     30   PF_MEMALLOC
		     73   PF_EXITING
		     92   PF_KTHREAD

   [ Or maybe for all of them - see below wrt. set_task_*(). ]

 - PF_ goes for 'per-process flag' and as such it is a total misnomer, 
   proudly misrepresented in <linux/sched.h>:

	/*
	 * Per process flags
	 */
	#define PF_VCPU                 0x00000001      /* I'm a virtual CPU */
	#define PF_IDLE                 0x00000002      /* I am an IDLE thread */

   Yeah, no: the PF_ flags haven't been "per process" ever since we 
   introduced threading 25 years ago...

   Renaming just for that causes churn, but it becomes easier once we 
   have the conversion patches for helpers for explicit uses of PF_ 
   flags.

 - We might want to add set_task_*() helpers as well, to totally 
   encapsulate PF_ uses. Maybe. I dislike how close it is to the 
   existing set_tsk*() methods that manipulate TIF_ flags. The 
   dichotomy between the TIF_ and PF_ space isn't really sensible these 
   days I think on a conceptual level - although merging them is 
   probably not practical due to possibly running out of easy 64-bit 
   word width.

I'll send out a full series if there's no better suggestions for the 
general approach.

Thanks,

	Ingo

============================>
From: Ingo Molnar <mingo@kernel.org>
Date: Sat, 26 Apr 2025 20:00:22 +0200
Subject: [PATCH] sched/core: Introduce task_*() helpers for PF_ flags

Add straightforward PF_ task flag helpers:

	if (!(task->flags & PF_KTHREAD))
		...

                |
               \|/
		V

	if (!task_kthread(task))
		...

( Fortunately there's no namespace collision for any of the new
  methods introduced. )

Suggested-by: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 include/linux/sched.h | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index f96ac1982893..ef5a2e98dd9e 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1747,6 +1747,38 @@ extern struct pid *cad_pid;
 #define PF__HOLE__40000000	0x40000000
 #define PF_SUSPEND_TASK		0x80000000      /* This thread called freeze_processes() and should not be frozen */
 
+/*
+ * Helpers for PF_ flags:
+ */
+#define task_vcpu(task)				((task)->flags & PF_VCPU)
+#define task_idle(task)				((task)->flags & PF_IDLE)
+#define task_exiting(task)			((task)->flags & PF_EXITING)
+#define task_postcoredump(task)			((task)->flags & PF_POSTCOREDUMP)
+#define task_io_worker(task)			((task)->flags & PF_IO_WORKER)
+#define task_wq_worker(task)			((task)->flags & PF_WQ_WORKER)
+#define task_forknoexec(task)			((task)->flags & PF_FORKNOEXEC)
+#define task_mce_process(task)			((task)->flags & PF_MCE_PROCESS)
+#define task_superpriv(task)			((task)->flags & PF_SUPERPRIV)
+#define task_dumpcore(task)			((task)->flags & PF_DUMPCORE)
+#define task_signaled(task)			((task)->flags & PF_SIGNALED)
+#define task_memalloc(task)			((task)->flags & PF_MEMALLOC)
+#define task_nproc_exceeded(task)		((task)->flags & PF_NPROC_EXCEEDED)
+#define task_used_math(task)			((task)->flags & PF_USED_MATH)
+#define task_user_worker(task)			((task)->flags & PF_USER_WORKER)
+#define task_nofreeze(task)			((task)->flags & PF_NOFREEZE)
+#define task_kcompactd(task)			((task)->flags & PF_KCOMPACTD)
+#define task_kswapd(task)			((task)->flags & PF_KSWAPD)
+#define task_memalloc_nofs(task)		((task)->flags & PF_MEMALLOC_NOFS)
+#define task_memalloc_noio(task)		((task)->flags & PF_MEMALLOC_NOIO)
+#define task_local_throttle(task)		((task)->flags & PF_LOCAL_THROTTLE)
+#define task_kthread(task)			((task)->flags & PF_KTHREAD)
+#define task_randomize(task)			((task)->flags & PF_RANDOMIZE)
+#define task_no_setaffinity(task)		((task)->flags & PF_NO_SETAFFINITY)
+#define task_mce_early(task)			((task)->flags & PF_MCE_EARLY)
+#define task_memalloc_pin(task)			((task)->flags & PF_MEMALLOC_PIN)
+#define task_block_ts(task)			((task)->flags & PF_BLOCK_TS)
+#define task_suspend_task(task)			((task)->flags & PF_SUSPEND_TASK)
+
 /*
  * Only the _current_ task can read/write to tsk->flags, but other
  * tasks can access tsk->flags in readonly mode for example

On Sat, 26 Apr 2025 20:42:21 +0200
Ingo Molnar <mingo@kernel.org> wrote:

> I'll send out a full series if there's no better suggestions for the 
> general approach.

Fine, you can take over. I have other things to work on.

Unless you add a task_user() or something similar that makes it very easy
to see that a task is a user thread other than testing if it's not a kernel
thread, it doesn't solve the issue that was my original motivation for my
patches. That is, there's places in the code that needs to only work on
user threads, and it would be nice to quickly see that the if statement is
checking if it is or not.

We have places that check if it's a kernel thread to exit out early:

	if (task_kernel(task))
		goto out;

And places that do something if it's a user thread:

	if (task_user(task))
		// do something special

By explicitly stating "user" in the test, makes that very easy to see,
where as:

	if (task_kernel(task))
		goto out;

and

	if (!task_kernel(task))
		// do something special

makes you have to look a bit harder. The kernel is complex enough, I
believe we should make it easier where there's low hanging fruit to do so.

-- Steve

On Sat, 26 Apr 2025 20:42:21 +0200
Ingo Molnar <mingo@kernel.org> wrote:


> > kernel/sched/core.c:    if (!curr->mm || (curr->flags & (PF_EXITING | PF_KTHREAD)) ||
> > kernel/sched/fair.c:    if (!curr->mm || (curr->flags & (PF_EXITING | PF_KTHREAD)) || work->next != work)
> > kernel/trace/bpf_trace.c:                    current->flags & (PF_KTHREAD | PF_EXITING)))
> > kernel/trace/bpf_trace.c:       if (unlikely(task->flags & (PF_KTHREAD | PF_EXITING)))
> > 
> > Maybe we can have a: is_user_exiting_or_kthread() ?  
> 
> No, we don't need is_user_exiting_or_kthread(). At all. Ever. In this 
> universe. Or in any alternative universes. We don't even need 
> is_user_exiting_or_kthread() in horror fiction novels written for 
> kernel developers: there's really a limit to the level of horror that 
> people are able to accept. Sheesh ...

Ingo,

A simple "No we do not need that" would suffice. This isn't 2005 anymore,
where we come up with creative ways to insult each other. We're better now.



> And no, we don't need separate helpers for !task_kthread() et al: the C 
> logical negation unary operator is perfectly readable when placed 
> before a function call or a macro invocation, and a competent Linux 
> kernel developer is expected to recognize it on sight:
> 
> 	if (!task_kthread(task))
> 		...

Not really. I originally tried just having a single "is_kernel_thread()"
where I would use the "!is_kernel_thread()" for user thread, but honestly,
it wasn't much better than the "!(task->flags & PF_KTHREAD)".

And just because it's not a kernel thread, does it mean it will always be a
user space thread? Could it one day also be a guest thread (if we decide to
have such a thing)?

Wanting to know if something is a user space thread, "if (!task_kthread(task))"
seems short sighted. As it assumes that we only have two types of threads.
It may be true today, but may not be the case in the future.

	if (!task_kthread(task))

Still takes a second more to understand that's a user space thread than:

	if (task_user(task))

would.

-- Steve

* Ingo Molnar <mingo@kernel.org> wrote:

>  - We might want to add set_task_*() helpers as well, to totally 
>    encapsulate PF_ uses. Maybe. I dislike how close it is to the 
>    existing set_tsk*() methods that manipulate TIF_ flags. The 
>    dichotomy between the TIF_ and PF_ space isn't really sensible these 
>    days I think on a conceptual level - although merging them is 
>    probably not practical due to possibly running out of easy 64-bit 
>    word width.

And yeah, the TIF_ space is per arch to a substantial degree, and is 
accessed from assembly code, plus is often operated on atomically, 
while the PF_ space is nicely generic and non-atomic - but still we 
could do better to express that these two per task flag spaces are 
rather similar in purpose, instead of this historic 'task/process' 
distinction that isn't actually true.

Thanks,

	Ingo

On Fri, 25 Apr 2025, Andrew Morton wrote:

> On Fri, 25 Apr 2025 16:41:20 -0400 Steven Rostedt <rostedt@goodmis.org> wrote:
>
> > While working on the deferred stacktrace code, Peter Zijlstra told
> > me to use task->flags & PF_KTHREAD instead of checking task->mm for NULL.
> > This seemed reasonable, but while working on it, as there were several
> > places that check if the task is a kernel thread and other places that
> > check if the task is a user space thread I found it a bit confusing
> > when looking at both:
> >
> > 	if (task->flags & PF_KTHREAD)
> > and
> > 	if (!(task->flags & PF_KTHREAD))
> >
> > Where I mixed them up sometimes, and checked for a user space thread when I
> > really wanted to check for a kernel thread. I found these mistakes before
> > sending out my patches, but going back and reviewing the code, I always had
> > to stop and spend a few unnecessary seconds making sure the check was
> > testing that flag correctly.
> >
> > To make this a bit more obvious, I introduced two helper functions:
> >
> > 	is_user_thread(task)
> > 	is_kernel_thread(task)
> >
> > which simply test the flag for you. Thus, seeing:
> >
> > 	if (is_user_thread(task))
> > or
> > 	if (is_kernel_thread(task))
> >
> > it was very obvious to which test you wanted to make.
>
> Seems sensible.  Please consider renaming PF_KTHREAD in order to break
> missed conversion sites.

Maybe:

@r depends on !(file in "include/linux/sched.h")@ // Kees's suggestion
position p;
expression e;
@@

(
e = (PF_KTHREAD | ...)
|
e |= (PF_KTHREAD | ...)
|
PF_KTHREAD@p
)

@script:ocaml@ // change to python if desired
p << r.p;
@@

Printf.printf "%s:%d: Warning: remaining use of PF_KTHREAD\n" (List.hd p).file (List.hd p).line

julia