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@xxxxxxxxx feedback. ]
> > >
> > >Signed-off-by: Paul E. McKenney <paulmck@xxxxxxxxxx>
> > >Cc: Christoph Hellwig <hch@xxxxxx>
> > >---
> > > 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
> > >
