From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751350AbcFVFNE (ORCPT ); Wed, 22 Jun 2016 01:13:04 -0400 Received: from mx0a-001b2d01.pphosted.com ([148.163.156.1]:41882 "EHLO mx0a-001b2d01.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750953AbcFVFND (ORCPT ); Wed, 22 Jun 2016 01:13:03 -0400 X-IBM-Helo: d03dlp02.boulder.ibm.com X-IBM-MailFrom: paulmck@linux.vnet.ibm.com Date: Tue, 21 Jun 2016 20:27:24 -0700 From: "Paul E. McKenney" To: Boqun Feng Cc: Arnd Bergmann , Josh Triplett , linux-kernel@vger.kernel.org Subject: Re: [PATCH] torture: use ktime_t consistently Reply-To: paulmck@linux.vnet.ibm.com 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> <20160622020024.GA25785@insomnia> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160622020024.GA25785@insomnia> User-Agent: Mutt/1.5.21 (2010-09-15) X-TM-AS-MML: disable X-Content-Scanned: Fidelis XPS MAILER x-cbid: 16062203-0028-0000-0000-000005029291 X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 16062203-0029-0000-0000-00002CE01C0A Message-Id: <20160622032724.GF3923@linux.vnet.ibm.com> X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2016-06-22_02:,, signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 spamscore=0 suspectscore=0 malwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1604210000 definitions=main-1606220035 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > > > > 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 > > 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 > > Acked-by: Arnd Bergmann > > > > 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 > > > > @@ -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); > > } > >