On Thu, 04 Jun 2026 11:40:59 -0700 "Paul E. McKenney" <[email protected]> wrote:
> On Fri, May 29, 2026 at 04:39:51PM +0300, Onur Özkan wrote: > > Add helper wrappers for SRCU functions that are exposed to Rust > > through generated bindings. > > > > Signed-off-by: Onur Özkan <[email protected]> > > > > --- > > include/linux/srcu.h | 29 ++++++++++++++++++++--------- > > kernel/rcu/srcutiny.c | 10 +++++----- > > kernel/rcu/srcutree.c | 9 +++++---- > > rust/helpers/helpers.c | 1 + > > rust/helpers/srcu.c | 30 ++++++++++++++++++++++++++++++ > > 5 files changed, 61 insertions(+), 18 deletions(-) > > create mode 100644 rust/helpers/srcu.c > > On the next version, please split the rust/helpers changes into a > separate patch. It looks like the simplification to SRCU adds code > rather than deleting it? > Well, it's mostly renaming (e.g. init_srcu_struct -> init_srcu_struct_generic) and moving code around (moving init_srcu_struct outside CONFIG_DEBUG_LOCK_ALLOC guard). The actual "added" code lines are around 10 lines or something. > But let me see if I understand the structure. > > > diff --git a/include/linux/srcu.h b/include/linux/srcu.h > > index 81b1938512d5..a028a5b5ebef 100644 > > --- a/include/linux/srcu.h > > +++ b/include/linux/srcu.h > > @@ -25,20 +25,19 @@ context_lock_struct(srcu_struct, __reentrant_ctx_lock); > > > > #ifdef CONFIG_DEBUG_LOCK_ALLOC > > > > -int __init_srcu_struct(struct srcu_struct *ssp, const char *name, struct > > lock_class_key *key); > > +int init_srcu_struct_lockdep(struct srcu_struct *ssp, const char *name, > > + struct lock_class_key *key); > > +static inline int __init_srcu_struct(struct srcu_struct *ssp, const char > > *name, > > + struct lock_class_key *key) > > +{ > > + return init_srcu_struct_lockdep(ssp, name, key); > > +} > > __init_srcu_struct() invokes init_srcu_struct_lockdep()... > > > #ifndef CONFIG_TINY_SRCU > > int __init_srcu_struct_fast(struct srcu_struct *ssp, const char *name, > > struct lock_class_key *key); > > int __init_srcu_struct_fast_updown(struct srcu_struct *ssp, const char > > *name, > > struct lock_class_key *key); > > #endif // #ifndef CONFIG_TINY_SRCU > > > > -#define init_srcu_struct(ssp) \ > > -({ \ > > - static struct lock_class_key __srcu_key; \ > > - \ > > - __init_srcu_struct((ssp), #ssp, &__srcu_key); \ > > -}) > > init_srcu_struct() invokes __init_srcu_struct()... (moved) > > > #define init_srcu_struct_fast(ssp) \ > > ({ \ > > static struct lock_class_key __srcu_key; \ > > @@ -56,7 +55,12 @@ int __init_srcu_struct_fast_updown(struct srcu_struct > > *ssp, const char *name, > > #define __SRCU_DEP_MAP_INIT(srcu_name) .dep_map = { .name = #srcu_name > > }, > > #else /* #ifdef CONFIG_DEBUG_LOCK_ALLOC */ > > > > -int init_srcu_struct(struct srcu_struct *ssp); > > +int init_srcu_struct_generic(struct srcu_struct *ssp); > > +static inline int __init_srcu_struct(struct srcu_struct *ssp, const char > > *name, > > + struct lock_class_key *key) > > +{ > > + return init_srcu_struct_generic(ssp); > > +} > > __init_srcu_struct() invokes init_srcu_struct_generic()... > > > #ifndef CONFIG_TINY_SRCU > > int init_srcu_struct_fast(struct srcu_struct *ssp); > > int init_srcu_struct_fast_updown(struct srcu_struct *ssp); > > @@ -65,6 +69,13 @@ int init_srcu_struct_fast_updown(struct srcu_struct > > *ssp); > > #define __SRCU_DEP_MAP_INIT(srcu_name) > > #endif /* #else #ifdef CONFIG_DEBUG_LOCK_ALLOC */ > > > > +#define init_srcu_struct(ssp) \ > > +({ \ > > + static struct lock_class_key __srcu_key; \ > > + \ > > + __init_srcu_struct((ssp), #ssp, &__srcu_key); \ > > +}) > > ...and here is where it moved to. > > > + > > /* Values for SRCU Tree srcu_data ->srcu_reader_flavor, but also used by > > rcutorture. */ > > #define SRCU_READ_FLAVOR_NORMAL 0x1 // > > srcu_read_lock(). > > #define SRCU_READ_FLAVOR_NMI 0x2 // > > srcu_read_lock_nmisafe(). > > diff --git a/kernel/rcu/srcutiny.c b/kernel/rcu/srcutiny.c > > index a2e2d516e51b..780a95e8cad7 100644 > > --- a/kernel/rcu/srcutiny.c > > +++ b/kernel/rcu/srcutiny.c > > @@ -48,15 +48,15 @@ static int init_srcu_struct_fields(struct srcu_struct > > *ssp) > > > > #ifdef CONFIG_DEBUG_LOCK_ALLOC > > > > -int __init_srcu_struct(struct srcu_struct *ssp, const char *name, > > - struct lock_class_key *key) > > +int init_srcu_struct_lockdep(struct srcu_struct *ssp, const char *name, > > + struct lock_class_key *key) > > { > > /* Don't re-initialize a lock while it is held. */ > > debug_check_no_locks_freed((void *)ssp, sizeof(*ssp)); > > lockdep_init_map(&ssp->dep_map, name, key, 0); > > return init_srcu_struct_fields(ssp); > > } > > -EXPORT_SYMBOL_GPL(__init_srcu_struct); > > +EXPORT_SYMBOL_GPL(init_srcu_struct_lockdep); > > init_srcu_struct_lockdep() invokes init_srcu_struct_fields()... > > > #else /* #ifdef CONFIG_DEBUG_LOCK_ALLOC */ > > > > @@ -68,11 +68,11 @@ EXPORT_SYMBOL_GPL(__init_srcu_struct); > > * to any other function. Each srcu_struct represents a separate domain > > * of SRCU protection. > > */ > > -int init_srcu_struct(struct srcu_struct *ssp) > > +int init_srcu_struct_generic(struct srcu_struct *ssp) > > { > > return init_srcu_struct_fields(ssp); > > } > > -EXPORT_SYMBOL_GPL(init_srcu_struct); > > +EXPORT_SYMBOL_GPL(init_srcu_struct_generic); > > init_srcu_struct_generic() invokes init_srcu_struct_fields()... > > > #endif /* #else #ifdef CONFIG_DEBUG_LOCK_ALLOC */ > > > > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c > > index 0d01cd8c4b4a..45154630c54e 100644 > > --- a/kernel/rcu/srcutree.c > > +++ b/kernel/rcu/srcutree.c > > @@ -266,12 +266,13 @@ __init_srcu_struct_common(struct srcu_struct *ssp, > > const char *name, struct lock > > return init_srcu_struct_fields(ssp, false); > > } > > > > -int __init_srcu_struct(struct srcu_struct *ssp, const char *name, struct > > lock_class_key *key) > > +int init_srcu_struct_lockdep(struct srcu_struct *ssp, const char *name, > > + struct lock_class_key *key) > > { > > ssp->srcu_reader_flavor = 0; > > return __init_srcu_struct_common(ssp, name, key); > > } > > -EXPORT_SYMBOL_GPL(__init_srcu_struct); > > +EXPORT_SYMBOL_GPL(init_srcu_struct_lockdep); > > init_srcu_struct_lockdep() invokes __init_srcu_struct_common()... > > > int __init_srcu_struct_fast(struct srcu_struct *ssp, const char *name, > > struct lock_class_key *key) > > { > > @@ -301,12 +302,12 @@ EXPORT_SYMBOL_GPL(__init_srcu_struct_fast_updown); > > * to any other function. Each srcu_struct represents a separate domain > > * of SRCU protection. > > */ > > -int init_srcu_struct(struct srcu_struct *ssp) > > +int init_srcu_struct_generic(struct srcu_struct *ssp) > > { > > ssp->srcu_reader_flavor = 0; > > return init_srcu_struct_fields(ssp, false); > > } > > -EXPORT_SYMBOL_GPL(init_srcu_struct); > > +EXPORT_SYMBOL_GPL(init_srcu_struct_generic); > > init_srcu_struct_generic() invokes init_srcu_struct_fields()... > > This gets me the structure shown in the attached PDF. Did I get it > right? > Yes, it looks right. > I am surprised not to see any changes involving cleanup_srcu_struct(). > What did I miss? > Do you mean changing cleanup_srcu_struct() on the C side? As I mentioned previously [1], I would prefer to keep the current approach for now and see what others think. If we receive complaints from other people, we can then consider proposing an alternative approach which would likely involve changing (or simply adding) C code. Personally, I prefer the current approach because it doesn't add any complexity. The problem is already documented and it's highly unlikely that we will ever hit it in practice. It's fairly easy to catch those kind of mistakes (leaking the guard explicitly with mem::forget) on review. Even if we add a dedicated cleanup function on the C side, leaking an SRCU guard should still not be allowed and should be treated as a "bad code" and should require a follow-up patch. Adding a special C function that changes counters just switches "don't leak an SRCU guard" problem into "don't call $the_special_cleanup function anywhere else". [1]: https://lore.kernel.org/all/[email protected] > And again, please put the following in a later patch to allow separate > evaluation of the restructuring. > Sure thing. I will split them in the next version. Thanks, Onur > Thanx, Paul > > > /** > > * init_srcu_struct_fast - initialize a fast-reader sleep-RCU structure > > diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c > > index 625921e27dfb..f3562d3b3888 100644 > > --- a/rust/helpers/helpers.c > > +++ b/rust/helpers/helpers.c > > @@ -88,6 +88,7 @@ > > #include "signal.c" > > #include "slab.c" > > #include "spinlock.c" > > +#include "srcu.c" > > #include "sync.c" > > #include "task.c" > > #include "time.c" > > diff --git a/rust/helpers/srcu.c b/rust/helpers/srcu.c > > new file mode 100644 > > index 000000000000..225b3bf9334a > > --- /dev/null > > +++ b/rust/helpers/srcu.c > > @@ -0,0 +1,30 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > + > > +#include <linux/srcu.h> > > + > > +__rust_helper int rust_helper_init_srcu_struct_with_key(struct srcu_struct > > *ssp, > > + const char *name, > > + struct lock_class_key > > *key) > > +{ > > + return __init_srcu_struct(ssp, name, key); > > +} > > + > > +__rust_helper int rust_helper_srcu_read_lock(struct srcu_struct *ssp) > > +{ > > + return srcu_read_lock(ssp); > > +} > > + > > +__rust_helper void rust_helper_srcu_read_unlock(struct srcu_struct *ssp, > > int idx) > > +{ > > + srcu_read_unlock(ssp, idx); > > +} > > + > > +__rust_helper void rust_helper_srcu_barrier(struct srcu_struct *ssp) > > +{ > > + srcu_barrier(ssp); > > +} > > + > > +__rust_helper void rust_helper_synchronize_srcu_expedited(struct > > srcu_struct *ssp) > > +{ > > + synchronize_srcu_expedited(ssp); > > +} > > -- > > 2.51.2 > >
