Messages in this thread | | | Date | Wed, 20 May 2020 20:28:00 +0200 | From | Sebastian Andrzej Siewior <> | Subject | Re: [PATCH 3/8] srcu: Use local_lock() for per-CPU struct srcu_data access |
| |
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
| |