From patchwork Thu Nov 16 22:47:45 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Doug Smythies X-Patchwork-Id: 10062163 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id E7F976023A for ; Thu, 16 Nov 2017 22:47:55 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id D8FED2AC41 for ; Thu, 16 Nov 2017 22:47:55 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id CC7C42AC43; Thu, 16 Nov 2017 22:47:55 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.0 required=2.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 61EF12AC41 for ; Thu, 16 Nov 2017 22:47:54 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S965787AbdKPWrx (ORCPT ); Thu, 16 Nov 2017 17:47:53 -0500 Received: from cmta18.telus.net ([209.171.16.91]:45299 "EHLO cmta18.telus.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S965567AbdKPWrw (ORCPT ); Thu, 16 Nov 2017 17:47:52 -0500 Received: from dougxps ([173.180.45.4]) by cmsmtp with SMTP id FSwle2H0TCR8bFSwmeQGzF; Thu, 16 Nov 2017 15:47:51 -0700 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=telus.net; s=neo; t=1510872471; bh=WtHiFLbPSkRCGbdZEKiNcCyS0ibzWG4CWHh3KrCvdhw=; h=From:To:Cc:References:In-Reply-To:Subject:Date; b=nkDSLPLTio8KHd1MrWjEQgAqSg5K4ZkTfv3Q6+wu+o8H2UlkY85nswx4nZd7kBOkl w030RR12pdPPHEtYt2hMGV3lugNGDEJieAVb45fX7diwF/RpCEpHpfRkIRxrR7QLXF Dz6uUEOPEmjlA4mGOGzPkey/IQZQV6+lzjSzojLlouikOYdKaxY2ji6sKsM7M5R/Uc VJJcglI7x1qTG6Swxm0hualPnEpgg7qPbvq4PMdVSUxnq7HaVpRRhtkwBGvo8/1ER0 bCQXH6hIY7hxWCxF0pjBvm34jpNBs3fuAUw48ANSMuD5ztqnWI3xlwuPgQ29EqOBfy MJw5D4OFVO9dA== X-Authority-Analysis: v=2.2 cv=RowqFGuK c=1 sm=1 tr=0 a=zJWegnE7BH9C0Gl4FFgQyA==:117 a=zJWegnE7BH9C0Gl4FFgQyA==:17 a=Pyq9K9CWowscuQLKlpiwfMBGOR0=:19 a=IkcTkHD0fZMA:10 a=eBDBtaPHDDHYOblggxkA:9 a=QEXdDO2ut3YA:10 From: "Doug Smythies" To: "'Thomas Ilsche'" Cc: =?UTF-8?Q?'Marcus_H=C3=A4hnel'?= , "'Daniel Hackenberg'" , =?UTF-8?Q?'Robert_Sch=C3=B6ne'?= , , "'Rafael J. Wysocki'" , "'Alex Shi'" , "'Ingo Molnar'" , "'Rik van Riel'" , "'Daniel Lezcano'" , "'Nicholas Piggin'" , , "'Len Brown'" , "'Yu Chen'" , "Doug Smythies" References: <000101d34938$da740870$8f5c1950$@net> <000801d34a78$cdd27890$697769b0$@net> <002c01d35d0f$8b0416f0$a10c44d0$@net> FMl8e97ZchCDuFMlDezTwJ In-Reply-To: FMl8e97ZchCDuFMlDezTwJ Subject: RE: [PATCH] cpuidle: Allow menu governor to enter deeper sleep states after some time Date: Thu, 16 Nov 2017 14:47:45 -0800 Message-ID: <001f01d35f2c$edc11490$c9433db0$@net> MIME-Version: 1.0 X-Mailer: Microsoft Office Outlook 12.0 Content-Language: en-ca Thread-Index: AdNe9ZE014aT3AWnTaKHVCaBD1kzGQAMWxrA X-CMAE-Envelope: MS4wfInJmN93252Sj1h94iEYPXG5S8EuvFXUPxcgawQ1xbg8eVII1efE9zf4NzbzOcTFytIWA+LdeshNusek7l5Se6cf5MXAmkeRyOt5aJFmSjtKp5GYqEFX VHzV42yNRRaV7JqqWlreZX3qk3kSv/tTdgJWHc/w0vxM9LRvZIOgc5o+Sh5chOlBiMrS95GJbid5+hn+rV0EfT7XbudgCvFOD4c+OtU+sfH3WWHVTVa+gGS3 3fOkPoHbtnh0iy4hQz18eQOc9emlD75H4Ilv0K768YzabTawliSiFzhgmyaBiIo7NYk7QpglJKCRsYa4/Cg4W4ibOCPoD6RodgYnyKT6jouvGUEqhCCCYJXo 5KaFv+mu5+9E4XwbCef0A5l59UzucvhNG3hY6X7bHRJRPoVXaMZhrFDV6fpTCJietmFL2IlKsYB2mYiUgEU/kT9hasIl4gijFDNfvAwAgN1czzcybBT5gCw+ mKb+XrUjMpx88TQ1AYuBB/xY0MmEyIWVi7TeHCK8qDHLgA8g1vY/J1IZgRAgKEhZfqmTg7nuQ4WLWPLRGTupc2wkmfgRA94iEefLAQ2qcZnKnDfZ2dDgBp0u Cx4qsh71bdtrzrwiSHvWyDuoJpwcMmuDr331GCjCTNEZBYBDcy2pqBHifJ6Fz0Kb+fo= Sender: linux-pm-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-pm@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On 2017.11.16 08:11 Thomas Ilsche wrote: >> Actually, the watchdog_timer_fn does set the "need_resched" condition, and will >> cause the state 0 idle to exit normally. >> >> But yes, tick_sched_timer and a few others (for example: sched_rt_period_timer, >> clocksource_watchdog) do not set the "need_resched" condition, and, as you >> mentioned, will not cause the state 0 idle to exit as it should. >> >> Conclusion: Currently the exit condition in drivers/cpuidle/poll_state.c >> is insufficient to guarantee proper operation. Or: Any interrupt out of the idle loop must return with "need_resched" >> >> This: >> >> while (!need_resched()) >> >> is not enough. > > I may very well have mistakenly included watchdog_timer_fn in the list, > but as you describe it is inconsequential. If there are timers that do > not set need_resched, and that itself is not considered a bug, then > there should be another break condition. > I suppose it is a good idea > to differentiate between the need for rescheduling and the need to > be able to go in another sleep state. See patch below. I think both conditions are satisfied. > What do you think about the idea to use idle_expires? > Although on second thought that may have issues regarding accuracy / > race conditions with the interrupt timer. For a couples of days now, and with excellent results, I have been testing variations on the following theme: Trace example 1: 9 [005] d... 1749.232242: cpu_idle: state=4 cpu_id=5 1055985 [005] d... 1750.288228: cpu_idle: state=4294967295 cpu_id=5 3 [005] d.h. 1750.288231: local_timer_entry: vector=239 1 [005] d.h. 1750.288233: local_timer_exit: vector=239 5 [005] d... 1750.288238: cpu_idle: state=0 cpu_id=5 0 [005] d.h. 1750.288238: local_timer_entry: vector=239 0 [005] d.h. 1750.288239: hrtimer_expire_entry: hrtimer=ffff91ca5f354880 function=tick_sched_timer now=1749980002791 3 [005] d.h. 1750.288242: hrtimer_expire_exit: hrtimer=ffff91ca5f354880 0 [005] d.h. 1750.288243: local_timer_exit: vector=239 1 [005] ..s. 1750.288244: timer_expire_entry: timer=ffffffffb4770ee0 function=__prandom_timer now=4295329792 4 [005] ..s. 1750.288249: timer_expire_exit: timer=ffffffffb4770ee0 5 [005] .... 1750.288254: cpu_idle: state=4294967295 cpu_id=5 "need_resched" is not set, but the next timer is far off, so poll_state.c with the above patch now exits. And properly now decides to go into idle state 4, because nothing is going to happen for an eternity. 1 [005] d... 1750.288256: cpu_idle: state=4 cpu_id=5 2087982 [005] d... 1752.376239: cpu_idle: state=4294967295 cpu_id=5 3 [005] d.h. 1752.376242: local_timer_entry: vector=239 0 [005] d.h. 1752.376243: local_timer_exit: vector=239 5 [005] d... 1752.376248: cpu_idle: state=1 cpu_id=5 15 [005] d... 1752.376263: cpu_idle: state=4294967295 cpu_id=5 0 [005] d.h. 1752.376263: local_timer_entry: vector=239 0 [005] d.h. 1752.376264: hrtimer_expire_entry: hrtimer=ffff91ca5f354a00 function=watchdog_timer_fn now=1752068001621 3 [005] dNh. 1752.376268: hrtimer_expire_exit: hrtimer=ffff91ca5f354a00 Trace example 2: 4 [000] d... 1792.272757: cpu_idle: state=0 cpu_id=0 1 [000] d.h. 1792.272758: local_timer_entry: vector=239 0 [000] d.h. 1792.272759: hrtimer_expire_entry: hrtimer=ffff91ca5f214880 function=tick_sched_timer now=1791964002768 3 [000] d.h. 1792.272762: hrtimer_expire_exit: hrtimer=ffff91ca5f214880 0 [000] d.h. 1792.272762: local_timer_exit: vector=239 The next timer is very short, so the poll_state.c loop does not exit. (even if it was going to exit, it might not have had time to. I didn't find a better example.) 0 [000] ..s. 1792.272763: timer_expire_entry: timer=ffff91ca4cde8478 function=dev_watchdog now=4295340288 3 [000] ..s. 1792.272766: timer_expire_exit: timer=ffff91ca4cde8478 The next timer is very short, so the poll_state.c loop does not exit. 0 [000] d.s. 1792.272767: timer_expire_entry: timer=ffffffffc0997440 function=delayed_work_timer_fn now=4295340288 5 [000] dNs. 1792.272772: timer_expire_exit: timer=ffffffffc0997440 This time "need_resched" is set. I assume it didn't have time to exit idle state 0 yet. 0 [000] dNs. 1792.272772: timer_expire_entry: timer=ffffffffb46faa40 function=delayed_work_timer_fn now=4295340288 0 [000] dNs. 1792.272773: timer_expire_exit: timer=ffffffffb46faa40 Now it exits idle state 0. 7 [000] .N.. 1792.272780: cpu_idle: state=4294967295 cpu_id=0 29 [000] d... 1792.272810: cpu_idle: state=4 cpu_id=0 And properly now decides to go into idle state 4, because nothing is going to happen for awhile. 91949 [000] d... 1792.364760: cpu_idle: state=4294967295 cpu_id=0 3 [000] d.h. 1792.364763: local_timer_entry: vector=239 0 [000] d.h. 1792.364764: hrtimer_expire_entry: hrtimer=ffff91ca5f214a00 function=watchdog_timer_fn now=1792056006926 ... Doug diff --git a/drivers/cpuidle/poll_state.c b/drivers/cpuidle/poll_state.c index 7416b16..4d17d3d 100644 --- a/drivers/cpuidle/poll_state.c +++ b/drivers/cpuidle/poll_state.c @@ -5,16 +5,31 @@ */ #include +#include #include #include static int __cpuidle poll_idle(struct cpuidle_device *dev, struct cpuidle_driver *drv, int index) { + unsigned int next_timer_us, i; + local_irq_enable(); if (!current_set_polling_and_test()) { - while (!need_resched()) + while (!need_resched()){ cpu_relax(); + + /* Occasionally check for a new and long expected residency time. */ + if (!(i++ % 1024)) { + local_irq_disable(); + next_timer_us = ktime_to_us(tick_nohz_get_sleep_length()); + local_irq_enable(); + /* need a better way to get threshold, including large margin */ + /* We are only trying to catch really bad cases here. */ + if (next_timer_us > 100) + break; + } + } } current_clr_polling();