From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Rafael J. Wysocki" Subject: Re: [PATCH v12 3/4] cpuidle: Export the next timer/tick expiration for a CPU Date: Mon, 25 Mar 2019 13:19:22 +0100 Message-ID: <2942111.nn8VVVUCAL@aspire.rjw.lan> References: <20190227195836.24739-1-ulf.hansson@linaro.org> <20190227195836.24739-4-ulf.hansson@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7Bit Return-path: In-Reply-To: <20190227195836.24739-4-ulf.hansson@linaro.org> Sender: linux-kernel-owner@vger.kernel.org To: Ulf Hansson Cc: linux-pm@vger.kernel.org, Frederic Weisbecker , Thomas Gleixner , Sudeep Holla , Lorenzo Pieralisi , Mark Rutland , Daniel Lezcano , "Raju P . L . S . S . S . N" , Stephen Boyd , Tony Lindgren , Kevin Hilman , Lina Iyer , Viresh Kumar , Vincent Guittot , Geert Uytterhoeven , linux-arm-kernel@lists.infradead.org, linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org List-Id: linux-arm-msm@vger.kernel.org On Wednesday, February 27, 2019 8:58:35 PM CET Ulf Hansson wrote: > To be able to predict the sleep duration for a CPU that is entering idle, > knowing when the next timer/tick is going to expire, is extremely useful. > Both the teo and the menu cpuidle governors already makes use of this > information, while selecting an idle state. > > Moving forward, the similar prediction needs to be done, but for a group of > idle CPUs rather than for a single idle CPU. Following changes implements a > new genpd governor, which needs this. > > Support this, by sharing a new function called > tick_nohz_get_next_hrtimer(), which returns the next hrtimer or the next > tick, whatever that expires first. > > Additionally, when cpuidle is about to invoke the ->enter() callback, then > call tick_nohz_get_next_hrtimer() and store its return value in the per CPU > struct cpuidle_device, as to make it available outside cpuidle. > > Do note, at the point when cpuidle calls tick_nohz_get_next_hrtimer(), the > governor's ->select() callback has already made a decision whether to stop > the tick or not. In this way, tick_nohz_get_next_hrtimer() actually returns > the next timer expiration, whatever origin. > > Cc: Lina Iyer > Co-developed-by: Lina Iyer > Co-developed-by: Daniel Lezcano > Signed-off-by: Ulf Hansson > --- > > Changes in v12: > - New patch. > > --- > drivers/cpuidle/cpuidle.c | 8 ++++++++ > include/linux/cpuidle.h | 1 + > include/linux/tick.h | 7 ++++++- > kernel/time/tick-sched.c | 12 ++++++++++++ > 4 files changed, 27 insertions(+), 1 deletion(-) > > diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c > index 7f108309e871..255365b1a6ab 100644 > --- a/drivers/cpuidle/cpuidle.c > +++ b/drivers/cpuidle/cpuidle.c > @@ -328,6 +328,14 @@ int cpuidle_select(struct cpuidle_driver *drv, struct cpuidle_device *dev, > int cpuidle_enter(struct cpuidle_driver *drv, struct cpuidle_device *dev, > int index) > { > + /* > + * Store the next hrtimer, which becomes either next tick or the next > + * timer event, whatever expires first. Additionally, to make this data > + * useful for consumers outside cpuidle, we rely on that the governor's > + * ->select() callback have decided, whether to stop the tick or not. > + */ > + dev->next_hrtimer = tick_nohz_get_next_hrtimer(); I would use WRITE_ONCE() to set next_hrtimer here and READ_ONCE() for reading that value in the next patch, as a matter of annotation if nothing else. > + > if (cpuidle_state_is_coupled(drv, index)) > return cpuidle_enter_state_coupled(dev, drv, index); > return cpuidle_enter_state(dev, drv, index); Also I would clear next_hrtimer here to avoid dragging stale values around. Apart from this the series LGTM. Thanks! From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.0 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY, SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 5E2A5C43381 for ; Mon, 25 Mar 2019 12:21:47 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 2CF1320830 for ; Mon, 25 Mar 2019 12:21:47 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="BR2n4tWg" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 2CF1320830 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=rjwysocki.net Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:MIME-Version:References:In-Reply-To: Message-ID:Date:Subject:To:From:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=LyiQqMk4eUCsKLN2JTJgNrfKHFT32tVAi6C17Fm7BDA=; b=BR2n4tWg6NDjJP Kk9eI7ECEVZmtGO4CbiyWWNupUkjnOp41kC7OoNtreYCPSVQ8c5GhhjA9p498TN7RiWLZjH4WwKaZ 4RYoS4mZvgqbTQhMJSPYyAnlZCDntHGw0adJ6UjKeIbWbNAOs/tbeWOIen7pT5GOI5Ag0+duDlSNC NVVq/XdxpXvInASrMVoCnMv6WwpJ0M0p831qdkShNb/ihSsXwK3e/a2qQBGtzRt3ai6ArCjh/XYUr 6HkR/sJDMa5q8nWybdUQh2eB6r+S9WBvmFDlOBHrHuy8wgTz9xLshNS/wTkQ2cmhL5uOUoWpq4V6x jqu1H0+BVp38rGB37E3w==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.90_1 #2 (Red Hat Linux)) id 1h8Obe-0000t2-0B; Mon, 25 Mar 2019 12:21:34 +0000 Received: from cloudserver094114.home.pl ([79.96.170.134]) by bombadil.infradead.org with esmtps (Exim 4.90_1 #2 (Red Hat Linux)) id 1h8ObY-0000s4-34 for linux-arm-kernel@lists.infradead.org; Mon, 25 Mar 2019 12:21:32 +0000 Received: from 79.184.253.239.ipv4.supernova.orange.pl (79.184.253.239) (HELO aspire.rjw.lan) by serwer1319399.home.pl (79.96.170.134) with SMTP (IdeaSmtpServer 0.83.213) id db5a24e253bd52a7; Mon, 25 Mar 2019 13:21:18 +0100 From: "Rafael J. Wysocki" To: Ulf Hansson Subject: Re: [PATCH v12 3/4] cpuidle: Export the next timer/tick expiration for a CPU Date: Mon, 25 Mar 2019 13:19:22 +0100 Message-ID: <2942111.nn8VVVUCAL@aspire.rjw.lan> In-Reply-To: <20190227195836.24739-4-ulf.hansson@linaro.org> References: <20190227195836.24739-1-ulf.hansson@linaro.org> <20190227195836.24739-4-ulf.hansson@linaro.org> MIME-Version: 1.0 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20190325_052128_343706_F981587D X-CRM114-Status: GOOD ( 21.39 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Mark Rutland , Lorenzo Pieralisi , Vincent Guittot , Geert Uytterhoeven , linux-pm@vger.kernel.org, Stephen Boyd , Frederic Weisbecker , linux-arm-msm@vger.kernel.org, Daniel Lezcano , Kevin Hilman , Lina Iyer , linux-kernel@vger.kernel.org, Tony Lindgren , Viresh Kumar , Sudeep Holla , Thomas Gleixner , "Raju P . L . S . S . S . N" , linux-arm-kernel@lists.infradead.org Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Wednesday, February 27, 2019 8:58:35 PM CET Ulf Hansson wrote: > To be able to predict the sleep duration for a CPU that is entering idle, > knowing when the next timer/tick is going to expire, is extremely useful. > Both the teo and the menu cpuidle governors already makes use of this > information, while selecting an idle state. > > Moving forward, the similar prediction needs to be done, but for a group of > idle CPUs rather than for a single idle CPU. Following changes implements a > new genpd governor, which needs this. > > Support this, by sharing a new function called > tick_nohz_get_next_hrtimer(), which returns the next hrtimer or the next > tick, whatever that expires first. > > Additionally, when cpuidle is about to invoke the ->enter() callback, then > call tick_nohz_get_next_hrtimer() and store its return value in the per CPU > struct cpuidle_device, as to make it available outside cpuidle. > > Do note, at the point when cpuidle calls tick_nohz_get_next_hrtimer(), the > governor's ->select() callback has already made a decision whether to stop > the tick or not. In this way, tick_nohz_get_next_hrtimer() actually returns > the next timer expiration, whatever origin. > > Cc: Lina Iyer > Co-developed-by: Lina Iyer > Co-developed-by: Daniel Lezcano > Signed-off-by: Ulf Hansson > --- > > Changes in v12: > - New patch. > > --- > drivers/cpuidle/cpuidle.c | 8 ++++++++ > include/linux/cpuidle.h | 1 + > include/linux/tick.h | 7 ++++++- > kernel/time/tick-sched.c | 12 ++++++++++++ > 4 files changed, 27 insertions(+), 1 deletion(-) > > diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c > index 7f108309e871..255365b1a6ab 100644 > --- a/drivers/cpuidle/cpuidle.c > +++ b/drivers/cpuidle/cpuidle.c > @@ -328,6 +328,14 @@ int cpuidle_select(struct cpuidle_driver *drv, struct cpuidle_device *dev, > int cpuidle_enter(struct cpuidle_driver *drv, struct cpuidle_device *dev, > int index) > { > + /* > + * Store the next hrtimer, which becomes either next tick or the next > + * timer event, whatever expires first. Additionally, to make this data > + * useful for consumers outside cpuidle, we rely on that the governor's > + * ->select() callback have decided, whether to stop the tick or not. > + */ > + dev->next_hrtimer = tick_nohz_get_next_hrtimer(); I would use WRITE_ONCE() to set next_hrtimer here and READ_ONCE() for reading that value in the next patch, as a matter of annotation if nothing else. > + > if (cpuidle_state_is_coupled(drv, index)) > return cpuidle_enter_state_coupled(dev, drv, index); > return cpuidle_enter_state(dev, drv, index); Also I would clear next_hrtimer here to avoid dragging stale values around. Apart from this the series LGTM. Thanks! _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel