Maverick [PATCH 1/2] PM / Runtime: Add runtime PM statistics (v3)

Yong Shen yong.shen at linaro.org
Wed Sep 15 04:13:55 BST 2010


Hi there,

rtpm_status_show() is still there in the code. You found that I removed the
rtpm_status_show() in the patch because that after back port, there are two
rtpm_status_show() in the code. So I removed one of them to resolve the
conflict.
Please look at patched source code.

Yong

On Wed, Sep 15, 2010 at 6:33 AM, Leann Ogasawara <
leann.ogasawara at canonical.com> wrote:

> On Tue, 2010-09-14 at 10:23 -0600, Tim Gardner wrote:
> > On 09/13/2010 12:31 AM, yong.shen at linaro.org wrote:
> > > From: Arjan van de Ven<arjan at linux.intel.com>
> > >
> > > 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<arjan at linux.intel.com>
> > > Signed-off-by: Rafael J. Wysocki<rjw at sisk.pl>
> > > Signed-off-by: Shen Yong<yong.shen at linaro.org>
> > > ---
> > >   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<linux/string.h>
> > >   #include<linux/pm_runtime.h>
> > >   #include<asm/atomic.h>
> > > +#include<linux/jiffies.h>
> > >   #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;
> > > -}
> > > -
>
> Completely removing rtpm_status_show() doesn't appear correct, as this
> is not removed in the upstream version of the patch.
>
> > >   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.
>
> Tim's patch was on the right track to provide an updated
> rtpm_status_show() routine from what we currently have in Maverick, but
> this still didn't originate from upstream commit 8d4b9d1b.
>
> I've thus taken the path of least resistance and cleanly cherry-picked
> the following for the Ubuntu Maverick kernel:
>
> commit 0fcb4eef8294492c8f1de8236b1ed81f09e42922
> Author: Alan Stern <stern at rowland.harvard.edu>
> Date:   Thu Jul 8 00:05:37 2010 +0200
>
>    PM / Runtime: Make runtime_status attribute not debug-only (v. 2)
>
> commit 8d4b9d1bfef117862a2889dec4dac227068544c9
> Author: Arjan van de Ven <arjan at linux.intel.com>
> Date:   Mon Jul 19 02:01:06 2010 +0200
>
>    PM / Runtime: Add runtime PM statistics (v3)
>
> Thanks,
> Leann
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.linaro.org/pipermail/linaro-dev/attachments/20100915/87384e76/attachment-0001.htm 


More information about the linaro-dev mailing list