Re: [PATCH 10/21] context_tracking: Take idle eqs entrypoints over RCU

Mark Rutland
2022-05-03T11:03:05Z

Hi Frederic,

On Tue, May 03, 2022 at 12:00:40PM +0200, Frederic Weisbecker wrote:
> The RCU dynticks counter is going to be merged into the context tracking
> subsystem. Start with moving the idle extended quiescent states
> entrypoints to context tracking. For now those are dumb redirections to
> existing RCU calls.

I was a bit confused looking at this, because that redirection only exists for
CONFIG_CONTEXT_TRACKING, and is empty otherwise.

I see this patch makes TREE_RCU select CONTEXT_TRACKING, which means that
works. Since that also means building the rest of the context tracking code, I
think it'd be worth mentioning that in the commit message.

Do all architectures which can use TREE_RCU today already support context
tracking? If not, do those work by default?

Thanks,
Mark.

> Signed-off-by: Frederic Weisbecker <frederic@kernel.org>
> Cc: Paul E. McKenney <paulmck@kernel.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Neeraj Upadhyay <quic_neeraju@quicinc.com>
> Cc: Uladzislau Rezki <uladzislau.rezki@sony.com>
> Cc: Joel Fernandes <joel@joelfernandes.org>
> Cc: Boqun Feng <boqun.feng@gmail.com>
> Cc: Nicolas Saenz Julienne <nsaenz@kernel.org>
> Cc: Marcelo Tosatti <mtosatti@redhat.com>
> Cc: Xiongfeng Wang <wangxiongfeng2@huawei.com>
> Cc: Yu Liao<liaoyu15@huawei.com>
> Cc: Phil Auld <pauld@redhat.com>
> Cc: Paul Gortmaker<paul.gortmaker@windriver.com>
> Cc: Alex Belits <abelits@marvell.com>
> ---
> Documentation/RCU/stallwarn.rst | 4 ++--
> arch/arm/mach-imx/cpuidle-imx6q.c | 5 +++--
> drivers/acpi/processor_idle.c | 5 +++--
> drivers/cpuidle/cpuidle.c | 9 +++++----
> include/linux/context_tracking.h | 8 ++++++++
> include/linux/rcupdate.h | 2 +-
> kernel/context_tracking.c | 12 ++++++++++++
> kernel/locking/lockdep.c | 2 +-
> kernel/rcu/Kconfig | 2 ++
> kernel/rcu/tree.c | 2 --
> kernel/rcu/update.c | 2 +-
> kernel/sched/idle.c | 10 +++++-----
> kernel/sched/sched.h | 1 +
> 13 files changed, 44 insertions(+), 20 deletions(-)
>
> diff --git a/Documentation/RCU/stallwarn.rst b/Documentation/RCU/stallwarn.rst
> index 1d863b04727c..bdd52b40f307 100644
> --- a/Documentation/RCU/stallwarn.rst
> +++ b/Documentation/RCU/stallwarn.rst
> @@ -97,8 +97,8 @@ warnings:
> which will include additional debugging information.
>
> - A low-level kernel issue that either fails to invoke one of the
> - variants of rcu_user_enter(), rcu_user_exit(), rcu_idle_enter(),
> - rcu_idle_exit(), rcu_irq_enter(), or rcu_irq_exit() on the one
> + variants of rcu_user_enter(), rcu_user_exit(), ct_idle_enter(),
> + ct_idle_exit(), rcu_irq_enter(), or rcu_irq_exit() on the one
> hand, or that invokes one of them too many times on the other.
> Historically, the most frequent issue has been an omission
> of either irq_enter() or irq_exit(), which in turn invoke
> diff --git a/arch/arm/mach-imx/cpuidle-imx6q.c b/arch/arm/mach-imx/cpuidle-imx6q.c
> index 094337dc1bc7..d086cbae09c3 100644
> --- a/arch/arm/mach-imx/cpuidle-imx6q.c
> +++ b/arch/arm/mach-imx/cpuidle-imx6q.c
> @@ -3,6 +3,7 @@
> * Copyright (C) 2012 Freescale Semiconductor, Inc.
> */
>
> +#include <linux/context_tracking.h>
> #include <linux/cpuidle.h>
> #include <linux/module.h>
> #include <asm/cpuidle.h>
> @@ -24,9 +25,9 @@ static int imx6q_enter_wait(struct cpuidle_device *dev,
> imx6_set_lpm(WAIT_UNCLOCKED);
> raw_spin_unlock(&cpuidle_lock);
>
> - rcu_idle_enter();
> + ct_idle_enter();
> cpu_do_idle();
> - rcu_idle_exit();
> + ct_idle_exit();
>
> raw_spin_lock(&cpuidle_lock);
> if (num_idle_cpus-- == num_online_cpus())
> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
> index 32b20efff5f8..935f4113d5f6 100644
> --- a/drivers/acpi/processor_idle.c
> +++ b/drivers/acpi/processor_idle.c
> @@ -22,6 +22,7 @@
> #include <linux/cpu.h>
> #include <linux/minmax.h>
> #include <acpi/processor.h>
> +#include <linux/context_tracking.h>
>
> /*
> * Include the apic definitions for x86 to have the APIC timer related defines
> @@ -648,11 +649,11 @@ static int acpi_idle_enter_bm(struct cpuidle_driver *drv,
> raw_spin_unlock(&c3_lock);
> }
>
> - rcu_idle_enter();
> + ct_idle_enter();
>
> acpi_idle_do_entry(cx);
>
> - rcu_idle_exit();
> + ct_idle_exit();
>
> /* Re-enable bus master arbitration */
> if (dis_bm) {
> diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
> index ef2ea1b12cd8..62dd956025f3 100644
> --- a/drivers/cpuidle/cpuidle.c
> +++ b/drivers/cpuidle/cpuidle.c
> @@ -23,6 +23,7 @@
> #include <linux/suspend.h>
> #include <linux/tick.h>
> #include <linux/mmu_context.h>
> +#include <linux/context_tracking.h>
> #include <trace/events/power.h>
>
> #include "cpuidle.h"
> @@ -150,12 +151,12 @@ static void enter_s2idle_proper(struct cpuidle_driver *drv,
> */
> stop_critical_timings();
> if (!(target_state->flags & CPUIDLE_FLAG_RCU_IDLE))
> - rcu_idle_enter();
> + ct_idle_enter();
> target_state->enter_s2idle(dev, drv, index);
> if (WARN_ON_ONCE(!irqs_disabled()))
> local_irq_disable();
> if (!(target_state->flags & CPUIDLE_FLAG_RCU_IDLE))
> - rcu_idle_exit();
> + ct_idle_exit();
> tick_unfreeze();
> start_critical_timings();
>
> @@ -233,10 +234,10 @@ int cpuidle_enter_state(struct cpuidle_device *dev, struct cpuidle_driver *drv,
>
> stop_critical_timings();
> if (!(target_state->flags & CPUIDLE_FLAG_RCU_IDLE))
> - rcu_idle_enter();
> + ct_idle_enter();
> entered_state = target_state->enter(dev, drv, index);
> if (!(target_state->flags & CPUIDLE_FLAG_RCU_IDLE))
> - rcu_idle_exit();
> + ct_idle_exit();
> start_critical_timings();
>
> sched_clock_idle_wakeup_event();
> diff --git a/include/linux/context_tracking.h b/include/linux/context_tracking.h
> index e35ae66b4794..27afb75f2650 100644
> --- a/include/linux/context_tracking.h
> +++ b/include/linux/context_tracking.h
> @@ -119,4 +119,12 @@ extern void context_tracking_init(void);
> static inline void context_tracking_init(void) { }
> #endif /* CONFIG_CONTEXT_TRACKING_USER_FORCE */
>
> +#ifdef CONFIG_CONTEXT_TRACKING
> +extern void ct_idle_enter(void);
> +extern void ct_idle_exit(void);
> +#else
> +static inline void ct_idle_enter(void) { }
> +static inline void ct_idle_exit(void) { }
> +#endif /* !CONFIG_CONTEXT_TRACKING */
> +
> #endif
> diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> index 7f12daa4708b..d3b25e65f144 100644
> --- a/include/linux/rcupdate.h
> +++ b/include/linux/rcupdate.h
> @@ -129,7 +129,7 @@ static inline void rcu_nocb_flush_deferred_wakeup(void) { }
> * @a: Code that RCU needs to pay attention to.
> *
> * RCU read-side critical sections are forbidden in the inner idle loop,
> - * that is, between the rcu_idle_enter() and the rcu_idle_exit() -- RCU
> + * that is, between the ct_idle_enter() and the ct_idle_exit() -- RCU
> * will happily ignore any such read-side critical sections. However,
> * things like powertop need tracepoints in the inner idle loop.
> *

> diff --git a/kernel/context_tracking.c b/kernel/context_tracking.c
> index da067a36b326..987f0b2a3962 100644
> --- a/kernel/context_tracking.c
> +++ b/kernel/context_tracking.c
> @@ -251,3 +251,15 @@ void __init context_tracking_init(void)
> #endif
>
> #endif /* #ifdef CONFIG_CONTEXT_TRACKING_USER */
> +
> +noinstr void ct_idle_enter(void)
> +{
> + rcu_idle_enter();
> +}
> +EXPORT_SYMBOL_GPL(ct_idle_enter);
> +
> +void ct_idle_exit(void)
> +{
> + rcu_idle_exit();
> +}
> +EXPORT_SYMBOL_GPL(ct_idle_exit);
> diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
> index c06cab6546ed..5f0dfe37234b 100644
> --- a/kernel/locking/lockdep.c
> +++ b/kernel/locking/lockdep.c
> @@ -6546,7 +6546,7 @@ void lockdep_rcu_suspicious(const char *file, const int line, const char *s)
>
> /*
> * If a CPU is in the RCU-free window in idle (ie: in the section
> - * between rcu_idle_enter() and rcu_idle_exit(), then RCU
> + * between ct_idle_enter() and ct_idle_exit(), then RCU
> * considers that CPU to be in an "extended quiescent state",
> * which means that RCU will be completely ignoring that CPU.
> * Therefore, rcu_read_lock() and friends have absolutely no
> diff --git a/kernel/rcu/Kconfig b/kernel/rcu/Kconfig
> index 27aab870ae4c..85f3b884fa09 100644
> --- a/kernel/rcu/Kconfig
> +++ b/kernel/rcu/Kconfig
> @@ -8,6 +8,8 @@ menu "RCU Subsystem"
> config TREE_RCU
> bool
> default y if SMP
> + # Dynticks-idle tracking
> + select CONTEXT_TRACKING
> help
> This option selects the RCU implementation that is
> designed for very large SMP system with hundreds or
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index 2a6e7723fc4d..92a288668e7c 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -664,7 +664,6 @@ noinstr void rcu_idle_enter(void)
> lockdep_assert_irqs_disabled();
> rcu_eqs_enter(false);
> }
> -EXPORT_SYMBOL_GPL(rcu_idle_enter);
>
> #ifdef CONFIG_NO_HZ_FULL
>
> @@ -912,7 +911,6 @@ void rcu_idle_exit(void)
> rcu_eqs_exit(false);
> local_irq_restore(flags);
> }
> -EXPORT_SYMBOL_GPL(rcu_idle_exit);
>
> #ifdef CONFIG_NO_HZ_FULL
> /**
> diff --git a/kernel/rcu/update.c b/kernel/rcu/update.c
> index 2e93acad1e31..738842c4886b 100644
> --- a/kernel/rcu/update.c
> +++ b/kernel/rcu/update.c
> @@ -85,7 +85,7 @@ module_param(rcu_normal_after_boot, int, 0444);
> * and while lockdep is disabled.
> *
> * Note that if the CPU is in the idle loop from an RCU point of view (ie:
> - * that we are in the section between rcu_idle_enter() and rcu_idle_exit())
> + * that we are in the section between ct_idle_enter() and ct_idle_exit())
> * then rcu_read_lock_held() sets ``*ret`` to false even if the CPU did an
> * rcu_read_lock(). The reason for this is that RCU ignores CPUs that are
> * in such a section, considering these as in extended quiescent state,
> diff --git a/kernel/sched/idle.c b/kernel/sched/idle.c
> index 8f8b5020e76a..6de222b23b49 100644
> --- a/kernel/sched/idle.c
> +++ b/kernel/sched/idle.c
> @@ -53,14 +53,14 @@ static noinline int __cpuidle cpu_idle_poll(void)
> {
> trace_cpu_idle(0, smp_processor_id());
> stop_critical_timings();
> - rcu_idle_enter();
> + ct_idle_enter();
> local_irq_enable();
>
> while (!tif_need_resched() &&
> (cpu_idle_force_poll || tick_check_broadcast_expired()))
> cpu_relax();
>
> - rcu_idle_exit();
> + ct_idle_exit();
> start_critical_timings();
> trace_cpu_idle(PWR_EVENT_EXIT, smp_processor_id());
>
> @@ -98,12 +98,12 @@ void __cpuidle default_idle_call(void)
> *
> * Trace IRQs enable here, then switch off RCU, and have
> * arch_cpu_idle() use raw_local_irq_enable(). Note that
> - * rcu_idle_enter() relies on lockdep IRQ state, so switch that
> + * ct_idle_enter() relies on lockdep IRQ state, so switch that
> * last -- this is very similar to the entry code.
> */
> trace_hardirqs_on_prepare();
> lockdep_hardirqs_on_prepare(_THIS_IP_);
> - rcu_idle_enter();
> + ct_idle_enter();
> lockdep_hardirqs_on(_THIS_IP_);
>
> arch_cpu_idle();
> @@ -116,7 +116,7 @@ void __cpuidle default_idle_call(void)
> */
> raw_local_irq_disable();
> lockdep_hardirqs_off(_THIS_IP_);
> - rcu_idle_exit();
> + ct_idle_exit();
> lockdep_hardirqs_on(_THIS_IP_);
> raw_local_irq_enable();
>
> diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
> index 58263f90c559..f398f2bf05f4 100644
> --- a/kernel/sched/sched.h
> +++ b/kernel/sched/sched.h
> @@ -27,6 +27,7 @@
> #include <linux/capability.h>
> #include <linux/cgroup_api.h>
> #include <linux/cgroup.h>
> +#include <linux/context_tracking.h>
> #include <linux/cpufreq.h>
> #include <linux/cpumask_api.h>
> #include <linux/ctype.h>
> --
> 2.25.1
>