Skip to content

Commit

Permalink
rcu/tree: Protect rcu_rdp_is_offloaded() invocations on RT
Browse files Browse the repository at this point in the history 
[ This is an all-in-one commit reverting the commit
    Revert "rcu/nocb: Protect NOCB state via local_lock() under PREEMPT_RT"
  and introducing this commit.
]

Valentin reported warnings about suspicious RCU usage on RT kernels. Those
happen when offloading of RCU callbacks is enabled:

  WARNING: suspicious RCU usage
  5.13.0-rt1 Freescale#20 Not tainted
  -----------------------------
  kernel/rcu/tree_plugin.h:69 Unsafe read of RCU_NOCB offloaded state!

  rcu_rdp_is_offloaded (kernel/rcu/tree_plugin.h:69 kernel/rcu/tree_plugin.h:58)
  rcu_core (kernel/rcu/tree.c:2332 kernel/rcu/tree.c:2398 kernel/rcu/tree.c:2777)
  rcu_cpu_kthread (./include/linux/bottom_half.h:32 kernel/rcu/tree.c:2876)

The reason is that rcu_rdp_is_offloaded() is invoked without one of the
required protections on RT enabled kernels because local_bh_disable() does
not disable preemption on RT.

Valentin proposed to add a local lock to the code in question, but that's
suboptimal in several aspects:

  1) local locks add extra code to !RT kernels for no value.

  2) All possible callsites have to audited and amended when affected
     possible at an outer function level due to lock nesting issues.

  3) As the local lock has to be taken at the outer functions it's required
     to release and reacquire them in the inner code sections which might
     voluntary schedule, e.g. rcu_do_batch().

Both callsites of rcu_rdp_is_offloaded() which trigger this check invoke
rcu_rdp_is_offloaded() in the variable declaration section right at the top
of the functions. But the actual usage of the result is either within a
section which provides the required protections or after such a section.

So the obvious solution is to move the invocation into the code sections
which provide the proper protections, which solves the problem for RT and
does not have any impact on !RT kernels.

Reported-by: Valentin Schneider <valentin.schneider@arm.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
  • Loading branch information
@KAGA-KOKO
KAGA-KOKO authored and Sebastian Andrzej Siewior committed Sep 22, 2021
1 parent 6b61ded commit 279b703
Show file tree
Hide file tree
Showing 4 changed files with 12 additions and 80 deletions.
11 changes: 4 additions & 7 deletions kernel/rcu/tree.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,6 @@ static DEFINE_PER_CPU_SHARED_ALIGNED(struct rcu_data, rcu_data) = {
.dynticks = ATOMIC_INIT(1),
#ifdef CONFIG_RCU_NOCB_CPU
.cblist.flags = SEGCBLIST_SOFTIRQ_ONLY,
.nocb_local_lock = INIT_LOCAL_LOCK(nocb_local_lock),
#endif
};
static struct rcu_state rcu_state = {
Expand Down Expand Up @@ -2279,13 +2278,13 @@ rcu_report_qs_rdp(struct rcu_data *rdp)
{
unsigned long flags;
unsigned long mask;
bool needwake = false;
const bool offloaded = rcu_rdp_is_offloaded(rdp);
bool offloaded, needwake = false;
struct rcu_node *rnp;

WARN_ON_ONCE(rdp->cpu != smp_processor_id());
rnp = rdp->mynode;
raw_spin_lock_irqsave_rcu_node(rnp, flags);
offloaded = rcu_rdp_is_offloaded(rdp);
if (rdp->cpu_no_qs.b.norm || rdp->gp_seq != rnp->gp_seq ||
rdp->gpwrap) {

Expand Down Expand Up @@ -2447,7 +2446,7 @@ static void rcu_do_batch(struct rcu_data *rdp)
int div;
bool __maybe_unused empty;
unsigned long flags;
const bool offloaded = rcu_rdp_is_offloaded(rdp);
bool offloaded;
struct rcu_head *rhp;
struct rcu_cblist rcl = RCU_CBLIST_INITIALIZER(rcl);
long bl, count = 0;
Expand All @@ -2473,6 +2472,7 @@ static void rcu_do_batch(struct rcu_data *rdp)
rcu_nocb_lock(rdp);
WARN_ON_ONCE(cpu_is_offline(smp_processor_id()));
pending = rcu_segcblist_n_cbs(&rdp->cblist);
offloaded = rcu_rdp_is_offloaded(rdp);
div = READ_ONCE(rcu_divisor);
div = div < 0 ? 7 : div > sizeof(long) * 8 - 2 ? sizeof(long) * 8 - 2 : div;
bl = max(rdp->blimit, pending >> div);
Expand Down Expand Up @@ -2812,12 +2812,10 @@ static void rcu_cpu_kthread(unsigned int cpu)
{
unsigned int *statusp = this_cpu_ptr(&rcu_data.rcu_cpu_kthread_status);
char work, *workp = this_cpu_ptr(&rcu_data.rcu_cpu_has_work);
struct rcu_data *rdp = this_cpu_ptr(&rcu_data);
int spincnt;

trace_rcu_utilization(TPS("Start CPU kthread@rcu_run"));
for (spincnt = 0; spincnt < 10; spincnt++) {
rcu_nocb_local_lock(rdp);
local_bh_disable();
*statusp = RCU_KTHREAD_RUNNING;
local_irq_disable();
Expand All @@ -2827,7 +2825,6 @@ static void rcu_cpu_kthread(unsigned int cpu)
if (work)
rcu_core();
local_bh_enable();
rcu_nocb_local_unlock(rdp);
if (*workp == 0) {
trace_rcu_utilization(TPS("End CPU kthread@rcu_wait"));
*statusp = RCU_KTHREAD_WAITING;
Expand Down
4 changes: 0 additions & 4 deletions kernel/rcu/tree.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -210,8 +210,6 @@ struct rcu_data {
struct timer_list nocb_timer; /* Enforce finite deferral. */
unsigned long nocb_gp_adv_time; /* Last call_rcu() CB adv (jiffies). */

local_lock_t nocb_local_lock;

/* The following fields are used by call_rcu, hence own cacheline. */
raw_spinlock_t nocb_bypass_lock ____cacheline_internodealigned_in_smp;
struct rcu_cblist nocb_bypass; /* Lock-contention-bypass CB list. */
Expand Down Expand Up @@ -447,8 +445,6 @@ static void rcu_nocb_unlock(struct rcu_data *rdp);
static void rcu_nocb_unlock_irqrestore(struct rcu_data *rdp,
unsigned long flags);
static void rcu_lockdep_assert_cblist_protected(struct rcu_data *rdp);
static void rcu_nocb_local_lock(struct rcu_data *rdp);
static void rcu_nocb_local_unlock(struct rcu_data *rdp);
#ifdef CONFIG_RCU_NOCB_CPU
static void __init rcu_organize_nocb_kthreads(void);
#define rcu_nocb_lock_irqsave(rdp, flags) \
Expand Down
39 changes: 0 additions & 39 deletions kernel/rcu/tree_nocb.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,6 @@ static inline int rcu_lockdep_is_held_nocb(struct rcu_data *rdp)
return lockdep_is_held(&rdp->nocb_lock);
}

static inline int rcu_lockdep_is_held_nocb_local(struct rcu_data *rdp)
{
return lockdep_is_held(&rdp->nocb_local_lock);
}

static inline bool rcu_current_is_nocb_kthread(struct rcu_data *rdp)
{
/* Race on early boot between thread creation and assignment */
Expand Down Expand Up @@ -186,22 +181,6 @@ static void rcu_nocb_unlock_irqrestore(struct rcu_data *rdp,
}
}

/*
* The invocation of rcu_core() within the RCU core kthreads remains preemptible
* under PREEMPT_RT, thus the offload state of a CPU could change while
* said kthreads are preempted. Prevent this from happening by protecting the
* offload state with a local_lock().
*/
static void rcu_nocb_local_lock(struct rcu_data *rdp)
{
local_lock(&rcu_data.nocb_local_lock);
}

static void rcu_nocb_local_unlock(struct rcu_data *rdp)
{
local_unlock(&rcu_data.nocb_local_lock);
}

/* Lockdep check that ->cblist may be safely accessed. */
static void rcu_lockdep_assert_cblist_protected(struct rcu_data *rdp)
{
Expand Down Expand Up @@ -969,7 +948,6 @@ static int rdp_offload_toggle(struct rcu_data *rdp,
if (rdp->nocb_cb_sleep)
rdp->nocb_cb_sleep = false;
rcu_nocb_unlock_irqrestore(rdp, flags);
rcu_nocb_local_unlock(rdp);

/*
* Ignore former value of nocb_cb_sleep and force wake up as it could
Expand Down Expand Up @@ -1001,7 +979,6 @@ static long rcu_nocb_rdp_deoffload(void *arg)

pr_info("De-offloading %d\n", rdp->cpu);

rcu_nocb_local_lock(rdp);
rcu_nocb_lock_irqsave(rdp, flags);
/*
* Flush once and for all now. This suffices because we are
Expand Down Expand Up @@ -1084,7 +1061,6 @@ static long rcu_nocb_rdp_offload(void *arg)
* Can't use rcu_nocb_lock_irqsave() while we are in
* SEGCBLIST_SOFTIRQ_ONLY mode.
*/
rcu_nocb_local_lock(rdp);
raw_spin_lock_irqsave(&rdp->nocb_lock, flags);

/*
Expand Down Expand Up @@ -1432,11 +1408,6 @@ static inline int rcu_lockdep_is_held_nocb(struct rcu_data *rdp)
return 0;
}

static inline int rcu_lockdep_is_held_nocb_local(struct rcu_data *rdp)
{
return 0;
}

static inline bool rcu_current_is_nocb_kthread(struct rcu_data *rdp)
{
return false;
Expand All @@ -1459,16 +1430,6 @@ static void rcu_nocb_unlock_irqrestore(struct rcu_data *rdp,
local_irq_restore(flags);
}

/* No ->nocb_local_lock to acquire. */
static void rcu_nocb_local_lock(struct rcu_data *rdp)
{
}

/* No ->nocb_local_lock to release. */
static void rcu_nocb_local_unlock(struct rcu_data *rdp)
{
}

/* Lockdep check that ->cblist may be safely accessed. */
static void rcu_lockdep_assert_cblist_protected(struct rcu_data *rdp)
{
Expand Down
38 changes: 8 additions & 30 deletions kernel/rcu/tree_plugin.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,45 +13,23 @@

#include "../locking/rtmutex_common.h"

/*
* Is a local read of the rdp's offloaded state safe and stable?
* See rcu_nocb_local_lock() & family.
*/
static inline bool rcu_local_offload_access_safe(struct rcu_data *rdp)
{
if (!preemptible())
return true;

if (!is_migratable()) {
if (!IS_ENABLED(CONFIG_RCU_NOCB))
return true;

return rcu_lockdep_is_held_nocb_local(rdp);
}

return false;
}

static bool rcu_rdp_is_offloaded(struct rcu_data *rdp)
{
/*
* In order to read the offloaded state of an rdp is a safe and stable
* way and prevent from its value to be changed under us, we must
* either...
* In order to read the offloaded state of an rdp is a safe
* and stable way and prevent from its value to be changed
* under us, we must either hold the barrier mutex, the cpu
* hotplug lock (read or write) or the nocb lock. Local
* non-preemptible reads are also safe. NOCB kthreads and
* timers have their own means of synchronization against the
* offloaded state updaters.
*/
RCU_LOCKDEP_WARN(
// ...hold the barrier mutex...
!(lockdep_is_held(&rcu_state.barrier_mutex) ||
// ... the cpu hotplug lock (read or write)...
(IS_ENABLED(CONFIG_HOTPLUG_CPU) && lockdep_is_cpus_held()) ||
// ... or the NOCB lock.
rcu_lockdep_is_held_nocb(rdp) ||
// Local reads still require the local state to remain stable
// (preemption disabled / local lock held)
(rdp == this_cpu_ptr(&rcu_data) &&
rcu_local_offload_access_safe(rdp)) ||
// NOCB kthreads and timers have their own means of
// synchronization against the offloaded state updaters.
!(IS_ENABLED(CONFIG_PREEMPT_COUNT) && preemptible())) ||
rcu_current_is_nocb_kthread(rdp)),
"Unsafe read of RCU_NOCB offloaded state"
);
Expand Down

0 comments on commit 279b703

Please sign in to comment.