On Wed, Jun 22, 2016 at 10:00:24AM +0800, Boqun Feng wrote: > 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 */ ?
Good eyes, fixed! > > 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)); > > ? Good point, adjusted and removed shutdown_secs. > (I found it a little weird that we don't have ktime_add_sec(), because > we already have ktime_add_{n,m,u}s()) There does seem to be an odd set of primitives! Thanx, Paul > Regards, > Boqun > > > ret = torture_create_kthread(torture_shutdown, NULL, > > shutdown_task); > > } > >