[PATCH v2 0/5] preempt: PREEMPT vs PREEMPT_DYNAMIC configs fixup

[PATCH v2 0/5] preempt: PREEMPT vs PREEMPT_DYNAMIC configs fixup

[Valentin Schneider]

Hi folks,

Thanks to suggestions from Mike, Frederic and Marco I ended up with
something that looks somewhat sane and with a minimal amount of crud.

Patches
=======

o Patch 1 is the meat of the topic and could be picked on its own if the
  rest is too icky.
o Patch 2 introduces helpers for the dynamic preempt state
o Patches 3-5 make use of said accessors where relevant.

Testing
=======

Briefly tested the dynamic part on an x86 kernel + QEMU. x86_64_defconfig
gets me:

  Dynamic Preempt: voluntary

and appending preempt=full gets me:

  Dynamic Preempt: full

Revisions
=========

v1: http://lore.kernel.org/r/20211105104035.3112162-1-valentin.schneider@arm.com
v1.5: http://lore.kernel.org/r/20211109151057.3489223-1-valentin.schneider@arm.com

This v2 is completely different from v1, so I felt like I could get away
without writing a version changelog...

Cheers,
Valentin

Valentin Schneider (5):
  preempt: Restore preemption model selection configs
  preempt/dynamic: Introduce preempt mode accessors
  powerpc: Use preemption model accessors
  kscan: Use preemption model accessors
  ftrace: Use preemption model accessors for trace header printout

 arch/powerpc/kernel/interrupt.c |  2 +-
 arch/powerpc/kernel/traps.c     |  2 +-
 include/linux/kernel.h          |  2 +-
 include/linux/sched.h           | 16 +++++++++++++
 include/linux/vermagic.h        |  2 +-
 init/Makefile                   |  2 +-
 kernel/Kconfig.preempt          | 42 ++++++++++++++++-----------------
 kernel/kcsan/kcsan_test.c       |  4 ++--
 kernel/sched/core.c             | 17 ++++++++++---
 kernel/trace/trace.c            | 14 ++++-------
 10 files changed, 62 insertions(+), 41 deletions(-)

--
2.25.1

[PATCH v2 1/5] preempt: Restore preemption model selection configs

[Valentin Schneider]

Commit c597bfddc9e9 ("sched: Provide Kconfig support for default dynamic
preempt mode") changed the selectable config names for the preemption
model. This means a config file must now select

  CONFIG_PREEMPT_BEHAVIOUR=y

rather than

  CONFIG_PREEMPT=y

to get a preemptible kernel. This means all arch config files would need to
be updated - right now they'll all end up with the default
CONFIG_PREEMPT_NONE_BEHAVIOUR.

Rather than touch a good hundred of config files, restore usage of
CONFIG_PREEMPT{_NONE, _VOLUNTARY}. Make them configure:
o The build-time preemption model when !PREEMPT_DYNAMIC
o The default boot-time preemption model when PREEMPT_DYNAMIC

Add siblings of those configs with the _BUILD suffix to unconditionally
designate the build-time preemption model (PREEMPT_DYNAMIC is built with
the "highest" preemption model it supports, aka PREEMPT). Downstream
configs should by now all be depending / selected by CONFIG_PREEMPTION
rather than CONFIG_PREEMPT, so only a few sites need patching up.

Signed-off-by: Valentin Schneider <valentin.schneider@arm.com>
---
 include/linux/kernel.h   |  2 +-
 include/linux/vermagic.h |  2 +-
 init/Makefile            |  2 +-
 kernel/Kconfig.preempt   | 42 ++++++++++++++++++++--------------------
 kernel/sched/core.c      |  6 +++---
 5 files changed, 27 insertions(+), 27 deletions(-)

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 2776423a587e..9c7d774ef809 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -88,7 +88,7 @@
 struct completion;
 struct user;
 
-#ifdef CONFIG_PREEMPT_VOLUNTARY
+#ifdef CONFIG_PREEMPT_VOLUNTARY_BUILD
 
 extern int __cond_resched(void);
 # define might_resched() __cond_resched()
diff --git a/include/linux/vermagic.h b/include/linux/vermagic.h
index 1eaaa93c37bf..329d63babaeb 100644
--- a/include/linux/vermagic.h
+++ b/include/linux/vermagic.h
@@ -15,7 +15,7 @@
 #else
 #define MODULE_VERMAGIC_SMP ""
 #endif
-#ifdef CONFIG_PREEMPT
+#ifdef CONFIG_PREEMPT_BUILD
 #define MODULE_VERMAGIC_PREEMPT "preempt "
 #elif defined(CONFIG_PREEMPT_RT)
 #define MODULE_VERMAGIC_PREEMPT "preempt_rt "
diff --git a/init/Makefile b/init/Makefile
index 2846113677ee..04eeee12c076 100644
--- a/init/Makefile
+++ b/init/Makefile
@@ -30,7 +30,7 @@ $(obj)/version.o: include/generated/compile.h
 quiet_cmd_compile.h = CHK     $@
       cmd_compile.h = \
 	$(CONFIG_SHELL) $(srctree)/scripts/mkcompile_h $@	\
-	"$(UTS_MACHINE)" "$(CONFIG_SMP)" "$(CONFIG_PREEMPT)"	\
+	"$(UTS_MACHINE)" "$(CONFIG_SMP)" "$(CONFIG_PREEMPT_BUILD)"	\
 	"$(CONFIG_PREEMPT_RT)" $(CONFIG_CC_VERSION_TEXT) "$(LD)"
 
 include/generated/compile.h: FORCE
diff --git a/kernel/Kconfig.preempt b/kernel/Kconfig.preempt
index 60f1bfc3c7b2..ce77f0265660 100644
--- a/kernel/Kconfig.preempt
+++ b/kernel/Kconfig.preempt
@@ -1,12 +1,23 @@
 # SPDX-License-Identifier: GPL-2.0-only
 
+config PREEMPT_NONE_BUILD
+	bool
+
+config PREEMPT_VOLUNTARY_BUILD
+	bool
+
+config PREEMPT_BUILD
+	bool
+	select PREEMPTION
+	select UNINLINE_SPIN_UNLOCK if !ARCH_INLINE_SPIN_UNLOCK
+
 choice
 	prompt "Preemption Model"
-	default PREEMPT_NONE_BEHAVIOUR
+	default PREEMPT_NONE
 
-config PREEMPT_NONE_BEHAVIOUR
+config PREEMPT_NONE
 	bool "No Forced Preemption (Server)"
-	select PREEMPT_NONE if !PREEMPT_DYNAMIC
+	select PREEMPT_NONE_BUILD if !PREEMPT_DYNAMIC
 	help
 	  This is the traditional Linux preemption model, geared towards
 	  throughput. It will still provide good latencies most of the
@@ -18,10 +29,10 @@ config PREEMPT_NONE_BEHAVIOUR
 	  raw processing power of the kernel, irrespective of scheduling
 	  latencies.
 
-config PREEMPT_VOLUNTARY_BEHAVIOUR
+config PREEMPT_VOLUNTARY
 	bool "Voluntary Kernel Preemption (Desktop)"
 	depends on !ARCH_NO_PREEMPT
-	select PREEMPT_VOLUNTARY if !PREEMPT_DYNAMIC
+	select PREEMPT_VOLUNTARY_BUILD if !PREEMPT_DYNAMIC
 	help
 	  This option reduces the latency of the kernel by adding more
 	  "explicit preemption points" to the kernel code. These new
@@ -37,10 +48,10 @@ config PREEMPT_VOLUNTARY_BEHAVIOUR
 
 	  Select this if you are building a kernel for a desktop system.
 
-config PREEMPT_BEHAVIOUR
+config PREEMPT
 	bool "Preemptible Kernel (Low-Latency Desktop)"
 	depends on !ARCH_NO_PREEMPT
-	select PREEMPT
+	select PREEMPT_BUILD
 	help
 	  This option reduces the latency of the kernel by making
 	  all kernel code (that is not executing in a critical section)
@@ -58,7 +69,7 @@ config PREEMPT_BEHAVIOUR
 
 config PREEMPT_RT
 	bool "Fully Preemptible Kernel (Real-Time)"
-	depends on EXPERT && ARCH_SUPPORTS_RT && !PREEMPT_DYNAMIC
+	depends on EXPERT && ARCH_SUPPORTS_RT
 	select PREEMPTION
 	help
 	  This option turns the kernel into a real-time kernel by replacing
@@ -75,17 +86,6 @@ config PREEMPT_RT
 
 endchoice
 
-config PREEMPT_NONE
-	bool
-
-config PREEMPT_VOLUNTARY
-	bool
-
-config PREEMPT
-	bool
-	select PREEMPTION
-	select UNINLINE_SPIN_UNLOCK if !ARCH_INLINE_SPIN_UNLOCK
-
 config PREEMPT_COUNT
        bool
 
@@ -95,8 +95,8 @@ config PREEMPTION
 
 config PREEMPT_DYNAMIC
 	bool "Preemption behaviour defined on boot"
-	depends on HAVE_PREEMPT_DYNAMIC
-	select PREEMPT
+	depends on HAVE_PREEMPT_DYNAMIC && !PREEMPT_RT
+	select PREEMPT_BUILD
 	default y
 	help
 	  This option allows to define the preemption model on the kernel
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index f2611b9cf503..97047aa7b6c2 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -6625,13 +6625,13 @@ __setup("preempt=", setup_preempt_mode);
 static void __init preempt_dynamic_init(void)
 {
 	if (preempt_dynamic_mode == preempt_dynamic_undefined) {
-		if (IS_ENABLED(CONFIG_PREEMPT_NONE_BEHAVIOUR)) {
+		if (IS_ENABLED(CONFIG_PREEMPT_NONE)) {
 			sched_dynamic_update(preempt_dynamic_none);
-		} else if (IS_ENABLED(CONFIG_PREEMPT_VOLUNTARY_BEHAVIOUR)) {
+		} else if (IS_ENABLED(CONFIG_PREEMPT_VOLUNTARY)) {
 			sched_dynamic_update(preempt_dynamic_voluntary);
 		} else {
 			/* Default static call setting, nothing to do */
-			WARN_ON_ONCE(!IS_ENABLED(CONFIG_PREEMPT_BEHAVIOUR));
+			WARN_ON_ONCE(!IS_ENABLED(CONFIG_PREEMPT));
 			preempt_dynamic_mode = preempt_dynamic_full;
 			pr_info("Dynamic Preempt: full\n");
 		}
-- 
2.25.1

Re: [PATCH v2 1/5] preempt: Restore preemption model selection configs

[Marco Elver]

On Wed, Nov 10, 2021 at 08:24PM +0000, Valentin Schneider wrote:
> Commit c597bfddc9e9 ("sched: Provide Kconfig support for default dynamic
> preempt mode") changed the selectable config names for the preemption
> model. This means a config file must now select
> 
>   CONFIG_PREEMPT_BEHAVIOUR=y
> 
> rather than
> 
>   CONFIG_PREEMPT=y
> 
> to get a preemptible kernel. This means all arch config files would need to
> be updated - right now they'll all end up with the default
> CONFIG_PREEMPT_NONE_BEHAVIOUR.
> 
> Rather than touch a good hundred of config files, restore usage of
> CONFIG_PREEMPT{_NONE, _VOLUNTARY}. Make them configure:
> o The build-time preemption model when !PREEMPT_DYNAMIC
> o The default boot-time preemption model when PREEMPT_DYNAMIC
> 
> Add siblings of those configs with the _BUILD suffix to unconditionally
> designate the build-time preemption model (PREEMPT_DYNAMIC is built with
> the "highest" preemption model it supports, aka PREEMPT). Downstream
> configs should by now all be depending / selected by CONFIG_PREEMPTION
> rather than CONFIG_PREEMPT, so only a few sites need patching up.
> 
> Signed-off-by: Valentin Schneider <valentin.schneider@arm.com>

Acked-by: Marco Elver <elver@google.com>

Much better, thank you!

> ---
>  include/linux/kernel.h   |  2 +-
>  include/linux/vermagic.h |  2 +-
>  init/Makefile            |  2 +-
>  kernel/Kconfig.preempt   | 42 ++++++++++++++++++++--------------------
>  kernel/sched/core.c      |  6 +++---
>  5 files changed, 27 insertions(+), 27 deletions(-)
> 
> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> index 2776423a587e..9c7d774ef809 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -88,7 +88,7 @@
>  struct completion;
>  struct user;
>  
> -#ifdef CONFIG_PREEMPT_VOLUNTARY
> +#ifdef CONFIG_PREEMPT_VOLUNTARY_BUILD
>  
>  extern int __cond_resched(void);
>  # define might_resched() __cond_resched()
> diff --git a/include/linux/vermagic.h b/include/linux/vermagic.h
> index 1eaaa93c37bf..329d63babaeb 100644
> --- a/include/linux/vermagic.h
> +++ b/include/linux/vermagic.h
> @@ -15,7 +15,7 @@
>  #else
>  #define MODULE_VERMAGIC_SMP ""
>  #endif
> -#ifdef CONFIG_PREEMPT
> +#ifdef CONFIG_PREEMPT_BUILD
>  #define MODULE_VERMAGIC_PREEMPT "preempt "
>  #elif defined(CONFIG_PREEMPT_RT)
>  #define MODULE_VERMAGIC_PREEMPT "preempt_rt "
> diff --git a/init/Makefile b/init/Makefile
> index 2846113677ee..04eeee12c076 100644
> --- a/init/Makefile
> +++ b/init/Makefile
> @@ -30,7 +30,7 @@ $(obj)/version.o: include/generated/compile.h
>  quiet_cmd_compile.h = CHK     $@
>        cmd_compile.h = \
>  	$(CONFIG_SHELL) $(srctree)/scripts/mkcompile_h $@	\
> -	"$(UTS_MACHINE)" "$(CONFIG_SMP)" "$(CONFIG_PREEMPT)"	\
> +	"$(UTS_MACHINE)" "$(CONFIG_SMP)" "$(CONFIG_PREEMPT_BUILD)"	\
>  	"$(CONFIG_PREEMPT_RT)" $(CONFIG_CC_VERSION_TEXT) "$(LD)"
>  
>  include/generated/compile.h: FORCE
> diff --git a/kernel/Kconfig.preempt b/kernel/Kconfig.preempt
> index 60f1bfc3c7b2..ce77f0265660 100644
> --- a/kernel/Kconfig.preempt
> +++ b/kernel/Kconfig.preempt
> @@ -1,12 +1,23 @@
>  # SPDX-License-Identifier: GPL-2.0-only
>  
> +config PREEMPT_NONE_BUILD
> +	bool
> +
> +config PREEMPT_VOLUNTARY_BUILD
> +	bool
> +
> +config PREEMPT_BUILD
> +	bool
> +	select PREEMPTION
> +	select UNINLINE_SPIN_UNLOCK if !ARCH_INLINE_SPIN_UNLOCK
> +
>  choice
>  	prompt "Preemption Model"
> -	default PREEMPT_NONE_BEHAVIOUR
> +	default PREEMPT_NONE
>  
> -config PREEMPT_NONE_BEHAVIOUR
> +config PREEMPT_NONE
>  	bool "No Forced Preemption (Server)"
> -	select PREEMPT_NONE if !PREEMPT_DYNAMIC
> +	select PREEMPT_NONE_BUILD if !PREEMPT_DYNAMIC
>  	help
>  	  This is the traditional Linux preemption model, geared towards
>  	  throughput. It will still provide good latencies most of the
> @@ -18,10 +29,10 @@ config PREEMPT_NONE_BEHAVIOUR
>  	  raw processing power of the kernel, irrespective of scheduling
>  	  latencies.
>  
> -config PREEMPT_VOLUNTARY_BEHAVIOUR
> +config PREEMPT_VOLUNTARY
>  	bool "Voluntary Kernel Preemption (Desktop)"
>  	depends on !ARCH_NO_PREEMPT
> -	select PREEMPT_VOLUNTARY if !PREEMPT_DYNAMIC
> +	select PREEMPT_VOLUNTARY_BUILD if !PREEMPT_DYNAMIC
>  	help
>  	  This option reduces the latency of the kernel by adding more
>  	  "explicit preemption points" to the kernel code. These new
> @@ -37,10 +48,10 @@ config PREEMPT_VOLUNTARY_BEHAVIOUR
>  
>  	  Select this if you are building a kernel for a desktop system.
>  
> -config PREEMPT_BEHAVIOUR
> +config PREEMPT
>  	bool "Preemptible Kernel (Low-Latency Desktop)"
>  	depends on !ARCH_NO_PREEMPT
> -	select PREEMPT
> +	select PREEMPT_BUILD
>  	help
>  	  This option reduces the latency of the kernel by making
>  	  all kernel code (that is not executing in a critical section)
> @@ -58,7 +69,7 @@ config PREEMPT_BEHAVIOUR
>  
>  config PREEMPT_RT
>  	bool "Fully Preemptible Kernel (Real-Time)"
> -	depends on EXPERT && ARCH_SUPPORTS_RT && !PREEMPT_DYNAMIC
> +	depends on EXPERT && ARCH_SUPPORTS_RT
>  	select PREEMPTION
>  	help
>  	  This option turns the kernel into a real-time kernel by replacing
> @@ -75,17 +86,6 @@ config PREEMPT_RT
>  
>  endchoice
>  
> -config PREEMPT_NONE
> -	bool
> -
> -config PREEMPT_VOLUNTARY
> -	bool
> -
> -config PREEMPT
> -	bool
> -	select PREEMPTION
> -	select UNINLINE_SPIN_UNLOCK if !ARCH_INLINE_SPIN_UNLOCK
> -
>  config PREEMPT_COUNT
>         bool
>  
> @@ -95,8 +95,8 @@ config PREEMPTION
>  
>  config PREEMPT_DYNAMIC
>  	bool "Preemption behaviour defined on boot"
> -	depends on HAVE_PREEMPT_DYNAMIC
> -	select PREEMPT
> +	depends on HAVE_PREEMPT_DYNAMIC && !PREEMPT_RT
> +	select PREEMPT_BUILD
>  	default y
>  	help
>  	  This option allows to define the preemption model on the kernel
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index f2611b9cf503..97047aa7b6c2 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -6625,13 +6625,13 @@ __setup("preempt=", setup_preempt_mode);
>  static void __init preempt_dynamic_init(void)
>  {
>  	if (preempt_dynamic_mode == preempt_dynamic_undefined) {
> -		if (IS_ENABLED(CONFIG_PREEMPT_NONE_BEHAVIOUR)) {
> +		if (IS_ENABLED(CONFIG_PREEMPT_NONE)) {
>  			sched_dynamic_update(preempt_dynamic_none);
> -		} else if (IS_ENABLED(CONFIG_PREEMPT_VOLUNTARY_BEHAVIOUR)) {
> +		} else if (IS_ENABLED(CONFIG_PREEMPT_VOLUNTARY)) {
>  			sched_dynamic_update(preempt_dynamic_voluntary);
>  		} else {
>  			/* Default static call setting, nothing to do */
> -			WARN_ON_ONCE(!IS_ENABLED(CONFIG_PREEMPT_BEHAVIOUR));
> +			WARN_ON_ONCE(!IS_ENABLED(CONFIG_PREEMPT));
>  			preempt_dynamic_mode = preempt_dynamic_full;
>  			pr_info("Dynamic Preempt: full\n");
>  		}
> -- 
> 2.25.1
> 

[PATCH v2 2/5] preempt/dynamic: Introduce preempt mode accessors

[Valentin Schneider]

CONFIG_PREEMPT{_NONE, _VOLUNTARY} designate either:
o The build-time preemption model when !PREEMPT_DYNAMIC
o The default boot-time preemption model when PREEMPT_DYNAMIC

IOW, using those on PREEMPT_DYNAMIC kernels is meaningless - the actual
model could have been set to something else by the "preempt=foo" cmdline
parameter.

Introduce a set of helpers to determine the actual preemption mode used by
the live kernel.

Suggested-by: Marco Elver <elver@google.com>
Signed-off-by: Valentin Schneider <valentin.schneider@arm.com>
---
 include/linux/sched.h | 16 ++++++++++++++++
 kernel/sched/core.c   | 11 +++++++++++
 2 files changed, 27 insertions(+)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index 5f8db54226af..0640d5622496 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -2073,6 +2073,22 @@ static inline void cond_resched_rcu(void)
 #endif
 }
 
+#ifdef CONFIG_PREEMPT_DYNAMIC
+
+extern bool is_preempt_none(void);
+extern bool is_preempt_voluntary(void);
+extern bool is_preempt_full(void);
+
+#else
+
+#define is_preempt_none() IS_ENABLED(CONFIG_PREEMPT_NONE)
+#define is_preempt_voluntary() IS_ENABLED(CONFIG_PREEMPT_VOLUNTARY)
+#define is_preempt_full() IS_ENABLED(CONFIG_PREEMPT)
+
+#endif
+
+#define is_preempt_rt() IS_ENABLED(CONFIG_PREEMPT_RT)
+
 /*
  * Does a critical section need to be broken due to another
  * task waiting?: (technically does not depend on CONFIG_PREEMPTION,
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 97047aa7b6c2..9db7f77e53c3 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -6638,6 +6638,17 @@ static void __init preempt_dynamic_init(void)
 	}
 }
 
+#define PREEMPT_MODE_ACCESSOR(mode) \
+	bool is_preempt_##mode(void)						 \
+	{									 \
+		WARN_ON_ONCE(preempt_dynamic_mode == preempt_dynamic_undefined); \
+		return preempt_dynamic_mode == preempt_dynamic_##mode;		 \
+	}
+
+PREEMPT_MODE_ACCESSOR(none)
+PREEMPT_MODE_ACCESSOR(voluntary)
+PREEMPT_MODE_ACCESSOR(full)
+
 #else /* !CONFIG_PREEMPT_DYNAMIC */
 
 static inline void preempt_dynamic_init(void) { }
-- 
2.25.1

Re: [PATCH v2 2/5] preempt/dynamic: Introduce preempt mode accessors

[Mike Galbraith]

On Wed, 2021-11-10 at 20:24 +0000, Valentin Schneider wrote:
>
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index 5f8db54226af..0640d5622496 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -2073,6 +2073,22 @@ static inline void cond_resched_rcu(void)
>  #endif
>  }
>  
> +#ifdef CONFIG_PREEMPT_DYNAMIC
> +
> +extern bool is_preempt_none(void);
> +extern bool is_preempt_voluntary(void);
> +extern bool is_preempt_full(void);
> +
> +#else
> +
> +#define is_preempt_none() IS_ENABLED(CONFIG_PREEMPT_NONE)
> +#define is_preempt_voluntary() IS_ENABLED(CONFIG_PREEMPT_VOLUNTARY)
> +#define is_preempt_full() IS_ENABLED(CONFIG_PREEMPT)

I think that should be IS_ENABLED(CONFIG_PREEMPTION), see c1a280b68d4e.

Noticed while applying the series to an RT tree, where tglx
has done that replacement to the powerpc spot your next patch diddles.

	-Mike

Re: [PATCH v2 2/5] preempt/dynamic: Introduce preempt mode accessors

[Mike Galbraith]

On Thu, 2021-11-11 at 04:16 +0100, Mike Galbraith wrote:
> On Wed, 2021-11-10 at 20:24 +0000, Valentin Schneider wrote:
> >
> > diff --git a/include/linux/sched.h b/include/linux/sched.h
> > index 5f8db54226af..0640d5622496 100644
> > --- a/include/linux/sched.h
> > +++ b/include/linux/sched.h
> > @@ -2073,6 +2073,22 @@ static inline void cond_resched_rcu(void)
> >  #endif
> >  }
> >  
> > +#ifdef CONFIG_PREEMPT_DYNAMIC
> > +
> > +extern bool is_preempt_none(void);
> > +extern bool is_preempt_voluntary(void);
> > +extern bool is_preempt_full(void);
> > +
> > +#else
> > +
> > +#define is_preempt_none() IS_ENABLED(CONFIG_PREEMPT_NONE)
> > +#define is_preempt_voluntary()
> > IS_ENABLED(CONFIG_PREEMPT_VOLUNTARY)
> > +#define is_preempt_full() IS_ENABLED(CONFIG_PREEMPT)
>
> I think that should be IS_ENABLED(CONFIG_PREEMPTION), see
> c1a280b68d4e.
>
> Noticed while applying the series to an RT tree, where tglx
> has done that replacement to the powerpc spot your next patch
> diddles.

Damn, then comes patch 5 properly differentiating PREEMPT/PREEMPT_RT.

	-Mike

Re: [PATCH v2 2/5] preempt/dynamic: Introduce preempt mode accessors

[Mike Galbraith]

On Thu, 2021-11-11 at 04:35 +0100, Mike Galbraith wrote:
> On Thu, 2021-11-11 at 04:16 +0100, Mike Galbraith wrote:
> > On Wed, 2021-11-10 at 20:24 +0000, Valentin Schneider wrote:
> > >
> > > diff --git a/include/linux/sched.h b/include/linux/sched.h
> > > index 5f8db54226af..0640d5622496 100644
> > > --- a/include/linux/sched.h
> > > +++ b/include/linux/sched.h
> > > @@ -2073,6 +2073,22 @@ static inline void cond_resched_rcu(void)
> > >  #endif
> > >  }
> > >  
> > > +#ifdef CONFIG_PREEMPT_DYNAMIC
> > > +
> > > +extern bool is_preempt_none(void);
> > > +extern bool is_preempt_voluntary(void);
> > > +extern bool is_preempt_full(void);
> > > +
> > > +#else
> > > +
> > > +#define is_preempt_none() IS_ENABLED(CONFIG_PREEMPT_NONE)
> > > +#define is_preempt_voluntary()
> > > IS_ENABLED(CONFIG_PREEMPT_VOLUNTARY)
> > > +#define is_preempt_full() IS_ENABLED(CONFIG_PREEMPT)
> >
> > I think that should be IS_ENABLED(CONFIG_PREEMPTION), see
> > c1a280b68d4e.
> >
> > Noticed while applying the series to an RT tree, where tglx
> > has done that replacement to the powerpc spot your next patch
> > diddles.
>
> Damn, then comes patch 5 properly differentiating PREEMPT/PREEMPT_RT.

So I suppose the powerpc spot should remain CONFIG_PREEMPT and become
CONFIG_PREEMPTION when the RT change gets merged, because that spot is
about full preemptibility, not a distinct preemption model.

That's rather annoying :-/

	-Mike

Re: [PATCH v2 2/5] preempt/dynamic: Introduce preempt mode accessors

[Mike Galbraith]

On Thu, 2021-11-11 at 04:47 +0100, Mike Galbraith wrote:
>
> So I suppose the powerpc spot should remain CONFIG_PREEMPT and become
> CONFIG_PREEMPTION when the RT change gets merged, because that spot is
> about full preemptibility, not a distinct preemption model.

KCSAN needs a little help to be usable by RT, but ditto that spot.

	-Mike

Re: [PATCH v2 2/5] preempt/dynamic: Introduce preempt mode accessors

[Marco Elver]

On Thu, 11 Nov 2021 at 04:47, Mike Galbraith <efault@gmx.de> wrote:
>
> On Thu, 2021-11-11 at 04:35 +0100, Mike Galbraith wrote:
> > On Thu, 2021-11-11 at 04:16 +0100, Mike Galbraith wrote:
> > > On Wed, 2021-11-10 at 20:24 +0000, Valentin Schneider wrote:
> > > >
> > > > diff --git a/include/linux/sched.h b/include/linux/sched.h
> > > > index 5f8db54226af..0640d5622496 100644
> > > > --- a/include/linux/sched.h
> > > > +++ b/include/linux/sched.h
> > > > @@ -2073,6 +2073,22 @@ static inline void cond_resched_rcu(void)
> > > >  #endif
> > > >  }
> > > >
> > > > +#ifdef CONFIG_PREEMPT_DYNAMIC
> > > > +
> > > > +extern bool is_preempt_none(void);
> > > > +extern bool is_preempt_voluntary(void);
> > > > +extern bool is_preempt_full(void);
> > > > +
> > > > +#else
> > > > +
> > > > +#define is_preempt_none() IS_ENABLED(CONFIG_PREEMPT_NONE)
> > > > +#define is_preempt_voluntary()
> > > > IS_ENABLED(CONFIG_PREEMPT_VOLUNTARY)
> > > > +#define is_preempt_full() IS_ENABLED(CONFIG_PREEMPT)
> > >
> > > I think that should be IS_ENABLED(CONFIG_PREEMPTION), see
> > > c1a280b68d4e.
> > >
> > > Noticed while applying the series to an RT tree, where tglx
> > > has done that replacement to the powerpc spot your next patch
> > > diddles.
> >
> > Damn, then comes patch 5 properly differentiating PREEMPT/PREEMPT_RT.
>
> So I suppose the powerpc spot should remain CONFIG_PREEMPT and become
> CONFIG_PREEMPTION when the RT change gets merged, because that spot is
> about full preemptibility, not a distinct preemption model.
>
> That's rather annoying :-/

I guess the question is if is_preempt_full() should be true also if
is_preempt_rt() is true?

Not sure all cases are happy with that, e.g. the kernel/trace/trace.c
case, which wants to print the precise preemption level.

To avoid confusion, I'd introduce another helper that says true if the
preemption level is "at least full", currently that'd be "full or rt".
Something like is_preempt_full_or_rt() (but might as well write
"is_preempt_full() || is_preempt_rt()"), or is_preemption() (to match
that Kconfig variable, although it's slightly confusing). The
implementation of that helper can just be a static inline function
returning "is_preempt_full() || is_preempt_rt()".

Would that help?

Re: [PATCH v2 2/5] preempt/dynamic: Introduce preempt mode accessors

[Mike Galbraith]

On Thu, 2021-11-11 at 10:36 +0100, Marco Elver wrote:
> On Thu, 11 Nov 2021 at 04:47, Mike Galbraith <efault@gmx.de> wrote:
> >
> > On Thu, 2021-11-11 at 04:35 +0100, Mike Galbraith wrote:
> > > On Thu, 2021-11-11 at 04:16 +0100, Mike Galbraith wrote:
> > > > On Wed, 2021-11-10 at 20:24 +0000, Valentin Schneider wrote:
> > > > >
> > > > > diff --git a/include/linux/sched.h b/include/linux/sched.h
> > > > > index 5f8db54226af..0640d5622496 100644
> > > > > --- a/include/linux/sched.h
> > > > > +++ b/include/linux/sched.h
> > > > > @@ -2073,6 +2073,22 @@ static inline void cond_resched_rcu(void)
> > > > >  #endif
> > > > >  }
> > > > >
> > > > > +#ifdef CONFIG_PREEMPT_DYNAMIC
> > > > > +
> > > > > +extern bool is_preempt_none(void);
> > > > > +extern bool is_preempt_voluntary(void);
> > > > > +extern bool is_preempt_full(void);
> > > > > +
> > > > > +#else
> > > > > +
> > > > > +#define is_preempt_none() IS_ENABLED(CONFIG_PREEMPT_NONE)
> > > > > +#define is_preempt_voluntary()
> > > > > IS_ENABLED(CONFIG_PREEMPT_VOLUNTARY)
> > > > > +#define is_preempt_full() IS_ENABLED(CONFIG_PREEMPT)
> > > >
> > > > I think that should be IS_ENABLED(CONFIG_PREEMPTION), see
> > > > c1a280b68d4e.
> > > >
> > > > Noticed while applying the series to an RT tree, where tglx
> > > > has done that replacement to the powerpc spot your next patch
> > > > diddles.
> > >
> > > Damn, then comes patch 5 properly differentiating PREEMPT/PREEMPT_RT.
> >
> > So I suppose the powerpc spot should remain CONFIG_PREEMPT and become
> > CONFIG_PREEMPTION when the RT change gets merged, because that spot is
> > about full preemptibility, not a distinct preemption model.
> >
> > That's rather annoying :-/
>
> I guess the question is if is_preempt_full() should be true also if
> is_preempt_rt() is true?

That's what CONFIG_PREEMPTION is.  More could follow, but it was added
to allow multiple models to say "preemptible".

> Not sure all cases are happy with that, e.g. the kernel/trace/trace.c
> case, which wants to print the precise preemption level.

Yeah, that's the "annoying" bit, needing one oddball model accessor
that isn't about a particular model.

> To avoid confusion, I'd introduce another helper that says true if the
> preemption level is "at least full", currently that'd be "full or rt".
> Something like is_preempt_full_or_rt() (but might as well write
> "is_preempt_full() || is_preempt_rt()"), or is_preemption() (to match
> that Kconfig variable, although it's slightly confusing). The
> implementation of that helper can just be a static inline function
> returning "is_preempt_full() || is_preempt_rt()".
>
> Would that help?

Yeah, as it sits two accessors are needed, one that says PREEMPT the
other PREEMPTION, spelling optional.

	-Mike

Re: [PATCH v2 2/5] preempt/dynamic: Introduce preempt mode accessors

[Valentin Schneider]

On 11/11/21 11:32, Mike Galbraith wrote:
> On Thu, 2021-11-11 at 10:36 +0100, Marco Elver wrote:
>> I guess the question is if is_preempt_full() should be true also if
>> is_preempt_rt() is true?
>
> That's what CONFIG_PREEMPTION is.  More could follow, but it was added
> to allow multiple models to say "preemptible".
>

That's what I was gonna say, but you can have CONFIG_PREEMPTION while being
is_preempt_none() due to PREEMPT_DYNAMIC...

>> Not sure all cases are happy with that, e.g. the kernel/trace/trace.c
>> case, which wants to print the precise preemption level.
>
> Yeah, that's the "annoying" bit, needing one oddball model accessor
> that isn't about a particular model.
>
>> To avoid confusion, I'd introduce another helper that says true if the
>> preemption level is "at least full", currently that'd be "full or rt".
>> Something like is_preempt_full_or_rt() (but might as well write
>> "is_preempt_full() || is_preempt_rt()"), or is_preemption() (to match
>> that Kconfig variable, although it's slightly confusing). The
>> implementation of that helper can just be a static inline function
>> returning "is_preempt_full() || is_preempt_rt()".
>>
>> Would that help?
>
> Yeah, as it sits two accessors are needed, one that says PREEMPT the
> other PREEMPTION, spelling optional.
>

Per the above, I think we need the full || rt thingie.

>       -Mike

Re: [PATCH v2 2/5] preempt/dynamic: Introduce preempt mode accessors

[Mike Galbraith]

On Thu, 2021-11-11 at 10:56 +0000, Valentin Schneider wrote:
> On 11/11/21 11:32, Mike Galbraith wrote:
> > On Thu, 2021-11-11 at 10:36 +0100, Marco Elver wrote:
> > > I guess the question is if is_preempt_full() should be true also if
> > > is_preempt_rt() is true?
> >
> > That's what CONFIG_PREEMPTION is.  More could follow, but it was added
> > to allow multiple models to say "preemptible".
> >
>
> That's what I was gonna say, but you can have CONFIG_PREEMPTION while being
> is_preempt_none() due to PREEMPT_DYNAMIC...

Ah, right.. this is gonna take some getting used to.

	-Mike

Re: [PATCH v2 2/5] preempt/dynamic: Introduce preempt mode accessors

[Marco Elver]

On Wed, Nov 10, 2021 at 08:24PM +0000, Valentin Schneider wrote:
[...]
> +#ifdef CONFIG_PREEMPT_DYNAMIC
> +
> +extern bool is_preempt_none(void);
> +extern bool is_preempt_voluntary(void);
> +extern bool is_preempt_full(void);
> +
> +#else
> +
> +#define is_preempt_none() IS_ENABLED(CONFIG_PREEMPT_NONE)
> +#define is_preempt_voluntary() IS_ENABLED(CONFIG_PREEMPT_VOLUNTARY)
> +#define is_preempt_full() IS_ENABLED(CONFIG_PREEMPT)
> +
> +#endif
> +
> +#define is_preempt_rt() IS_ENABLED(CONFIG_PREEMPT_RT)
> +

Can these callables be real functions in all configs, making the
!DYNAMIC ones just static inline bool ones? That'd catch invalid use in
#if etc. in all configs.

>  /*
>   * Does a critical section need to be broken due to another
>   * task waiting?: (technically does not depend on CONFIG_PREEMPTION,
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 97047aa7b6c2..9db7f77e53c3 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -6638,6 +6638,17 @@ static void __init preempt_dynamic_init(void)
>  	}
>  }
>  
> +#define PREEMPT_MODE_ACCESSOR(mode) \
> +	bool is_preempt_##mode(void)						 \
> +	{									 \
> +		WARN_ON_ONCE(preempt_dynamic_mode == preempt_dynamic_undefined); \
> +		return preempt_dynamic_mode == preempt_dynamic_##mode;		 \
> +	}

This needs an EXPORT_SYMBOL, so it's usable from modules like the
kcsan_test module.

Re: [PATCH v2 2/5] preempt/dynamic: Introduce preempt mode accessors

[Valentin Schneider]

On 11/11/21 09:54, Marco Elver wrote:
> On Wed, Nov 10, 2021 at 08:24PM +0000, Valentin Schneider wrote:
> [...]
>> +#ifdef CONFIG_PREEMPT_DYNAMIC
>> +
>> +extern bool is_preempt_none(void);
>> +extern bool is_preempt_voluntary(void);
>> +extern bool is_preempt_full(void);
>> +
>> +#else
>> +
>> +#define is_preempt_none() IS_ENABLED(CONFIG_PREEMPT_NONE)
>> +#define is_preempt_voluntary() IS_ENABLED(CONFIG_PREEMPT_VOLUNTARY)
>> +#define is_preempt_full() IS_ENABLED(CONFIG_PREEMPT)
>> +
>> +#endif
>> +
>> +#define is_preempt_rt() IS_ENABLED(CONFIG_PREEMPT_RT)
>> +
>
> Can these callables be real functions in all configs, making the
> !DYNAMIC ones just static inline bool ones? That'd catch invalid use in
> #if etc. in all configs.
>

Ack

>>  /*
>>   * Does a critical section need to be broken due to another
>>   * task waiting?: (technically does not depend on CONFIG_PREEMPTION,
>> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
>> index 97047aa7b6c2..9db7f77e53c3 100644
>> --- a/kernel/sched/core.c
>> +++ b/kernel/sched/core.c
>> @@ -6638,6 +6638,17 @@ static void __init preempt_dynamic_init(void)
>>      }
>>  }
>>
>> +#define PREEMPT_MODE_ACCESSOR(mode) \
>> +	bool is_preempt_##mode(void)						 \
>> +	{									 \
>> +		WARN_ON_ONCE(preempt_dynamic_mode == preempt_dynamic_undefined); \
>> +		return preempt_dynamic_mode == preempt_dynamic_##mode;		 \
>> +	}
>
> This needs an EXPORT_SYMBOL, so it's usable from modules like the
> kcsan_test module.

Ah, wasn't sure about that one, thanks!

Re: [PATCH v2 2/5] preempt/dynamic: Introduce preempt mode accessors

[Christophe Leroy]

Le 10/11/2021 à 21:24, Valentin Schneider a écrit :
> CONFIG_PREEMPT{_NONE, _VOLUNTARY} designate either:
> o The build-time preemption model when !PREEMPT_DYNAMIC
> o The default boot-time preemption model when PREEMPT_DYNAMIC
> 
> IOW, using those on PREEMPT_DYNAMIC kernels is meaningless - the actual
> model could have been set to something else by the "preempt=foo" cmdline
> parameter.
> 
> Introduce a set of helpers to determine the actual preemption mode used by
> the live kernel.
> 
> Suggested-by: Marco Elver <elver@google.com>
> Signed-off-by: Valentin Schneider <valentin.schneider@arm.com>
> ---
>   include/linux/sched.h | 16 ++++++++++++++++
>   kernel/sched/core.c   | 11 +++++++++++
>   2 files changed, 27 insertions(+)
> 
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index 5f8db54226af..0640d5622496 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -2073,6 +2073,22 @@ static inline void cond_resched_rcu(void)
>   #endif
>   }
>   
> +#ifdef CONFIG_PREEMPT_DYNAMIC
> +
> +extern bool is_preempt_none(void);
> +extern bool is_preempt_voluntary(void);
> +extern bool is_preempt_full(void);

Those are trivial tests supposed to be used in fast pathes. They should 
be static inlines in order to minimise the overhead.

> +
> +#else
> +
> +#define is_preempt_none() IS_ENABLED(CONFIG_PREEMPT_NONE)
> +#define is_preempt_voluntary() IS_ENABLED(CONFIG_PREEMPT_VOLUNTARY)
> +#define is_preempt_full() IS_ENABLED(CONFIG_PREEMPT)

Would be better to use static inlines here as well instead of macros.

> +
> +#endif
> +
> +#define is_preempt_rt() IS_ENABLED(CONFIG_PREEMPT_RT)
> +
>   /*
>    * Does a critical section need to be broken due to another
>    * task waiting?: (technically does not depend on CONFIG_PREEMPTION,
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 97047aa7b6c2..9db7f77e53c3 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -6638,6 +6638,17 @@ static void __init preempt_dynamic_init(void)
>   	}
>   }
>   
> +#define PREEMPT_MODE_ACCESSOR(mode) \
> +	bool is_preempt_##mode(void)						 \
> +	{									 \
> +		WARN_ON_ONCE(preempt_dynamic_mode == preempt_dynamic_undefined); \

Not sure using WARN_ON is a good idea here, as it may be called very 
early, see comment on powerpc patch.

> +		return preempt_dynamic_mode == preempt_dynamic_##mode;		 \
> +	}

I'm not sure that's worth a macro. You only have 3 accessors, 2 lines of 
code each. Just define all 3 in plain text.

CONFIG_PREEMPT_DYNAMIC is based on using strategies like static_calls in 
order to minimise the overhead. For those accessors you should use the 
same kind of approach and use things like jump_labels in order to not 
redo the test at each time and minimise overhead as much as possible.

> +
> +PREEMPT_MODE_ACCESSOR(none)
> +PREEMPT_MODE_ACCESSOR(voluntary)
> +PREEMPT_MODE_ACCESSOR(full)
> +
>   #else /* !CONFIG_PREEMPT_DYNAMIC */
>   
>   static inline void preempt_dynamic_init(void) { }
> 

Re: [PATCH v2 2/5] preempt/dynamic: Introduce preempt mode accessors

[Valentin Schneider]

On 16/11/21 14:29, Christophe Leroy wrote:
> Le 10/11/2021 à 21:24, Valentin Schneider a écrit :
>> CONFIG_PREEMPT{_NONE, _VOLUNTARY} designate either:
>> o The build-time preemption model when !PREEMPT_DYNAMIC
>> o The default boot-time preemption model when PREEMPT_DYNAMIC
>>
>> IOW, using those on PREEMPT_DYNAMIC kernels is meaningless - the actual
>> model could have been set to something else by the "preempt=foo" cmdline
>> parameter.
>>
>> Introduce a set of helpers to determine the actual preemption mode used by
>> the live kernel.
>>
>> Suggested-by: Marco Elver <elver@google.com>
>> Signed-off-by: Valentin Schneider <valentin.schneider@arm.com>
>> ---
>>   include/linux/sched.h | 16 ++++++++++++++++
>>   kernel/sched/core.c   | 11 +++++++++++
>>   2 files changed, 27 insertions(+)
>>
>> diff --git a/include/linux/sched.h b/include/linux/sched.h
>> index 5f8db54226af..0640d5622496 100644
>> --- a/include/linux/sched.h
>> +++ b/include/linux/sched.h
>> @@ -2073,6 +2073,22 @@ static inline void cond_resched_rcu(void)
>>   #endif
>>   }
>>
>> +#ifdef CONFIG_PREEMPT_DYNAMIC
>> +
>> +extern bool is_preempt_none(void);
>> +extern bool is_preempt_voluntary(void);
>> +extern bool is_preempt_full(void);
>
> Those are trivial tests supposed to be used in fast pathes. They should
> be static inlines in order to minimise the overhead.
>
>> +
>> +#else
>> +
>> +#define is_preempt_none() IS_ENABLED(CONFIG_PREEMPT_NONE)
>> +#define is_preempt_voluntary() IS_ENABLED(CONFIG_PREEMPT_VOLUNTARY)
>> +#define is_preempt_full() IS_ENABLED(CONFIG_PREEMPT)
>
> Would be better to use static inlines here as well instead of macros.
>

I realize I stripped all ppc folks from the cclist after dropping the ppc
snippet, but you guys might still be interested - my bad. That's done in
v3:

https://lore.kernel.org/lkml/20211112185203.280040-1-valentin.schneider@arm.com/

>> +
>> +#endif
>> +
>> +#define is_preempt_rt() IS_ENABLED(CONFIG_PREEMPT_RT)
>> +
>>   /*
>>    * Does a critical section need to be broken due to another
>>    * task waiting?: (technically does not depend on CONFIG_PREEMPTION,
>> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
>> index 97047aa7b6c2..9db7f77e53c3 100644
>> --- a/kernel/sched/core.c
>> +++ b/kernel/sched/core.c
>> @@ -6638,6 +6638,17 @@ static void __init preempt_dynamic_init(void)
>>      }
>>   }
>>
>> +#define PREEMPT_MODE_ACCESSOR(mode) \
>> +	bool is_preempt_##mode(void)						 \
>> +	{									 \
>> +		WARN_ON_ONCE(preempt_dynamic_mode == preempt_dynamic_undefined); \
>
> Not sure using WARN_ON is a good idea here, as it may be called very
> early, see comment on powerpc patch.

Bah, I was gonna say that you *don't* want users of is_preempt_*() to be
called before the "final" preemption model is set up (such users would need
to make use of static_calls), but I realize there's a debug interface to
flip the preemption model at will... Say an initcall sees
is_preempt_voluntary() and sets things up accordingly, and then the debug
knob switches to preempt_full. I don't think there's much we can really do
here though :/

>
>> +		return preempt_dynamic_mode == preempt_dynamic_##mode;		 \
>> +	}
>
> I'm not sure that's worth a macro. You only have 3 accessors, 2 lines of
> code each. Just define all 3 in plain text.
>
> CONFIG_PREEMPT_DYNAMIC is based on using strategies like static_calls in
> order to minimise the overhead. For those accessors you should use the
> same kind of approach and use things like jump_labels in order to not
> redo the test at each time and minimise overhead as much as possible.
>

That's a valid point, though the few paths that need patching up and don't
make use of static calls already (AFAICT the ppc irq path I was touching in
v2 needs to make use of irqentry_exit_cond_resched()) really seem like
slow-paths.

>> +
>> +PREEMPT_MODE_ACCESSOR(none)
>> +PREEMPT_MODE_ACCESSOR(voluntary)
>> +PREEMPT_MODE_ACCESSOR(full)
>> +
>>   #else /* !CONFIG_PREEMPT_DYNAMIC */
>>
>>   static inline void preempt_dynamic_init(void) { }
>>

[PATCH v2 3/5] powerpc: Use preemption model accessors

[Valentin Schneider]

Per PREEMPT_DYNAMIC, checking CONFIG_PREEMPT doesn't tell you the actual
preemption model of the live kernel. Use the newly-introduced accessors
instead.

sched_init() -> preempt_dynamic_init() happens way before IRQs are set up,
so this should be fine.

Signed-off-by: Valentin Schneider <valentin.schneider@arm.com>
---
 arch/powerpc/kernel/interrupt.c | 2 +-
 arch/powerpc/kernel/traps.c     | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/kernel/interrupt.c b/arch/powerpc/kernel/interrupt.c
index de10a2697258..c56c10b59be3 100644
--- a/arch/powerpc/kernel/interrupt.c
+++ b/arch/powerpc/kernel/interrupt.c
@@ -552,7 +552,7 @@ notrace unsigned long interrupt_exit_kernel_prepare(struct pt_regs *regs)
 		/* Returning to a kernel context with local irqs enabled. */
 		WARN_ON_ONCE(!(regs->msr & MSR_EE));
 again:
-		if (IS_ENABLED(CONFIG_PREEMPT)) {
+		if (is_preempt_full()) {
 			/* Return to preemptible kernel context */
 			if (unlikely(current_thread_info()->flags & _TIF_NEED_RESCHED)) {
 				if (preempt_count() == 0)
diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index aac8c0412ff9..1cb31bbdc925 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c
@@ -265,7 +265,7 @@ static int __die(const char *str, struct pt_regs *regs, long err)
 	printk("%s PAGE_SIZE=%luK%s%s%s%s%s%s %s\n",
 	       IS_ENABLED(CONFIG_CPU_LITTLE_ENDIAN) ? "LE" : "BE",
 	       PAGE_SIZE / 1024, get_mmu_str(),
-	       IS_ENABLED(CONFIG_PREEMPT) ? " PREEMPT" : "",
+	       is_preempt_full() ? " PREEMPT" : "",
 	       IS_ENABLED(CONFIG_SMP) ? " SMP" : "",
 	       IS_ENABLED(CONFIG_SMP) ? (" NR_CPUS=" __stringify(NR_CPUS)) : "",
 	       debug_pagealloc_enabled() ? " DEBUG_PAGEALLOC" : "",
-- 
2.25.1

Re: [PATCH v2 3/5] powerpc: Use preemption model accessors

[Michael Ellerman]

Valentin Schneider <valentin.schneider@arm.com> writes:
> Per PREEMPT_DYNAMIC, checking CONFIG_PREEMPT doesn't tell you the actual
> preemption model of the live kernel. Use the newly-introduced accessors
> instead.
>
> sched_init() -> preempt_dynamic_init() happens way before IRQs are set up,
> so this should be fine.

Despite the name interrupt_exit_kernel_prepare() is called before IRQs
are setup, traps and page faults are "interrupts" here.

So I'm not sure about adding that call there, because it will trigger a
WARN if called early in boot, which will trigger a trap and depending on
the context we may not survive.

I'd be happier if we can make it a build-time check.

cheers

> diff --git a/arch/powerpc/kernel/interrupt.c b/arch/powerpc/kernel/interrupt.c
> index de10a2697258..c56c10b59be3 100644
> --- a/arch/powerpc/kernel/interrupt.c
> +++ b/arch/powerpc/kernel/interrupt.c
> @@ -552,7 +552,7 @@ notrace unsigned long interrupt_exit_kernel_prepare(struct pt_regs *regs)
>  		/* Returning to a kernel context with local irqs enabled. */
>  		WARN_ON_ONCE(!(regs->msr & MSR_EE));
>  again:
> -		if (IS_ENABLED(CONFIG_PREEMPT)) {
> +		if (is_preempt_full()) {
>  			/* Return to preemptible kernel context */
>  			if (unlikely(current_thread_info()->flags & _TIF_NEED_RESCHED)) {
>  				if (preempt_count() == 0)
> diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
> index aac8c0412ff9..1cb31bbdc925 100644
> --- a/arch/powerpc/kernel/traps.c
> +++ b/arch/powerpc/kernel/traps.c
> @@ -265,7 +265,7 @@ static int __die(const char *str, struct pt_regs *regs, long err)
>  	printk("%s PAGE_SIZE=%luK%s%s%s%s%s%s %s\n",
>  	       IS_ENABLED(CONFIG_CPU_LITTLE_ENDIAN) ? "LE" : "BE",
>  	       PAGE_SIZE / 1024, get_mmu_str(),
> -	       IS_ENABLED(CONFIG_PREEMPT) ? " PREEMPT" : "",
> +	       is_preempt_full() ? " PREEMPT" : "",
>  	       IS_ENABLED(CONFIG_SMP) ? " SMP" : "",
>  	       IS_ENABLED(CONFIG_SMP) ? (" NR_CPUS=" __stringify(NR_CPUS)) : "",
>  	       debug_pagealloc_enabled() ? " DEBUG_PAGEALLOC" : "",
> -- 
> 2.25.1

Re: [PATCH v2 3/5] powerpc: Use preemption model accessors

[Valentin Schneider]

Doh, thought I had sent this one out already...

On 11/11/21 15:55, Michael Ellerman wrote:
> Valentin Schneider <valentin.schneider@arm.com> writes:
>> Per PREEMPT_DYNAMIC, checking CONFIG_PREEMPT doesn't tell you the actual
>> preemption model of the live kernel. Use the newly-introduced accessors
>> instead.
>>
>> sched_init() -> preempt_dynamic_init() happens way before IRQs are set up,
>> so this should be fine.
>
> Despite the name interrupt_exit_kernel_prepare() is called before IRQs
> are setup, traps and page faults are "interrupts" here.
>
> So I'm not sure about adding that call there, because it will trigger a
> WARN if called early in boot, which will trigger a trap and depending on
> the context we may not survive.
>
> I'd be happier if we can make it a build-time check.
>

This can't be done at build-time for PREEMPT_DYNAMIC, but that can be
punted off to whoever will implement ppc support for that :-) AFAICT if
this can't use preempt_dynamic_mode (due to how "late" it is setup), the
preempt_schedule_irq() needs to go and ppc needs to use irqentry_exit() /
irqentry_exit_cond_resched().

I dropped that for v2.

> cheers
>

Re: [PATCH v2 3/5] powerpc: Use preemption model accessors

[Christophe Leroy]

Le 10/11/2021 à 21:24, Valentin Schneider a écrit :
> Per PREEMPT_DYNAMIC, checking CONFIG_PREEMPT doesn't tell you the actual
> preemption model of the live kernel. Use the newly-introduced accessors
> instead.

Is that change worth it for now ? As far as I can see powerpc doesn't 
have DYNAMIC PREEMPT, a lot of work needs to be done before being able 
to use it:
- Implement GENERIC_ENTRY
- Implement STATIC_CALLS (already done on PPC32, to be done on PPC64)

> 
> sched_init() -> preempt_dynamic_init() happens way before IRQs are set up,
> so this should be fine.

It looks like you are mixing up interrupts and IRQs (also known as 
"external interrupts").

ISI (Instruction Storage Interrupt) and DSI (Data Storage Interrupt) for 
instance are also interrupts. They happen everytime there is a page 
fault so may happen pretty early.

Traps generated by WARN_ON() are also interrupts that may happen at any 
time.

> 
> Signed-off-by: Valentin Schneider <valentin.schneider@arm.com>
> ---
>   arch/powerpc/kernel/interrupt.c | 2 +-
>   arch/powerpc/kernel/traps.c     | 2 +-
>   2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/interrupt.c b/arch/powerpc/kernel/interrupt.c
> index de10a2697258..c56c10b59be3 100644
> --- a/arch/powerpc/kernel/interrupt.c
> +++ b/arch/powerpc/kernel/interrupt.c
> @@ -552,7 +552,7 @@ notrace unsigned long interrupt_exit_kernel_prepare(struct pt_regs *regs)
>   		/* Returning to a kernel context with local irqs enabled. */
>   		WARN_ON_ONCE(!(regs->msr & MSR_EE));
>   again:
> -		if (IS_ENABLED(CONFIG_PREEMPT)) {
> +		if (is_preempt_full()) {

I think the cost of that additionnal test should be analysed. Maybe it's 
worth not doing the test at all and check _TIF_NEED_RESCHED everytime, 
unless that recurrent test is changed into a jump label as suggested in 
patch 2.


>   			/* Return to preemptible kernel context */
>   			if (unlikely(current_thread_info()->flags & _TIF_NEED_RESCHED)) {
>   				if (preempt_count() == 0)
> diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
> index aac8c0412ff9..1cb31bbdc925 100644
> --- a/arch/powerpc/kernel/traps.c
> +++ b/arch/powerpc/kernel/traps.c
> @@ -265,7 +265,7 @@ static int __die(const char *str, struct pt_regs *regs, long err)
>   	printk("%s PAGE_SIZE=%luK%s%s%s%s%s%s %s\n",
>   	       IS_ENABLED(CONFIG_CPU_LITTLE_ENDIAN) ? "LE" : "BE",
>   	       PAGE_SIZE / 1024, get_mmu_str(),
> -	       IS_ENABLED(CONFIG_PREEMPT) ? " PREEMPT" : "",
> +	       is_preempt_full() ? " PREEMPT" : "",
>   	       IS_ENABLED(CONFIG_SMP) ? " SMP" : "",
>   	       IS_ENABLED(CONFIG_SMP) ? (" NR_CPUS=" __stringify(NR_CPUS)) : "",
>   	       debug_pagealloc_enabled() ? " DEBUG_PAGEALLOC" : "",
> 

Would it be interesting as well to know that we are indeed in a DYNAMIC 
PREEMPT context when dying ?

Christophe

Re: [PATCH v2 3/5] powerpc: Use preemption model accessors

[Valentin Schneider]

On 16/11/21 14:41, Christophe Leroy wrote:
> Le 10/11/2021 à 21:24, Valentin Schneider a écrit :
>> Per PREEMPT_DYNAMIC, checking CONFIG_PREEMPT doesn't tell you the actual
>> preemption model of the live kernel. Use the newly-introduced accessors
>> instead.
>
> Is that change worth it for now ? As far as I can see powerpc doesn't
> have DYNAMIC PREEMPT, a lot of work needs to be done before being able
> to use it:
> - Implement GENERIC_ENTRY
> - Implement STATIC_CALLS (already done on PPC32, to be done on PPC64)
>

You're right, I ditched this patch for v3 - AFAICT the change wasn't even
valid as the preempt_schedule_irq() call needs to be replaced with
irqentry_exit_cond_resched() (IOW this needs to make use of the generic
entry code).

>>
>> sched_init() -> preempt_dynamic_init() happens way before IRQs are set up,
>> so this should be fine.
>
> It looks like you are mixing up interrupts and IRQs (also known as
> "external interrupts").
>
> ISI (Instruction Storage Interrupt) and DSI (Data Storage Interrupt) for
> instance are also interrupts. They happen everytime there is a page
> fault so may happen pretty early.
>
> Traps generated by WARN_ON() are also interrupts that may happen at any
> time.
>

Michael pointed this out and indeed triggering a WARN_ON() there is not
super smart. Thanks for teaching me a bit of what I'm putting my grubby
hands in :)

[PATCH v2 4/5] kscan: Use preemption model accessors

[Valentin Schneider]

Per PREEMPT_DYNAMIC, checking CONFIG_PREEMPT doesn't tell you the actual
preemption model of the live kernel. Use the newly-introduced accessors
instead.

Signed-off-by: Valentin Schneider <valentin.schneider@arm.com>
---
 kernel/kcsan/kcsan_test.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/kcsan/kcsan_test.c b/kernel/kcsan/kcsan_test.c
index dc55fd5a36fc..14d811eb9a21 100644
--- a/kernel/kcsan/kcsan_test.c
+++ b/kernel/kcsan/kcsan_test.c
@@ -1005,13 +1005,13 @@ static const void *nthreads_gen_params(const void *prev, char *desc)
 	else
 		nthreads *= 2;
 
-	if (!IS_ENABLED(CONFIG_PREEMPT) || !IS_ENABLED(CONFIG_KCSAN_INTERRUPT_WATCHER)) {
+	if (!is_preempt_full() || !IS_ENABLED(CONFIG_KCSAN_INTERRUPT_WATCHER)) {
 		/*
 		 * Without any preemption, keep 2 CPUs free for other tasks, one
 		 * of which is the main test case function checking for
 		 * completion or failure.
 		 */
-		const long min_unused_cpus = IS_ENABLED(CONFIG_PREEMPT_NONE) ? 2 : 0;
+		const long min_unused_cpus = is_preempt_none() ? 2 : 0;
 		const long min_required_cpus = 2 + min_unused_cpus;
 
 		if (num_online_cpus() < min_required_cpus) {
-- 
2.25.1

Re: [PATCH v2 4/5] kscan: Use preemption model accessors

[Marco Elver]

Subject s/kscan/kcsan/

On Wed, Nov 10, 2021 at 08:24PM +0000, Valentin Schneider wrote:
> Per PREEMPT_DYNAMIC, checking CONFIG_PREEMPT doesn't tell you the actual
> preemption model of the live kernel. Use the newly-introduced accessors
> instead.
> 
> Signed-off-by: Valentin Schneider <valentin.schneider@arm.com>

Reviewed-by: Marco Elver <elver@google.com>

Though it currently doesn't compile as a module due to missing
EXPORT_SYMBOL of is_preempt*().

> ---
>  kernel/kcsan/kcsan_test.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/kcsan/kcsan_test.c b/kernel/kcsan/kcsan_test.c
> index dc55fd5a36fc..14d811eb9a21 100644
> --- a/kernel/kcsan/kcsan_test.c
> +++ b/kernel/kcsan/kcsan_test.c
> @@ -1005,13 +1005,13 @@ static const void *nthreads_gen_params(const void *prev, char *desc)
>  	else
>  		nthreads *= 2;
>  
> -	if (!IS_ENABLED(CONFIG_PREEMPT) || !IS_ENABLED(CONFIG_KCSAN_INTERRUPT_WATCHER)) {
> +	if (!is_preempt_full() || !IS_ENABLED(CONFIG_KCSAN_INTERRUPT_WATCHER)) {
>  		/*
>  		 * Without any preemption, keep 2 CPUs free for other tasks, one
>  		 * of which is the main test case function checking for
>  		 * completion or failure.
>  		 */
> -		const long min_unused_cpus = IS_ENABLED(CONFIG_PREEMPT_NONE) ? 2 : 0;
> +		const long min_unused_cpus = is_preempt_none() ? 2 : 0;
>  		const long min_required_cpus = 2 + min_unused_cpus;
>  
>  		if (num_online_cpus() < min_required_cpus) {
> -- 
> 2.25.1

Re: [PATCH v2 4/5] kscan: Use preemption model accessors

[Marco Elver]

On Thu, 11 Nov 2021 at 10:11, Marco Elver <elver@google.com> wrote:
>
> Subject s/kscan/kcsan/
>
> On Wed, Nov 10, 2021 at 08:24PM +0000, Valentin Schneider wrote:
> > Per PREEMPT_DYNAMIC, checking CONFIG_PREEMPT doesn't tell you the actual
> > preemption model of the live kernel. Use the newly-introduced accessors
> > instead.
> >
> > Signed-off-by: Valentin Schneider <valentin.schneider@arm.com>
>
> Reviewed-by: Marco Elver <elver@google.com>
>
> Though it currently doesn't compile as a module due to missing
> EXPORT_SYMBOL of is_preempt*().
>
> > ---
> >  kernel/kcsan/kcsan_test.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/kernel/kcsan/kcsan_test.c b/kernel/kcsan/kcsan_test.c
> > index dc55fd5a36fc..14d811eb9a21 100644
> > --- a/kernel/kcsan/kcsan_test.c
> > +++ b/kernel/kcsan/kcsan_test.c
> > @@ -1005,13 +1005,13 @@ static const void *nthreads_gen_params(const void *prev, char *desc)
> >       else
> >               nthreads *= 2;
> >
> > -     if (!IS_ENABLED(CONFIG_PREEMPT) || !IS_ENABLED(CONFIG_KCSAN_INTERRUPT_WATCHER)) {
> > +     if (!is_preempt_full() || !IS_ENABLED(CONFIG_KCSAN_INTERRUPT_WATCHER)) {

In case you introduce the 5th helper I suggested
(is_preempt_full_or_rt() or whatever you'll call it), this one can be
switched, because this check really does want to know if "at least
full preemption" and not "precisely full preemption".

Thanks,
-- Marco

Re: [PATCH v2 4/5] kscan: Use preemption model accessors

[Valentin Schneider]

On 11/11/21 10:11, Marco Elver wrote:
> Subject s/kscan/kcsan/
>

Woops...

> On Wed, Nov 10, 2021 at 08:24PM +0000, Valentin Schneider wrote:
>> Per PREEMPT_DYNAMIC, checking CONFIG_PREEMPT doesn't tell you the actual
>> preemption model of the live kernel. Use the newly-introduced accessors
>> instead.
>>
>> Signed-off-by: Valentin Schneider <valentin.schneider@arm.com>
>
> Reviewed-by: Marco Elver <elver@google.com>
>
> Though it currently doesn't compile as a module due to missing
> EXPORT_SYMBOL of is_preempt*().
>
>> ---
>>  kernel/kcsan/kcsan_test.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/kernel/kcsan/kcsan_test.c b/kernel/kcsan/kcsan_test.c
>> index dc55fd5a36fc..14d811eb9a21 100644
>> --- a/kernel/kcsan/kcsan_test.c
>> +++ b/kernel/kcsan/kcsan_test.c
>> @@ -1005,13 +1005,13 @@ static const void *nthreads_gen_params(const void *prev, char *desc)
>>      else
>>              nthreads *= 2;
>>
>> -	if (!IS_ENABLED(CONFIG_PREEMPT) || !IS_ENABLED(CONFIG_KCSAN_INTERRUPT_WATCHER)) {
>> +	if (!is_preempt_full() || !IS_ENABLED(CONFIG_KCSAN_INTERRUPT_WATCHER)) {
>>              /*
>>               * Without any preemption, keep 2 CPUs free for other tasks, one
>>               * of which is the main test case function checking for
>>               * completion or failure.
>>               */
>> -		const long min_unused_cpus = IS_ENABLED(CONFIG_PREEMPT_NONE) ? 2 : 0;
>> +		const long min_unused_cpus = is_preempt_none() ? 2 : 0;
>>              const long min_required_cpus = 2 + min_unused_cpus;
>>
>>              if (num_online_cpus() < min_required_cpus) {
>> --
>> 2.25.1

[PATCH v2 5/5] ftrace: Use preemption model accessors for trace header printout

[Valentin Schneider]

Per PREEMPT_DYNAMIC, checking CONFIG_PREEMPT doesn't tell you the actual
preemption model of the live kernel. Use the newly-introduced accessors
instead.

Signed-off-by: Valentin Schneider <valentin.schneider@arm.com>
---
 kernel/trace/trace.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 7896d30d90f7..71f293569ed0 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -4271,17 +4271,11 @@ print_trace_header(struct seq_file *m, struct trace_iterator *iter)
 		   entries,
 		   total,
 		   buf->cpu,
-#if defined(CONFIG_PREEMPT_NONE)
-		   "server",
-#elif defined(CONFIG_PREEMPT_VOLUNTARY)
-		   "desktop",
-#elif defined(CONFIG_PREEMPT)
-		   "preempt",
-#elif defined(CONFIG_PREEMPT_RT)
-		   "preempt_rt",
-#else
+		   is_preempt_none()      ? "server" :
+		   is_preempt_voluntary() ? "desktop" :
+		   is_preempt_full()      ? "preempt" :
+		   is_preempt_rt()        ? "preempt_rt" :
 		   "unknown",
-#endif
 		   /* These are reserved for later use */
 		   0, 0, 0, 0);
 #ifdef CONFIG_SMP
-- 
2.25.1

Re: [PATCH v2 5/5] ftrace: Use preemption model accessors for trace header printout

[Steven Rostedt]

On Wed, 10 Nov 2021 20:24:48 +0000
Valentin Schneider <valentin.schneider@arm.com> wrote:

> Per PREEMPT_DYNAMIC, checking CONFIG_PREEMPT doesn't tell you the actual
> preemption model of the live kernel. Use the newly-introduced accessors
> instead.
> 
> Signed-off-by: Valentin Schneider <valentin.schneider@arm.com>
> ---
>  kernel/trace/trace.c | 14 ++++----------
>  1 file changed, 4 insertions(+), 10 deletions(-)

Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

-- Steve