Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   "Zhang, Qiang1" <qiang1.zhang@intel.com>
To:     "Paul E. McKenney" <paulmck@kernel.org>,
        "rcu@vger.kernel.org" <rcu@vger.kernel.org>
CC:     "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "kernel-team@meta.com" <kernel-team@meta.com>,
        "rostedt@goodmis.org" <rostedt@goodmis.org>,
        "hch@lst.de" <hch@lst.de>,
        "qiang.zhang1211@gmail.com" <qiang.zhang1211@gmail.com>
Subject: RE: [PATCH RFC rcu 02/19] srcu: Use static init for statically
 allocated in-module srcu_struct
Thread-Topic: [PATCH RFC rcu 02/19] srcu: Use static init for statically
 allocated in-module srcu_struct
Thread-Index: AQHZXeZkMjCksJUJIEigTtRmvPjKta8SunIg
Date:   Thu, 30 Mar 2023 04:11:05 +0000
Message-ID: <PH0PR11MB5880CA682074874FF64B3412DA8E9@PH0PR11MB5880.namprd11.prod.outlook.com>
References: <3db82572-f156-4a5d-b711-841aa28bd996@paulmck-laptop>
 <20230324001938.3443499-2-paulmck@kernel.org>
In-Reply-To: <20230324001938.3443499-2-paulmck@kernel.org>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?ryG2qxpvsZJw296xWorNDkTKQNAeJVYG60iMiKfivIulm7k2vtAZLHAa68ho?=
 =?us-ascii?Q?BujVl/8cLC1Cwyu6QSbstQgC0nKWn6RP1fZMvWFBSpYeB7g2wVaVIq3bSLdq?=
 =?us-ascii?Q?7sFX+4OukBqf3TM795+vH51eiMMUQxsHvIhpAt5dLbPZ8P0K1t163jt9UK8g?=
 =?us-ascii?Q?XfoPLk812Rb0IiXZPht5eQfhFVgLC09FBUFQFWG7cmygLRZz54f+oprEuGbc?=
 =?us-ascii?Q?Ccy8LdAOpOaUzKYunEqtMldUIAy/lzyiVKx+sz+Q5LQt/aurLwNNqNmtiEge?=
 =?us-ascii?Q?XMVW0vuw8/BiZe7gP/TskHFhGlhH2AG4nBAzf1cy8hYg3x7vLi1Ux+bE3ple?=
 =?us-ascii?Q?N85zS0ncZ866GnMRpwKVEYcrGoUp72rJYX0oOGDaKbympdlvri6MUl1ibBqH?=
 =?us-ascii?Q?WCK/ElqiKWH4h21hfT+uY5FkX4E3RM7IEHF077tC34IssI3TYJ4Febd3YPnW?=
 =?us-ascii?Q?GAAK2+dRuAhQI9F38Pl6DsQ2wLDqzkSi1GT+nS/R1F+wyPhhUfzDCuwkWsrv?=
 =?us-ascii?Q?skn0pWZVJF7Vxnt9Orac/pL6iDl1RBur2dtXaFuETRQiwSvKdWNxsEfBEsKq?=
 =?us-ascii?Q?n/Ar7M5OWkHSdo/MyC70bw1JmaVmDCi6eTEcMHjDvSxuAWj8TqJVGzWPvMZ+?=
 =?us-ascii?Q?wWC6YfpQzm7HL42dxSoivUjvJjpM0OAp1I+VLAau2+UTNf8AGYC/ZaZ8JpUw?=
 =?us-ascii?Q?v7PnXZA4Q6dh1xIUKZDQDDdteSSNpowW17LOayLnwPgu1upT+qGe3TYEivXs?=
 =?us-ascii?Q?a/cUxYgXZyqx/4DeClyurW11AXR9Sj7e2X5wJdqhOGZDMo0fCu9IVdTBdzWD?=
 =?us-ascii?Q?30Q9RUBbmV4zicFrgFcJlsHoZjAQzrquPBGXqcDLwU93NPbJyzpDX7YBWc71?=
 =?us-ascii?Q?+Mkd76cWdC9Zn037ZiaoYUZYIT8hsawosihCNJlm21jUtMerFVgMJphxxEQB?=
 =?us-ascii?Q?AxIoBQinztkVcdbdIoDIhShESSl8fZXlJOGlVZIQ5SzypkF9dZBjEXKIFLf5?=
 =?us-ascii?Q?eZYLTYdwS4R0FBgTlrpdv5eqrA0PTSM7CaRpRuFqhF4YlQ9AksreflV2ozyb?=
 =?us-ascii?Q?VFYdPN51Cf3sXcYCGXZkJuTRlTXS3pcT7jAH3h3G7QOed/1IuzKN8NwERbuw?=
 =?us-ascii?Q?JtnXwVPdjBOcoJj5lqPVPsTZ0of4CBMBP7uMfeSL7MvdznzRu1ElxA814ck3?=
 =?us-ascii?Q?R8FiBx2e+Nfv6thoWK8gIFWxLuO1H1DqjBsTcd8B0YufwQGin15Fjp9Xi4rL?=
 =?us-ascii?Q?T4sOwis5ZHHXIIeRH0uS4V8M+Cq7KNf2Y+nqVu3C/TFC16xMHERpuN/PxY/o?=
 =?us-ascii?Q?ZslJzfxa+Vl3bB+lKqkoJwm0ufYH4XPDwa3+nGe0RsqNiixCqGv7hK8/dzKe?=
 =?us-ascii?Q?toN1P2H1wDAJmIT0LCu52swXhWOG8D3fWVV99vClNbQl0yPn8+nWanNMvvd5?=
 =?us-ascii?Q?PUNltGRbKf8obxy+aUS0yiJU1LkSYic7y6WBjkceNb67R4gRPPN00IY59UF4?=
 =?us-ascii?Q?j/8xvsO1K//C0Z5M8QVtyqkUSY379EWcYP5KSv47kPUVoC34+yXCREiuAVSj?=
 =?us-ascii?Q?oaP1hNTItG64yNBEWf8yuIg4i0nxC5BRowXXOa52?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: PH0PR11MB5880.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 66bfffa7-47ee-4ce3-016c-08db30d4c86b
X-MS-Exchange-CrossTenant-originalarrivaltime: 30 Mar 2023 04:11:05.6225
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: pWJqw22MvBqrhZEt5Wwo0R8EB7ofiFxdBsd01hk7Rc65EbE5060bJUQ25VBFoTFNhAcAQ+xxL2urwoZnsZ8foQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR11MB6858
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-2.5 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,SPF_HELO_NONE,
        SPF_NONE autolearn=unavailable autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

>
>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
>=20
>-#define __SRCU_STRUCT_INIT(name, pcpu_name)							\
>-{												\
>-	.sda =3D &pcpu_name,									\
>+#define __SRCU_STRUCT_INIT_COMMON(name)								\
> 	.lock =3D __SPIN_LOCK_UNLOCKED(name.lock),						\
> 	.srcu_gp_seq_needed =3D -1UL,								\
> 	.work =3D __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)								\
> }
>=20
>+#define __SRCU_STRUCT_INIT(name, pcpu_name)							\
>+{												\
>+	.sda =3D &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 =3D __SRCU_STRUCT_INIT_MODULE(name);		=
	\
> 	extern struct srcu_struct * const __srcu_struct_##name;					\
> 	struct srcu_struct * const __srcu_struct_##name						\
> 		__section("___srcu_struct_ptrs") =3D &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 =3D mod->srcu_struct_ptrs;
>-	int ret;
>=20
> 	for (i =3D 0; i < mod->num_srcu_structs; i++) {
>-		ret =3D init_srcu_struct(*(sspp++));
>-		if (WARN_ON_ONCE(ret))
>-			return ret;
>+		ssp =3D *(sspp++);
>+		ssp->sda =3D 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 =3D mod->srcu_struct_ptrs;
>=20
>-	for (i =3D 0; i < mod->num_srcu_structs; i++)
>-		cleanup_srcu_struct(*(sspp++));
>+	for (i =3D 0; i < mod->num_srcu_structs; i++) {
>+		ssp =3D *(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


>+	}
> }
>=20
> /* Handle one module, either coming or going. */
>--=20
>2.40.0.rc2
>