diff mbox series

[rcu/next,2/3] rcu: Move trace_rcu_callback() before bypassing

Message ID 20220915001419.55617-3-joel@joelfernandes.org (mailing list archive)
State Superseded
Headers show
Series Preparatory patches borrowed from lazy rcu v5 | expand

Commit Message

Joel Fernandes Sept. 15, 2022, 12:14 a.m. UTC
If any CB is queued into the bypass list, then trace_rcu_callback() does
not show it. This makes it not clear when a callback was actually
queued, as you only end up getting a trace_rcu_invoke_callback() trace.
Fix it by moving trace_rcu_callback() before
trace_rcu_nocb_try_bypass().

Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
 kernel/rcu/tree.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

Comments

Frederic Weisbecker Sept. 16, 2022, 11:09 a.m. UTC | #1
On Thu, Sep 15, 2022 at 12:14:18AM +0000, Joel Fernandes (Google) wrote:
> If any CB is queued into the bypass list, then trace_rcu_callback() does
> not show it. This makes it not clear when a callback was actually
> queued, as you only end up getting a trace_rcu_invoke_callback() trace.
> Fix it by moving trace_rcu_callback() before
> trace_rcu_nocb_try_bypass().
> 
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> ---
>  kernel/rcu/tree.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index 5ec97e3f7468..9fe581be8696 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -2809,10 +2809,7 @@ 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))
> -		return; // Enqueued onto ->nocb_bypass, so just leave.
> -	// 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,
> @@ -2821,6 +2818,11 @@ void call_rcu(struct rcu_head *head, rcu_callback_t func)
>  		trace_rcu_callback(rcu_state.name, head,
>  				   rcu_segcblist_n_cbs(&rdp->cblist));
>  
> +	if (rcu_nocb_try_bypass(rdp, head, &was_alldone, flags))
> +		return; // Enqueued onto ->nocb_bypass, so just leave.
> +	// If no-CBs CPU gets here, rcu_nocb_try_bypass() acquired ->nocb_lock.
> +	rcu_segcblist_enqueue(&rdp->cblist, head);
> +
>  	trace_rcu_segcb_stats(&rdp->cblist, TPS("SegCBQueued"));
>  
>  	/* Go handle any RCU core processing required. */

Two subtle changes induced here:

* rcu_segcblist_n_cbs() is now read lockless. It's just tracing so no huge deal
  but still, if this races with callbacks invocation, we may on some rare occasion
  read stale numbers on traces while enqueuing (think about rcu_top for example)

* trace_rcu_callback() will now show the number of callbacks _before_ enqueuing
  instead of _after_. Not sure if it matters, but sometimes tools rely on trace
  events.

To avoid all that, how about a new trace_rcu_nocb_bypass() instead?

Thanks.
Joel Fernandes Sept. 16, 2022, 2:10 p.m. UTC | #2
On Fri, Sep 16, 2022 at 01:09:49PM +0200, Frederic Weisbecker wrote:
> On Thu, Sep 15, 2022 at 12:14:18AM +0000, Joel Fernandes (Google) wrote:
> > If any CB is queued into the bypass list, then trace_rcu_callback() does
> > not show it. This makes it not clear when a callback was actually
> > queued, as you only end up getting a trace_rcu_invoke_callback() trace.
> > Fix it by moving trace_rcu_callback() before
> > trace_rcu_nocb_try_bypass().
> > 
> > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> > ---
> >  kernel/rcu/tree.c | 10 ++++++----
> >  1 file changed, 6 insertions(+), 4 deletions(-)
> > 
> > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > index 5ec97e3f7468..9fe581be8696 100644
> > --- a/kernel/rcu/tree.c
> > +++ b/kernel/rcu/tree.c
> > @@ -2809,10 +2809,7 @@ 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))
> > -		return; // Enqueued onto ->nocb_bypass, so just leave.
> > -	// 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,
> > @@ -2821,6 +2818,11 @@ void call_rcu(struct rcu_head *head, rcu_callback_t func)
> >  		trace_rcu_callback(rcu_state.name, head,
> >  				   rcu_segcblist_n_cbs(&rdp->cblist));
> >  
> > +	if (rcu_nocb_try_bypass(rdp, head, &was_alldone, flags))
> > +		return; // Enqueued onto ->nocb_bypass, so just leave.
> > +	// If no-CBs CPU gets here, rcu_nocb_try_bypass() acquired ->nocb_lock.
> > +	rcu_segcblist_enqueue(&rdp->cblist, head);
> > +
> >  	trace_rcu_segcb_stats(&rdp->cblist, TPS("SegCBQueued"));
> >  
> >  	/* Go handle any RCU core processing required. */
> 
> Two subtle changes induced here:
> 
> * rcu_segcblist_n_cbs() is now read lockless. It's just tracing so no huge deal
>   but still, if this races with callbacks invocation, we may on some rare occasion
>   read stale numbers on traces while enqueuing (think about rcu_top for example)
> 
> * trace_rcu_callback() will now show the number of callbacks _before_ enqueuing
>   instead of _after_. Not sure if it matters, but sometimes tools rely on trace
>   events.
> 
> To avoid all that, how about a new trace_rcu_nocb_bypass() instead?

Great points, thanks much and you rock. How about something like the
following? That way we don't need to add yet another trace point:

---8<-----------------------

From: "Joel Fernandes (Google)" <joel@joelfernandes.org>
Subject: [PATCH v2] rcu: Call trace_rcu_callback() also for bypassing

If any CB is queued into the bypass list, then trace_rcu_callback() does
not show it. This makes it not clear when a callback was actually
queued, as you only end up getting a trace_rcu_invoke_callback() trace.
Fix it by calling the tracing function even for bypass queue.

[Frederic: Hold lock while tracing]

Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
 kernel/rcu/tree.c | 29 +++++++++++++++++++++--------
 1 file changed, 21 insertions(+), 8 deletions(-)

diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index 5ec97e3f7468..85609ccbb8ed 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -2728,6 +2728,22 @@ static void check_cb_ovld(struct rcu_data *rdp)
 	raw_spin_unlock_rcu_node(rnp);
 }
 
+/*
+ * Trace RCU callback helper, call after enqueuing callback.
+ * The ->cblist must be locked when called.
+ */
+static void trace_rcu_callback_locked(struct rcu_head *head,
+				      struct rcu_data *rdp)
+{
+	if (__is_kvfree_rcu_offset((unsigned long)head->func))
+		trace_rcu_kvfree_callback(rcu_state.name, head,
+					 (unsigned long)head->func,
+					 rcu_segcblist_n_cbs(&rdp->cblist));
+	else
+		trace_rcu_callback(rcu_state.name, head,
+				   rcu_segcblist_n_cbs(&rdp->cblist));
+}
+
 /**
  * call_rcu() - Queue an RCU callback for invocation after a grace period.
  * @head: structure to be used for queueing the RCU updates.
@@ -2809,17 +2825,14 @@ 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_locked(head, rdp);
 		return; // Enqueued onto ->nocb_bypass, so just leave.
+	}
 	// 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_locked(head, rdp);
 
 	trace_rcu_segcb_stats(&rdp->cblist, TPS("SegCBQueued"));
Joel Fernandes Sept. 16, 2022, 2:14 p.m. UTC | #3
On Fri, Sep 16, 2022 at 10:10 AM Joel Fernandes <joel@joelfernandes.org> wrote:
>
> On Fri, Sep 16, 2022 at 01:09:49PM +0200, Frederic Weisbecker wrote:
> > On Thu, Sep 15, 2022 at 12:14:18AM +0000, Joel Fernandes (Google) wrote:
> > > If any CB is queued into the bypass list, then trace_rcu_callback() does
> > > not show it. This makes it not clear when a callback was actually
> > > queued, as you only end up getting a trace_rcu_invoke_callback() trace.
> > > Fix it by moving trace_rcu_callback() before
> > > trace_rcu_nocb_try_bypass().
> > >
> > > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> > > ---
> > >  kernel/rcu/tree.c | 10 ++++++----
> > >  1 file changed, 6 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > > index 5ec97e3f7468..9fe581be8696 100644
> > > --- a/kernel/rcu/tree.c
> > > +++ b/kernel/rcu/tree.c
> > > @@ -2809,10 +2809,7 @@ 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))
> > > -           return; // Enqueued onto ->nocb_bypass, so just leave.
> > > -   // 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,
> > > @@ -2821,6 +2818,11 @@ void call_rcu(struct rcu_head *head, rcu_callback_t func)
> > >             trace_rcu_callback(rcu_state.name, head,
> > >                                rcu_segcblist_n_cbs(&rdp->cblist));
> > >
> > > +   if (rcu_nocb_try_bypass(rdp, head, &was_alldone, flags))
> > > +           return; // Enqueued onto ->nocb_bypass, so just leave.
> > > +   // If no-CBs CPU gets here, rcu_nocb_try_bypass() acquired ->nocb_lock.
> > > +   rcu_segcblist_enqueue(&rdp->cblist, head);
> > > +
> > >     trace_rcu_segcb_stats(&rdp->cblist, TPS("SegCBQueued"));
> > >
> > >     /* Go handle any RCU core processing required. */
> >
> > Two subtle changes induced here:
> >
> > * rcu_segcblist_n_cbs() is now read lockless. It's just tracing so no huge deal
> >   but still, if this races with callbacks invocation, we may on some rare occasion
> >   read stale numbers on traces while enqueuing (think about rcu_top for example)
> >
> > * trace_rcu_callback() will now show the number of callbacks _before_ enqueuing
> >   instead of _after_. Not sure if it matters, but sometimes tools rely on trace
> >   events.
> >
> > To avoid all that, how about a new trace_rcu_nocb_bypass() instead?
>
> Great points, thanks much and you rock. How about something like the
> following? That way we don't need to add yet another trace point:
>
> ---8<-----------------------
>
> From: "Joel Fernandes (Google)" <joel@joelfernandes.org>
> Subject: [PATCH v2] rcu: Call trace_rcu_callback() also for bypassing
>
> If any CB is queued into the bypass list, then trace_rcu_callback() does
> not show it. This makes it not clear when a callback was actually
> queued, as you only end up getting a trace_rcu_invoke_callback() trace.
> Fix it by calling the tracing function even for bypass queue.
>
> [Frederic: Hold lock while tracing]
>
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> ---
>  kernel/rcu/tree.c | 29 +++++++++++++++++++++--------
>  1 file changed, 21 insertions(+), 8 deletions(-)
>
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index 5ec97e3f7468..85609ccbb8ed 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -2728,6 +2728,22 @@ static void check_cb_ovld(struct rcu_data *rdp)
>         raw_spin_unlock_rcu_node(rnp);
>  }
>
> +/*
> + * Trace RCU callback helper, call after enqueuing callback.
> + * The ->cblist must be locked when called.
> + */
> +static void trace_rcu_callback_locked(struct rcu_head *head,
> +                                     struct rcu_data *rdp)
> +{
> +       if (__is_kvfree_rcu_offset((unsigned long)head->func))
> +               trace_rcu_kvfree_callback(rcu_state.name, head,
> +                                        (unsigned long)head->func,
> +                                        rcu_segcblist_n_cbs(&rdp->cblist));
> +       else
> +               trace_rcu_callback(rcu_state.name, head,
> +                                  rcu_segcblist_n_cbs(&rdp->cblist));
> +}
> +
>  /**
>   * call_rcu() - Queue an RCU callback for invocation after a grace period.
>   * @head: structure to be used for queueing the RCU updates.
> @@ -2809,17 +2825,14 @@ 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_locked(head, rdp);

Never mind, this is still broken. I need to move the call site into
rcu_nocb_try_bypass() to be before where it releases the lock. I just
landed so I'll take a small break before getting back at it.

thanks,

 - Joel
Joel Fernandes Sept. 16, 2022, 10:19 p.m. UTC | #4
On 9/16/2022 7:09 AM, Frederic Weisbecker wrote:
> On Thu, Sep 15, 2022 at 12:14:18AM +0000, Joel Fernandes (Google) wrote:
>> If any CB is queued into the bypass list, then trace_rcu_callback() does
>> not show it. This makes it not clear when a callback was actually
>> queued, as you only end up getting a trace_rcu_invoke_callback() trace.
>> Fix it by moving trace_rcu_callback() before
>> trace_rcu_nocb_try_bypass().
>>
>> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
>> ---
>>  kernel/rcu/tree.c | 10 ++++++----
>>  1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
>> index 5ec97e3f7468..9fe581be8696 100644
>> --- a/kernel/rcu/tree.c
>> +++ b/kernel/rcu/tree.c
>> @@ -2809,10 +2809,7 @@ 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))
>> -		return; // Enqueued onto ->nocb_bypass, so just leave.
>> -	// 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,
>> @@ -2821,6 +2818,11 @@ void call_rcu(struct rcu_head *head, rcu_callback_t
>> func)
>>  		trace_rcu_callback(rcu_state.name, head,
>>  				   rcu_segcblist_n_cbs(&rdp->cblist));
>>  
>> +	if (rcu_nocb_try_bypass(rdp, head, &was_alldone, flags))
>> +		return; // Enqueued onto ->nocb_bypass, so just leave.
>> +	// If no-CBs CPU gets here, rcu_nocb_try_bypass() acquired
>> ->nocb_lock.
>> +	rcu_segcblist_enqueue(&rdp->cblist, head);
>> +
>>  	trace_rcu_segcb_stats(&rdp->cblist, TPS("SegCBQueued"));
>>  
>>  	/* Go handle any RCU core processing required. */
>
> Two subtle changes induced here:
>
> * rcu_segcblist_n_cbs() is now read lockless. It's just tracing so no huge
> deal
>   but still, if this races with callbacks invocation, we may on some rare
>   occasion
>   read stale numbers on traces while enqueuing (think about rcu_top for
>   example)

Actually I disagree with this point now. Changes to the number of callbacks
in the main ->cblist can be lockless. It uses atomic on CONFIG_RCU_NOCB. On
non CONFIG_RCU_NOCB, CB cannot be queued as interrupts will be disabled.

Also, in rcu_do_batch(), the count is manipulated after calling
rcu_nocb_lock_irqsave(rdp, flags);

> * trace_rcu_callback() will now show the number of callbacks _before_
> enqueuing
>   instead of _after_. Not sure if it matters, but sometimes tools rely on
>   trace
>   events.

Yeah this is fixable and good point. So how about the below?

---8<-----------------------

From: "Joel Fernandes (Google)" <joel@joelfernandes.org>
Subject: [PATCH] rcu: Call trace_rcu_callback() also for bypass queuing

If any CB is queued into the bypass list, then trace_rcu_callback() does
not show it. This makes it not clear when a callback was actually
queued, as you only end up getting a trace_rcu_invoke_callback() trace.
Fix it by calling the tracing function even for bypass queue.

Also, while at it, optimize the tracing so that rcu_state is not
accessed here if tracing is disabled, because that's useless if we are
not tracing. A quick inspection of the generated assembler shows that
rcu_state is accessed even if the jump label for the tracepoint is
disabled.

__trace_rcu_callback:
        movq    8(%rdi), %rcx
        movq    rcu_state+3640(%rip), %rax
        movq    %rdi, %rdx
        cmpq    $4095, %rcx
        ja      .L3100
        movq    192(%rsi), %r8
        1:jmp .L3101 # objtool NOPs this
        .pushsection __jump_table,  "aw"
         .balign 8
        .long 1b - .
        .long .L3101 - .
         .quad __tracepoint_rcu_kvfree_callback+8 + 2 - .
        .popsection

With this change, the jump label check which is NOOPed is moved to the
beginning of the function.

Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
 kernel/rcu/tree.c | 31 +++++++++++++++++++++++--------
 1 file changed, 23 insertions(+), 8 deletions(-)

diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index 5ec97e3f7468..b64df55f7f55 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -2728,6 +2728,23 @@ static void check_cb_ovld(struct rcu_data *rdp)
 	raw_spin_unlock_rcu_node(rnp);
 }
 
+/*
+ * Trace RCU callback helper, call after enqueuing callback.
+ * The ->cblist must be locked when called.
+ */
+void __trace_rcu_callback(struct rcu_head *head,
+				      struct rcu_data *rdp)
+{
+	if (trace_rcu_kvfree_callback_enabled() &&
+	    __is_kvfree_rcu_offset((unsigned long)head->func))
+		trace_rcu_kvfree_callback(rcu_state.name, head,
+					 (unsigned long)head->func,
+					 rcu_segcblist_n_cbs(&rdp->cblist));
+	else if (trace_rcu_callback_enabled())
+		trace_rcu_callback(rcu_state.name, head,
+				   rcu_segcblist_n_cbs(&rdp->cblist));
+}
+
 /**
  * call_rcu() - Queue an RCU callback for invocation after a grace period.
  * @head: structure to be used for queueing the RCU updates.
@@ -2809,17 +2826,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.
+	}
+
 	// 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"));
Joel Fernandes Sept. 16, 2022, 10:21 p.m. UTC | #5
On Fri, Sep 16, 2022 at 10:19:14PM +0000, Joel Fernandes wrote:
[...]
> >> +	if (rcu_nocb_try_bypass(rdp, head, &was_alldone, flags))
> >> +		return; // Enqueued onto ->nocb_bypass, so just leave.
> >> +	// If no-CBs CPU gets here, rcu_nocb_try_bypass() acquired
> >> ->nocb_lock.
> >> +	rcu_segcblist_enqueue(&rdp->cblist, head);
> >> +
> >>  	trace_rcu_segcb_stats(&rdp->cblist, TPS("SegCBQueued"));
> >>  
> >>  	/* Go handle any RCU core processing required. */
> >
> > Two subtle changes induced here:
> >
> > * rcu_segcblist_n_cbs() is now read lockless. It's just tracing so no huge
> > deal
> >   but still, if this races with callbacks invocation, we may on some rare
> >   occasion
> >   read stale numbers on traces while enqueuing (think about rcu_top for
> >   example)
> 
> Actually I disagree with this point now. Changes to the number of callbacks
> in the main ->cblist can be lockless. It uses atomic on CONFIG_RCU_NOCB. On
> non CONFIG_RCU_NOCB, CB cannot be queued as interrupts will be disabled.
> 
> Also, in rcu_do_batch(), the count is manipulated after calling
> rcu_nocb_lock_irqsave(rdp, flags);
> 
> > * trace_rcu_callback() will now show the number of callbacks _before_
> > enqueuing
> >   instead of _after_. Not sure if it matters, but sometimes tools rely on
> >   trace
> >   events.
> 
> Yeah this is fixable and good point. So how about the below?
> 
> ---8<-----------------------
> 
> From: "Joel Fernandes (Google)" <joel@joelfernandes.org>
> Subject: [PATCH] rcu: Call trace_rcu_callback() also for bypass queuing
> 
> If any CB is queued into the bypass list, then trace_rcu_callback() does
> not show it. This makes it not clear when a callback was actually
> queued, as you only end up getting a trace_rcu_invoke_callback() trace.
> Fix it by calling the tracing function even for bypass queue.
> 
> Also, while at it, optimize the tracing so that rcu_state is not
> accessed here if tracing is disabled, because that's useless if we are
> not tracing. A quick inspection of the generated assembler shows that
> rcu_state is accessed even if the jump label for the tracepoint is
> disabled.
> 
> __trace_rcu_callback:
>         movq    8(%rdi), %rcx
>         movq    rcu_state+3640(%rip), %rax
>         movq    %rdi, %rdx
>         cmpq    $4095, %rcx
>         ja      .L3100
>         movq    192(%rsi), %r8
>         1:jmp .L3101 # objtool NOPs this
>         .pushsection __jump_table,  "aw"
>          .balign 8
>         .long 1b - .
>         .long .L3101 - .
>          .quad __tracepoint_rcu_kvfree_callback+8 + 2 - .
>         .popsection
> 
> With this change, the jump label check which is NOOPed is moved to the
> beginning of the function.
> 
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> ---
>  kernel/rcu/tree.c | 31 +++++++++++++++++++++++--------
>  1 file changed, 23 insertions(+), 8 deletions(-)
> 
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index 5ec97e3f7468..b64df55f7f55 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -2728,6 +2728,23 @@ static void check_cb_ovld(struct rcu_data *rdp)
>  	raw_spin_unlock_rcu_node(rnp);
>  }
>  
> +/*
> + * Trace RCU callback helper, call after enqueuing callback.
> + * The ->cblist must be locked when called.

Also sorry for the spam, this comment is stale now so will delete this line.
diff mbox series

Patch

diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index 5ec97e3f7468..9fe581be8696 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -2809,10 +2809,7 @@  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))
-		return; // Enqueued onto ->nocb_bypass, so just leave.
-	// 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,
@@ -2821,6 +2818,11 @@  void call_rcu(struct rcu_head *head, rcu_callback_t func)
 		trace_rcu_callback(rcu_state.name, head,
 				   rcu_segcblist_n_cbs(&rdp->cblist));
 
+	if (rcu_nocb_try_bypass(rdp, head, &was_alldone, flags))
+		return; // Enqueued onto ->nocb_bypass, so just leave.
+	// If no-CBs CPU gets here, rcu_nocb_try_bypass() acquired ->nocb_lock.
+	rcu_segcblist_enqueue(&rdp->cblist, head);
+
 	trace_rcu_segcb_stats(&rdp->cblist, TPS("SegCBQueued"));
 
 	/* Go handle any RCU core processing required. */