From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752213AbcFVB5d (ORCPT ); Tue, 21 Jun 2016 21:57:33 -0400 Received: from mail-io0-f177.google.com ([209.85.223.177]:35155 "EHLO mail-io0-f177.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751880AbcFVB5b (ORCPT ); Tue, 21 Jun 2016 21:57:31 -0400 Date: Wed, 22 Jun 2016 10:00:24 +0800 From: Boqun Feng To: "Paul E. McKenney" Cc: Arnd Bergmann , Josh Triplett , linux-kernel@vger.kernel.org Subject: Re: [PATCH] torture: use ktime_t consistently Message-ID: <20160622020024.GA25785@insomnia> References: <20160620155651.2497676-1-arnd@arndb.de> <7277827.7A0WjZ9kZU@wuerfel> <20160620183757.GX3923@linux.vnet.ibm.com> <17304047.6zmVOzdKjg@wuerfel> <20160621183946.GT3923@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="vtzGhvizbBRQ85DL" Content-Disposition: inline In-Reply-To: <20160621183946.GT3923@linux.vnet.ibm.com> User-Agent: Mutt/1.6.1 (2016-04-27) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --vtzGhvizbBRQ85DL Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Hi Paul, On Tue, Jun 21, 2016 at 11:39:46AM -0700, Paul E. McKenney wrote: > On Mon, Jun 20, 2016 at 09:29:56PM +0200, Arnd Bergmann wrote: > > On Monday, June 20, 2016 11:37:57 AM CEST Paul E. McKenney wrote: > > > On Mon, Jun 20, 2016 at 08:29:48PM +0200, Arnd Bergmann wrote: > > > > On Monday, June 20, 2016 11:21:05 AM CEST Paul E. McKenney wrote: > > > > > On Mon, Jun 20, 2016 at 05:56:40PM +0200, Arnd Bergmann wrote: > > > > > =20 > > > > > @@ -446,9 +447,9 @@ EXPORT_SYMBOL_GPL(torture_shuffle_cleanup); > > > > > * Variables for auto-shutdown. This allows "lights out" tortur= e runs > > > > > * to be fully scripted. > > > > > */ > > > > > -static int shutdown_secs; /* desired test duration in seconds. = */ > > > > > +static ktime_t shutdown_ms; /* desired test duration in seconds= =2E */ > > > >=20 > > > > the variable name is a bit odd. > > >=20 > > > The comment is certainly now wrong, good catch! > > >=20 > > > If there was an s_to_ktime(), I would have kept the old name, but I c= ould > > > not find one. Possibly due to me being blind... > >=20 > > I used "ktime_set(ssecs, 0)", which is almost what you want. Given that > > the majority of users of ktime_set() actually pass a zero nanoseconds p= ortion, > > it would probably be nice to add secs_to_ktime() as well. >=20 > And there are quite a few that pass in zero for the seconds portion, and > many that pass in zero for both. >=20 > For the moment, I switched to ktime_set(), as you suggested. >=20 > > > > > @@ -511,10 +513,10 @@ int torture_shutdown_init(int ssecs, void (= *cleanup)(void)) > > > > > { > > > > > int ret =3D 0; > > > > > =20 > > > > > - shutdown_secs =3D ssecs; > > > > > torture_shutdown_hook =3D cleanup; > > > > > - if (shutdown_secs > 0) { > > > > > - shutdown_time =3D jiffies + shutdown_secs * HZ; > > > > > + if (ssecs > 0) { > > > > > + shutdown_ms =3D ms_to_ktime(ssecs * 1000ULL); > > > > > + shutdown_time =3D ktime_add(ktime_get(), shutdown_ms); > > > > > ret =3D torture_create_kthread(torture_shutdown, NULL, > > > > > shutdown_task); > > > >=20 > > > > and I picked ktime_set(ssecs, 0) instead of ms_to_ktime(ssecs * 100= 0ULL), but > > > > both differences are just cosmetic and should end up in exactly the > > > > same object code that I suggested. Unless we both made the same mis= take, > > > > your version should be good too. > > >=20 > > > Thank you for looking it over! My I apply your Acked-by:, Reviewed-b= y:, > > > or some such? > >=20 > > I had not looked over the original changes before, but have done that n= ow, > > please add my=20 > >=20 > > Acked-by: Arnd Bergmann >=20 > Thank you! >=20 > > I found one small detail that you could change: instead of using > > HRTIMER_MODE_REL, you could actually use HRTIME_MODE_ABS and just > > pass the end time instead of computing the difference every time. > >=20 > > You still need to take the difference for printing, but you could > > do that in place then: > >=20 > > if (verbose) > > pr_alert("%s" TORTURE_FLAG > > "torture_shutdown task: %llu ms remaining\n", > > torture_type, ktime_ms_delta(shutdown_time, k= time_get())); >=20 > Good point! I did this, but used ktime_snap instead of ktime_get(). > I have to capture ktime_snap anyway for the loop termination condition. > Updated commit below. >=20 > Thanx, Paul >=20 > ------------------------------------------------------------------------ >=20 > commit 0a387a5b8718feace7aadd847937fdc336567a36 > Author: Paul E. McKenney > Date: Sat Jun 18 07:45:43 2016 -0700 >=20 > torture: Convert torture_shutdown() to hrtimer > =20 > Upcoming changes to the timer wheel introduce significant inaccuracy > and possibly also an ultimate limit on timeout duration. This is > a problem for the current implementation of torture_shutdown() because > (1) shutdown times are user-specified, and can therefore be quite > long, and (2) the torture scripting will kill a test instance that > runs for more than a few minutes longer than scheduled. This commit > therefore converts the torture_shutdown() timed waits to an hrtimer. > =20 > Signed-off-by: Paul E. McKenney > Acked-by: Arnd Bergmann >=20 > diff --git a/kernel/torture.c b/kernel/torture.c > index 75961b3decfe..08f45621394a 100644 > --- a/kernel/torture.c > +++ b/kernel/torture.c > @@ -43,6 +43,7 @@ > #include > #include > #include > +#include > #include > #include > =20 > @@ -446,9 +447,9 @@ EXPORT_SYMBOL_GPL(torture_shuffle_cleanup); > * Variables for auto-shutdown. This allows "lights out" torture runs > * to be fully scripted. > */ > -static int shutdown_secs; /* desired test duration in seconds. */ > +static ktime_t shutdown_secs; /* desired test duration in seconds. */ > static struct task_struct *shutdown_task; > -static unsigned long shutdown_time; /* jiffies to system shutdown. */ > +static ktime_t shutdown_time; /* jiffies to system shutdown. */ /* time to system shutdown */ ? > static void (*torture_shutdown_hook)(void); > =20 > /* > @@ -471,20 +472,20 @@ EXPORT_SYMBOL_GPL(torture_shutdown_absorb); > */ > static int torture_shutdown(void *arg) > { > - long delta; > - unsigned long jiffies_snap; > + ktime_t ktime_snap; > =20 > VERBOSE_TOROUT_STRING("torture_shutdown task started"); > - jiffies_snap =3D jiffies; > - while (ULONG_CMP_LT(jiffies_snap, shutdown_time) && > + ktime_snap =3D ktime_get(); > + while (ktime_before(ktime_snap, shutdown_time) && > !torture_must_stop()) { > - delta =3D shutdown_time - jiffies_snap; > if (verbose) > pr_alert("%s" TORTURE_FLAG > - "torture_shutdown task: %lu jiffies remaining\n", > - torture_type, delta); > - schedule_timeout_interruptible(delta); > - jiffies_snap =3D jiffies; > + "torture_shutdown task: %llu ms remaining\n", > + torture_type, > + ktime_ms_delta(shutdown_time, ktime_snap)); > + set_current_state(TASK_INTERRUPTIBLE); > + schedule_hrtimeout(&shutdown_time, HRTIMER_MODE_ABS); > + ktime_snap =3D ktime_get(); > } > if (torture_must_stop()) { > torture_kthread_stopping("torture_shutdown"); > @@ -511,10 +512,10 @@ int torture_shutdown_init(int ssecs, void (*cleanup= )(void)) > { > int ret =3D 0; > =20 > - shutdown_secs =3D ssecs; > torture_shutdown_hook =3D cleanup; > - if (shutdown_secs > 0) { > - shutdown_time =3D jiffies + shutdown_secs * HZ; > + if (ssecs > 0) { > + shutdown_secs =3D ktime_set(ssecs, 0); > + shutdown_time =3D ktime_add(ktime_get(), shutdown_secs); As the 'shutdown_secs' here is only for 'shutdown_time' calculation, is it better that we kill 'shutdown_secs' entirely and calculate 'shutdown_time' via: shutdown_time =3D ktime_add(ktime_get(), ktime_set(ssecs, 0));=20 ? (I found it a little weird that we don't have ktime_add_sec(), because we already have ktime_add_{n,m,u}s()) Regards, Boqun > ret =3D torture_create_kthread(torture_shutdown, NULL, > shutdown_task); > } >=20 --vtzGhvizbBRQ85DL Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQEcBAABCAAGBQJXafEzAAoJEEl56MO1B/q4ALgH/2xfq2bdvzuc3u7F4yaXTSbD ABfYWFOwPXT4cp2D9bRsuwQIVaGiQs5af9gcp7zcoZcDqyPG4qFAFKHvyWYNsdde k1FelAh36/nZnhz/cEfnyuswej9OoDjELuanlPuFz4rc4M97FvDaz+7eKDfhldT4 vp5zOtz4ti8lkBwoPeGUgmc1WaMtwTFFEMQQrW6fKZYPp3kGmjh6gqOlWH3f85Vi T2+gNVGXL3cvdat0DyGiVra7VR8we72Xgr3CivMfwUGH0BrPvh4APVlo3RySO4sE Q8EFBuff89rknt70Qvq5fVxrORaWiqaweHNTcP1ZXwPstsyFdIvMbJShmLUFTGo= =JKZK -----END PGP SIGNATURE----- --vtzGhvizbBRQ85DL--