| Message ID | 20230324001938.3443499-2-paulmck@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Further shrink srcu_struct to promote cache locality | expand |
> >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); - 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); Thanks Zqiang >+ } > } > > /* Handle one module, either coming or going. */ >-- >2.40.0.rc2 >
On Thu, Mar 30, 2023 at 04:11:05AM +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); > - 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()? Thanx, Paul > Thanks > Zqiang > > > >+ } > > } > > > > /* Handle one module, either coming or going. */ > >-- > >2.40.0.rc2 > >
> >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. > - 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()? Thanks Zqiang > > Thanx, Paul > > Thanks > Zqiang > > > >+ } > > } > > > > /* Handle one module, either coming or going. */ > >-- > >2.40.0.rc2 > >
> >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. > - 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 > >
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 > > >
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); + } } /* Handle one module, either coming or going. */
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(-)