Re: [PATCH rcu 03/12] srcu: Renaming in preparation for additional reader flavor

From: Neeraj Upadhyay
Date: Mon Oct 14 2024 - 23:26:10 EST

Next message: sergeh: "Re: [PATCH v20 2/6] security: Add EXEC_RESTRICT_FILE and EXEC_DENY_INTERACTIVE securebits"
Previous message: Zhang, Rui: "Re: [RFC PATCH] x86/acpi: Ignore invalid x2APIC entries"
In reply to: Paul E. McKenney: "Re: [PATCH rcu 03/12] srcu: Renaming in preparation for additional reader flavor"
Next in thread: Paul E. McKenney: "[PATCH rcu 05/12] srcu: Standardize srcu_data pointers to "sdp" and similar"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 10/14/2024 10:22 PM, Paul E. McKenney wrote:
> On Mon, Oct 14, 2024 at 02:40:35PM +0530, Neeraj Upadhyay wrote:
>> On 10/9/2024 11:37 PM, Paul E. McKenney wrote:
>>> Currently, there are only two flavors of readers, normal and NMI-safe.
>>> A number of fields, functions, and types reflect this restriction.
>>> This renaming-only commit prepares for the addition of light-weight
>>> (as in memory-barrier-free) readers. OK, OK, there is also a drive-by
>>> white-space fixeup!
>>>
>>> Signed-off-by: Paul E. McKenney <paulmck@xxxxxxxxxx>
>>> Cc: Alexei Starovoitov <ast@xxxxxxxxxx>
>>> Cc: Andrii Nakryiko <andrii@xxxxxxxxxx>
>>> Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
>>> Cc: Kent Overstreet <kent.overstreet@xxxxxxxxx>
>>> Cc: <bpf@xxxxxxxxxxxxxxx>
>>> ---
>>> include/linux/srcu.h | 21 ++++++++++-----------
>>> include/linux/srcutree.h | 2 +-
>>> kernel/rcu/srcutree.c | 22 +++++++++++-----------
>>> 3 files changed, 22 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/include/linux/srcu.h b/include/linux/srcu.h
>>> index 835bbb2d1f88a..06728ef6f32a4 100644
>>> --- a/include/linux/srcu.h
>>> +++ b/include/linux/srcu.h
>>> @@ -181,10 +181,9 @@ static inline int srcu_read_lock_held(const struct srcu_struct *ssp)
>>> #define SRCU_NMI_SAFE 0x2
>>>
>>> #if defined(CONFIG_PROVE_RCU) && defined(CONFIG_TREE_SRCU)
>>> -void srcu_check_nmi_safety(struct srcu_struct *ssp, bool nmi_safe);
>>> +void srcu_check_read_flavor(struct srcu_struct *ssp, int read_flavor);
>>> #else
>>> -static inline void srcu_check_nmi_safety(struct srcu_struct *ssp,
>>> - bool nmi_safe) { }
>>> +static inline void srcu_check_read_flavor(struct srcu_struct *ssp, int read_flavor) { }
>>> #endif
>>>
>>>
>>> @@ -245,7 +244,7 @@ static inline int srcu_read_lock(struct srcu_struct *ssp) __acquires(ssp)
>>> {
>>> int retval;
>>>
>>> - srcu_check_nmi_safety(ssp, false);
>>> + srcu_check_read_flavor(ssp, false);
>>
>> As srcu_check_read_flavor() takes an "int" now, passing a macro for the type of reader would
>> be better here?
>
> Agreed, and a later commit does introduce macros.
>
>>> retval = __srcu_read_lock(ssp);
>>> srcu_lock_acquire(&ssp->dep_map);
>>> return retval;
>>> @@ -262,7 +261,7 @@ static inline int srcu_read_lock_nmisafe(struct srcu_struct *ssp) __acquires(ssp
>>> {
>>> int retval;
>>>
>>> - srcu_check_nmi_safety(ssp, true);
>>> + srcu_check_read_flavor(ssp, true);
>>> retval = __srcu_read_lock_nmisafe(ssp);
>>> rcu_try_lock_acquire(&ssp->dep_map);
>>> return retval;
>>> @@ -274,7 +273,7 @@ srcu_read_lock_notrace(struct srcu_struct *ssp) __acquires(ssp)
>>> {
>>> int retval;
>>>
>>> - srcu_check_nmi_safety(ssp, false);
>>> + srcu_check_read_flavor(ssp, false);
>>> retval = __srcu_read_lock(ssp);
>>> return retval;
>>> }
>>> @@ -303,7 +302,7 @@ srcu_read_lock_notrace(struct srcu_struct *ssp) __acquires(ssp)
>>> static inline int srcu_down_read(struct srcu_struct *ssp) __acquires(ssp)
>>> {
>>> WARN_ON_ONCE(in_nmi());
>>> - srcu_check_nmi_safety(ssp, false);
>>> + srcu_check_read_flavor(ssp, false);
>>> return __srcu_read_lock(ssp);
>>> }
>>>
>>> @@ -318,7 +317,7 @@ static inline void srcu_read_unlock(struct srcu_struct *ssp, int idx)
>>> __releases(ssp)
>>> {
>>> WARN_ON_ONCE(idx & ~0x1);
>>> - srcu_check_nmi_safety(ssp, false);
>>> + srcu_check_read_flavor(ssp, false);
>>> srcu_lock_release(&ssp->dep_map);
>>> __srcu_read_unlock(ssp, idx);
>>> }
>>> @@ -334,7 +333,7 @@ static inline void srcu_read_unlock_nmisafe(struct srcu_struct *ssp, int idx)
>>> __releases(ssp)
>>> {
>>> WARN_ON_ONCE(idx & ~0x1);
>>> - srcu_check_nmi_safety(ssp, true);
>>> + srcu_check_read_flavor(ssp, true);
>>> rcu_lock_release(&ssp->dep_map);
>>> __srcu_read_unlock_nmisafe(ssp, idx);
>>> }
>>> @@ -343,7 +342,7 @@ static inline void srcu_read_unlock_nmisafe(struct srcu_struct *ssp, int idx)
>>> static inline notrace void
>>> srcu_read_unlock_notrace(struct srcu_struct *ssp, int idx) __releases(ssp)
>>> {
>>> - srcu_check_nmi_safety(ssp, false);
>>> + srcu_check_read_flavor(ssp, false);
>>> __srcu_read_unlock(ssp, idx);
>>> }
>>>
>>> @@ -360,7 +359,7 @@ static inline void srcu_up_read(struct srcu_struct *ssp, int idx)
>>> {
>>> WARN_ON_ONCE(idx & ~0x1);
>>> WARN_ON_ONCE(in_nmi());
>>> - srcu_check_nmi_safety(ssp, false);
>>> + srcu_check_read_flavor(ssp, false);
>>> __srcu_read_unlock(ssp, idx);
>>> }
>>>
>>> diff --git a/include/linux/srcutree.h b/include/linux/srcutree.h
>>> index ed57598394de3..ab7d8d215b84b 100644
>>> --- a/include/linux/srcutree.h
>>> +++ b/include/linux/srcutree.h
>>> @@ -25,7 +25,7 @@ struct srcu_data {
>>> /* Read-side state. */
>>> atomic_long_t srcu_lock_count[2]; /* Locks per CPU. */
>>> atomic_long_t srcu_unlock_count[2]; /* Unlocks per CPU. */
>>> - int srcu_nmi_safety; /* NMI-safe srcu_struct structure? */
>>> + int srcu_reader_flavor; /* Reader flavor for srcu_struct structure? */
>>
>> This is a mask for the reader flavor, so s/srcu_reader_flavor/srcu_reader_flavor_mask ?
>
> Yes, it is a mask, but one that should only ever have a single bit set.
> So calling it a mask might or might not be a service to the reader.
>

Ok. The usage of reader_flavor as a shifted val here and without
shift as arg of srcu_check_read_flavor() was a bit confusing.

- Neeraj

...

>>>
>>> #ifdef CONFIG_PROVE_RCU
>>> /*
>>> - * Check for consistent NMI safety.
>>> + * Check for consistent reader flavor.
>>> */
>>> -void srcu_check_nmi_safety(struct srcu_struct *ssp, bool nmi_safe)
>>> +void srcu_check_read_flavor(struct srcu_struct *ssp, int read_flavor)
>>> {
>>> - int nmi_safe_mask = 1 << nmi_safe;
>>> - int old_nmi_safe_mask;
>>> + int reader_flavor_mask = 1 << read_flavor;
>>> + int old_reader_flavor_mask;
>>> struct srcu_data *sdp;
>>>
>>> /* NMI-unsafe use in NMI is a bad sign */
>>> - WARN_ON_ONCE(!nmi_safe && in_nmi());
>>> + WARN_ON_ONCE(!read_flavor && in_nmi());
>>> sdp = raw_cpu_ptr(ssp->sda);
>>> - old_nmi_safe_mask = READ_ONCE(sdp->srcu_nmi_safety);
>>> - if (!old_nmi_safe_mask) {
>>> - WRITE_ONCE(sdp->srcu_nmi_safety, nmi_safe_mask);
>>> + old_reader_flavor_mask = READ_ONCE(sdp->srcu_reader_flavor);
>>> + if (!old_reader_flavor_mask) {
>>> + WRITE_ONCE(sdp->srcu_reader_flavor, reader_flavor_mask);
>>> return;
>>> }
>>> - WARN_ONCE(old_nmi_safe_mask != nmi_safe_mask, "CPU %d old state %d new state %d\n", sdp->cpu, old_nmi_safe_mask, nmi_safe_mask);
>>> + WARN_ONCE(old_reader_flavor_mask != reader_flavor_mask, "CPU %d old state %d new state %d\n", sdp->cpu, old_reader_flavor_mask, reader_flavor_mask);
>>> }
>>> -EXPORT_SYMBOL_GPL(srcu_check_nmi_safety);
>>> +EXPORT_SYMBOL_GPL(srcu_check_read_flavor);
>>> #endif /* CONFIG_PROVE_RCU */
>>>
>>> /*
>>

Next message: sergeh: "Re: [PATCH v20 2/6] security: Add EXEC_RESTRICT_FILE and EXEC_DENY_INTERACTIVE securebits"
Previous message: Zhang, Rui: "Re: [RFC PATCH] x86/acpi: Ignore invalid x2APIC entries"
In reply to: Paul E. McKenney: "Re: [PATCH rcu 03/12] srcu: Renaming in preparation for additional reader flavor"
Next in thread: Paul E. McKenney: "[PATCH rcu 05/12] srcu: Standardize srcu_data pointers to "sdp" and similar"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]