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

[Joel Fernandes <joel@joelfernandes.org>]

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"));
 
-- 
2.37.3.968.ga6b4b080e4-goog