From patchwork Tue Sep 14 16:23:27 2010 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Tim Gardner X-Patchwork-Id: 64727 X-Patchwork-Delegate: leann.ogasawara@canonical.com Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from chlorine.canonical.com (chlorine.canonical.com [91.189.94.204]) by ozlabs.org (Postfix) with ESMTP id 69B11B6F01 for ; Wed, 15 Sep 2010 02:23:48 +1000 (EST) Received: from localhost ([127.0.0.1] helo=chlorine.canonical.com) by chlorine.canonical.com with esmtp (Exim 4.69) (envelope-from ) id 1OvYIH-00006C-KN; Tue, 14 Sep 2010 17:23:41 +0100 Received: from mail.tpi.com ([70.99.223.143]) by chlorine.canonical.com with esmtp (Exim 4.69) (envelope-from ) id 1OvYIA-0008Up-R9 for kernel-team@lists.ubuntu.com; Tue, 14 Sep 2010 17:23:35 +0100 Received: from [10.0.2.5] (unknown [10.0.2.5]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client did not present a certificate) by mail.tpi.com (Postfix) with ESMTP id CC4B526747D; Tue, 14 Sep 2010 09:23:14 -0700 (PDT) Message-ID: <4C8FA17F.2060702@canonical.com> Date: Tue, 14 Sep 2010 10:23:27 -0600 From: Tim Gardner User-Agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1.12) Gecko/20100826 Thunderbird/3.0.7 MIME-Version: 1.0 To: yong.shen@linaro.org Subject: Re: Maverick [PATCH 1/2] PM / Runtime: Add runtime PM statistics (v3) References: <1284359477-2177-1-git-send-email-yong.shen@linaro.org> <1284359477-2177-2-git-send-email-yong.shen@linaro.org> In-Reply-To: <1284359477-2177-2-git-send-email-yong.shen@linaro.org> Cc: kevinshen1167@gmail.com, kernel-team@lists.ubuntu.com, linaro-dev@lists.linaro.org, Leann Ogasawara X-BeenThere: kernel-team@lists.ubuntu.com X-Mailman-Version: 2.1.9 Precedence: list Reply-To: tim.gardner@canonical.com List-Id: Kernel team discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: kernel-team-bounces@lists.ubuntu.com Errors-To: kernel-team-bounces@lists.ubuntu.com On 09/13/2010 12:31 AM, yong.shen@linaro.org wrote: > From: Arjan van de Ven > > In order for PowerTOP to be able to report how well the new runtime PM is > working for the various drivers, the kernel needs to export some basic > statistics in sysfs. > > This patch adds two sysfs files in the runtime PM domain that expose the > total time a device has been active, and the time a device has been > suspended. > > With this PowerTOP can compute the activity percentage > > Active %age = 100 * (delta active) / (delta active + delta suspended) > > and present the information to the user. > > I've written the PowerTOP code (slated for version 1.12) already, and the > output looks like this: > > Runtime Device Power Management statistics > Active Device name > 10.0% 06:00.0 Ethernet controller: Realtek Semiconductor Co., Ltd. RTL8101E/RTL8102E PCI Express Fast Ethernet controller > > [version 2: fix stat update bugs noticed by Alan Stern] > [version 3: rebase to -next and move the sysfs declaration] > > Signed-off-by: Arjan van de Ven > Signed-off-by: Rafael J. Wysocki > Signed-off-by: Shen Yong > --- > drivers/base/power/runtime.c | 54 ++++++++++++++++++++++++---- > drivers/base/power/sysfs.c | 83 ++++++++++++++++++++++++++++++++---------- > include/linux/pm.h | 6 +++ > 3 files changed, 116 insertions(+), 27 deletions(-) > > diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c > index b0ec0e9..b78c401 100644 > --- a/drivers/base/power/runtime.c > +++ b/drivers/base/power/runtime.c > @@ -123,6 +123,45 @@ int pm_runtime_idle(struct device *dev) > } > EXPORT_SYMBOL_GPL(pm_runtime_idle); > > + > +/** > + * update_pm_runtime_accounting - Update the time accounting of power states > + * @dev: Device to update the accounting for > + * > + * In order to be able to have time accounting of the various power states > + * (as used by programs such as PowerTOP to show the effectiveness of runtime > + * PM), we need to track the time spent in each state. > + * update_pm_runtime_accounting must be called each time before the > + * runtime_status field is updated, to account the time in the old state > + * correctly. > + */ > +void update_pm_runtime_accounting(struct device *dev) > +{ > + unsigned long now = jiffies; > + int delta; > + > + delta = now - dev->power.accounting_timestamp; > + > + if (delta< 0) > + delta = 0; > + > + dev->power.accounting_timestamp = now; > + > + if (dev->power.disable_depth> 0) > + return; > + > + if (dev->power.runtime_status == RPM_SUSPENDED) > + dev->power.suspended_jiffies += delta; > + else > + dev->power.active_jiffies += delta; > +} > + > +static void __update_runtime_status(struct device *dev, enum rpm_status status) > +{ > + update_pm_runtime_accounting(dev); > + dev->power.runtime_status = status; > +} > + > /** > * __pm_runtime_suspend - Carry out run-time suspend of given device. > * @dev: Device to suspend. > @@ -197,7 +236,7 @@ int __pm_runtime_suspend(struct device *dev, bool from_wq) > goto repeat; > } > > - dev->power.runtime_status = RPM_SUSPENDING; > + __update_runtime_status(dev, RPM_SUSPENDING); > dev->power.deferred_resume = false; > > if (dev->bus&& dev->bus->pm&& dev->bus->pm->runtime_suspend) { > @@ -228,7 +267,7 @@ int __pm_runtime_suspend(struct device *dev, bool from_wq) > } > > if (retval) { > - dev->power.runtime_status = RPM_ACTIVE; > + __update_runtime_status(dev, RPM_ACTIVE); > if (retval == -EAGAIN || retval == -EBUSY) { > if (dev->power.timer_expires == 0) > notify = true; > @@ -237,7 +276,7 @@ int __pm_runtime_suspend(struct device *dev, bool from_wq) > pm_runtime_cancel_pending(dev); > } > } else { > - dev->power.runtime_status = RPM_SUSPENDED; > + __update_runtime_status(dev, RPM_SUSPENDED); > pm_runtime_deactivate_timer(dev); > > if (dev->parent) { > @@ -381,7 +420,7 @@ int __pm_runtime_resume(struct device *dev, bool from_wq) > goto repeat; > } > > - dev->power.runtime_status = RPM_RESUMING; > + __update_runtime_status(dev, RPM_RESUMING); > > if (dev->bus&& dev->bus->pm&& dev->bus->pm->runtime_resume) { > spin_unlock_irq(&dev->power.lock); > @@ -411,10 +450,10 @@ int __pm_runtime_resume(struct device *dev, bool from_wq) > } > > if (retval) { > - dev->power.runtime_status = RPM_SUSPENDED; > + __update_runtime_status(dev, RPM_SUSPENDED); > pm_runtime_cancel_pending(dev); > } else { > - dev->power.runtime_status = RPM_ACTIVE; > + __update_runtime_status(dev, RPM_ACTIVE); > if (parent) > atomic_inc(&parent->power.child_count); > } > @@ -848,7 +887,7 @@ int __pm_runtime_set_status(struct device *dev, unsigned int status) > } > > out_set: > - dev->power.runtime_status = status; > + __update_runtime_status(dev, status); > dev->power.runtime_error = 0; > out: > spin_unlock_irqrestore(&dev->power.lock, flags); > @@ -1077,6 +1116,7 @@ void pm_runtime_init(struct device *dev) > dev->power.request_pending = false; > dev->power.request = RPM_REQ_NONE; > dev->power.deferred_resume = false; > + dev->power.accounting_timestamp = jiffies; > INIT_WORK(&dev->power.work, pm_runtime_work); > > dev->power.timer_expires = 0; > diff --git a/drivers/base/power/sysfs.c b/drivers/base/power/sysfs.c > index a4c33bc..b831ec5 100644 > --- a/drivers/base/power/sysfs.c > +++ b/drivers/base/power/sysfs.c > @@ -6,6 +6,7 @@ > #include > #include > #include > +#include > #include "power.h" > > /* > @@ -108,6 +109,65 @@ static ssize_t control_store(struct device * dev, struct device_attribute *attr, > } > > static DEVICE_ATTR(control, 0644, control_show, control_store); > + > +static ssize_t rtpm_active_time_show(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + int ret; > + spin_lock_irq(&dev->power.lock); > + update_pm_runtime_accounting(dev); > + ret = sprintf(buf, "%i\n", jiffies_to_msecs(dev->power.active_jiffies)); > + spin_unlock_irq(&dev->power.lock); > + return ret; > +} > + > +static DEVICE_ATTR(runtime_active_time, 0444, rtpm_active_time_show, NULL); > + > +static ssize_t rtpm_suspended_time_show(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + int ret; > + spin_lock_irq(&dev->power.lock); > + update_pm_runtime_accounting(dev); > + ret = sprintf(buf, "%i\n", > + jiffies_to_msecs(dev->power.suspended_jiffies)); > + spin_unlock_irq(&dev->power.lock); > + return ret; > +} > + > +static DEVICE_ATTR(runtime_suspended_time, 0444, rtpm_suspended_time_show, NULL); > + > +static ssize_t rtpm_status_show(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + const char *p; > + > + if (dev->power.runtime_error) { > + p = "error\n"; > + } else if (dev->power.disable_depth) { > + p = "unsupported\n"; > + } else { > + switch (dev->power.runtime_status) { > + case RPM_SUSPENDED: > + p = "suspended\n"; > + break; > + case RPM_SUSPENDING: > + p = "suspending\n"; > + break; > + case RPM_RESUMING: > + p = "resuming\n"; > + break; > + case RPM_ACTIVE: > + p = "active\n"; > + break; > + default: > + return -EIO; > + } > + } > + return sprintf(buf, p); > +} > + > +static DEVICE_ATTR(runtime_status, 0444, rtpm_status_show, NULL); > #endif > > static ssize_t > @@ -172,27 +232,8 @@ static ssize_t rtpm_enabled_show(struct device *dev, > return sprintf(buf, "enabled\n"); > } > > -static ssize_t rtpm_status_show(struct device *dev, > - struct device_attribute *attr, char *buf) > -{ > - if (dev->power.runtime_error) > - return sprintf(buf, "error\n"); > - switch (dev->power.runtime_status) { > - case RPM_SUSPENDED: > - return sprintf(buf, "suspended\n"); > - case RPM_SUSPENDING: > - return sprintf(buf, "suspending\n"); > - case RPM_RESUMING: > - return sprintf(buf, "resuming\n"); > - case RPM_ACTIVE: > - return sprintf(buf, "active\n"); > - } > - return -EIO; > -} > - > static DEVICE_ATTR(runtime_usage, 0444, rtpm_usagecount_show, NULL); > static DEVICE_ATTR(runtime_active_kids, 0444, rtpm_children_show, NULL); > -static DEVICE_ATTR(runtime_status, 0444, rtpm_status_show, NULL); > static DEVICE_ATTR(runtime_enabled, 0444, rtpm_enabled_show, NULL); > > #endif > @@ -228,6 +269,9 @@ static DEVICE_ATTR(async, 0644, async_show, async_store); > static struct attribute * power_attrs[] = { > #ifdef CONFIG_PM_RUNTIME > &dev_attr_control.attr, > + &dev_attr_runtime_status.attr, > + &dev_attr_runtime_suspended_time.attr, > + &dev_attr_runtime_active_time.attr, > #endif > &dev_attr_wakeup.attr, > #ifdef CONFIG_PM_ADVANCED_DEBUG > @@ -235,7 +279,6 @@ static struct attribute * power_attrs[] = { > #ifdef CONFIG_PM_RUNTIME > &dev_attr_runtime_usage.attr, > &dev_attr_runtime_active_kids.attr, > - &dev_attr_runtime_status.attr, > &dev_attr_runtime_enabled.attr, > #endif > #endif > diff --git a/include/linux/pm.h b/include/linux/pm.h > index 8e258c7..dca597f 100644 > --- a/include/linux/pm.h > +++ b/include/linux/pm.h > @@ -476,9 +476,15 @@ struct dev_pm_info { > enum rpm_request request; > enum rpm_status runtime_status; > int runtime_error; > + unsigned long active_jiffies; > + unsigned long suspended_jiffies; > + unsigned long accounting_timestamp; > #endif > }; > > +extern void update_pm_runtime_accounting(struct device *dev); > + > + > /* > * The PM_EVENT_ messages are also used by drivers implementing the legacy > * suspend framework, based on the ->suspend() and ->resume() callbacks common This isn't a faithful backport of 8d4b9d1bfef117862a2889dec4dac227068544c9. What happened to rtpm_status_show() ? Even if its not used I'd prefer to keep that function in the patch so as to cause fewer conflicts if there are upstream stable patches. I prefer the attached version. rtg diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c index b0ec0e9..b78c401 100644 --- a/drivers/base/power/runtime.c +++ b/drivers/base/power/runtime.c @@ -123,6 +123,45 @@ int pm_runtime_idle(struct device *dev) } EXPORT_SYMBOL_GPL(pm_runtime_idle); + +/** + * update_pm_runtime_accounting - Update the time accounting of power states + * @dev: Device to update the accounting for + * + * In order to be able to have time accounting of the various power states + * (as used by programs such as PowerTOP to show the effectiveness of runtime + * PM), we need to track the time spent in each state. + * update_pm_runtime_accounting must be called each time before the + * runtime_status field is updated, to account the time in the old state + * correctly. + */ +void update_pm_runtime_accounting(struct device *dev) +{ + unsigned long now = jiffies; + int delta; + + delta = now - dev->power.accounting_timestamp; + + if (delta < 0) + delta = 0; + + dev->power.accounting_timestamp = now; + + if (dev->power.disable_depth > 0) + return; + + if (dev->power.runtime_status == RPM_SUSPENDED) + dev->power.suspended_jiffies += delta; + else + dev->power.active_jiffies += delta; +} + +static void __update_runtime_status(struct device *dev, enum rpm_status status) +{ + update_pm_runtime_accounting(dev); + dev->power.runtime_status = status; +} + /** * __pm_runtime_suspend - Carry out run-time suspend of given device. * @dev: Device to suspend. @@ -197,7 +236,7 @@ int __pm_runtime_suspend(struct device *dev, bool from_wq) goto repeat; } - dev->power.runtime_status = RPM_SUSPENDING; + __update_runtime_status(dev, RPM_SUSPENDING); dev->power.deferred_resume = false; if (dev->bus && dev->bus->pm && dev->bus->pm->runtime_suspend) { @@ -228,7 +267,7 @@ int __pm_runtime_suspend(struct device *dev, bool from_wq) } if (retval) { - dev->power.runtime_status = RPM_ACTIVE; + __update_runtime_status(dev, RPM_ACTIVE); if (retval == -EAGAIN || retval == -EBUSY) { if (dev->power.timer_expires == 0) notify = true; @@ -237,7 +276,7 @@ int __pm_runtime_suspend(struct device *dev, bool from_wq) pm_runtime_cancel_pending(dev); } } else { - dev->power.runtime_status = RPM_SUSPENDED; + __update_runtime_status(dev, RPM_SUSPENDED); pm_runtime_deactivate_timer(dev); if (dev->parent) { @@ -381,7 +420,7 @@ int __pm_runtime_resume(struct device *dev, bool from_wq) goto repeat; } - dev->power.runtime_status = RPM_RESUMING; + __update_runtime_status(dev, RPM_RESUMING); if (dev->bus && dev->bus->pm && dev->bus->pm->runtime_resume) { spin_unlock_irq(&dev->power.lock); @@ -411,10 +450,10 @@ int __pm_runtime_resume(struct device *dev, bool from_wq) } if (retval) { - dev->power.runtime_status = RPM_SUSPENDED; + __update_runtime_status(dev, RPM_SUSPENDED); pm_runtime_cancel_pending(dev); } else { - dev->power.runtime_status = RPM_ACTIVE; + __update_runtime_status(dev, RPM_ACTIVE); if (parent) atomic_inc(&parent->power.child_count); } @@ -848,7 +887,7 @@ int __pm_runtime_set_status(struct device *dev, unsigned int status) } out_set: - dev->power.runtime_status = status; + __update_runtime_status(dev, status); dev->power.runtime_error = 0; out: spin_unlock_irqrestore(&dev->power.lock, flags); @@ -1077,6 +1116,7 @@ void pm_runtime_init(struct device *dev) dev->power.request_pending = false; dev->power.request = RPM_REQ_NONE; dev->power.deferred_resume = false; + dev->power.accounting_timestamp = jiffies; INIT_WORK(&dev->power.work, pm_runtime_work); dev->power.timer_expires = 0; diff --git a/drivers/base/power/sysfs.c b/drivers/base/power/sysfs.c index a4c33bc..0c2ff38 100644 --- a/drivers/base/power/sysfs.c +++ b/drivers/base/power/sysfs.c @@ -6,6 +6,7 @@ #include #include #include +#include #include "power.h" /* @@ -108,6 +109,65 @@ static ssize_t control_store(struct device * dev, struct device_attribute *attr, } static DEVICE_ATTR(control, 0644, control_show, control_store); + +static ssize_t rtpm_active_time_show(struct device *dev, + struct device_attribute *attr, char *buf) +{ + int ret; + spin_lock_irq(&dev->power.lock); + update_pm_runtime_accounting(dev); + ret = sprintf(buf, "%i\n", jiffies_to_msecs(dev->power.active_jiffies)); + spin_unlock_irq(&dev->power.lock); + return ret; +} + +static DEVICE_ATTR(runtime_active_time, 0444, rtpm_active_time_show, NULL); + +static ssize_t rtpm_suspended_time_show(struct device *dev, + struct device_attribute *attr, char *buf) +{ + int ret; + spin_lock_irq(&dev->power.lock); + update_pm_runtime_accounting(dev); + ret = sprintf(buf, "%i\n", + jiffies_to_msecs(dev->power.suspended_jiffies)); + spin_unlock_irq(&dev->power.lock); + return ret; +} + +static DEVICE_ATTR(runtime_suspended_time, 0444, rtpm_suspended_time_show, NULL); + +static ssize_t rtpm_status_show(struct device *dev, + struct device_attribute *attr, char *buf) +{ + const char *p; + + if (dev->power.runtime_error) { + p = "error\n"; + } else if (dev->power.disable_depth) { + p = "unsupported\n"; + } else { + switch (dev->power.runtime_status) { + case RPM_SUSPENDED: + p = "suspended\n"; + break; + case RPM_SUSPENDING: + p = "suspending\n"; + break; + case RPM_RESUMING: + p = "resuming\n"; + break; + case RPM_ACTIVE: + p = "active\n"; + break; + default: + return -EIO; + } + } + return sprintf(buf, p); +} + +static DEVICE_ATTR(runtime_status, 0444, rtpm_status_show, NULL); #endif static ssize_t @@ -228,6 +288,9 @@ static DEVICE_ATTR(async, 0644, async_show, async_store); static struct attribute * power_attrs[] = { #ifdef CONFIG_PM_RUNTIME &dev_attr_control.attr, + &dev_attr_runtime_status.attr, + &dev_attr_runtime_suspended_time.attr, + &dev_attr_runtime_active_time.attr, #endif &dev_attr_wakeup.attr, #ifdef CONFIG_PM_ADVANCED_DEBUG diff --git a/include/linux/pm.h b/include/linux/pm.h index 8e258c7..dca597f 100644 --- a/include/linux/pm.h +++ b/include/linux/pm.h @@ -476,9 +476,15 @@ struct dev_pm_info { enum rpm_request request; enum rpm_status runtime_status; int runtime_error; + unsigned long active_jiffies; + unsigned long suspended_jiffies; + unsigned long accounting_timestamp; #endif }; +extern void update_pm_runtime_accounting(struct device *dev); + + /* * The PM_EVENT_ messages are also used by drivers implementing the legacy * suspend framework, based on the ->suspend() and ->resume() callbacks common