Re: [PATCH v3 2/4] srcu: Add srcu_read_lock_fast_notrace() and srcu_read_unlock_fast_notrace()

[Mathieu Desnoyers <mathieu.desnoyers@efficios.com>]

On 2025-07-22 18:34, Paul E. McKenney wrote:
> On Tue, Jul 22, 2025 at 06:11:00PM -0400, Joel Fernandes wrote:
>> On Mon, Jul 21, 2025 at 09:24:31AM -0700, Paul E. McKenney wrote:
>>> This commit adds no-trace variants of the srcu_read_lock_fast() and
>>> srcu_read_unlock_fast() functions for tracing use.
>>>
>>> Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
>>> Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
>>> Cc: Steven Rostedt <rostedt@goodmis.org>
>>> Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
>>> ---
>>>   include/linux/srcu.h | 25 +++++++++++++++++++++++++
>>>   1 file changed, 25 insertions(+)
>>>
>>> diff --git a/include/linux/srcu.h b/include/linux/srcu.h
>>> index 478c73d067f7d..7a692bf8f99b9 100644
>>> --- a/include/linux/srcu.h
>>> +++ b/include/linux/srcu.h
>>> @@ -282,6 +282,20 @@ static inline struct srcu_ctr __percpu *srcu_read_lock_fast(struct srcu_struct *
>>>   	return retval;
>>>   }
>>>   
>>> +/*
>>> + * Used by tracing, cannot be traced and cannot call lockdep.
>>> + * See srcu_read_lock_fast() for more information.
>>> + */
>>> +static inline struct srcu_ctr __percpu *srcu_read_lock_fast_notrace(struct srcu_struct *ssp)
>>> +	__acquires(ssp)
>>
>> Should these also be marked with 'notrace' attribute?
>>
>> I am not sure what the precedent is, I do see a few examples of 'notrace' and
>> 'inline' in the same function signature though.
> 
> Heh!!!
> 
> There are six instance of static-inline notrace functions, and eight
> instances of static-inline non-notrace functions whose names contain
> "_notrace", not counting the srcu_read_lock_fast_notrace() and
> srcu_read_unlock_fast() functions currently under review.
> 
> My guess is that I should add "notrace" to handle the possible case
> where the compiler declines to inline this function.  I will do this
> on the next rebase unless I hear otherwise.
> 
> Steven, Mathieu, thoughts?

AFAIR, the always_inline gcc attribute takes care of making sure the
notrace is not needed in addition.

So I suspect that kernel APIs need to abide by the following rules
to be usable by instrumentation code:

- if it's a function call, have a notrace attribute.
- if it's an inline (which the compiler may decide to implement as a
   function call), have a notrace attribute.
- if it's an __always_inline, then there is no way the compiler can
   possibly make it a function call, so the notrace would be useless
   there.

So you may want to choose for either:

- inline notrace, or
- __always_inline

Depending on how much freedom you want to grant the compiler with
respect to its inlining practices.

Thanks,

Mathieu


> 
> 							Thanx, Paul
> 
>> Other than that one nit:
>> Reviewed-by: Joel Fernandes <joelagnelf@nvidia.com>
>>
>> thanks,
>>
>>   - Joel
>>
>>
>>> +{
>>> +	struct srcu_ctr __percpu *retval;
>>> +
>>> +	srcu_check_read_flavor_force(ssp, SRCU_READ_FLAVOR_FAST);
>>> +	retval = __srcu_read_lock_fast(ssp);
>>> +	return retval;
>>> +}
>>> +
>>>   /**
>>>    * srcu_down_read_fast - register a new reader for an SRCU-protected structure.
>>>    * @ssp: srcu_struct in which to register the new reader.
>>> @@ -394,6 +408,17 @@ static inline void srcu_read_unlock_fast(struct srcu_struct *ssp, struct srcu_ct
>>>   	RCU_LOCKDEP_WARN(!rcu_is_watching(), "RCU must be watching srcu_read_unlock_fast().");
>>>   }
>>>   
>>> +/*
>>> + * Used by tracing, cannot be traced and cannot call lockdep.
>>> + * See srcu_read_unlock_fast() for more information.
>>> + */
>>> +static inline void srcu_read_unlock_fast_notrace(struct srcu_struct *ssp,
>>> +						 struct srcu_ctr __percpu *scp) __releases(ssp)
>>> +{
>>> +	srcu_check_read_flavor(ssp, SRCU_READ_FLAVOR_FAST);
>>> +	__srcu_read_unlock_fast(ssp, scp);
>>> +}
>>> +
>>>   /**
>>>    * srcu_up_read_fast - unregister a old reader from an SRCU-protected structure.
>>>    * @ssp: srcu_struct in which to unregister the old reader.
>>> -- 
>>> 2.40.1
>>>


-- 
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com