linux-kernel - RE: [PATCH RFC rcu 02/19] srcu: Use static init for statically allocated in-module srcu

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <PH0PR11MB5880CA682074874FF64B3412DA8E9@PH0PR11MB5880.namprd11.prod.outlook.com>
Date:   Thu, 30 Mar 2023 04:11:05 +0000
From:   "Zhang, Qiang1" <qiang1.zhang@...el.com>
To:     "Paul E. McKenney" <paulmck@...nel.org>,
        "rcu@...r.kernel.org" <rcu@...r.kernel.org>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "kernel-team@...a.com" <kernel-team@...a.com>,
        "rostedt@...dmis.org" <rostedt@...dmis.org>,
        "hch@....de" <hch@....de>,
        "qiang.zhang1211@...il.com" <qiang.zhang1211@...il.com>
Subject: RE: [PATCH RFC rcu 02/19] srcu: Use static init for statically
 allocated in-module srcu_struct

>
>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@...el.com feedback. ]
>
>Signed-off-by: Paul E. McKenney <paulmck@...nel.org>
>Cc: Christoph Hellwig <hch@....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
>