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: > > > > > > > > > > @@ -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_ms; /* desired test duration in > > > > > seconds. */ > > > > > > > > the variable name is a bit odd. > > > > > > The comment is certainly now wrong, good catch! > > > > > > If there was an s_to_ktime(), I would have kept the old name, but I could > > > not find one. Possibly due to me being blind... > > > > 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 > > portion, > > it would probably be nice to add secs_to_ktime() as well. > > And there are quite a few that pass in zero for the seconds portion, and > many that pass in zero for both. > > For the moment, I switched to ktime_set(), as you suggested. > > > > > > @@ -511,10 +513,10 @@ int torture_shutdown_init(int ssecs, void > > > > > (*cleanup)(void)) > > > > > { > > > > > int ret = 0; > > > > > > > > > > - shutdown_secs = ssecs; > > > > > torture_shutdown_hook = cleanup; > > > > > - if (shutdown_secs > 0) { > > > > > - shutdown_time = jiffies + shutdown_secs * HZ; > > > > > + if (ssecs > 0) { > > > > > + shutdown_ms = ms_to_ktime(ssecs * 1000ULL); > > > > > + shutdown_time = ktime_add(ktime_get(), shutdown_ms); > > > > > ret = torture_create_kthread(torture_shutdown, NULL, > > > > > shutdown_task); > > > > > > > > and I picked ktime_set(ssecs, 0) instead of ms_to_ktime(ssecs * > > > > 1000ULL), 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 mistake, > > > > your version should be good too. > > > > > > Thank you for looking it over! My I apply your Acked-by:, Reviewed-by:, > > > or some such? > > > > I had not looked over the original changes before, but have done that now, > > please add my > > > > Acked-by: Arnd Bergmann <a...@arndb.de> > > Thank you! > > > 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. > > > > You still need to take the difference for printing, but you could > > do that in place then: > > > > if (verbose) > > pr_alert("%s" TORTURE_FLAG > > "torture_shutdown task: %llu ms remaining\n", > > torture_type, ktime_ms_delta(shutdown_time, > > ktime_get())); > > 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. > > Thanx, Paul > > ------------------------------------------------------------------------ > > commit 0a387a5b8718feace7aadd847937fdc336567a36 > Author: Paul E. McKenney <paul...@linux.vnet.ibm.com> > Date: Sat Jun 18 07:45:43 2016 -0700 > > torture: Convert torture_shutdown() to hrtimer > > 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. > > Signed-off-by: Paul E. McKenney <paul...@linux.vnet.ibm.com> > Acked-by: Arnd Bergmann <a...@arndb.de> > > 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 <linux/stat.h> > #include <linux/slab.h> > #include <linux/trace_clock.h> > +#include <linux/ktime.h> > #include <asm/byteorder.h> > #include <linux/torture.h> > > @@ -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); > > /* > @@ -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; > > VERBOSE_TOROUT_STRING("torture_shutdown task started"); > - jiffies_snap = jiffies; > - while (ULONG_CMP_LT(jiffies_snap, shutdown_time) && > + ktime_snap = ktime_get(); > + while (ktime_before(ktime_snap, shutdown_time) && > !torture_must_stop()) { > - delta = 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 = 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 = 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 = 0; > > - shutdown_secs = ssecs; > torture_shutdown_hook = cleanup; > - if (shutdown_secs > 0) { > - shutdown_time = jiffies + shutdown_secs * HZ; > + if (ssecs > 0) { > + shutdown_secs = ktime_set(ssecs, 0); > + shutdown_time = 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 = ktime_add(ktime_get(), ktime_set(ssecs, 0)); ? (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 = torture_create_kthread(torture_shutdown, NULL, > shutdown_task); > } >
signature.asc
Description: PGP signature