On Tue, Jul 02, 2024 at 09:18:57PM +0200, Peter Zijlstra wrote:
> On Tue, Jul 02, 2024 at 10:54:51AM -0700, Andrii Nakryiko wrote:
>
> > > @@ -593,6 +595,12 @@ static struct uprobe *get_uprobe(struct uprobe *uprobe)
> > > return uprobe;
> > > }
> > >
> > > +static void uprobe_free_rcu(struct rcu_head *rcu)
> > > +{
> > > + struct uprobe *uprobe = container_of(rcu, struct uprobe, rcu);
> > > + kfree(uprobe);
> > > +}
> > > +
> > > static void put_uprobe(struct uprobe *uprobe)
> > > {
> > > if (refcount_dec_and_test(&uprobe->ref)) {
> > > @@ -604,7 +612,8 @@ static void put_uprobe(struct uprobe *uprobe)
> >
> > right above this we have roughly this:
> >
> > percpu_down_write(&uprobes_treelock);
> >
> > /* refcount check */
> > rb_erase(&uprobe->rb_node, &uprobes_tree);
> >
> > percpu_up_write(&uprobes_treelock);
> >
> >
> > This writer lock is necessary for modification of the RB tree. And I
> > was under impression that I shouldn't be doing
> > percpu_(down|up)_write() inside the normal
> > rcu_read_lock()/rcu_read_unlock() region (percpu_down_write has
> > might_sleep() in it). But maybe I'm wrong, hopefully Paul can help to
> > clarify.
>
> preemptible RCU or SRCU would work.
I agree that SRCU would work from a functional viewpoint. No so for
preemptible RCU, which permits preemption (and on -rt, blocking for
spinlocks), it does not permit full-up blocking, and for good reason.
> > But actually what's wrong with RCU Tasks Trace flavor?
>
> Paul, isn't this the RCU flavour you created to deal with
> !rcu_is_watching()? The flavour that never should have been created in
> favour of just cleaning up the mess instead of making more.
My guess is that you are instead thinking of RCU Tasks Rude, which can
be eliminated once all architectures get their entry/exit/deep-idle
functions either inlined or marked noinstr.
> > I will
> > ultimately use it anyway to avoid uprobe taking unnecessary refcount
> > and to protect uprobe->consumers iteration and uc->handler() calls,
> > which could be sleepable, so would need rcu_read_lock_trace().
>
> I don't think you need trace-rcu for that. SRCU would do nicely I think.
