Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758607AbcDLWly (ORCPT ); Tue, 12 Apr 2016 18:41:54 -0400 Received: from shadbolt.e.decadent.org.uk ([88.96.1.126]:55361 "EHLO shadbolt.e.decadent.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751648AbcDLWlx (ORCPT ); Tue, 12 Apr 2016 18:41:53 -0400 Message-ID: <1460500903.2705.12.camel@decadent.org.uk> Subject: Re: [PATCH 4.5 142/238] watchdog: dont run proc_watchdog_update if new value is same as old From: Ben Hutchings To: Greg Kroah-Hartman , linux-kernel@vger.kernel.org Cc: stable@vger.kernel.org, Josh Hunt , Don Zickus , Aaron Tomlin , Ulrich Obergfell , Andrew Morton , Linus Torvalds Date: Tue, 12 Apr 2016 23:41:43 +0100 In-Reply-To: <20160410183504.421546277@linuxfoundation.org> References: <20160410183456.398741366@linuxfoundation.org> <20160410183504.421546277@linuxfoundation.org> Content-Type: multipart/signed; micalg="pgp-sha512"; protocol="application/pgp-signature"; boundary="=-UOUBf9Ejj9y17PAJKwL2" X-Mailer: Evolution 3.18.5.1-1 Mime-Version: 1.0 X-SA-Exim-Connect-IP: 2a02:8011:400e:2:6f00:88c8:c921:d332 X-SA-Exim-Mail-From: ben@decadent.org.uk X-SA-Exim-Scanned: No (on shadbolt.decadent.org.uk); SAEximRunCond expanded to false Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3748 Lines: 106 --=-UOUBf9Ejj9y17PAJKwL2 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Sun, 2016-04-10 at 11:35 -0700, Greg Kroah-Hartman wrote: > 4.5-stable review patch.=C2=A0=C2=A0If anyone has any objections, please = let me know. >=20 > ------------------ >=20 > From: Joshua Hunt >=20 > commit a1ee1932aa6bea0bb074f5e3ced112664e4637ed upstream. >=20 > While working on a script to restore all sysctl params before a series of > tests I found that writing any value into the > /proc/sys/kernel/{nmi_watchdog,soft_watchdog,watchdog,watchdog_thresh} > causes them to call proc_watchdog_update(). >=20 > =C2=A0 NMI watchdog: enabled on all CPUs, permanently consumes one hw-PMU= counter. > =C2=A0 NMI watchdog: enabled on all CPUs, permanently consumes one hw-PMU= counter. > =C2=A0 NMI watchdog: enabled on all CPUs, permanently consumes one hw-PMU= counter. > =C2=A0 NMI watchdog: enabled on all CPUs, permanently consumes one hw-PMU= counter. >=20 > There doesn't appear to be a reason for doing this work every time a writ= e > occurs, so only do it when the values change. >=20 > Signed-off-by: Josh Hunt > Acked-by: Don Zickus > Reviewed-by: Aaron Tomlin > Cc: Ulrich Obergfell > Signed-off-by: Andrew Morton > Signed-off-by: Linus Torvalds > Signed-off-by: Greg Kroah-Hartman >=20 > --- > =C2=A0kernel/watchdog.c |=C2=A0=C2=A0=C2=A0=C2=A09 ++++++++- > =C2=A01 file changed, 8 insertions(+), 1 deletion(-) >=20 > --- a/kernel/watchdog.c > +++ b/kernel/watchdog.c [...] > @@ -967,7 +970,7 @@ int proc_soft_watchdog(struct ctl_table > =C2=A0int proc_watchdog_thresh(struct ctl_table *table, int write, > =C2=A0 =C2=A0void __user *buffer, size_t *lenp, loff_t *ppos) > =C2=A0{ > - int err, old; > + int err, old, new; > =C2=A0 > =C2=A0 get_online_cpus(); > =C2=A0 mutex_lock(&watchdog_proc_mutex); > @@ -987,6 +990,10 @@ int proc_watchdog_thresh(struct ctl_tabl > =C2=A0 /* > =C2=A0 =C2=A0* Update the sample period. Restore on failure. > =C2=A0 =C2=A0*/ > + new =3D ACCESS_ONCE(watchdog_thresh); This ACCESS_ONCE() doesn't make any sense to me. =C2=A0Isn't watchdog_thres= h protected by watchdog_proc_mutex? =C2=A0If a race on watchdog_thresh is still possible then the check for old =3D=3D new isn't a valid optimisation, and if it isn't possible then ACCESS_ONCE() shouldn't be used here. Ben. > + if (old =3D=3D new) > + goto out; > + > =C2=A0 set_sample_period(); > =C2=A0 err =3D proc_watchdog_update(); > =C2=A0 if (err) { --=C2=A0 Ben Hutchings This sentence contradicts itself - no actually it doesn't. --=-UOUBf9Ejj9y17PAJKwL2 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: This is a digitally signed message part -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAABCgAGBQJXDXmnAAoJEOe/yOyVhhEJ/5IP/3X0bIkuKjwqc9lcJnK8lCxY 5TsGw8MIwyNqVnqY930qtjqUbg2ZLAEuZrcH6apNxR5Xnp6JOHhNZEBqhpV/MCRL 1mAk482fHNRVs1D1cGqCMuicqfqCMiXmzQozgRe5k7eb/0LiG/6UlrmchibJv2fy tCrYZIVfqNy00ZVwx5XUJY7SF7MBLGWfwZqOs+r2Bexl9YF4ex52vQbhb16grH0U 71+lfNTkswL89LSKKd3gKm3udKgF5olW1sSkDCK3OISPpVNCSe0BVYlBFR51ZCXx hQU7ZxD27/k7ZOynyaASJfr99be9FohiXcsf4U9Sdr6+/5mpVWs3B4l/4Oq3mqrr idCgw5pDyjr+BhJmJeGEwj+1JxzLA+KsajhSEhbO6o7EOBD/pS/pkHJgpgVNVcBF wgNFb53LA8qJTN4AuDt6SscnygXqv1dngvIb6N1qNMlKCewR5St+2YWHinr/AJkL oyLOmknfldB36AWTnDv8YgGlGiG3+znaGjmvKS1AuTbUmP3siuyD8dEIHEMT3qQi +3CA+VopnFwgTb0IAdBCFjygV7IzFiQlZrgVcJ9Mh3CqYMi3LJDMDHzpolACcibW buhvgPDXL+sQHJAlnhdRu9UIPENaoxu5UUx1DzS49s0Uw59HJ72jaSW54wxXz01p a+pZ1vGfMz/oCu/nnGwX =yvGH -----END PGP SIGNATURE----- --=-UOUBf9Ejj9y17PAJKwL2--