| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <36076d14-6732-4bbc-b96e-9bab1212c9dd@paulmck-laptop>
Date: Mon, 14 Oct 2024 09:52:43 -0700
From: "Paul E. McKenney" <paulmck@...nel.org>
To: Neeraj Upadhyay <Neeraj.Upadhyay@....com>
Cc: frederic@...nel.org, rcu@...r.kernel.org, linux-kernel@...r.kernel.org,
kernel-team@...a.com, rostedt@...dmis.org,
Alexei Starovoitov <ast@...nel.org>,
Andrii Nakryiko <andrii@...nel.org>,
Peter Zijlstra <peterz@...radead.org>,
Kent Overstreet <kent.overstreet@...ux.dev>, bpf@...r.kernel.org
Subject: Re: [PATCH rcu 03/12] srcu: Renaming in preparation for additional
reader flavor
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@...nel.org>
> > Cc: Alexei Starovoitov <ast@...nel.org>
> > Cc: Andrii Nakryiko <andrii@...nel.org>
> > Cc: Peter Zijlstra <peterz@...radead.org>
> > Cc: Kent Overstreet <kent.overstreet@...ux.dev>
> > Cc: <bpf@...r.kernel.org>
> > ---
> > 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.
Thanx, Paul
> - Neeraj
>
> >
> > /* Update-side state. */
> > spinlock_t __private lock ____cacheline_internodealigned_in_smp;
> > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> > index e29c6cbffbcb0..18f2eae5e14bd 100644
> > --- a/kernel/rcu/srcutree.c
> > +++ b/kernel/rcu/srcutree.c
> > @@ -460,7 +460,7 @@ static unsigned long srcu_readers_unlock_idx(struct srcu_struct *ssp, int idx)
> >
> > sum += atomic_long_read(&cpuc->srcu_unlock_count[idx]);
> > if (IS_ENABLED(CONFIG_PROVE_RCU))
> > - mask = mask | READ_ONCE(cpuc->srcu_nmi_safety);
> > + mask = mask | READ_ONCE(cpuc->srcu_reader_flavor);
> > }
> > WARN_ONCE(IS_ENABLED(CONFIG_PROVE_RCU) && (mask & (mask >> 1)),
> > "Mixed NMI-safe readers for srcu_struct at %ps.\n", ssp);
> > @@ -699,25 +699,25 @@ EXPORT_SYMBOL_GPL(cleanup_srcu_struct);
> >
> > #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 */
> >
> > /*
>
Powered by blists - more mailing lists