From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1948515AbdEZSEb (ORCPT ); Fri, 26 May 2017 14:04:31 -0400 Received: from mail-eopbgr40058.outbound.protection.outlook.com ([40.107.4.58]:42272 "EHLO EUR03-DB5-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1753660AbdEZSEW (ORCPT ); Fri, 26 May 2017 14:04:22 -0400 From: Octavian Purdila To: "fweisbec@gmail.com" CC: "tglx@linutronix.de" , "linux-kernel@vger.kernel.org" , "peterz@infradead.org" , Leonard Crestez Subject: Re: [RFC] tick/nohz: schedule TIMER_SOFTIRQ immediately for expired timers Thread-Topic: [RFC] tick/nohz: schedule TIMER_SOFTIRQ immediately for expired timers Thread-Index: AQHS1IrSrfV3m85GZ0GROushBrDJSqIGlZKAgABWUgA= Date: Fri, 26 May 2017 18:04:17 +0000 Message-ID: <1495821856.7100.4.camel@nxp.com> References: <1495629564-28904-1-git-send-email-octavian.purdila@nxp.com> <20170526125517.GA29833@lerouge> In-Reply-To: <20170526125517.GA29833@lerouge> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: authentication-results: gmail.com; dkim=none (message not signed) header.d=none;gmail.com; dmarc=none action=none header.from=nxp.com; x-originating-ip: [192.88.146.1] x-ms-publictraffictype: Email x-microsoft-exchange-diagnostics: 1;DBXPR04MB350;7:shMZDMYdc1EZrWtqPoLFtsVGLpP6J0DFVzbbnd9+6Fz/XOUY22ocGuxg1cBB9aNdlbO6eMri6u+k84GVsIa0e9mOAnzDrQ0vEGGGdqWOMVytHQkEYcDB/vmLYvHTd5i6SPbAhJk4BHCPvJZ2KaMsPZ7FdhW3MM3NaZBL64UbDME/kJB9+oFh76fx4GHuFTsd0xuNFsSqHmwB+RYhM1AmovTNpjUW7lu50nkgqz11xUQtXG21FkSO5cvSaUYpjQ8JcPWO2uR8TWKDX6D4dtocGM/eT/fgELnhu0/mme5gpX7amPH0mTwzGIR2HIJ6BFvsCbVb+uqS0wbYZK10IvqpkQ== x-forefront-antispam-report: SFV:SKI;SCL:-1SFV:NSPM;SFS:(10009020)(6009001)(39850400002)(39410400002)(39400400002)(39860400002)(39840400002)(39450400003)(377424004)(24454002)(51914003)(81166006)(2501003)(2900100001)(5660300001)(25786009)(1730700003)(53936002)(5640700003)(54906002)(6506006)(110136004)(229853002)(99286003)(103116003)(39060400002)(36756003)(4326008)(66066001)(33646002)(1411001)(6486002)(6246003)(8676002)(38730400002)(6512007)(2351001)(76176999)(102836003)(5250100002)(3846002)(2950100002)(6116002)(478600001)(3660700001)(3280700002)(54356999)(50986999)(7736002)(2906002)(14454004)(189998001)(6436002)(8936002)(6916009)(86362001)(305945005)(14773001);DIR:OUT;SFP:1101;SCL:1;SRVR:DBXPR04MB350;H:VI1PR04MB1406.eurprd04.prod.outlook.com;FPR:;SPF:None;MLV:ovrnspm;PTR:InfoNoRecords;LANG:en; x-ms-traffictypediagnostic: DBXPR04MB350: x-ms-office365-filtering-correlation-id: 2d365d0f-b932-4056-9b5d-08d4a461a043 x-ms-office365-filtering-ht: Tenant x-microsoft-antispam: UriScan:;BCL:0;PCL:0;RULEID:(22001)(2017030254075)(48565401081)(201703131423075)(201703031133081)(201702281549075);SRVR:DBXPR04MB350; x-microsoft-antispam-prvs: x-exchange-antispam-report-test: UriScan:(185117386973197); x-exchange-antispam-report-cfa-test: BCL:0;PCL:0;RULEID:(6040450)(601004)(2401047)(8121501046)(5005006)(10201501046)(93006095)(93001095)(3002001)(6055026)(6041248)(20161123562025)(20161123560025)(20161123555025)(201703131423075)(201702281528075)(201703061421075)(201703061406153)(20161123564025)(20161123558100)(6072148);SRVR:DBXPR04MB350;BCL:0;PCL:0;RULEID:;SRVR:DBXPR04MB350; x-forefront-prvs: 031996B7EF spamdiagnosticoutput: 1:99 spamdiagnosticmetadata: NSPM Content-Type: text/plain; charset="utf-8" Content-ID: <9FF9EDD8A89055469CB87FA44E97D38D@eurprd04.prod.outlook.com> MIME-Version: 1.0 X-OriginatorOrg: nxp.com X-MS-Exchange-CrossTenant-originalarrivaltime: 26 May 2017 18:04:17.2459 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635 X-MS-Exchange-Transport-CrossTenantHeadersStamped: DBXPR04MB350 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from base64 to 8bit by mail.home.local id v4QI4ZjB017040 On Vi, 2017-05-26 at 14:55 +0200, Frederic Weisbecker wrote: > On Wed, May 24, 2017 at 03:39:24PM +0300, Octavian Purdila wrote: > > > > Currently, when detecting expired timers in > > tick_nohz_stop_sched_tick > > we just schedule a new hrtimer and let its handler to schedule > > TIMER_SOFITRQ. > > > > This can lead to indefinite timer stalls if the system is busy with > > a > > stream of interrupts that have the period shorter or about the same > > with the value of the minimum delay of the current clocksource > > driver: > > > > -> idle > > -> IRQ (time = x) > >    -> irq_exit -> tick_nohz_irq_exit -> tick_nohz_stop_sched_tick > >       -> tick_nohz_restart > >          -> cancel hrtimer (set clocksource event to x + max_delay) > >          -> set clocksource to x + min_delay > > ... > > -> IRQ (time = y, where y < x + min_delay) > >    -> irq_exit -> tick_nohz_irq_exit -> tick_nohz_stop_sched_tick > >       -> tick_nohz_restart > >          -> cancel hrtimer (set clocksource event to x + max_delay) > >          -> set clocksource to y + min_delay > > > > So, instead of prodding the hrtimer interrupt, schedule > > TIMER_SOFTIRQ > > since we know that timers are ready. The timers will run either at > > the > > next interrupt or from ksoftirq so no hrtimer interrupt is > > needed. This also avoids spurious programming of the clocksource in > > this scenario. > > > > Signed-off-by: Octavian Purdila > > --- > >  kernel/time/tick-sched.c | 9 ++++++++- > >  1 file changed, 8 insertions(+), 1 deletion(-) > > > > diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c > > index 3bcb61b..0a30278 100644 > > --- a/kernel/time/tick-sched.c > > +++ b/kernel/time/tick-sched.c > > @@ -727,9 +727,16 @@ static ktime_t > > tick_nohz_stop_sched_tick(struct tick_sched *ts, > >    * > >    * Only once we exit the idle loop will we re- > > enable the tick, > >    * see tick_nohz_idle_exit(). > > +  * > > +  * Also, make sure we schedule TIMER_SOFTIRQ now > > instead of > > +  * relying on the hrtimer interrupt to do it to > > avoid > > +  * postponing processing of expired timers. If we > > have a > > +  * constant stream of interrupts with a period > > shorter than > > +  * the minimum delay of the current clocksource we > > can end up > > +  * postponing the timers indefinitely. > >    */ > >   if (delta == 0) { > > - tick_nohz_restart(ts, now); > > + raise_softirq(TIMER_SOFTIRQ); > >   goto out; > >   } > >   } > Nice catch! And the patch should work but that actually restores a > behaviour we've > removed some time ago. We wanted to get rid of that softirq raise. > > So discussing this with Thomas, here is an alternate solution. That > tick restart looks > like an unnecessary special case. In fact the normal path ending with > hrtimer_start() > in tick_nohz_stop_sched_tick() should work if the delta deadline is > 0. And if > we do so, we benefit from the optimization path: > >         if (ts->tick_stopped && (expires == dev->next_event)) > > ...which avoids the cancel/reprog game and therefore should fix that > issue. > > diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c > index ed18ca5..58c257c 100644 > --- a/kernel/time/tick-sched.c > +++ b/kernel/time/tick-sched.c > @@ -713,8 +713,6 @@ static ktime_t tick_nohz_stop_sched_tick(struct > tick_sched *ts, >    */ >   delta = next_tick - basemono; >   if (delta <= (u64)TICK_NSEC) { > - tick = 0; > - >   /* >    * Tell the timer code that the base is not idle, > i.e. undo >    * the effect of get_next_timer_interrupt(): > @@ -724,23 +722,8 @@ static ktime_t tick_nohz_stop_sched_tick(struct > tick_sched *ts, >    * We've not stopped the tick yet, and there's a > timer in the >    * next period, so no point in stopping it either, > bail. >    */ > - if (!ts->tick_stopped) > - goto out; > - > - /* > -  * If, OTOH, we did stop it, but there's a pending > (expired) > -  * timer reprogram the timer hardware to fire now. > -  * > -  * We will not restart the tick proper, just prod > the timer > -  * hardware into firing an interrupt to process the > pending > -  * timers. Just like tick_irq_exit() will not > restart the tick > -  * for 'normal' interrupts. > -  * > -  * Only once we exit the idle loop will we re-enable > the tick, > -  * see tick_nohz_idle_exit(). > -  */ > - if (delta == 0) { > - tick_nohz_restart(ts, now); > + if (!ts->tick_stopped) { > + tick = 0; >   goto out; >   } >   } > Nice, it is less expensive and deletes some code :-) Thanks for the quick fix Frederic, I confirm it solves my issue. Tested-by: Octavian Purdila