diff mbox series

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

Message ID 20230324001938.3443499-2-paulmck@kernel.org
State New
Headers
Series Further shrink srcu_struct to promote cache locality |

Commit Message

Paul E. McKenney March 24, 2023, 12:19 a.m. UTC 
  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(-)

Comments

Zqiang March 30, 2023, 4:11 a.m. UTC | #1 
>
>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
>
Paul E. McKenney March 30, 2023, 2:55 p.m. UTC | #2 
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
> >
Zqiang March 30, 2023, 3:03 p.m. UTC | #3 
> >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
> >
Zqiang March 30, 2023, 3:20 p.m. UTC | #4 
> >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
> >
Paul E. McKenney March 30, 2023, 5:05 p.m. UTC | #5 
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 mbox series

Patch

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. */