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:     "frederic@kernel.org" <frederic@kernel.org>,
        "joel@joelfernandes.org" <joel@joelfernandes.org>,
        "rcu@vger.kernel.org" <rcu@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: RE: [PATCH] srcu: Fix flush sup work warning in cleanup_srcu_struct()
Thread-Topic: [PATCH] srcu: Fix flush sup work warning in
 cleanup_srcu_struct()
Thread-Index: AQHZW8x3wcHPlwuNM0Wizzk5AGbgwq8FVToAgAClkaA=
Date:   Wed, 22 Mar 2023 01:15:02 +0000
Message-ID: <PH0PR11MB588087645FE02C9F2C113CC8DA869@PH0PR11MB5880.namprd11.prod.outlook.com>
References: <20230321081346.192000-1-qiang1.zhang@intel.com>
 <e7951c31-4a24-4493-be4b-603fec0730ff@paulmck-laptop>
In-Reply-To: <e7951c31-4a24-4493-be4b-603fec0730ff@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?gJwvaQA6H3S4DeBOUk6ht4oCLRIsUoP4cHeRpg/BigRJeB+XBzlLMvawgCUE?=
 =?us-ascii?Q?kOTgsylOIGrfIstCJqHdZN3lgxOBQK/TJAjemyAZsvuxuJw/6NhJNxkmxjqg?=
 =?us-ascii?Q?7F1vPpPMvYQys8aKD3Z2w61iZosbu/zFvhTRzPRxHYYlXT24II6sC08dZhBi?=
 =?us-ascii?Q?KmsrhlUoO9nXP60HE61NssohHQ1yB0HtrNSpMstK3Ye4lwQ2RuEEMU1aJ7v9?=
 =?us-ascii?Q?HYbynzBGCwOl4og6oIpvW4ev73GAamsKOzgQj+ru/vcKNjL5Oo5LbKKYS/lW?=
 =?us-ascii?Q?Gq/Tr+L+avd+yWphZ2O2E0XQurscrqvF2NjUEklF8/4EEbBMSe7rAe4nG0O5?=
 =?us-ascii?Q?ZQpKiGysL0GgPKNujxhx+1wDsNp+b2WT6zKKNmryzHdOghcNB29W+4Db/i2y?=
 =?us-ascii?Q?NGqAGg2q3qzUu+SzroNrlgWGGDZThfoloU6WWw+y41LfQ1Bqa4+NefGyCh60?=
 =?us-ascii?Q?pM0Y3tT0M5wGLzAyAQGp5CceHzZ81YgYFp/JV/8cR8+FCicp3B9/P8y7kD46?=
 =?us-ascii?Q?4fWJQ/U3u6ecJdaLtKmdL7l7DIDIsWBB5/U9oaEhA0LjrSlGJ9l4OYFr0US8?=
 =?us-ascii?Q?FV+4BEJeLlMNibzUjaCfpncnSDMaWlsV+icrmOuqvqzKgoOqAh0eZT5EgmFL?=
 =?us-ascii?Q?iT5FgChu6zp0a3Ljt76r+fODLAzNcreM0BstK7CaSgouTKceEePHm/mE3Zj/?=
 =?us-ascii?Q?aVPXZvfuDP4pTE1a8vrp9lIPYo6vkP9XuIXVWUXPeTjc20ZpHitBbuJyIrhK?=
 =?us-ascii?Q?Yze/5e+4m1Gu1QGoEaiIeyzPkQ5lbL52W0Z5pTcEBFUY2c3sWnaPPJdNt0LT?=
 =?us-ascii?Q?lWPpDBnrKHoVbO2ev3BE572NOLlbL/nQWpXLjiEfcOdR9WkRR5WNEKaUuHB/?=
 =?us-ascii?Q?GTuHMzteLaFa8b1JLLXl5hEtuS753jBQOt3OkeSlcFHPkU08DgV+N6oHKu4h?=
 =?us-ascii?Q?21hbeA/Ljwlra7RZBsErHOH48w0dVuzw2wI8bXAHSz+tep3EFn6uThqHtcSz?=
 =?us-ascii?Q?cHI4j0FJQc5Et5ycS3xcSCTeC7xgK492N8rNs6WGojKmbjWplhHm9TZm6lTb?=
 =?us-ascii?Q?Pq6FEOKzhVyrIbnlBGWMRLCPL0NlA7E96eoXTvq7sLie9CdB8N6v47HegIRK?=
 =?us-ascii?Q?V6zILqMgsd+ZybBgOyb81iFOsf4rmcfX1o9lsq/Um+A1+sOODSNBwm/2yuoY?=
 =?us-ascii?Q?938gohdY93hacHxfgWHhTgYJbwDLLuS6c6RTVbsu4apTwcQxDW2uv8jv+ePY?=
 =?us-ascii?Q?1Jwf8a8TWZCfV5Myeh91VWz3Ua+HkgGkcMznklzbNXuHdn7gQa2vzxYajoEp?=
 =?us-ascii?Q?9xkpXPUiACAIFk31pxcmq2/KOMHMn8KAEgZknrp/HhU+/T1R/2b7d+jEpTFQ?=
 =?us-ascii?Q?/cSGBskLqlQGkLIWfXDHOe7/uG1uFbS1ZE2qfLnFuWBBX4i25vsM1tvY3ERB?=
 =?us-ascii?Q?5kIuMISKs5b9D3m44DPbzAjrqTxIEMnavptYMRtUbZlIg5EgxXTZxTDlKpnq?=
 =?us-ascii?Q?ATFhQeZWTgpqoVzfVOptpSmsAnbSTk2zv0k5EDjDDz3xp3Xo5cJCm3xVmBQF?=
 =?us-ascii?Q?h3x6xrTAjumMeGCTmJeXeZhtMvFqUG6QIkoK+nLXsyogt7HaX98WTf7pZ/+n?=
 =?us-ascii?Q?NttFn8tLP9cjlnF7oxlyqPjisg96JXDj5J5pdQSL8FGH?=
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: 4efdc9c2-4fb3-4215-a9ae-08db2a72dcda
X-MS-Exchange-CrossTenant-originalarrivaltime: 22 Mar 2023 01:15:02.2544
 (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: Jc/dZHkcRH+uX7uSi/fqqTKU2b+T3F8MXGLJ30lhUpMnocaY5Crr5kdx8MM3p8cX/KeioicrqwnVULHU+Z7GHg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR11MB4783
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,URIBL_BLOCKED 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

> insmod rcutorture.ko
> rmmod rcutorture.ko
>=20
> [  209.437327] WARNING: CPU: 0 PID: 508 at kernel/workqueue.c:3167 __flus=
h_work+0x50a/0x540
> [  209.437346] Modules linked in: rcutorture(-) torture [last unloaded: r=
cutorture]
> [  209.437382] CPU: 0 PID: 508 Comm: rmmod Tainted: G  W  6.3.0-rc1-yocto=
-standard+
> [  209.437406] RIP: 0010:__flush_work+0x50a/0x540
> .....
> [  209.437758]  flush_delayed_work+0x36/0x90
> [  209.437776]  cleanup_srcu_struct+0x68/0x2e0
> [  209.437817]  srcu_module_notify+0x71/0x140
> [  209.437854]  blocking_notifier_call_chain+0x9d/0xd0
> [  209.437880]  __x64_sys_delete_module+0x223/0x2e0
> [  209.438046]  do_syscall_64+0x43/0x90
> [  209.438062]  entry_SYSCALL_64_after_hwframe+0x72/0xdc
>=20
> For srcu objects defined with DEFINE_SRCU() or DEFINE_STATIC_SRCU(),
> when compiling and loading as modules, the srcu_module_coming() is invoke=
d,
> allocate memory for srcu structure's->sda and initialize sda structure,
> due to not fully initialize srcu structure's->sup, so at this time the
> sup structure's->delaywork.func is null, if not invoke init_srcu_struct_f=
ields()
> before unloading modules, in srcu_module_going() the __flush_work() find
> work->func is empty, will raise the warning above.
>=20
> This commit add init_srcu_struct_fields() to initialize srcu structure's-=
>sup,
> in srcu_module_coming().
>=20
> Signed-off-by: Zqiang <qiang1.zhang@intel.com>
>
>Good catch, and thank you for testing the in-module case!
>
>One question below...
>
>							Thanx, Paul
>
> ---
>  kernel/rcu/srcutree.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
>=20
> diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> index 1fb078abbdc9..42d8720e016c 100644
> --- a/kernel/rcu/srcutree.c
> +++ b/kernel/rcu/srcutree.c
> @@ -1921,7 +1921,8 @@ static int srcu_module_coming(struct module *mod)
>  		ssp->sda =3D alloc_percpu(struct srcu_data);
>  		if (WARN_ON_ONCE(!ssp->sda))
>  			return -ENOMEM;
> -		init_srcu_struct_data(ssp);
> +		if (WARN_ON_ONCE(init_srcu_struct_fields(ssp, true)))
> +			return -ENOMEM;
>
>Wouldn't it be better to simply delete the init_srcu_struct_data()?
>
>Then the first call to check_init_srcu_struct() would take care of
>the initialization, just as for the non-module case.  Or am I missing
>something subtle?

Hi Paul

Maybe the check_init_srcu_struct() is always not invoked, for example,
In rcutorture.c,   here is such a definition DEFINE_STATIC_SRCU(srcu_ctl),
but we use torture_type=3Drcu to test,  there will not be any interface cal=
ling
check_init_srcu_struct() to initialize srcu_ctl and set  structure's->delay=
work.func=20
is process_srcu().
when we unload the rcutorture module, invoke cleanup_srcu_struct()
to flush sup structure's->delaywork.func, due to the func pointer is not in=
itialize,
it's null, will trigger warning.

About kernel/workqueue.c:3167

__flush_work
     if (WARN_ON(!work->func))   <---------trigger waning
	return false;


and  in  init_srcu_struct_fields(ssp, true), wil set srcu_sup->sda_is_stati=
c is true
and set srcu_sup-> srcu_gp_seq_needed is zero,  after that when we call
 check_init_srcu_struct() again, it not be initialized again.

Thanks
Zqiang

>
>It should also be possible to eliminate duplicate code between the
>in-module and non-module statically allocated initialization cases,
>come to think of it.
>
>  	}
>  	return 0;
>  }
> @@ -1931,9 +1932,13 @@ static void srcu_module_going(struct module *mod)
>  {
>  	int i;
>  	struct srcu_struct **sspp =3D mod->srcu_struct_ptrs;
> +	struct srcu_struct *ssp;
> =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++);
> +		cleanup_srcu_struct(ssp);
> +		free_percpu(ssp->sda);
> +	}
>
>And good catch on another memory leak with this one, looks proper
>to me.
>
>  }
> =20
>  /* Handle one module, either coming or going. */
> --=20
> 2.25.1
>=20