Re: [PATCH RFC v3 02/13] rcu-tasks-trace: Inline rcu_read_lock_trace() and annotate inside the reader

[bot+bpf-ci@kernel.org]

[-- Attachment #1: Type: text/plain, Size: 3253 bytes --]

> diff --git a/include/linux/rcupdate_trace.h b/include/linux/rcupdate_trace.h
> index 273c59a032512..4035054309d7c 100644
> --- a/include/linux/rcupdate_trace.h
> +++ b/include/linux/rcupdate_trace.h
> @@ -93,22 +93,20 @@ static inline void rcu_read_unlock_tasks_trace(struct srcu_ctr __percpu *scp)
>   *
>   * For more details, please see the documentation for rcu_read_lock().
>   */
> -static inline void rcu_read_lock_trace(void)
> +static __always_inline void rcu_read_lock_trace(void)
>  {
>  	int n;
>  	struct task_struct *t = current;
>
> -	rcu_try_lock_acquire(&rcu_tasks_trace_srcu_struct.dep_map);
>  	n = READ_ONCE(t->trc_reader_nesting);
>  	WRITE_ONCE(t->trc_reader_nesting, n + 1);
> -	if (n) {
> -		// In case we interrupted a Tasks Trace RCU reader.
> -		return;
> -	}
> -	barrier();  // nesting before scp to protect against interrupt handler.
> -	t->trc_reader_scp = __srcu_read_lock_fast(&rcu_tasks_trace_srcu_struct);
> -	if (!IS_ENABLED(CONFIG_TASKS_TRACE_RCU_NO_MB))
> -		smp_mb(); // Placeholder for more selective ordering
> +	if (!n) {
> +		barrier();  // nesting before scp to protect against interrupt handler.
> +		t->trc_reader_scp = __srcu_read_lock_fast(&rcu_tasks_trace_srcu_struct);
> +		if (!IS_ENABLED(CONFIG_TASKS_TRACE_RCU_NO_MB))
> +			smp_mb(); // Placeholder for more selective ordering
> +	} // Else we interrupted a Tasks Trace RCU reader.
> +	rcu_try_lock_acquire(&rcu_tasks_trace_srcu_struct.dep_map);
>  }
>
>  /**
> @@ -120,12 +118,13 @@ static inline void rcu_read_lock_trace(void)
>   *
>   * For more details, please see the documentation for rcu_read_unlock().
>   */
> -static inline void rcu_read_unlock_trace(void)
> +static __always_inline void rcu_read_unlock_trace(void)
>  {
>  	int n;
>  	struct srcu_ctr __percpu *scp;
>  	struct task_struct *t = current;
>
> +	srcu_lock_release(&rcu_tasks_trace_srcu_struct.dep_map);
>  	n = READ_ONCE(t->trc_reader_nesting) - 1;
>  	if (n) {
>  		WRITE_ONCE(t->trc_reader_nesting, n);
> @@ -137,7 +136,6 @@ static inline void rcu_read_unlock_trace(void)
>  			smp_mb(); // Placeholder for more selective ordering
>  		__srcu_read_unlock_fast(&rcu_tasks_trace_srcu_struct, scp);
>  	}
> -	srcu_lock_release(&rcu_tasks_trace_srcu_struct.dep_map);
>  }

This isn't a bug, but the changelog notes that rcu_read_lock_tasks_trace()
already annotates inside the reader "on the lock side" and then says "Make
the trace variants match."

rcu_read_unlock_tasks_trace(), four functions above the changed code in the
same header, still calls srcu_lock_release() after __srcu_read_unlock_fast(),
i.e. the exact ordering being corrected in rcu_read_unlock_trace().

After this patch the two unlock helpers in the file disagree. This has no
functional effect (srcu_lock_release() is a lockdep annotation and compiles
away without CONFIG_DEBUG_LOCK_ALLOC), but would it be worth either flipping
rcu_read_unlock_tasks_trace() the same way, or saying in the changelog why
only the rcu_read_{,un}lock_trace() pair is being changed?


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/34975311706