Advanced
Mailing List Archive

Mailing List Archive

  1. Home >
  2. Linux>
  3. Kernel
[PATCH RFC rcu 02/19] srcu: Use static init for statically allocated in-module srcu_struct
paulmck at kernel
Mar 23, 2023, 6:15 PM
Post #1 of 6 (50 views)
Permalink
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);
+ }
}

/* Handle one module, either coming or going. */
--
2.40.0.rc2
RE: [PATCH RFC rcu 02/19] srcu: Use static init for statically allocated in-module srcu_struct [ In reply to ]
qiang1.zhang at intel
Mar 29, 2023, 10:15 PM
Post #2 of 6 (50 views)
Permalink
>
>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
>
Re: [PATCH RFC rcu 02/19] srcu: Use static init for statically allocated in-module srcu_struct [ In reply to ]
paulmck at kernel
Mar 30, 2023, 8:15 AM
Post #3 of 6 (50 views)
Permalink
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
> >
RE: [PATCH RFC rcu 02/19] srcu: Use static init for statically allocated in-module srcu_struct [ In reply to ]
qiang1.zhang at intel
Mar 30, 2023, 8:15 AM
Post #4 of 6 (50 views)
Permalink
> >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
> >
RE: [PATCH RFC rcu 02/19] srcu: Use static init for statically allocated in-module srcu_struct [ In reply to ]
qiang1.zhang at intel
Mar 30, 2023, 10:15 AM
Post #5 of 6 (50 views)
Permalink
> >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
> >
Re: [PATCH RFC rcu 02/19] srcu: Use static init for statically allocated in-module srcu_struct [ In reply to ]
paulmck at kernel
Mar 30, 2023, 10:15 AM
Post #6 of 6 (50 views)
Permalink
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
> > >

©2024 GT.net. A Gossamer Threads company.
5th Floor, 455 Granville St., Vancouver, BC V6C 1T1, Canada | Legal