[1/9] sched/core: add is_kthread() helper

Message ID 20190814104131.20190-2-mark.rutland@arm.com
State New
Headers show
Series
  • kthread detection cleanup
Related show

Commit Message

Mark Rutland Aug. 14, 2019, 10:41 a.m.
Code checking whether a task is a kthread isn't very consistent. Some
code correctly tests task->flags & PF_THREAD, while other code checks
task->mm (which can be true for a kthread which calls use_mm()).

So that we can clean this up and keep the code easy to follow, let's add
an obvious helper function to test whether a task is a kthread.
Subsequent patches will use this as part of cleaning up and correcting
open-coded tests.

At the same time, let's fix up the kerneldoc for is_idle_task() for
consistency with the new helper, using true/false rather than 0/1, given
the functions return bool.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>

Cc: Ingo Molnar <mingo@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
---
 include/linux/sched.h | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

-- 
2.11.0

Comments

Geert Uytterhoeven Aug. 14, 2019, 11:26 a.m. | #1
Hi Mark,

On Wed, Aug 14, 2019 at 12:43 PM Mark Rutland <mark.rutland@arm.com> wrote:
> Code checking whether a task is a kthread isn't very consistent. Some

> code correctly tests task->flags & PF_THREAD, while other code checks

> task->mm (which can be true for a kthread which calls use_mm()).

>

> So that we can clean this up and keep the code easy to follow, let's add

> an obvious helper function to test whether a task is a kthread.

> Subsequent patches will use this as part of cleaning up and correcting

> open-coded tests.

>

> At the same time, let's fix up the kerneldoc for is_idle_task() for

> consistency with the new helper, using true/false rather than 0/1, given

> the functions return bool.

>

> Signed-off-by: Mark Rutland <mark.rutland@arm.com>


Thanks for your patch!

> --- a/include/linux/sched.h

> +++ b/include/linux/sched.h

> @@ -1621,13 +1621,24 @@ extern struct task_struct *idle_task(int cpu);

>   * is_idle_task - is the specified task an idle task?

>   * @p: the task in question.

>   *

> - * Return: 1 if @p is an idle task. 0 otherwise.

> + * Return: true if @p is an idle task, false otherwise.

>   */

>  static inline bool is_idle_task(const struct task_struct *p)

>  {

>         return !!(p->flags & PF_IDLE);

>  }

>

> +/**

> + * is_kthread - is the specified task a kthread

> + * @p: the task in question.

> + *

> + * Return: true if @p is a kthread, false otherwise.

> + */

> +static inline bool is_kthread(const struct task_struct *p)

> +{

> +       return !!(p->flags & PF_KTHREAD);


The !! is not really needed.
Probably you followed is_idle_task() above (where it's also not needed).

> +}

> +


Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
Mark Rutland Aug. 14, 2019, 11:32 a.m. | #2
On Wed, Aug 14, 2019 at 01:26:43PM +0200, Geert Uytterhoeven wrote:
> Hi Mark,

> 

> On Wed, Aug 14, 2019 at 12:43 PM Mark Rutland <mark.rutland@arm.com> wrote:

> > Code checking whether a task is a kthread isn't very consistent. Some

> > code correctly tests task->flags & PF_THREAD, while other code checks

> > task->mm (which can be true for a kthread which calls use_mm()).

> >

> > So that we can clean this up and keep the code easy to follow, let's add

> > an obvious helper function to test whether a task is a kthread.

> > Subsequent patches will use this as part of cleaning up and correcting

> > open-coded tests.

> >

> > At the same time, let's fix up the kerneldoc for is_idle_task() for

> > consistency with the new helper, using true/false rather than 0/1, given

> > the functions return bool.

> >

> > Signed-off-by: Mark Rutland <mark.rutland@arm.com>

> 

> Thanks for your patch!

> 

> > --- a/include/linux/sched.h

> > +++ b/include/linux/sched.h

> > @@ -1621,13 +1621,24 @@ extern struct task_struct *idle_task(int cpu);

> >   * is_idle_task - is the specified task an idle task?

> >   * @p: the task in question.

> >   *

> > - * Return: 1 if @p is an idle task. 0 otherwise.

> > + * Return: true if @p is an idle task, false otherwise.

> >   */

> >  static inline bool is_idle_task(const struct task_struct *p)

> >  {

> >         return !!(p->flags & PF_IDLE);

> >  }

> >

> > +/**

> > + * is_kthread - is the specified task a kthread

> > + * @p: the task in question.

> > + *

> > + * Return: true if @p is a kthread, false otherwise.

> > + */

> > +static inline bool is_kthread(const struct task_struct *p)

> > +{

> > +       return !!(p->flags & PF_KTHREAD);

> 

> The !! is not really needed.

> Probably you followed is_idle_task() above (where it's also not needed).


Indeed! I'm aware of the implicit bool conversion, but kept that for
consistency.

Peter, Ingo, do you have a preference?

Thanks,
Mark.
Ingo Molnar Aug. 19, 2019, 8:52 a.m. | #3
* Mark Rutland <mark.rutland@arm.com> wrote:

> On Wed, Aug 14, 2019 at 01:26:43PM +0200, Geert Uytterhoeven wrote:

> > Hi Mark,

> > 

> > On Wed, Aug 14, 2019 at 12:43 PM Mark Rutland <mark.rutland@arm.com> wrote:

> > > Code checking whether a task is a kthread isn't very consistent. Some

> > > code correctly tests task->flags & PF_THREAD, while other code checks

> > > task->mm (which can be true for a kthread which calls use_mm()).

> > >

> > > So that we can clean this up and keep the code easy to follow, let's add

> > > an obvious helper function to test whether a task is a kthread.

> > > Subsequent patches will use this as part of cleaning up and correcting

> > > open-coded tests.

> > >

> > > At the same time, let's fix up the kerneldoc for is_idle_task() for

> > > consistency with the new helper, using true/false rather than 0/1, given

> > > the functions return bool.

> > >

> > > Signed-off-by: Mark Rutland <mark.rutland@arm.com>

> > 

> > Thanks for your patch!

> > 

> > > --- a/include/linux/sched.h

> > > +++ b/include/linux/sched.h

> > > @@ -1621,13 +1621,24 @@ extern struct task_struct *idle_task(int cpu);

> > >   * is_idle_task - is the specified task an idle task?

> > >   * @p: the task in question.

> > >   *

> > > - * Return: 1 if @p is an idle task. 0 otherwise.

> > > + * Return: true if @p is an idle task, false otherwise.

> > >   */

> > >  static inline bool is_idle_task(const struct task_struct *p)

> > >  {

> > >         return !!(p->flags & PF_IDLE);

> > >  }

> > >

> > > +/**

> > > + * is_kthread - is the specified task a kthread

> > > + * @p: the task in question.

> > > + *

> > > + * Return: true if @p is a kthread, false otherwise.

> > > + */

> > > +static inline bool is_kthread(const struct task_struct *p)

> > > +{

> > > +       return !!(p->flags & PF_KTHREAD);

> > 

> > The !! is not really needed.

> > Probably you followed is_idle_task() above (where it's also not needed).

> 

> Indeed! I'm aware of the implicit bool conversion, but kept that for

> consistency.

> 

> Peter, Ingo, do you have a preference?


So the !! pattern is useful where the return value is an integer (i.e. 
there's a risk of non-bool use) - but the return value is an explicit 
bool here, so !! is IMO an entirely superfluous obfuscation.

Should probably be fixed for is_idle_task() as well?

Thanks,

	Ingo
Peter Zijlstra Aug. 21, 2019, 11:21 a.m. | #4
On Mon, Aug 19, 2019 at 10:52:13AM +0200, Ingo Molnar wrote:
> * Mark Rutland <mark.rutland@arm.com> wrote:

> > On Wed, Aug 14, 2019 at 01:26:43PM +0200, Geert Uytterhoeven wrote:

> > > On Wed, Aug 14, 2019 at 12:43 PM Mark Rutland <mark.rutland@arm.com> wrote:


> > > > +static inline bool is_kthread(const struct task_struct *p)

> > > > +{

> > > > +       return !!(p->flags & PF_KTHREAD);

> > > 

> > > The !! is not really needed.

> > > Probably you followed is_idle_task() above (where it's also not needed).

> > 

> > Indeed! I'm aware of the implicit bool conversion, but kept that for

> > consistency.

> > 

> > Peter, Ingo, do you have a preference?

> 

> So the !! pattern is useful where the return value is an integer (i.e. 

> there's a risk of non-bool use) - but the return value is an explicit 

> bool here, so !! is IMO an entirely superfluous obfuscation.


Yeah, no real preference, for giggles, (_Bool) also seems to work.

Patch

diff --git a/include/linux/sched.h b/include/linux/sched.h
index 9f51932bd543..b7e96409d75f 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1621,13 +1621,24 @@  extern struct task_struct *idle_task(int cpu);
  * is_idle_task - is the specified task an idle task?
  * @p: the task in question.
  *
- * Return: 1 if @p is an idle task. 0 otherwise.
+ * Return: true if @p is an idle task, false otherwise.
  */
 static inline bool is_idle_task(const struct task_struct *p)
 {
 	return !!(p->flags & PF_IDLE);
 }
 
+/**
+ * is_kthread - is the specified task a kthread
+ * @p: the task in question.
+ *
+ * Return: true if @p is a kthread, false otherwise.
+ */
+static inline bool is_kthread(const struct task_struct *p)
+{
+	return !!(p->flags & PF_KTHREAD);
+}
+
 extern struct task_struct *curr_task(int cpu);
 extern void ia64_set_curr_task(int cpu, struct task_struct *p);