[PATCH 0/2] context_tracking, rcu: Standardize naming related to contex_tracking.state

[PATCH 0/2] context_tracking, rcu: Standardize naming related to contex_tracking.state

[Valentin Schneider]

Hi folks,

As I'm working on respinning [1] I'm trying to send standalone things
separately, and this is one of them.

No code change, just enum/const renaming, see [2].

[1]: https://lore.kernel.org/lkml/20230720163056.2564824-1-vschneid@redhat.com/
[2]: https://lore.kernel.org/lkml/ZL6QI4mV-NKlh4Ox@localhost.localdomain/#t

Cheers,
Valentin

Valentin Schneider (2):
  treewide: context_tracking: Rename CONTEXT_FOO to CT_STATE_FOO
  context_tracking, rcu: Rename RCU_DYNTICKS_IDX to CT_DYNTICKS_IDX

 arch/Kconfig                           |  2 +-
 arch/arm64/kernel/entry-common.c       |  2 +-
 arch/powerpc/include/asm/interrupt.h   |  6 +++---
 arch/powerpc/kernel/interrupt.c        |  6 +++---
 arch/powerpc/kernel/syscall.c          |  2 +-
 arch/x86/entry/common.c                |  2 +-
 include/linux/context_tracking.h       | 22 +++++++++----------
 include/linux/context_tracking_state.h | 20 ++++++++---------
 include/linux/entry-common.h           |  2 +-
 kernel/context_tracking.c              | 30 +++++++++++++-------------
 kernel/entry/common.c                  |  2 +-
 kernel/rcu/tree.c                      | 12 +++++------
 kernel/sched/core.c                    |  4 ++--
 13 files changed, 56 insertions(+), 56 deletions(-)

-- 
2.43.0

[PATCH 1/2] treewide: context_tracking: Rename CONTEXT_FOO to CT_STATE_FOO

[Valentin Schneider]

Context tracking state related symbols currently use a mix of the
CONTEXT_ (e.g. CONTEXT_KERNEL) and CT_SATE_ (e.g. CT_STATE_MASK) prefixes.

Clean up the naming and make the ctx_state enum use the CT_STATE_ prefix.

Suggested-by: Frederic Weisbecker <frederic@kernel.org>
Signed-off-by: Valentin Schneider <vschneid@redhat.com>
---
 arch/Kconfig                           |  2 +-
 arch/arm64/kernel/entry-common.c       |  2 +-
 arch/powerpc/include/asm/interrupt.h   |  6 +++---
 arch/powerpc/kernel/interrupt.c        |  6 +++---
 arch/powerpc/kernel/syscall.c          |  2 +-
 arch/x86/entry/common.c                |  2 +-
 include/linux/context_tracking.h       | 16 ++++++++--------
 include/linux/context_tracking_state.h | 20 ++++++++++----------
 include/linux/entry-common.h           |  2 +-
 kernel/context_tracking.c              | 12 ++++++------
 kernel/entry/common.c                  |  2 +-
 kernel/sched/core.c                    |  4 ++--
 12 files changed, 38 insertions(+), 38 deletions(-)

diff --git a/arch/Kconfig b/arch/Kconfig
index 9f066785bb71d..b0fb7182648a2 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -845,7 +845,7 @@ config HAVE_CONTEXT_TRACKING_USER_OFFSTACK
 	  Architecture neither relies on exception_enter()/exception_exit()
 	  nor on schedule_user(). Also preempt_schedule_notrace() and
 	  preempt_schedule_irq() can't be called in a preemptible section
-	  while context tracking is CONTEXT_USER. This feature reflects a sane
+	  while context tracking is CT_STATE_USER. This feature reflects a sane
 	  entry implementation where the following requirements are met on
 	  critical entry code, ie: before user_exit() or after user_enter():
 
diff --git a/arch/arm64/kernel/entry-common.c b/arch/arm64/kernel/entry-common.c
index b77a15955f28b..3fcd9d080bf2a 100644
--- a/arch/arm64/kernel/entry-common.c
+++ b/arch/arm64/kernel/entry-common.c
@@ -103,7 +103,7 @@ static void noinstr exit_to_kernel_mode(struct pt_regs *regs)
 static __always_inline void __enter_from_user_mode(void)
 {
 	lockdep_hardirqs_off(CALLER_ADDR0);
-	CT_WARN_ON(ct_state() != CONTEXT_USER);
+	CT_WARN_ON(ct_state() != CT_STATE_USER);
 	user_exit_irqoff();
 	trace_hardirqs_off_finish();
 	mte_disable_tco_entry(current);
diff --git a/arch/powerpc/include/asm/interrupt.h b/arch/powerpc/include/asm/interrupt.h
index 7b610864b3645..dcb4bd8529f68 100644
--- a/arch/powerpc/include/asm/interrupt.h
+++ b/arch/powerpc/include/asm/interrupt.h
@@ -177,7 +177,7 @@ static inline void interrupt_enter_prepare(struct pt_regs *regs)
 
 	if (user_mode(regs)) {
 		kuap_lock();
-		CT_WARN_ON(ct_state() != CONTEXT_USER);
+		CT_WARN_ON(ct_state() != CT_STATE_USER);
 		user_exit_irqoff();
 
 		account_cpu_user_entry();
@@ -189,8 +189,8 @@ static inline void interrupt_enter_prepare(struct pt_regs *regs)
 		 * so avoid recursion.
 		 */
 		if (TRAP(regs) != INTERRUPT_PROGRAM)
-			CT_WARN_ON(ct_state() != CONTEXT_KERNEL &&
-				   ct_state() != CONTEXT_IDLE);
+			CT_WARN_ON(ct_state() != CT_STATE_KERNEL &&
+				   ct_state() != CT_STATE_IDLE);
 		INT_SOFT_MASK_BUG_ON(regs, is_implicit_soft_masked(regs));
 		INT_SOFT_MASK_BUG_ON(regs, arch_irq_disabled_regs(regs) &&
 					   search_kernel_restart_table(regs->nip));
diff --git a/arch/powerpc/kernel/interrupt.c b/arch/powerpc/kernel/interrupt.c
index eca293794a1e8..af62ec974b970 100644
--- a/arch/powerpc/kernel/interrupt.c
+++ b/arch/powerpc/kernel/interrupt.c
@@ -266,7 +266,7 @@ notrace unsigned long syscall_exit_prepare(unsigned long r3,
 	unsigned long ret = 0;
 	bool is_not_scv = !IS_ENABLED(CONFIG_PPC_BOOK3S_64) || !scv;
 
-	CT_WARN_ON(ct_state() == CONTEXT_USER);
+	CT_WARN_ON(ct_state() == CT_STATE_USER);
 
 	kuap_assert_locked();
 
@@ -344,7 +344,7 @@ notrace unsigned long interrupt_exit_user_prepare(struct pt_regs *regs)
 
 	BUG_ON(regs_is_unrecoverable(regs));
 	BUG_ON(arch_irq_disabled_regs(regs));
-	CT_WARN_ON(ct_state() == CONTEXT_USER);
+	CT_WARN_ON(ct_state() == CT_STATE_USER);
 
 	/*
 	 * We don't need to restore AMR on the way back to userspace for KUAP.
@@ -386,7 +386,7 @@ notrace unsigned long interrupt_exit_kernel_prepare(struct pt_regs *regs)
 	if (!IS_ENABLED(CONFIG_PPC_BOOK3E_64) &&
 	    TRAP(regs) != INTERRUPT_PROGRAM &&
 	    TRAP(regs) != INTERRUPT_PERFMON)
-		CT_WARN_ON(ct_state() == CONTEXT_USER);
+		CT_WARN_ON(ct_state() == CT_STATE_USER);
 
 	kuap = kuap_get_and_assert_locked();
 
diff --git a/arch/powerpc/kernel/syscall.c b/arch/powerpc/kernel/syscall.c
index f6f868e817e63..be159ad4b77bd 100644
--- a/arch/powerpc/kernel/syscall.c
+++ b/arch/powerpc/kernel/syscall.c
@@ -27,7 +27,7 @@ notrace long system_call_exception(struct pt_regs *regs, unsigned long r0)
 
 	trace_hardirqs_off(); /* finish reconciling */
 
-	CT_WARN_ON(ct_state() == CONTEXT_KERNEL);
+	CT_WARN_ON(ct_state() == CT_STATE_KERNEL);
 	user_exit_irqoff();
 
 	BUG_ON(regs_is_unrecoverable(regs));
diff --git a/arch/x86/entry/common.c b/arch/x86/entry/common.c
index 6356060caaf31..f42a125093e54 100644
--- a/arch/x86/entry/common.c
+++ b/arch/x86/entry/common.c
@@ -150,7 +150,7 @@ early_param("ia32_emulation", ia32_emulation_override_cmdline);
 #endif
 
 /*
- * Invoke a 32-bit syscall.  Called with IRQs on in CONTEXT_KERNEL.
+ * Invoke a 32-bit syscall.  Called with IRQs on in CT_STATE_KERNEL.
  */
 static __always_inline void do_syscall_32_irqs_on(struct pt_regs *regs, int nr)
 {
diff --git a/include/linux/context_tracking.h b/include/linux/context_tracking.h
index 6e76b9dba00e7..28fcfa1849032 100644
--- a/include/linux/context_tracking.h
+++ b/include/linux/context_tracking.h
@@ -26,26 +26,26 @@ extern void user_exit_callable(void);
 static inline void user_enter(void)
 {
 	if (context_tracking_enabled())
-		ct_user_enter(CONTEXT_USER);
+		ct_user_enter(CT_STATE_USER);
 
 }
 static inline void user_exit(void)
 {
 	if (context_tracking_enabled())
-		ct_user_exit(CONTEXT_USER);
+		ct_user_exit(CT_STATE_USER);
 }
 
 /* Called with interrupts disabled.  */
 static __always_inline void user_enter_irqoff(void)
 {
 	if (context_tracking_enabled())
-		__ct_user_enter(CONTEXT_USER);
+		__ct_user_enter(CT_STATE_USER);
 
 }
 static __always_inline void user_exit_irqoff(void)
 {
 	if (context_tracking_enabled())
-		__ct_user_exit(CONTEXT_USER);
+		__ct_user_exit(CT_STATE_USER);
 }
 
 static inline enum ctx_state exception_enter(void)
@@ -57,7 +57,7 @@ static inline enum ctx_state exception_enter(void)
 		return 0;
 
 	prev_ctx = __ct_state();
-	if (prev_ctx != CONTEXT_KERNEL)
+	if (prev_ctx != CT_STATE_KERNEL)
 		ct_user_exit(prev_ctx);
 
 	return prev_ctx;
@@ -67,7 +67,7 @@ static inline void exception_exit(enum ctx_state prev_ctx)
 {
 	if (!IS_ENABLED(CONFIG_HAVE_CONTEXT_TRACKING_USER_OFFSTACK) &&
 	    context_tracking_enabled()) {
-		if (prev_ctx != CONTEXT_KERNEL)
+		if (prev_ctx != CT_STATE_KERNEL)
 			ct_user_enter(prev_ctx);
 	}
 }
@@ -75,7 +75,7 @@ static inline void exception_exit(enum ctx_state prev_ctx)
 static __always_inline bool context_tracking_guest_enter(void)
 {
 	if (context_tracking_enabled())
-		__ct_user_enter(CONTEXT_GUEST);
+		__ct_user_enter(CT_STATE_GUEST);
 
 	return context_tracking_enabled_this_cpu();
 }
@@ -83,7 +83,7 @@ static __always_inline bool context_tracking_guest_enter(void)
 static __always_inline void context_tracking_guest_exit(void)
 {
 	if (context_tracking_enabled())
-		__ct_user_exit(CONTEXT_GUEST);
+		__ct_user_exit(CT_STATE_GUEST);
 }
 
 #define CT_WARN_ON(cond) WARN_ON(context_tracking_enabled() && (cond))
diff --git a/include/linux/context_tracking_state.h b/include/linux/context_tracking_state.h
index bbff5f7f88030..f1c53125edee2 100644
--- a/include/linux/context_tracking_state.h
+++ b/include/linux/context_tracking_state.h
@@ -10,18 +10,18 @@
 #define DYNTICK_IRQ_NONIDLE	((LONG_MAX / 2) + 1)
 
 enum ctx_state {
-	CONTEXT_DISABLED	= -1,	/* returned by ct_state() if unknown */
-	CONTEXT_KERNEL		= 0,
-	CONTEXT_IDLE		= 1,
-	CONTEXT_USER		= 2,
-	CONTEXT_GUEST		= 3,
-	CONTEXT_MAX		= 4,
+	CT_STATE_DISABLED	= -1,	/* returned by ct_state() if unknown */
+	CT_STATE_KERNEL		= 0,
+	CT_STATE_IDLE		= 1,
+	CT_STATE_USER		= 2,
+	CT_STATE_GUEST		= 3,
+	CT_STATE_MAX		= 4,
 };
 
 /* Even value for idle, else odd. */
-#define RCU_DYNTICKS_IDX CONTEXT_MAX
+#define RCU_DYNTICKS_IDX CT_STATE_MAX
 
-#define CT_STATE_MASK (CONTEXT_MAX - 1)
+#define CT_STATE_MASK (CT_STATE_MAX - 1)
 #define CT_DYNTICKS_MASK (~CT_STATE_MASK)
 
 struct context_tracking {
@@ -123,14 +123,14 @@ static inline bool context_tracking_enabled_this_cpu(void)
  *
  * Returns the current cpu's context tracking state if context tracking
  * is enabled.  If context tracking is disabled, returns
- * CONTEXT_DISABLED.  This should be used primarily for debugging.
+ * CT_STATE_DISABLED.  This should be used primarily for debugging.
  */
 static __always_inline int ct_state(void)
 {
 	int ret;
 
 	if (!context_tracking_enabled())
-		return CONTEXT_DISABLED;
+		return CT_STATE_DISABLED;
 
 	preempt_disable();
 	ret = __ct_state();
diff --git a/include/linux/entry-common.h b/include/linux/entry-common.h
index b0fb775a600d9..1e50cdb83ae50 100644
--- a/include/linux/entry-common.h
+++ b/include/linux/entry-common.h
@@ -108,7 +108,7 @@ static __always_inline void enter_from_user_mode(struct pt_regs *regs)
 	arch_enter_from_user_mode(regs);
 	lockdep_hardirqs_off(CALLER_ADDR0);
 
-	CT_WARN_ON(__ct_state() != CONTEXT_USER);
+	CT_WARN_ON(__ct_state() != CT_STATE_USER);
 	user_exit_irqoff();
 
 	instrumentation_begin();
diff --git a/kernel/context_tracking.c b/kernel/context_tracking.c
index 70ae70d038233..21f2cf90ed0bf 100644
--- a/kernel/context_tracking.c
+++ b/kernel/context_tracking.c
@@ -317,7 +317,7 @@ void noinstr ct_nmi_enter(void)
 void noinstr ct_idle_enter(void)
 {
 	WARN_ON_ONCE(IS_ENABLED(CONFIG_RCU_EQS_DEBUG) && !raw_irqs_disabled());
-	ct_kernel_exit(false, RCU_DYNTICKS_IDX + CONTEXT_IDLE);
+	ct_kernel_exit(false, RCU_DYNTICKS_IDX + CT_STATE_IDLE);
 }
 EXPORT_SYMBOL_GPL(ct_idle_enter);
 
@@ -335,7 +335,7 @@ void noinstr ct_idle_exit(void)
 	unsigned long flags;
 
 	raw_local_irq_save(flags);
-	ct_kernel_enter(false, RCU_DYNTICKS_IDX - CONTEXT_IDLE);
+	ct_kernel_enter(false, RCU_DYNTICKS_IDX - CT_STATE_IDLE);
 	raw_local_irq_restore(flags);
 }
 EXPORT_SYMBOL_GPL(ct_idle_exit);
@@ -485,7 +485,7 @@ void noinstr __ct_user_enter(enum ctx_state state)
 			 * user_exit() or ct_irq_enter(). Let's remove RCU's dependency
 			 * on the tick.
 			 */
-			if (state == CONTEXT_USER) {
+			if (state == CT_STATE_USER) {
 				instrumentation_begin();
 				trace_user_enter(0);
 				vtime_user_enter(current);
@@ -621,7 +621,7 @@ void noinstr __ct_user_exit(enum ctx_state state)
 			 * run a RCU read side critical section anytime.
 			 */
 			ct_kernel_enter(true, RCU_DYNTICKS_IDX - state);
-			if (state == CONTEXT_USER) {
+			if (state == CT_STATE_USER) {
 				instrumentation_begin();
 				vtime_user_exit(current);
 				trace_user_exit(0);
@@ -634,12 +634,12 @@ void noinstr __ct_user_exit(enum ctx_state state)
 			 * In this we case we don't care about any concurrency/ordering.
 			 */
 			if (!IS_ENABLED(CONFIG_CONTEXT_TRACKING_IDLE))
-				raw_atomic_set(&ct->state, CONTEXT_KERNEL);
+				raw_atomic_set(&ct->state, CT_STATE_KERNEL);
 
 		} else {
 			if (!IS_ENABLED(CONFIG_CONTEXT_TRACKING_IDLE)) {
 				/* Tracking for vtime only, no concurrent RCU EQS accounting */
-				raw_atomic_set(&ct->state, CONTEXT_KERNEL);
+				raw_atomic_set(&ct->state, CT_STATE_KERNEL);
 			} else {
 				/*
 				 * Tracking for vtime and RCU EQS. Make sure we don't race
diff --git a/kernel/entry/common.c b/kernel/entry/common.c
index 90843cc385880..5b6934e23c21d 100644
--- a/kernel/entry/common.c
+++ b/kernel/entry/common.c
@@ -182,7 +182,7 @@ static void syscall_exit_to_user_mode_prepare(struct pt_regs *regs)
 	unsigned long work = READ_ONCE(current_thread_info()->syscall_work);
 	unsigned long nr = syscall_get_nr(current, regs);
 
-	CT_WARN_ON(ct_state() != CONTEXT_KERNEL);
+	CT_WARN_ON(ct_state() != CT_STATE_KERNEL);
 
 	if (IS_ENABLED(CONFIG_PROVE_LOCKING)) {
 		if (WARN(irqs_disabled(), "syscall %lu left IRQs disabled", nr))
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 7019a40457a6d..35dd462a55abe 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -5980,7 +5980,7 @@ static inline void schedule_debug(struct task_struct *prev, bool preempt)
 		preempt_count_set(PREEMPT_DISABLED);
 	}
 	rcu_sleep_check();
-	SCHED_WARN_ON(ct_state() == CONTEXT_USER);
+	SCHED_WARN_ON(ct_state() == CT_STATE_USER);
 
 	profile_hit(SCHED_PROFILING, __builtin_return_address(0));
 
@@ -6875,7 +6875,7 @@ asmlinkage __visible void __sched schedule_user(void)
 	 * we find a better solution.
 	 *
 	 * NB: There are buggy callers of this function.  Ideally we
-	 * should warn if prev_state != CONTEXT_USER, but that will trigger
+	 * should warn if prev_state != CT_STATE_USER, but that will trigger
 	 * too frequently to make sense yet.
 	 */
 	enum ctx_state prev_state = exception_enter();
-- 
2.43.0

Re: [PATCH 1/2] treewide: context_tracking: Rename CONTEXT_FOO to CT_STATE_FOO

[Frederic Weisbecker]

Le Wed, Mar 27, 2024 at 12:29:01PM +0100, Valentin Schneider a écrit :
> Context tracking state related symbols currently use a mix of the
> CONTEXT_ (e.g. CONTEXT_KERNEL) and CT_SATE_ (e.g. CT_STATE_MASK) prefixes.
> 
> Clean up the naming and make the ctx_state enum use the CT_STATE_ prefix.
> 
> Suggested-by: Frederic Weisbecker <frederic@kernel.org>
> Signed-off-by: Valentin Schneider <vschneid@redhat.com>

Acked-by: Frederic Weisbecker <frederic@kernel.org>

[PATCH 2/2] context_tracking, rcu: Rename RCU_DYNTICKS_IDX to CT_DYNTICKS_IDX

[Valentin Schneider]

The symbols relating to the CT_STATE part of context_tracking.state are now
all prefixed with CT_STATE.

The RCU dynticks counter part of that atomic variable still involves
symbols with different prefixes, align them all to be prefixed with
CT_DYNTICKS, as CT_DYNTICKS_MASK already is.

Signed-off-by: Valentin Schneider <vschneid@redhat.com>
---
 include/linux/context_tracking.h       |  6 +++---
 include/linux/context_tracking_state.h |  2 +-
 kernel/context_tracking.c              | 22 +++++++++++-----------
 kernel/rcu/tree.c                      | 12 ++++++------
 4 files changed, 21 insertions(+), 21 deletions(-)

diff --git a/include/linux/context_tracking.h b/include/linux/context_tracking.h
index 28fcfa1849032..a9c3f2c0e04e3 100644
--- a/include/linux/context_tracking.h
+++ b/include/linux/context_tracking.h
@@ -119,7 +119,7 @@ extern void ct_idle_exit(void);
  */
 static __always_inline bool rcu_dynticks_curr_cpu_in_eqs(void)
 {
-	return !(raw_atomic_read(this_cpu_ptr(&context_tracking.state)) & RCU_DYNTICKS_IDX);
+	return !(raw_atomic_read(this_cpu_ptr(&context_tracking.state)) & CT_DYNTICKS_IDX);
 }
 
 /*
@@ -142,7 +142,7 @@ static __always_inline bool warn_rcu_enter(void)
 	preempt_disable_notrace();
 	if (rcu_dynticks_curr_cpu_in_eqs()) {
 		ret = true;
-		ct_state_inc(RCU_DYNTICKS_IDX);
+		ct_state_inc(CT_DYNTICKS_IDX);
 	}
 
 	return ret;
@@ -151,7 +151,7 @@ static __always_inline bool warn_rcu_enter(void)
 static __always_inline void warn_rcu_exit(bool rcu)
 {
 	if (rcu)
-		ct_state_inc(RCU_DYNTICKS_IDX);
+		ct_state_inc(CT_DYNTICKS_IDX);
 	preempt_enable_notrace();
 }
 
diff --git a/include/linux/context_tracking_state.h b/include/linux/context_tracking_state.h
index f1c53125edee2..73af5a13b4fc5 100644
--- a/include/linux/context_tracking_state.h
+++ b/include/linux/context_tracking_state.h
@@ -19,7 +19,7 @@ enum ctx_state {
 };
 
 /* Even value for idle, else odd. */
-#define RCU_DYNTICKS_IDX CT_STATE_MAX
+#define CT_DYNTICKS_IDX CT_STATE_MAX
 
 #define CT_STATE_MASK (CT_STATE_MAX - 1)
 #define CT_DYNTICKS_MASK (~CT_STATE_MASK)
diff --git a/kernel/context_tracking.c b/kernel/context_tracking.c
index 21f2cf90ed0bf..586c17fef1327 100644
--- a/kernel/context_tracking.c
+++ b/kernel/context_tracking.c
@@ -31,7 +31,7 @@ DEFINE_PER_CPU(struct context_tracking, context_tracking) = {
 	.dynticks_nesting = 1,
 	.dynticks_nmi_nesting = DYNTICK_IRQ_NONIDLE,
 #endif
-	.state = ATOMIC_INIT(RCU_DYNTICKS_IDX),
+	.state = ATOMIC_INIT(CT_DYNTICKS_IDX),
 };
 EXPORT_SYMBOL_GPL(context_tracking);
 
@@ -90,7 +90,7 @@ static noinstr void ct_kernel_exit_state(int offset)
 	rcu_dynticks_task_trace_enter();  // Before ->dynticks update!
 	seq = ct_state_inc(offset);
 	// RCU is no longer watching.  Better be in extended quiescent state!
-	WARN_ON_ONCE(IS_ENABLED(CONFIG_RCU_EQS_DEBUG) && (seq & RCU_DYNTICKS_IDX));
+	WARN_ON_ONCE(IS_ENABLED(CONFIG_RCU_EQS_DEBUG) && (seq & CT_DYNTICKS_IDX));
 }
 
 /*
@@ -110,7 +110,7 @@ static noinstr void ct_kernel_enter_state(int offset)
 	seq = ct_state_inc(offset);
 	// RCU is now watching.  Better not be in an extended quiescent state!
 	rcu_dynticks_task_trace_exit();  // After ->dynticks update!
-	WARN_ON_ONCE(IS_ENABLED(CONFIG_RCU_EQS_DEBUG) && !(seq & RCU_DYNTICKS_IDX));
+	WARN_ON_ONCE(IS_ENABLED(CONFIG_RCU_EQS_DEBUG) && !(seq & CT_DYNTICKS_IDX));
 }
 
 /*
@@ -236,7 +236,7 @@ void noinstr ct_nmi_exit(void)
 	instrumentation_end();
 
 	// RCU is watching here ...
-	ct_kernel_exit_state(RCU_DYNTICKS_IDX);
+	ct_kernel_exit_state(CT_DYNTICKS_IDX);
 	// ... but is no longer watching here.
 
 	if (!in_nmi())
@@ -277,7 +277,7 @@ void noinstr ct_nmi_enter(void)
 			rcu_dynticks_task_exit();
 
 		// RCU is not watching here ...
-		ct_kernel_enter_state(RCU_DYNTICKS_IDX);
+		ct_kernel_enter_state(CT_DYNTICKS_IDX);
 		// ... but is watching here.
 
 		instrumentation_begin();
@@ -317,7 +317,7 @@ void noinstr ct_nmi_enter(void)
 void noinstr ct_idle_enter(void)
 {
 	WARN_ON_ONCE(IS_ENABLED(CONFIG_RCU_EQS_DEBUG) && !raw_irqs_disabled());
-	ct_kernel_exit(false, RCU_DYNTICKS_IDX + CT_STATE_IDLE);
+	ct_kernel_exit(false, CT_DYNTICKS_IDX + CT_STATE_IDLE);
 }
 EXPORT_SYMBOL_GPL(ct_idle_enter);
 
@@ -335,7 +335,7 @@ void noinstr ct_idle_exit(void)
 	unsigned long flags;
 
 	raw_local_irq_save(flags);
-	ct_kernel_enter(false, RCU_DYNTICKS_IDX - CT_STATE_IDLE);
+	ct_kernel_enter(false, CT_DYNTICKS_IDX - CT_STATE_IDLE);
 	raw_local_irq_restore(flags);
 }
 EXPORT_SYMBOL_GPL(ct_idle_exit);
@@ -504,7 +504,7 @@ void noinstr __ct_user_enter(enum ctx_state state)
 			 * CPU doesn't need to maintain the tick for RCU maintenance purposes
 			 * when the CPU runs in userspace.
 			 */
-			ct_kernel_exit(true, RCU_DYNTICKS_IDX + state);
+			ct_kernel_exit(true, CT_DYNTICKS_IDX + state);
 
 			/*
 			 * Special case if we only track user <-> kernel transitions for tickless
@@ -534,7 +534,7 @@ void noinstr __ct_user_enter(enum ctx_state state)
 				/*
 				 * Tracking for vtime and RCU EQS. Make sure we don't race
 				 * with NMIs. OTOH we don't care about ordering here since
-				 * RCU only requires RCU_DYNTICKS_IDX increments to be fully
+				 * RCU only requires CT_DYNTICKS_IDX increments to be fully
 				 * ordered.
 				 */
 				raw_atomic_add(state, &ct->state);
@@ -620,7 +620,7 @@ void noinstr __ct_user_exit(enum ctx_state state)
 			 * Exit RCU idle mode while entering the kernel because it can
 			 * run a RCU read side critical section anytime.
 			 */
-			ct_kernel_enter(true, RCU_DYNTICKS_IDX - state);
+			ct_kernel_enter(true, CT_DYNTICKS_IDX - state);
 			if (state == CT_STATE_USER) {
 				instrumentation_begin();
 				vtime_user_exit(current);
@@ -644,7 +644,7 @@ void noinstr __ct_user_exit(enum ctx_state state)
 				/*
 				 * Tracking for vtime and RCU EQS. Make sure we don't race
 				 * with NMIs. OTOH we don't care about ordering here since
-				 * RCU only requires RCU_DYNTICKS_IDX increments to be fully
+				 * RCU only requires CT_DYNTICKS_IDX increments to be fully
 				 * ordered.
 				 */
 				raw_atomic_sub(state, &ct->state);
diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index d9642dd06c253..3dbdcd12b876e 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -259,9 +259,9 @@ void rcu_softirq_qs(void)
  */
 static void rcu_dynticks_eqs_online(void)
 {
-	if (ct_dynticks() & RCU_DYNTICKS_IDX)
+	if (ct_dynticks() & CT_DYNTICKS_IDX)
 		return;
-	ct_state_inc(RCU_DYNTICKS_IDX);
+	ct_state_inc(CT_DYNTICKS_IDX);
 }
 
 /*
@@ -280,7 +280,7 @@ static int rcu_dynticks_snap(int cpu)
  */
 static bool rcu_dynticks_in_eqs(int snap)
 {
-	return !(snap & RCU_DYNTICKS_IDX);
+	return !(snap & CT_DYNTICKS_IDX);
 }
 
 /*
@@ -302,7 +302,7 @@ bool rcu_dynticks_zero_in_eqs(int cpu, int *vp)
 	int snap;
 
 	// If not quiescent, force back to earlier extended quiescent state.
-	snap = ct_dynticks_cpu(cpu) & ~RCU_DYNTICKS_IDX;
+	snap = ct_dynticks_cpu(cpu) & ~CT_DYNTICKS_IDX;
 	smp_rmb(); // Order ->dynticks and *vp reads.
 	if (READ_ONCE(*vp))
 		return false;  // Non-zero, so report failure;
@@ -328,9 +328,9 @@ notrace void rcu_momentary_dyntick_idle(void)
 	int seq;
 
 	raw_cpu_write(rcu_data.rcu_need_heavy_qs, false);
-	seq = ct_state_inc(2 * RCU_DYNTICKS_IDX);
+	seq = ct_state_inc(2 * CT_DYNTICKS_IDX);
 	/* It is illegal to call this from idle state. */
-	WARN_ON_ONCE(!(seq & RCU_DYNTICKS_IDX));
+	WARN_ON_ONCE(!(seq & CT_DYNTICKS_IDX));
 	rcu_preempt_deferred_qs(current);
 }
 EXPORT_SYMBOL_GPL(rcu_momentary_dyntick_idle);
-- 
2.43.0

Re: [PATCH 2/2] context_tracking, rcu: Rename RCU_DYNTICKS_IDX to CT_DYNTICKS_IDX

[Frederic Weisbecker]

Le Wed, Mar 27, 2024 at 12:29:02PM +0100, Valentin Schneider a écrit :
> The symbols relating to the CT_STATE part of context_tracking.state are now
> all prefixed with CT_STATE.
> 
> The RCU dynticks counter part of that atomic variable still involves
> symbols with different prefixes, align them all to be prefixed with
> CT_DYNTICKS, as CT_DYNTICKS_MASK already is.
> 
> Signed-off-by: Valentin Schneider <vschneid@redhat.com>

It used to be that RCU extended quiescent state and dynticks enter/exit
were coupled. But this isn't the case anymore. Nowadays RCU stops watching
some time later after dynticks is entered.

I wonder if we shouldn't take advantage of that cleanup for a meaning that
really reflect that RCU stops watching from there.

Paul what do you think? CT_EQS_IDX ? CT_RCUEQS_IDX? CT_RCUOFF_IDX? ...?

Thanks.

Re: [PATCH 2/2] context_tracking, rcu: Rename RCU_DYNTICKS_IDX to CT_DYNTICKS_IDX

[Paul E. McKenney]

On Thu, Apr 04, 2024 at 03:38:40PM +0200, Frederic Weisbecker wrote:
> Le Wed, Mar 27, 2024 at 12:29:02PM +0100, Valentin Schneider a écrit :
> > The symbols relating to the CT_STATE part of context_tracking.state are now
> > all prefixed with CT_STATE.
> > 
> > The RCU dynticks counter part of that atomic variable still involves
> > symbols with different prefixes, align them all to be prefixed with
> > CT_DYNTICKS, as CT_DYNTICKS_MASK already is.
> > 
> > Signed-off-by: Valentin Schneider <vschneid@redhat.com>
> 
> It used to be that RCU extended quiescent state and dynticks enter/exit
> were coupled. But this isn't the case anymore. Nowadays RCU stops watching
> some time later after dynticks is entered.

I knew that consolidation of atomic operations was too good to last...

> I wonder if we shouldn't take advantage of that cleanup for a meaning that
> really reflect that RCU stops watching from there.
> 
> Paul what do you think? CT_EQS_IDX ? CT_RCUEQS_IDX? CT_RCUOFF_IDX? ...?

"After what you just did?  You can just RCU off!!!"

Sorry, couldn't resist...

I am having a hard time getting too excited about the name.  I could
suggest CT_RCU_WATCHING_IDX, but that isn't exactly the shortest
possible name.

							Thanx, Paul

Re: [PATCH 2/2] context_tracking, rcu: Rename RCU_DYNTICKS_IDX to CT_DYNTICKS_IDX

[Frederic Weisbecker]

Le Tue, Apr 09, 2024 at 12:53:03PM -0700, Paul E. McKenney a écrit :
> On Thu, Apr 04, 2024 at 03:38:40PM +0200, Frederic Weisbecker wrote:
> > Le Wed, Mar 27, 2024 at 12:29:02PM +0100, Valentin Schneider a écrit :
> > > The symbols relating to the CT_STATE part of context_tracking.state are now
> > > all prefixed with CT_STATE.
> > > 
> > > The RCU dynticks counter part of that atomic variable still involves
> > > symbols with different prefixes, align them all to be prefixed with
> > > CT_DYNTICKS, as CT_DYNTICKS_MASK already is.
> > > 
> > > Signed-off-by: Valentin Schneider <vschneid@redhat.com>
> > 
> > It used to be that RCU extended quiescent state and dynticks enter/exit
> > were coupled. But this isn't the case anymore. Nowadays RCU stops watching
> > some time later after dynticks is entered.
> 
> I knew that consolidation of atomic operations was too good to last...
> 
> > I wonder if we shouldn't take advantage of that cleanup for a meaning that
> > really reflect that RCU stops watching from there.
> > 
> > Paul what do you think? CT_EQS_IDX ? CT_RCUEQS_IDX? CT_RCUOFF_IDX? ...?
> 
> "After what you just did?  You can just RCU off!!!"
> 
> Sorry, couldn't resist...
> 
> I am having a hard time getting too excited about the name.  I could
> suggest CT_RCU_WATCHING_IDX, but that isn't exactly the shortest
> possible name.

I really like CT_RCU_WATCHING. It says everything. The _IDX isn't even
needed after all. What do you think?

Thanks.

> 							Thanx, Paul

Re: [PATCH 2/2] context_tracking, rcu: Rename RCU_DYNTICKS_IDX to CT_DYNTICKS_IDX

[Paul E. McKenney]

On Wed, Apr 10, 2024 at 12:30:55PM +0200, Frederic Weisbecker wrote:
> Le Tue, Apr 09, 2024 at 12:53:03PM -0700, Paul E. McKenney a écrit :
> > On Thu, Apr 04, 2024 at 03:38:40PM +0200, Frederic Weisbecker wrote:
> > > Le Wed, Mar 27, 2024 at 12:29:02PM +0100, Valentin Schneider a écrit :
> > > > The symbols relating to the CT_STATE part of context_tracking.state are now
> > > > all prefixed with CT_STATE.
> > > > 
> > > > The RCU dynticks counter part of that atomic variable still involves
> > > > symbols with different prefixes, align them all to be prefixed with
> > > > CT_DYNTICKS, as CT_DYNTICKS_MASK already is.
> > > > 
> > > > Signed-off-by: Valentin Schneider <vschneid@redhat.com>
> > > 
> > > It used to be that RCU extended quiescent state and dynticks enter/exit
> > > were coupled. But this isn't the case anymore. Nowadays RCU stops watching
> > > some time later after dynticks is entered.
> > 
> > I knew that consolidation of atomic operations was too good to last...
> > 
> > > I wonder if we shouldn't take advantage of that cleanup for a meaning that
> > > really reflect that RCU stops watching from there.
> > > 
> > > Paul what do you think? CT_EQS_IDX ? CT_RCUEQS_IDX? CT_RCUOFF_IDX? ...?
> > 
> > "After what you just did?  You can just RCU off!!!"
> > 
> > Sorry, couldn't resist...
> > 
> > I am having a hard time getting too excited about the name.  I could
> > suggest CT_RCU_WATCHING_IDX, but that isn't exactly the shortest
> > possible name.
> 
> I really like CT_RCU_WATCHING. It says everything. The _IDX isn't even
> needed after all. What do you think?

Works for me!

							Thanx, Paul

Re: [PATCH 2/2] context_tracking, rcu: Rename RCU_DYNTICKS_IDX to CT_DYNTICKS_IDX

[Valentin Schneider]

On 10/04/24 12:19, Paul E. McKenney wrote:
> On Wed, Apr 10, 2024 at 12:30:55PM +0200, Frederic Weisbecker wrote:
>> Le Tue, Apr 09, 2024 at 12:53:03PM -0700, Paul E. McKenney a écrit :
>> > I am having a hard time getting too excited about the name.  I could
>> > suggest CT_RCU_WATCHING_IDX, but that isn't exactly the shortest
>> > possible name.
>>
>> I really like CT_RCU_WATCHING. It says everything. The _IDX isn't even
>> needed after all. What do you think?
>
> Works for me!
>

Sounds good to me too, thanks for the suggestion :)

Now, what about ct_dynticks() & friends? I was about to do:

-static __always_inline int ct_dynticks(void)
+static __always_inline int ct_rcu_watching(void)
 {
-	return atomic_read(this_cpu_ptr(&context_tracking.state)) & CT_DYNTICKS_MASK;
+	return atomic_read(this_cpu_ptr(&context_tracking.state)) & CT_RCU_WATCHING_MASK;
 }

... but then realised that there's more siblings to the rcu_dynticks*()
family;

AFAICT dynticks_nesting could also get the rcu_watching prefix treatment,
`rcu_dynticks_task_exit() -> rcu_watching_task_exit` doesn't sound as
obvious though. The rcu_dyntick event probably can't be renamed either.

I'm not sure how far to take the renaming; seeing things like:

  notrace bool rcu_is_watching(void)
  {
          bool ret;

          preempt_disable_notrace();
          ret = !rcu_dynticks_curr_cpu_in_eqs();
          preempt_enable_notrace();
          return ret;
  }
  EXPORT_SYMBOL_GPL(rcu_is_watching);

makes me think most of the rcu_*dynticks / rcu_*eqs stuff could get an
rcu_watching facelift?

Here are my current considerations for identifiers used in context_tracking
in decreasing order of confidence:

| Old                                   | New                                                           |
|---------------------------------------+---------------------------------------------------------------|
| RCU_DYNTICKS_IDX                      | CT_RCU_WATCHING                                               |
| RCU_DYNTICKS_MASK                     | CT_RCU_WATCHING_MASK                                          |
| context_tracking.dynticks_nesting     | context_tracking.rcu_watching_nesting                         |
| context_tracking.dynticks_nmi_nesting | context_tracking.rcu_watching_nmi_nesting [bit of a mouthful] |
| rcu_dynticks_curr_cpu_in_eqs()        | rcu_watching_curr_cpu() [with an added negation]              |
|---------------------------------------+---------------------------------------------------------------|
| TRACE_EVENT_RCU(rcu_dyntick,          | [Can't change?]                                               |
|---------------------------------------+---------------------------------------------------------------|
| rcu_dynticks_task_enter()             | rcu_watching_task_enter()                                     |
| rcu_dynticks_task_exit()              | rcu_watching_task_exit()                                      |
| rcu_dynticks_task_trace_enter()       | rcu_watching_task_trace_enter()                               |
| rcu_dynticks_task_trace_exit()        | rcu_watching_task_trace_exit()                                |

Thoughts?

Re: [PATCH 2/2] context_tracking, rcu: Rename RCU_DYNTICKS_IDX to CT_DYNTICKS_IDX

[Frederic Weisbecker]

Le Mon, Apr 15, 2024 at 06:36:31PM +0200, Valentin Schneider a écrit :
> On 10/04/24 12:19, Paul E. McKenney wrote:
> > On Wed, Apr 10, 2024 at 12:30:55PM +0200, Frederic Weisbecker wrote:
> >> Le Tue, Apr 09, 2024 at 12:53:03PM -0700, Paul E. McKenney a écrit :
> >> > I am having a hard time getting too excited about the name.  I could
> >> > suggest CT_RCU_WATCHING_IDX, but that isn't exactly the shortest
> >> > possible name.
> >>
> >> I really like CT_RCU_WATCHING. It says everything. The _IDX isn't even
> >> needed after all. What do you think?
> >
> > Works for me!
> >
> 
> Sounds good to me too, thanks for the suggestion :)
> 
> Now, what about ct_dynticks() & friends? I was about to do:
> 
> -static __always_inline int ct_dynticks(void)
> +static __always_inline int ct_rcu_watching(void)
>  {
> -	return atomic_read(this_cpu_ptr(&context_tracking.state)) & CT_DYNTICKS_MASK;
> +	return atomic_read(this_cpu_ptr(&context_tracking.state)) & CT_RCU_WATCHING_MASK;
>  }

Yup!

> 
> ... but then realised that there's more siblings to the rcu_dynticks*()
> family;

Ouch right, sorry I forgot there is so much of this namespace. But in case you're
willing to clean that up:

> 
> AFAICT dynticks_nesting could also get the rcu_watching prefix treatment,
> `rcu_dynticks_task_exit() -> rcu_watching_task_exit` doesn't sound as

rcu_tasks_exit() ?

But Paul, is there a reason why check_holdout_task() doesn't check
ct_dynticks_cpu(task_cpu(t)) instead of maintaining this separate counter?

> obvious though. The rcu_dyntick event probably can't be renamed either.

I think we can rename trace_rcu_dyntick() to trace_rcu_watching()

> 
> I'm not sure how far to take the renaming; seeing things like:
> 
>   notrace bool rcu_is_watching(void)
>   {
>           bool ret;
> 
>           preempt_disable_notrace();
>           ret = !rcu_dynticks_curr_cpu_in_eqs();
>           preempt_enable_notrace();
>           return ret;
>   }
>   EXPORT_SYMBOL_GPL(rcu_is_watching);
> 
> makes me think most of the rcu_*dynticks / rcu_*eqs stuff could get an
> rcu_watching facelift?

The eqs part can stay as-is. But the *dynticks* needs an update.

> 
> Here are my current considerations for identifiers used in context_tracking
> in decreasing order of confidence:
> 
> | Old                                   | New                                                           |
> |---------------------------------------+---------------------------------------------------------------|
> | RCU_DYNTICKS_IDX                      | CT_RCU_WATCHING                                               |
> | RCU_DYNTICKS_MASK                     | CT_RCU_WATCHING_MASK                                          |
> | context_tracking.dynticks_nesting     | context_tracking.rcu_watching_nesting                         |

This can be context_tracking.nesting (and yes one day we might need to lock up
context_tracking.nesting and context_tracking.recursion together in a room and see
who wins after a day or two).

> | context_tracking.dynticks_nmi_nesting | context_tracking.rcu_watching_nmi_nesting [bit of a mouthful] |

context_tracking.nmi_nesting

> | rcu_dynticks_curr_cpu_in_eqs()        | rcu_watching_curr_cpu() [with an added negation]              |

Nice!

> |---------------------------------------+---------------------------------------------------------------|
> | TRACE_EVENT_RCU(rcu_dyntick,          | [Can't change?]                                               |

It can change. Officially trace events aren't ABI. Unoficially I wouldn't dare
changing the sched switch trace event but this one is fine.

> |---------------------------------------+---------------------------------------------------------------|
> | rcu_dynticks_task_enter()             | rcu_watching_task_enter()> | |

rcu_tasks_enter() ?

> | rcu_dynticks_task_exit()              | rcu_watching_task_exit()                                      |

rcu_tasks_exit() ?

> | rcu_dynticks_task_trace_enter()       | rcu_watching_task_trace_enter()                               |

rcu_tasks_trace_enter()?

> | rcu_dynticks_task_trace_exit()        | rcu_watching_task_trace_exit()                                |

rcu_tasks_trace_exit() ?

Thanks.

> 
> Thoughts?
> 

Re: [PATCH 2/2] context_tracking, rcu: Rename RCU_DYNTICKS_IDX to CT_DYNTICKS_IDX

[Valentin Schneider]

On 15/04/24 23:08, Frederic Weisbecker wrote:
> Le Mon, Apr 15, 2024 at 06:36:31PM +0200, Valentin Schneider a écrit :
>>
>> Sounds good to me too, thanks for the suggestion :)
>>
>> Now, what about ct_dynticks() & friends? I was about to do:
>>
>> -static __always_inline int ct_dynticks(void)
>> +static __always_inline int ct_rcu_watching(void)
>>  {
>> -	return atomic_read(this_cpu_ptr(&context_tracking.state)) & CT_DYNTICKS_MASK;
>> +	return atomic_read(this_cpu_ptr(&context_tracking.state)) & CT_RCU_WATCHING_MASK;
>>  }
>
> Yup!
>
>>
>> ... but then realised that there's more siblings to the rcu_dynticks*()
>> family;
>
> Ouch right, sorry I forgot there is so much of this namespace. But in case you're
> willing to clean that up:
>

While I'm at it, I figure I might as well.

>>
>> AFAICT dynticks_nesting could also get the rcu_watching prefix treatment,
>> `rcu_dynticks_task_exit() -> rcu_watching_task_exit` doesn't sound as
>
> rcu_tasks_exit() ?
>
> But Paul, is there a reason why check_holdout_task() doesn't check
> ct_dynticks_cpu(task_cpu(t)) instead of maintaining this separate counter?
>
>> obvious though. The rcu_dyntick event probably can't be renamed either.
>
> I think we can rename trace_rcu_dyntick() to trace_rcu_watching()
>
>>
>> I'm not sure how far to take the renaming; seeing things like:
>>
>>   notrace bool rcu_is_watching(void)
>>   {
>>           bool ret;
>>
>>           preempt_disable_notrace();
>>           ret = !rcu_dynticks_curr_cpu_in_eqs();
>>           preempt_enable_notrace();
>>           return ret;
>>   }
>>   EXPORT_SYMBOL_GPL(rcu_is_watching);
>>
>> makes me think most of the rcu_*dynticks / rcu_*eqs stuff could get an
>> rcu_watching facelift?
>
> The eqs part can stay as-is. But the *dynticks* needs an update.
>
>>
>> Here are my current considerations for identifiers used in context_tracking
>> in decreasing order of confidence:
>>
>> | Old                                   | New                                                           |
>> |---------------------------------------+---------------------------------------------------------------|
>> | RCU_DYNTICKS_IDX                      | CT_RCU_WATCHING                                               |
>> | RCU_DYNTICKS_MASK                     | CT_RCU_WATCHING_MASK                                          |
>> | context_tracking.dynticks_nesting     | context_tracking.rcu_watching_nesting                         |
>
> This can be context_tracking.nesting (and yes one day we might need to lock up
> context_tracking.nesting and context_tracking.recursion together in a room and see
> who wins after a day or two).
>

Much better!

>> | context_tracking.dynticks_nmi_nesting | context_tracking.rcu_watching_nmi_nesting [bit of a mouthful] |
>
> context_tracking.nmi_nesting
>
>> | rcu_dynticks_curr_cpu_in_eqs()        | rcu_watching_curr_cpu() [with an added negation]              |
>
> Nice!
>
>> |---------------------------------------+---------------------------------------------------------------|
>> | TRACE_EVENT_RCU(rcu_dyntick,          | [Can't change?]                                               |
>
> It can change. Officially trace events aren't ABI. Unoficially I wouldn't dare
> changing the sched switch trace event but this one is fine.
>

Cool, away it goes then :)

>> |---------------------------------------+---------------------------------------------------------------|
>> | rcu_dynticks_task_enter()             | rcu_watching_task_enter()> | |
>
> rcu_tasks_enter() ?
>
>> | rcu_dynticks_task_exit()              | rcu_watching_task_exit()                                      |
>
> rcu_tasks_exit() ?
>
>> | rcu_dynticks_task_trace_enter()       | rcu_watching_task_trace_enter()                               |
>
> rcu_tasks_trace_enter()?
>
>> | rcu_dynticks_task_trace_exit()        | rcu_watching_task_trace_exit()                                |
>
> rcu_tasks_trace_exit() ?
>

Now that you point it out, it looks obvious!

> Thanks.
>
>>
>> Thoughts?
>>