On Fri, Aug 20, 2021 at 09:42:36AM +0200, Sebastian Andrzej Siewior wrote: > From: "From: Scott Wood" <swood@xxxxxxxxxx> > > rcutorture is generating some nesting scenarios that are not compatible on PREEMPT_RT. > For example: > preempt_disable(); > rcu_read_lock_bh(); > preempt_enable(); > rcu_read_unlock_bh(); > > The problem here is that on PREEMPT_RT the bottom halves have to be > disabled and enabled in preemptible context. > > Reorder locking: start with BH locking and continue with then with > disabling preemption or interrupts. In the unlocking do it reverse by > first enabling interrupts and preemption and BH at the very end. > Ensure that on PREEMPT_RT BH locking remains unchanged if in > non-preemptible context. > > Link: https://lkml.kernel.org/r/20190911165729.11178-6-swood@xxxxxxxxxx > Link: https://lkml.kernel.org/r/20210819182035.GF4126399@paulmck-ThinkPad-P17-Gen-1 > Signed-off-by: Scott Wood <swood@xxxxxxxxxx> > [bigeasy: Drop ATOM_BH, make it only about changing BH in atomic > context. Allow enabling RCU in IRQ-off section. Reword commit message.] > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@xxxxxxxxxxxxx> Looks plausible. ;-) I have queued this for testing and further review. If all goes well, perhaps the v5.16 merge window. Thanx, Paul > --- > v1…v2: > - Drop the ATOM_BH* bits. There don't seem to be needed, Paul did not > ant the preempt-disable around enabling/disabling BH as it might fix > things that RCU should take care. > > - Allow enabling RCU with disabled interrupts on RT. Scott confirmed > that it was needed but might no longer be needed. Paul said that it > might have been required at some point. It survived multiple 6h long > TREE01 and TREE06 testing. > > kernel/rcu/rcutorture.c | 48 ++++++++++++++++++++++++++++++----------- > 1 file changed, 36 insertions(+), 12 deletions(-) > > diff --git a/kernel/rcu/rcutorture.c b/kernel/rcu/rcutorture.c > index 40ef5417d9545..d2ef535530b10 100644 > --- a/kernel/rcu/rcutorture.c > +++ b/kernel/rcu/rcutorture.c > @@ -1432,28 +1432,34 @@ static void rcutorture_one_extend(int *readstate, int newstate, > /* First, put new protection in place to avoid critical-section gap. */ > if (statesnew & RCUTORTURE_RDR_BH) > local_bh_disable(); > + if (statesnew & RCUTORTURE_RDR_RBH) > + rcu_read_lock_bh(); > if (statesnew & RCUTORTURE_RDR_IRQ) > local_irq_disable(); > if (statesnew & RCUTORTURE_RDR_PREEMPT) > preempt_disable(); > - if (statesnew & RCUTORTURE_RDR_RBH) > - rcu_read_lock_bh(); > if (statesnew & RCUTORTURE_RDR_SCHED) > rcu_read_lock_sched(); > if (statesnew & RCUTORTURE_RDR_RCU) > idxnew = cur_ops->readlock() << RCUTORTURE_RDR_SHIFT; > > - /* Next, remove old protection, irq first due to bh conflict. */ > + /* > + * Next, remove old protection, in decreasing order of strength > + * to avoid unlock paths that aren't safe in the stronger > + * context. Namely: BH can not be enabled with disabled interrupts. > + * Additionally PREEMPT_RT requires that BH is enabled in preemptible > + * context. > + */ > if (statesold & RCUTORTURE_RDR_IRQ) > local_irq_enable(); > - if (statesold & RCUTORTURE_RDR_BH) > - local_bh_enable(); > if (statesold & RCUTORTURE_RDR_PREEMPT) > preempt_enable(); > - if (statesold & RCUTORTURE_RDR_RBH) > - rcu_read_unlock_bh(); > if (statesold & RCUTORTURE_RDR_SCHED) > rcu_read_unlock_sched(); > + if (statesold & RCUTORTURE_RDR_BH) > + local_bh_enable(); > + if (statesold & RCUTORTURE_RDR_RBH) > + rcu_read_unlock_bh(); > if (statesold & RCUTORTURE_RDR_RCU) { > bool lockit = !statesnew && !(torture_random(trsp) & 0xffff); > > @@ -1496,6 +1502,9 @@ rcutorture_extend_mask(int oldmask, struct torture_random_state *trsp) > int mask = rcutorture_extend_mask_max(); > unsigned long randmask1 = torture_random(trsp) >> 8; > unsigned long randmask2 = randmask1 >> 3; > + unsigned long preempts = RCUTORTURE_RDR_PREEMPT | RCUTORTURE_RDR_SCHED; > + unsigned long preempts_irq = preempts | RCUTORTURE_RDR_IRQ; > + unsigned long bhs = RCUTORTURE_RDR_BH | RCUTORTURE_RDR_RBH; > > WARN_ON_ONCE(mask >> RCUTORTURE_RDR_SHIFT); > /* Mostly only one bit (need preemption!), sometimes lots of bits. */ > @@ -1503,11 +1512,26 @@ rcutorture_extend_mask(int oldmask, struct torture_random_state *trsp) > mask = mask & randmask2; > else > mask = mask & (1 << (randmask2 % RCUTORTURE_RDR_NBITS)); > - /* Can't enable bh w/irq disabled. */ > - if ((mask & RCUTORTURE_RDR_IRQ) && > - ((!(mask & RCUTORTURE_RDR_BH) && (oldmask & RCUTORTURE_RDR_BH)) || > - (!(mask & RCUTORTURE_RDR_RBH) && (oldmask & RCUTORTURE_RDR_RBH)))) > - mask |= RCUTORTURE_RDR_BH | RCUTORTURE_RDR_RBH; > + > + /* > + * Can't enable bh w/irq disabled. > + */ > + if (mask & RCUTORTURE_RDR_IRQ) > + mask |= oldmask & bhs; > + > + /* > + * Ideally these sequences would be detected in debug builds > + * (regardless of RT), but until then don't stop testing > + * them on non-RT. > + */ > + if (IS_ENABLED(CONFIG_PREEMPT_RT)) { > + /* Can't modify BH in atomic context */ > + if (oldmask & preempts_irq) > + mask &= ~bhs; > + if ((oldmask | mask) & preempts_irq) > + mask |= oldmask & bhs; > + } > + > return mask ?: RCUTORTURE_RDR_RCU; > } > > -- > 2.33.0 >
