[PATCH] torture: use ktime_t consistently

[PATCH] torture: use ktime_t consistently

[Arnd Bergmann]

A recent change accidentally introduced a 64-bit division in torture_shutdown,
which fails to build on 32-bit architectures:

kernel/built-in.o: In function `torture_shutdown':
:(.text+0x4b29a): undefined reference to `__aeabi_uldivmod'

This converts the function to use ktime_t instead, which also simplifies
it a little.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Fixes: b4aa201e0c7c ("torture: Convert torture_shutdown() to hrtimer")
---
 kernel/torture.c | 25 ++++++++++---------------
 1 file changed, 10 insertions(+), 15 deletions(-)

diff --git a/kernel/torture.c b/kernel/torture.c
index 55f0a317fbb5..bbcc92088bf9 100644
--- a/kernel/torture.c
+++ b/kernel/torture.c
@@ -448,7 +448,7 @@ EXPORT_SYMBOL_GPL(torture_shuffle_cleanup);
  */
 static int 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;		/* time of system shutdown. */
 static void (*torture_shutdown_hook)(void);
 
 /*
@@ -471,24 +471,19 @@ EXPORT_SYMBOL_GPL(torture_shutdown_absorb);
  */
 static int torture_shutdown(void *arg)
 {
-	unsigned long long delta;
-	unsigned long jiffies_snap;
-	ktime_t wait_duration;
+	ktime_t now, delta;
 
 	VERBOSE_TOROUT_STRING("torture_shutdown task started");
-	jiffies_snap = jiffies;
-	while (ULONG_CMP_LT(jiffies_snap, shutdown_time) &&
-	       !torture_must_stop()) {
-		delta = shutdown_time - jiffies_snap;
+	now = ktime_get();
+	while (ktime_before(now, shutdown_time) && !torture_must_stop()) {
+		delta = ktime_sub(shutdown_time, now);
 		if (verbose)
 			pr_alert("%s" TORTURE_FLAG
-				 "torture_shutdown task: %llu jiffies remaining\n",
-				 torture_type, delta);
-		delta = delta * 1000ULL * 1000ULL * 1000ULL / HZ;
-		wait_duration = ns_to_ktime(delta);
+				 "torture_shutdown task: %llu ms remaining\n",
+				 torture_type, ktime_to_ms(delta));
 		set_current_state(TASK_INTERRUPTIBLE);
-		schedule_hrtimeout(&wait_duration, HRTIMER_MODE_REL);
-		jiffies_snap = jiffies;
+		schedule_hrtimeout(&delta, HRTIMER_MODE_REL);
+		now = ktime_get();
 	}
 	if (torture_must_stop()) {
 		torture_kthread_stopping("torture_shutdown");
@@ -518,7 +513,7 @@ int torture_shutdown_init(int ssecs, void (*cleanup)(void))
 	shutdown_secs = ssecs;
 	torture_shutdown_hook = cleanup;
 	if (shutdown_secs > 0) {
-		shutdown_time = jiffies + shutdown_secs * HZ;
+		shutdown_time = ktime_add(ktime_get(), ktime_set(ssecs, 0));
 		ret = torture_create_kthread(torture_shutdown, NULL,
 					     shutdown_task);
 	}
-- 
2.9.0

Re: [PATCH] torture: use ktime_t consistently

[Paul E. McKenney]

On Mon, Jun 20, 2016 at 05:56:40PM +0200, Arnd Bergmann wrote:
> A recent change accidentally introduced a 64-bit division in torture_shutdown,
> which fails to build on 32-bit architectures:
> 
> kernel/built-in.o: In function `torture_shutdown':
> :(.text+0x4b29a): undefined reference to `__aeabi_uldivmod'
> 
> This converts the function to use ktime_t instead, which also simplifies
> it a little.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> Fixes: b4aa201e0c7c ("torture: Convert torture_shutdown() to hrtimer")

Thank you, Arnd!

I ended up squashing in the alternative change below before I saw your
email address.  This is currently untested, so I might well end up using
yours instead should breakage ensue.  Given that I haven't done much
with ktime_t, breakage is rather likely.

Thoughts?

							Thanx, Paul

------------------------------------------------------------------------

commit 303c4c9474e84112ab2474cd383282f5dbfc75c3
Author: Paul E. McKenney <paulmck@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 <paulmck@linux.vnet.ibm.com>

diff --git a/kernel/torture.c b/kernel/torture.c
index 75961b3decfe..1c1b5960d27f 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_ms;		/* 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. */
 static void (*torture_shutdown_hook)(void);
 
 /*
@@ -471,20 +472,21 @@ EXPORT_SYMBOL_GPL(torture_shutdown_absorb);
  */
 static int torture_shutdown(void *arg)
 {
-	long delta;
-	unsigned long jiffies_snap;
+	ktime_t delta;
+	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;
+		delta = ktime_sub(shutdown_time, ktime_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_to_ms(delta));
+		set_current_state(TASK_INTERRUPTIBLE);
+		schedule_hrtimeout(&delta, HRTIMER_MODE_REL);
+		ktime_snap = ktime_get();
 	}
 	if (torture_must_stop()) {
 		torture_kthread_stopping("torture_shutdown");
@@ -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);
 	}

Re: [PATCH] torture: use ktime_t consistently

[Arnd Bergmann]

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.

> @@ -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.

Thanks,

	Arnd

Re: [PATCH] torture: use ktime_t consistently

[Paul E. McKenney]

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...

> > @@ -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?

							Thanx, Paul

Re: [PATCH] torture: use ktime_t consistently

[Arnd Bergmann]

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.

> > > @@ -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 <arnd@arndb.de>

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()));

	Arnd

Re: [PATCH] torture: use ktime_t consistently

[Paul E. McKenney]

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 <arnd@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 <paulmck@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 <paulmck@linux.vnet.ibm.com>
    Acked-by: Arnd Bergmann <arnd@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. */
 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);
 		ret = torture_create_kthread(torture_shutdown, NULL,
 					     shutdown_task);
 	}

Re: [PATCH] torture: use ktime_t consistently

[Boqun Feng]

[-- Attachment #1: Type: text/plain, Size: 7312 bytes --]

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 <arnd@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 <paulmck@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 <paulmck@linux.vnet.ibm.com>
>     Acked-by: Arnd Bergmann <arnd@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);
>  	}
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH] torture: use ktime_t consistently

[Paul E. McKenney]

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 <arnd@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 <paulmck@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 <paulmck@linux.vnet.ibm.com>
> >     Acked-by: Arnd Bergmann <arnd@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);
> >  	}
> >