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:     "paulmck@kernel.org" <paulmck@kernel.org>
CC:     "rcu@vger.kernel.org" <rcu@vger.kernel.org>,
        "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: AQHZXeZkMjCksJUJIEigTtRmvPjKta8SunIggAC5AYCAAAGhAA==
Date:   Thu, 30 Mar 2023 15:03:50 +0000
Message-ID: <PH0PR11MB58807EB1C375AE03F8586C92DA8E9@PH0PR11MB5880.namprd11.prod.outlook.com>
References: <3db82572-f156-4a5d-b711-841aa28bd996@paulmck-laptop>
 <20230324001938.3443499-2-paulmck@kernel.org>
 <PH0PR11MB5880CA682074874FF64B3412DA8E9@PH0PR11MB5880.namprd11.prod.outlook.com>
 <b66d52cb-ed73-4c7f-bab0-201dc13d164a@paulmck-laptop>
In-Reply-To: <b66d52cb-ed73-4c7f-bab0-201dc13d164a@paulmck-laptop>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?ZLr/aEwhmIorCb3TVqZQQk+Rate/dtxoTXfcyvrq2/XpqyltA9PWr/OTIdAb?=
 =?us-ascii?Q?du43O4WUf2sTJQUaD/6aEbup3iT8w8GkP5xnld3ha3rdROFiePXpotajZ/Q1?=
 =?us-ascii?Q?S6CLPnrXyFPqWRx+ClCJian0CV7p29VRq+Uh8h4H13fMbCrExZsizjMIe+oa?=
 =?us-ascii?Q?vPu3XOxZS2sNgvMCBta7i8P+YXSzbs4FdcfO8kjSUfphSn6KjI9XBiLKKB3x?=
 =?us-ascii?Q?Awu5/A4R0sHLW6OEJwka6jdzRaipSJNFKIGt3f5Vx/a5xIXc0wD8RsKBhLe/?=
 =?us-ascii?Q?YDbCclyMps5h9knBNZUqnmGoX7dqgK6mbRp2EPK0xHB51Uj7+bBFbwGZikfv?=
 =?us-ascii?Q?Knd8ZRqZcC2tjje+3p1RjtKySo/6hTsuokz1UdyDeZV9l5lPjddZQwtQuieT?=
 =?us-ascii?Q?1Q2NREoj1iC8dsJeKenqG0EmRychECJBjwtvTcUPwbQOG4If7pymj8Yi/pap?=
 =?us-ascii?Q?TNq9xHzMnx7u0zijYVuFIKE96UeN0CvREhQVFGh4ToJIKwYuuu80ex86yZyP?=
 =?us-ascii?Q?bvqF8f6IW0fWw5o4c3JZEVOBG91YEiSCBZsfyBs1AYhdektM+sZmeBZTwhH6?=
 =?us-ascii?Q?bmxWP+ddREvCoISJNb76qJghHZuFFXmO5iZYoAoF1OwIlaHeVfVFKvFVepfu?=
 =?us-ascii?Q?LGw+fJmZtzLslyGzAm1G+NnN0mTA7AQxE88u21H2CJUIM53kTVPREpDaT3gl?=
 =?us-ascii?Q?STZdCXEZ8xNEqyQcf7Xkpr9+mxYh0OzYRA50kmUmM/r53qxfIGBVgLM5Re0W?=
 =?us-ascii?Q?R4Q8z1F6k1xRdwfaI/df9L2wd7t6jNr2FJMr5pG6ss8iwmcDnDJodU/nJ0R+?=
 =?us-ascii?Q?XdwGIlmCKunM23bGtibD/+vH1MhH/BL/TcXayoH/SlVyeKO9s1tMEuJXfXD+?=
 =?us-ascii?Q?dnVOTDe7p1lStRsYWw5SML4+mdlTQ4mXN1w8HI51AgZva9Q4Cj4NXjoJLQaB?=
 =?us-ascii?Q?h5FDpuqJ6/xI+pyulN9YEeMVl2OkWK67+9UkAjEL0jmhqPdcEdZGsBMyJsfN?=
 =?us-ascii?Q?KL7gF9tPrBnJMtC6BDQkXmtFayyUW84199qeR6zjAan/9SlUEIrfmMyrhjMg?=
 =?us-ascii?Q?Qp7zbluyfq5OkYofNC3swFarL6cdzHvUoPmfs4FxFi6I0+wdAVJ83Et0RUZf?=
 =?us-ascii?Q?IpKzAINrGkxpq5ILYSidxoxtxUXiz3OKztP1+X4V2z0zVX/vF0+stilqQCZb?=
 =?us-ascii?Q?DuzLWsEgG2Uwf6fXJvNaQXfbaDH/Bk4I5m4T9P0xNG7D1eMq17h4tc02KSze?=
 =?us-ascii?Q?mT3etvfybo7RVKfZmenc6TvTzZ+vSY6L1TfSMtJ/VSVsKPRePjF/BIAzKNFt?=
 =?us-ascii?Q?d9ENxZiImt6Jng/xh/6Rc4Znd+Vh+pOntZZrH+GR48EoXdm84X5L0/Ddtwqg?=
 =?us-ascii?Q?7adgJWIddj0t623gQ5W3pEN26+ZqLc0px+fYT+Dz8d/EDHyVttPyovVUyiPY?=
 =?us-ascii?Q?R5L3F5HEa0gLjsMB64EpMYnR2JYKeMprsGN/1t6czcOcVcE1PEVSXNi/l8ia?=
 =?us-ascii?Q?GpNctdfdqFKPUHvlg4+rrGSPxqFRe/mFnEDHCQupQ1O7xM8HKU9LTCAcagGZ?=
 =?us-ascii?Q?hkzkFXrxBrjLA8Wuolgih66OxHn1bwto7Kl8GZtV?=
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: b73d077e-cc19-4fea-5c12-08db312ff8b1
X-MS-Exchange-CrossTenant-originalarrivaltime: 30 Mar 2023 15:03:50.8269
 (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: tlza55QJg9y72RifoUtqi4q2hpN8WiNis6s3ry80NaQDng8nJEK2xbtAd2Ro1WEVZmWQWYlUS5/HvFPnG0aZzA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN7PR11MB7511
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 memor=
y
> >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_neede=
d)) &&
> >+		    !WARN_ON_ONCE(!ssp->srcu_sup->sda_is_static))
> >+				cleanup_srcu_struct(ssp);
> >+		free_percpu(ssp->sda);
>=20
>=20
> Hi Paul
>=20
> About 037b80b8865fb ("srcu: Check for readers at module-exit time ")
>=20
> --- 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);
>=20
> Should the else statement be removed?  like this:
>=20
> 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
>=20
>=20
> >+	}
> > }
> >=20
> > /* Handle one module, either coming or going. */
> >--=20
> >2.40.0.rc2
> >