Re: [PATCH rcu/next 3/3] rcu: Call trace_rcu_callback() also for bypass queuing (v2)

[Frederic Weisbecker]

On Sat, Sep 17, 2022 at 05:43:06PM -0400, Joel Fernandes wrote:
> On Sat, Sep 17, 2022 at 3:58 PM Frederic Weisbecker <frederic@xxxxxxxxxx> wrote:
> >
> > On Sat, Sep 17, 2022 at 04:42:00PM +0000, Joel Fernandes (Google) wrote:
> > > @@ -2809,17 +2825,15 @@ void call_rcu(struct rcu_head *head, rcu_callback_t func)
> > >       }
> > >
> > >       check_cb_ovld(rdp);
> > > -     if (rcu_nocb_try_bypass(rdp, head, &was_alldone, flags))
> > > +
> > > +     if (rcu_nocb_try_bypass(rdp, head, &was_alldone, flags)) {
> > > +             __trace_rcu_callback(head, rdp);
> > >               return; // Enqueued onto ->nocb_bypass, so just leave.
> > > +     }
> >
> > I think the bypass enqueues should be treated differently. Either with extending
> > the current trace_rcu_callback/trace_rcu_kvfree_callback (might break tools)
> >
> > or
> > with creating a new trace_rcu_callback_bypass()/trace_rcu_kvfree_callback_bypass().
> >
> > Those could later be paired with a trace_rcu_bypass_flush().
> 
> I am having a hard time seeing why it should be treated differently.
> We already increment the length of the main segcblist even when
> bypassing. Why not just call the trace point instead of omitting it?

I'm not suggesting to omit it. I'm suggesting to improve its precision.

> Otherwise it becomes a bit confusing IMO (say someone does not enable
> your proposed new bypass tracepoint and only enables the existing one,
> then they would see weird traces where call_rcu is called but their
> traces are missing trace_rcu_callback).

Well, if they decided to see only half of the information...

> Not to mention - your
> suggestion will also complicate writing tools that use the existing
> rcu_callback tracepoint to monitor call_rcu().

If we add another tracepoint, the prototype will be the same as the
existing one, not many lines to add. If instead we extend the existing
tracepoint, it's merely just a flag to check or ignore.

OTOH your suggestion doesn't provide any bypass related information.

> 
> Also if you see the definition of rcu_callback, "Tracepoint for the
> registration of a single RCU callback function.".  That pretty much
> fits the usage here.

Doesn't tell if it's a bypass or not.

> 
> As for tracing of the flushing, I don’t care about tracing that at the
> moment using tracepoints

You will soon enough ;-)

> but I don’t mind if it is added later.
> Maybe let’s let Paul help resolve our disagreement on this one? :)

FWIW, I would be personally interested in such tracepoints (or the extension
of the existing ones, whichever way you guys prefer), they would be of great help
for debugging.

Also if rcu_top is ever released, I really hope the kernel will be ready in
case we want the tool to display bypass related informations.

Please be careful while designing tracepoints that may be consumed by userspace
released tools. Such tracepoints eventually turn into ABI and there is no way
back after that.

Thanks.