[1/3] PM / Runtime: Add runtime PM statistics (v3)

From: Arjan van de Ven <arjan@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@linux.intel.com>
Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/base/power/runtime.c |   54 ++++++++++++++++++++++++++++++----
 drivers/base/power/sysfs.c   |   65 +++++++++++++++++++++++++++++++++++++++++-
 include/linux/pm.h           |    6 ++++
 3 files changed, 117 insertions(+), 8 deletions(-)

On Thu, 2010-09-09 at 14:30 +0300, Amit Kucheria wrote:
> Yong,
> 
> You forgot to add a note as to why you need these patches applied :)
> 
> Reviewers - these patches expose some new information related to
> runtime PM that powertop can show.
> 
> This patch -1/3, is already in 2.6.36-rc. The other two aren't upstream.
> 
> Please consider applying to the Ubuntu kernel.

As Amit noted, this appears to be upstream:

commit 8d4b9d1bfef117862a2889dec4dac227068544c9
Author: Arjan van de Ven <arjan@linux.intel.com>
Date:   Mon Jul 19 02:01:06 2010 +0200

    PM / Runtime: Add runtime PM statistics (v3)

However, was the patch that was inlined below even compiled or tested?
It appears the patch has some unwanted remnants from when the conflicts
of the cherry-pick were being fixed up (see inlined comment below).
Yong, care to fix this up and re-send.  Care to also provide your SOB
for all three patches?

Thanks,
Leann

> Regards,
> Amit
> 
> On Thu, Sep 9, 2010 at 2:20 PM,  <yong.shen@linaro.org> wrote:
> > From: Arjan van de Ven <arjan@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@linux.intel.com>
> > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > ---
> >  drivers/base/power/runtime.c |   54 ++++++++++++++++++++++++++++++----
> >  drivers/base/power/sysfs.c   |   65 +++++++++++++++++++++++++++++++++++++++++-
> >  include/linux/pm.h           |    6 ++++
> >  3 files changed, 117 insertions(+), 8 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..7ee064f 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,66 @@ 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);
> > +>>>>>>> 8d4b9d1... PM / Runtime: Add runtime PM statistics (v3):drivers/base/power/sysfs.c

Seems the above was accidentally included when fixing up conflicts ...

> >  #endif
> >
> >  static ssize_t
> > @@ -228,6 +289,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 +299,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
> > --
> > 1.6.3.3
> >
> >
>

This patch doesn't apply cleanly to the current Ubuntu Maverick linux
kernel as it appears we're already carrying a version of this patch:

commit ce3cdc8812cc8a6f5645a9c372c7d6227676a7e5
Author: Arjan van de Ven <arjan@linux.intel.com>
Date:   Sun Oct 25 15:37:04 2009 -0700

    UBUNTU: SAUCE: vfs: Add a trace point in the mark_inode_dirty function

There's only a 1 line discrepancy (see below) between what we currently
have in the Maverick Ubuntu kernel and the patch inlined below.  Care to
re-send an updated patch to only add the additional one line change?
Care to also include your SOB on the patch?

Thanks,
Leann

On Thu, 2010-09-09 at 19:20 +0800, yong.shen@linaro.org wrote:
> From: Arjan van de Ven <arjan@linux.intel.com>
> 
> PowerTOP would like to be able to show who is keeping the disk
> busy by dirtying data. The most logical spot for this is in the vfs
> in the mark_inode_dirty() function, doing this on the block level
> is not possible because by the time the IO hits the block layer the
> guilty party can no longer be found ("kjournald" and "pdflush" are not
> useful answers to "who caused this file to be dirty).
> 
> The trace point follows the same logic/style as the block_dump code
> and pretty much dumps the same data, just not to dmesg (and thus to
> /var/log/messages) but via the trace events streams.
> 
> Signed-of-by: Arjan van de Ven <arjan@linux.intel.com>
> ---
>  fs/fs-writeback.c          |    4 +++
>  fs/inode.c                 |    4 +++
>  include/trace/events/vfs.h |   53 ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 61 insertions(+), 0 deletions(-)
>  create mode 100644 include/trace/events/vfs.h
> 
> diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
> index d5be169..6a2af1d 100644
> --- a/fs/fs-writeback.c
> +++ b/fs/fs-writeback.c
> @@ -26,6 +26,7 @@
>  #include <linux/blkdev.h>
>  #include <linux/backing-dev.h>
>  #include <linux/buffer_head.h>
> +#include <trace/events/vfs.h>
>  #include "internal.h"
>  
>  #define inode_to_bdi(inode)	((inode)->i_mapping->backing_dev_info)
> @@ -900,6 +901,9 @@ void __mark_inode_dirty(struct inode *inode, int flags)
>  			sb->s_op->dirty_inode(inode);
>  	}
>  
> +	if (flags & (I_DIRTY_SYNC | I_DIRTY_DATASYNC | I_DIRTY_PAGES))

Looks like the above line is the only additional change from what we
already have.

> +		trace_dirty_inode(inode, current);
> +
>  	/*
>  	 * make sure that changes are seen by all cpus before we test i_state
>  	 * -- mikulas
> diff --git a/fs/inode.c b/fs/inode.c
> index 722860b..a06184f 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -1626,3 +1626,7 @@ void inode_init_owner(struct inode *inode, const struct inode *dir,
>  	inode->i_mode = mode;
>  }
>  EXPORT_SYMBOL(inode_init_owner);
> +
> +#define CREATE_TRACE_POINTS  
> +#include <trace/events/vfs.h>
> +
> diff --git a/include/trace/events/vfs.h b/include/trace/events/vfs.h
> new file mode 100644
> index 0000000..21cf9fb
> --- /dev/null
> +++ b/include/trace/events/vfs.h
> @@ -0,0 +1,53 @@
> +#undef TRACE_SYSTEM
> +#define TRACE_SYSTEM vfs
> +
> +#if !defined(_TRACE_VFS_H) || defined(TRACE_HEADER_MULTI_READ)
> +#define _TRACE_VFS_H
> +
> +/*
> + * Tracepoint for dirtying an inode:
> + */
> +TRACE_EVENT(dirty_inode,
> +
> +	TP_PROTO(struct inode *inode, struct task_struct *task),
> +
> +	TP_ARGS(inode, task),
> +
> +	TP_STRUCT__entry(
> +		__array( char,	comm,	TASK_COMM_LEN	)
> +		__field( pid_t,	pid			)
> +		__array( char,  dev,    16		)
> +		__array( char,  file,   32		)
> +	),
> +
> +	TP_fast_assign(
> +		if (inode->i_ino || strcmp(inode->i_sb->s_id, "bdev")) {
> +                	struct dentry *dentry;
> +                	const char *name = "?";
> +
> +			dentry = d_find_alias(inode);
> +			if (dentry) {
> +				spin_lock(&dentry->d_lock);
> +				name = (const char *) dentry->d_name.name;
> +			}
> +
> +			memcpy(__entry->comm, task->comm, TASK_COMM_LEN);
> +			__entry->pid = task->pid;
> +			strlcpy(__entry->file, name, 32);
> +			strlcpy(__entry->dev, inode->i_sb->s_id, 16);
> +
> +			if (dentry) {
> +				spin_unlock(&dentry->d_lock);
> +				dput(dentry);
> +			}
> +		}
> +	),
> +
> +	TP_printk("task=%i (%s) file=%s dev=%s",
> +		__entry->pid, __entry->comm, __entry->file, __entry->dev)
> +);
> +
> +#endif /* _TRACE_VFS_H */
> +
> +/* This part must be outside protection */
> +#include <trace/define_trace.h>
> -- 
> 1.6.3.3
> 
>