Re: [PATCH RFC rcu 02/19] srcu: Use static init for statically allocated in-module srcu_struct

["Paul E. McKenney" <paulmck@kernel.org>]

On Thu, Mar 30, 2023 at 03:20:15PM +0000, Zhang, Qiang1 wrote:
> > >Further shrinking the srcu_struct structure is eased by requiring 
> > >that in-module srcu_struct structures rely more heavily on static 
> > >initialization.  In particular, this preserves the property that a 
> > >module-load-time srcu_struct initialization can fail only due to 
> > >memory-allocation failure of the per-CPU srcu_data structures.
> > >It might also slightly improve robustness by keeping the number of 
> > >memory allocations that must succeed down percpu_alloc() call.
> > >
> > >This is in preparation for splitting an srcu_usage structure out of 
> > >the srcu_struct structure.
> > >
> > >[ paulmck: Fold in qiang1.zhang@intel.com feedback. ]
> > >
> > >Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> > >Cc: Christoph Hellwig <hch@lst.de>
> > >---
> > > include/linux/srcutree.h | 19 ++++++++++++++-----
> > > kernel/rcu/srcutree.c    | 19 +++++++++++++------
> > > 2 files changed, 27 insertions(+), 11 deletions(-)
> > >
> > >diff --git a/include/linux/srcutree.h b/include/linux/srcutree.h 
> > >index ac8af12f93b3..428480152375 100644
> > >--- a/include/linux/srcutree.h
> > >+++ b/include/linux/srcutree.h
> > >@@ -121,15 +121,24 @@ struct srcu_struct {
> > > #define SRCU_STATE_SCAN1	1
> > > #define SRCU_STATE_SCAN2	2
> > > 
> > >-#define __SRCU_STRUCT_INIT(name, pcpu_name)							\
> > >-{												\
> > >-	.sda = &pcpu_name,									\
> > >+#define __SRCU_STRUCT_INIT_COMMON(name)								\
> > > 	.lock = __SPIN_LOCK_UNLOCKED(name.lock),						\
> > > 	.srcu_gp_seq_needed = -1UL,								\
> > > 	.work = __DELAYED_WORK_INITIALIZER(name.work, NULL, 0),					\
> > >-	__SRCU_DEP_MAP_INIT(name)								\
> > >+	__SRCU_DEP_MAP_INIT(name)
> > >+
> > >+#define __SRCU_STRUCT_INIT_MODULE(name)								\
> > >+{												\
> > >+	__SRCU_STRUCT_INIT_COMMON(name)								\
> > > }
> > > 
> > >+#define __SRCU_STRUCT_INIT(name, pcpu_name)							\
> > >+{												\
> > >+	.sda = &pcpu_name,									\
> > >+	__SRCU_STRUCT_INIT_COMMON(name)								\
> > >+}
> > >+
> > >+
> > > /*
> > >  * Define and initialize a srcu struct at build time.
> > >  * Do -not- call init_srcu_struct() nor cleanup_srcu_struct() on it.
> > >@@ -151,7 +160,7 @@ struct srcu_struct {
> > >  */
> > > #ifdef MODULE
> > > # define __DEFINE_SRCU(name, is_static)								\
> > >-	is_static struct srcu_struct name;							\
> > >+	is_static struct srcu_struct name = __SRCU_STRUCT_INIT_MODULE(name);			\
> > > 	extern struct srcu_struct * const __srcu_struct_##name;					\
> > > 	struct srcu_struct * const __srcu_struct_##name						\
> > > 		__section("___srcu_struct_ptrs") = &name diff --git 
> > >a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c index 
> > >cd46fe063e50..7a6d9452a5d0 100644
> > >--- a/kernel/rcu/srcutree.c
> > >+++ b/kernel/rcu/srcutree.c
> > >@@ -1895,13 +1895,14 @@ void __init srcu_init(void)  static int 
> > >srcu_module_coming(struct module *mod)  {
> > > 	int i;
> > >+	struct srcu_struct *ssp;
> > > 	struct srcu_struct **sspp = mod->srcu_struct_ptrs;
> > >-	int ret;
> > > 
> > > 	for (i = 0; i < mod->num_srcu_structs; i++) {
> > >-		ret = init_srcu_struct(*(sspp++));
> > >-		if (WARN_ON_ONCE(ret))
> > >-			return ret;
> > >+		ssp = *(sspp++);
> > >+		ssp->sda = alloc_percpu(struct srcu_data);
> > >+		if (WARN_ON_ONCE(!ssp->sda))
> > >+			return -ENOMEM;
> > > 	}
> > > 	return 0;
> > > }
> > >@@ -1910,10 +1911,16 @@ static int srcu_module_coming(struct module 
> > >*mod)  static void srcu_module_going(struct module *mod)  {
> > > 	int i;
> > >+	struct srcu_struct *ssp;
> > > 	struct srcu_struct **sspp = mod->srcu_struct_ptrs;
> > > 
> > >-	for (i = 0; i < mod->num_srcu_structs; i++)
> > >-		cleanup_srcu_struct(*(sspp++));
> > >+	for (i = 0; i < mod->num_srcu_structs; i++) {
> > >+		ssp = *(sspp++);
> > >+		if (!rcu_seq_state(smp_load_acquire(&ssp->srcu_sup->srcu_gp_seq_needed)) &&
> > >+		    !WARN_ON_ONCE(!ssp->srcu_sup->sda_is_static))
> > >+				cleanup_srcu_struct(ssp);
> > >+		free_percpu(ssp->sda);
> > 
> > 
> > Hi Paul
> > 
> > About 037b80b8865fb ("srcu: Check for readers at module-exit time ")
> > 
> > --- a/kernel/rcu/srcutree.c
> > +++ b/kernel/rcu/srcutree.c
> > @@ -1911,7 +1911,8 @@ static void srcu_module_going(struct module *mod)
> >                 if (!rcu_seq_state(smp_load_acquire(&ssp->srcu_sup->srcu_gp_seq_needed)) &&
> >                     !WARN_ON_ONCE(!ssp->srcu_sup->sda_is_static))
> >                         cleanup_srcu_struct(ssp);
> 
> 
> The srcu_sup->sda_is_static is true, in cleanup_srcu_struct(), the ssp->sda can not be freed.

Very good, thank you!  I will fold your suggested fix into this commit:

037b80b8865f ("srcu: Check for readers at module-exit time")

							Thanx, Paul

> > -               free_percpu(ssp->sda);
> > +               else if (!WARN_ON(srcu_readers_active(ssp)))
> > +                       free_percpu(ssp->sda);
> > 
> > Should the else statement be removed?  like this:
> > 
> > if (!WARN_ON(srcu_readers_active(ssp)))
> > 	free_percpu(ssp->sda);
> >
> >Mightn't that cause us to double-free ssp->sda?  Once in free_percpu(), 
> >and before that in cleanup_srcu_struct()?
> 
> 
> how about this? any thought?
> 
> --- a/kernel/rcu/srcutree.c
> +++ b/kernel/rcu/srcutree.c
> @@ -1937,7 +1937,7 @@ static void srcu_module_going(struct module *mod)
>                 if (!rcu_seq_state(smp_load_acquire(&ssp->srcu_sup->srcu_gp_seq_needed)) &&
>                     !WARN_ON_ONCE(!ssp->srcu_sup->sda_is_static))
>                         cleanup_srcu_struct(ssp);
> -               else if (!WARN_ON(srcu_readers_active(ssp)))
> +               if (!WARN_ON(srcu_readers_active(ssp)))
>                         free_percpu(ssp->sda);
>         }
>  }
> 
> Thanks
> Zqiang
> 
> >
> >							Thanx, Paul
> >
> > Thanks
> > Zqiang
> > 
> > 
> > >+	}
> > > }
> > > 
> > > /* Handle one module, either coming or going. */
> > >--
> > >2.40.0.rc2
> > >