Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220917164200.511783-1-joel@joelfernandes.org>
 <20220917164200.511783-4-joel@joelfernandes.org> <20220917195807.GA39579@lothringen>
In-Reply-To: <20220917195807.GA39579@lothringen>
From:   Joel Fernandes <joel@joelfernandes.org>
Date:   Sat, 17 Sep 2022 17:43:06 -0400
Message-ID: <CAEXW_YQxwDF5jhS9gdLa0FsNeD+WoZL0PQydMbT4hsUtLm0hMA@mail.gmail.com>
Subject: Re: [PATCH rcu/next 3/3] rcu: Call trace_rcu_callback() also for
 bypass queuing (v2)
To:     Frederic Weisbecker <frederic@kernel.org>
Cc:     LKML <linux-kernel@vger.kernel.org>,
        Neeraj upadhyay <neeraj.iitr10@gmail.com>,
        "Paul E. McKenney" <paulmck@kernel.org>, rcu <rcu@vger.kernel.org>,
        Steven Rostedt <rostedt@goodmis.org>,
        Rushikesh S Kadam <rushikesh.s.kadam@intel.com>,
        "Uladzislau Rezki (Sony)" <urezki@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Sat, Sep 17, 2022 at 3:58 PM Frederic Weisbecker <frederic@kernel.org> w=
rote:
>
> 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_callba=
ck_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 ex=
tending
> the current trace_rcu_callback/trace_rcu_kvfree_callback (might break too=
ls)
>
> 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?
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). Not to mention - your
suggestion will also complicate writing tools that use the existing
rcu_callback tracepoint to monitor call_rcu().

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.

As for tracing of the flushing, I don=E2=80=99t care about tracing that at =
the
moment using tracepoints, but I don=E2=80=99t mind if it is added later.

Maybe let=E2=80=99s let Paul help resolve our disagreement on this one? :)

thanks,

 - Joel

>
>
> Thanks.
>
>
> > +
> >       // If no-CBs CPU gets here, rcu_nocb_try_bypass() acquired ->nocb=
_lock.
> >       rcu_segcblist_enqueue(&rdp->cblist, head);
> > -     if (__is_kvfree_rcu_offset((unsigned long)func))
> > -             trace_rcu_kvfree_callback(rcu_state.name, head,
> > -                                      (unsigned long)func,
> > -                                      rcu_segcblist_n_cbs(&rdp->cblist=
));
> > -     else
> > -             trace_rcu_callback(rcu_state.name, head,
> > -                                rcu_segcblist_n_cbs(&rdp->cblist));
> > +     __trace_rcu_callback(head, rdp);
> >
> >       trace_rcu_segcb_stats(&rdp->cblist, TPS("SegCBQueued"));
> >
> > --
> > 2.37.3.968.ga6b4b080e4-goog
> >