On 2020-05-20 13:42:59 [-0400], Joel Fernandes wrote: > Hi Sebastian, Hi Joel,
> For pointer stability, can we just use get_local_ptr() and put_local_ptr() > instead of adding an extra lock? This keeps the pointer stable while keeping > the section preemptible on -rt. And we already have a lock in rcu_data, I > prefer not to add another lock if possible. What is this get_local_ptr() doing? I can't find it anywhere… > I wrote a diff below with get_local_ptr() (just build tested). Does this > solve your issue? see below. > > I remember Paul looked at that patch a few years ago and he said that > > that disabling interrupts here is important and matches the other part > > instance where the interrupts are disabled. Looking at it now, it seems > > that there is just pointer stability but I can't tell if > > rcu_segcblist_pend_cbs() needs more than just this. > > Which 'other part' are you referring to? Your patch removed local_irq_save() > from other places as well right? The patch converted hunks. > thanks, > > - Joel > > ---8<----------------------- > > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c > index 8ff71e5d0fe8b..5f49919205317 100644 > --- a/kernel/rcu/srcutree.c > +++ b/kernel/rcu/srcutree.c > @@ -778,13 +778,17 @@ static bool srcu_might_be_idle(struct srcu_struct *ssp) > unsigned long tlast; > > /* If the local srcu_data structure has callbacks, not idle. */ > - local_irq_save(flags); > - sdp = this_cpu_ptr(ssp->sda); > + sdp = get_local_ptr(ssp->sda); > + spin_lock_irqsave_rcu_node(sdp, flags); You acquire the node lock which was not acquired before. Is that okay? How is get_local_ptr() different to raw_cpu_ptr()? > if (rcu_segcblist_pend_cbs(&sdp->srcu_cblist)) { > - local_irq_restore(flags); > + spin_unlock_irqrestore_rcu_node(sdp, flags); > + put_local_ptr(sdp); > return false; /* Callbacks already present, so not idle. */ > } > - local_irq_restore(flags); > + > + spin_unlock_irqrestore_rcu_node(sdp, flags); > + put_local_ptr(sdp); > > /* > * No local callbacks, so probabalistically probe global state. Sebastian