Re: [linux-pm] [PATCH]PM QOS refresh against next-20100430

Previous thread: [PATCH]PM QOS refresh against next-20100430 by mark gross on Friday, April 30, 2010 - 2:17 pm. (1 message)

Next thread: [PATCH] rt: Fix the reminder block accounting for CONFIG_FUNCTION_TRACER by John Kacur on Thursday, April 29, 2010 - 1:07 pm. (1 message)
From: mark gross
Date: Friday, April 30, 2010 - 2:20 pm

The following is a refresh of the PM_QOS implementation, this patch
updates some documentation input I got from Randy.

This patch changes the string based list management to a handle base
implementation to help with the hot path use of pm-qos, it also renames
much of the API to use "request" as opposed to "requirement" that was
used in the initial implementation.  I did this because request more
accurately represents what it actually does.

Also, I added a string based ABI for users wanting to use a string
interface.  So if the user writes 0xDDDDDDDD formatted hex it will be
accepted by the interface.  (someone asked me for it and I don't think
it hurts anything.)

I really would like to get this refresh taken care of.  Its been taking
me too long to close this.  please review or include it in next.

Thanks!


--mgross

Ooops! forgot the signed off by line!

Signed-off-by: mark gross <mgross@linux.intel.com>

From c45d8d86f89ac55fbb9a499fbc754e35258bf818 Mon Sep 17 00:00:00 2001
From: mgross <mark.gross@gmail.com>
Date: Sat, 13 Mar 2010 08:18:36 -0800
Subject: [PATCH 1/2] PM_QOS to use handle based list implementation and exported function name changes to be more descriptive of what is actually happening.

---
 Documentation/power/pm_qos_interface.txt |   48 ++++---
 drivers/acpi/processor_idle.c            |    2 +-
 drivers/cpuidle/governors/ladder.c       |    2 +-
 drivers/cpuidle/governors/menu.c         |    2 +-
 drivers/net/e1000e/netdev.c              |   22 ++--
 drivers/net/igbvf/netdev.c               |    6 +-
 drivers/net/wireless/ipw2x00/ipw2100.c   |   11 +-
 include/linux/netdevice.h                |    4 +
 include/linux/pm_qos_params.h            |   14 +-
 include/sound/pcm.h                      |    3 +-
 kernel/pm_qos_params.c                   |  214 ++++++++++++++---------------
 net/mac80211/mlme.c                      |    2 +-
 net/mac80211/scan.c                      |    2 +-
 sound/core/pcm.c                         |    3 -
 ...
From: Rafael J. Wysocki
Date: Friday, April 30, 2010 - 3:13 pm

Well, I'd take it to suspend-2.6/linux-next, but first, it touches
subsystems whose maintainers were not in the Cc list, like the network
drivers, wireless and ACPI.  The changes are trivial, so I hope they don't
mind.

Second, my tree is based on the Linus' tree rather than linux-next and
the change in net/mac80211/scan.c doesn't seem to match that.  Please tell me
what I'm supposed to do about that.

Thanks,

--

From: mark gross
Date: Friday, April 30, 2010 - 4:05 pm

You can waite for monday and I'll send a rebased version to linus' tree.

I thought linux-next was where folks wanted me to put it.

I'll email out a new one monday.

Thanks,

--

From: Rafael J. Wysocki
Date: Friday, April 30, 2010 - 4:08 pm

Great, thanks!

Rafael
--

From: mark gross
Date: Tuesday, May 4, 2010 - 7:30 am

Sorry I'm late, 


From: markgross <mark.gross@intel.com>
Date: Mon, 3 May 2010 12:46:42 -0700
Subject: [PATCH] PM_QOS to use handle based list implementation and exported function name changes to be more descriptive of what is actually happening.

---
 Documentation/power/pm_qos_interface.txt |   48 ++++---
 drivers/acpi/processor_idle.c            |    2 +-
 drivers/cpuidle/governors/ladder.c       |    2 +-
 drivers/cpuidle/governors/menu.c         |    2 +-
 drivers/net/e1000e/netdev.c              |   22 ++--
 drivers/net/igbvf/netdev.c               |    6 +-
 drivers/net/wireless/ipw2x00/ipw2100.c   |   11 +-
 include/linux/netdevice.h                |    4 +
 include/linux/pm_qos_params.h            |   14 +-
 include/sound/pcm.h                      |    3 +-
 kernel/pm_qos_params.c                   |  214 ++++++++++++++---------------
 net/mac80211/mlme.c                      |    2 +-
 sound/core/pcm.c                         |    3 -
 sound/core/pcm_native.c                  |   14 +-
 14 files changed, 176 insertions(+), 171 deletions(-)

diff --git a/Documentation/power/pm_qos_interface.txt b/Documentation/power/pm_qos_interface.txt
index c40866e..bfed898 100644
--- a/Documentation/power/pm_qos_interface.txt
+++ b/Documentation/power/pm_qos_interface.txt
@@ -18,44 +18,46 @@ and pm_qos_params.h.  This is done because having the available parameters
 being runtime configurable or changeable from a driver was seen as too easy to
 abuse.
 
-For each parameter a list of performance requirements is maintained along with
+For each parameter a list of performance requests is maintained along with
 an aggregated target value.  The aggregated target value is updated with
-changes to the requirement list or elements of the list.  Typically the
-aggregated target value is simply the max or min of the requirement values held
+changes to the request list or elements of the list.  Typically the
+aggregated target value is simply the max or min of the request values ...
From: Rafael J. Wysocki
Date: Wednesday, May 5, 2010 - 4:59 pm

Applied to suspend-2.6/linux-next.  Please verify the changelog.

Rafael
--

From: Jonathan Corbet
Date: Monday, May 3, 2010 - 9:33 am

On Fri, 30 Apr 2010 14:20:43 -0700

Having taken a quick look, I think the API change makes a lot of
sense.  Hot paths are one thing; avoidance of accidental conflicts
would be another.

One question, though...  one clear use of this API is for drivers to
say "don't go into C3 or deeper because things go wrong"; I'm about to
add another one of those.  It works, but the use of a
PM_QOS_CPU_DMA_LATENCY requirement with a hard-coded number that one
hopes is small enough seems a bit...indirect.  I wonder if it would be
clearer and more robust to add a new requirement^Wrequest type saying
"the quality of service I need is shallow sleeps only"?

jon
--

From: Kevin Hilman
Date: Monday, May 3, 2010 - 9:40 am

The problem with that is portability.

What does "shallow" mean?  

Even between different SoCs in the same family, shallow might have
different meaning, but across architectures, this certainly would not
be portable.

Kevin
--

From: Jonathan Corbet
Date: Monday, May 3, 2010 - 9:42 am

On Mon, 03 May 2010 09:40:11 -0700

Well, shallow could mean that the state lacks the CPUIDLE_FLAG_DEEP
flag; that should be relatively portable.  In any case, it seems more
so than "if I put in a 55us latency requirement, I'll stay out of C3".

Just a thought, anyway; it's not like I've really worked through a
plausible alternative API.

jon
--

From: Kevin Hilman
Date: Monday, May 3, 2010 - 10:01 am

I guess it depends on your goal.  Do you just want to stay out of C3
on your current platform?  or do you want to stay out of any low-power
state (on any platform) where you'll have a latency of > 55 usecs?

The former is not portable (as C-states don't have the same meanings
across arches/SoCs) where as using a real-world number in usecs will

My main concern is that drivers not be written with latency
constraints that assume an Intel-centric set of power states.  There
are other SoCs out there with different sets of states and
corresponding latencies, so keeping things in real-world numbers
(latency, throughput, etc.) seems to me to be the only portable way.

Kevin

--

From: Jonathan Corbet
Date: Monday, May 3, 2010 - 10:10 am

On Mon, 03 May 2010 10:01:50 -0700

My case is a camera driver; as long as latencies are on the scale of
the frame rate, things won't go too wrong.  1ms latencies would not be
a huge problem.  What happens is that the hardware corrupts the data if
it goes into C3 while video capture is happening.

Comments in drivers/net/e1000e/netdev.c suggest that they were dealing

Understood.  But the end result is that I've ended up specifying a
latency requirement that is far tighter than the situation really needs

Understood.  Perhaps the pm_qos interface isn't the best way to achieve
the real goal here, but it's all we have at the moment.

Thanks,

jon
--

From: Matthew Garrett
Date: Monday, May 3, 2010 - 2:39 pm

I'd say that one plausible requirement is "DMA works", with another 
being "all interrupts work" as distinguished from "wakeup interrupts 
work".

-- 
Matthew Garrett | mjg59@srcf.ucam.org
--

From: Pavel Machek
Date: Monday, May 3, 2010 - 9:16 pm

While C3 has maximum allowed latency (101usec, iirc), I believe
there's no minimum, so someone could create platform with really quick
C3 with perhaps only 10usec to enter...

								Pavel


-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
--

From: mark gross
Date: Monday, May 3, 2010 - 9:41 pm

Well, adding such an api to cpuidle could do this for you.  "shallow" is
hard to define, but one could do what google did and define a
cpu_wake_lock type of API to block entry into any low power halt/mwait
type of state.  i.e. shallow would be C0, or just halt in idle.

If such a thing fits a "hotpath" API extention to pm_qos basically
implementing a low overhead disable lowpower behavior without having to
walk the pm_qos lists to get an agrigate value.  But, if such an api
belongs in cpu_idle thats fine too, as the only user of such and API
really is devices (like spi devices hangining of a clock that goes down
in the deeper idle states) wanting to block low power idle states, so be
it.

--

Previous thread: [PATCH]PM QOS refresh against next-20100430 by mark gross on Friday, April 30, 2010 - 2:17 pm. (1 message)

Next thread: [PATCH] rt: Fix the reminder block accounting for CONFIG_FUNCTION_TRACER by John Kacur on Thursday, April 29, 2010 - 1:07 pm. (1 message)