QoS power Management enabling patch set
Subsystems (4)
The following patches implement a more generalized infrastructure (than latency.c) for connecting drivers and subsystem's that could implement power performance optimizations with the data needed to implement such policies.
These patches are following up on the discussions and presentations at the power management summit last summer.
The idea is that from an abstract point of view how much to throttle hardware can be expressed as a function of QoS types of parameters; Latency, throughput, and idle time outs.
The qos_parameter patch is intended to put into place the registration and notification infrastructure for enabling QoS based policy choices by drivers, where constraints on throttling are communicated through the qos_params module.
Note: it implements a user mode registration protocol where user mode QoS dependents open a misc device node and write there constraints. As long as the file is held open the QoS constraint is included, when the file closes this constraint is removed and a new constraint is computed.
I would like some feed back on the idea, implementation and possible applications of this concept we could work on.
The current patch set is 2 patches.
1) qos_param : implements the infrastructure and user mode interface
2) no latency : removes latency.c from the kernel tree replacing it with qos_param use.
I have a 3rd patch that's a hack application of this qos interface for communicating latency constraints to e1000.c for tweaking the interrupt processing of the e1000 based on acceptable latency gathered from the infrastructure. I'm not proud of this last one but it helps express the idea.
Also there is some more documentation and commentary (and an older patch set) on
http://www.lesswatts.org/projects/power-qos/
thanks,
--mgross
Re: [RFC] QoS params patch
The following is the qos_param patch that implements a genralization of latency.c.
include/linux/qos_params.h +35 -0
@@ -0,0 +1,35 @@ +/* interface for the qos_power infrastructure of the linux kernel. +* +* Mark Gross +*/ +#include <linux/list.h> +#include <linux/notifier.h> +#include <linux/miscdevice.h> + +struct requirement_list { +struct list_head list; +union { +s32 value; +s32 usec; +s32 kbps; + }; +char *name; +}; + +#define QOS_RESERVED 0 +#define QOS_CPU_DMA_LATENCY 1 +#define QOS_NETWORK_LATENCY 2 +#define QOS_NETWORK_THROUGHPUT 3 + +#define QOS_NUM_CLASSES 4 +#define QOS_DEFAULT_VALUE -1 + +int qos_add_requirement (int qos,char *name,s32 value); +int qos_update_requirement (int qos,char *name,s32 new_value); +void qos_remove_requirement (int qos,char *name); + +int qos_requirement (int qos); + +int qos_add_notifier (int qos,struct notifier_block *notifier); +int qos_remove_notifier (int qos,struct notifier_block *notifier); +
kernel/Makefile +1 -1
@@ -9,7 +9,7 @@ rcupdate.o extable.o params.o posix-timers.o \ kthread.o wait.o kfifo.o sys_ni.o posix-cpu-timers.o mutex.o \ hrtimer.o rwsem.o latency.o nsproxy.o srcu.o die_notifier.o \ - utsname.o + utsname.o qos_params.o obj-$(CONFIG_STACKTRACE) += stacktrace.o obj-y += time/
kernel/qos_params.c +354 -0
354 lines not on this page · raw message
Re: [RFC] QoS params patch
In reply to Mark GrossThe following is the qos_param patch that implements a genralization of latency.c.
include/linux/qos_params.h +35 -0
@@ -0,0 +1,35 @@ +/* interface for the qos_power infrastructure of the linux kernel. +* +* Mark Gross +*/ +#include <linux/list.h> +#include <linux/notifier.h> +#include <linux/miscdevice.h> + +struct requirement_list { +struct list_head list; +union { +s32 value; +s32 usec; +s32 kbps; + }; +char *name; +}; + +#define QOS_RESERVED 0 +#define QOS_CPU_DMA_LATENCY 1 +#define QOS_NETWORK_LATENCY 2 +#define QOS_NETWORK_THROUGHPUT 3 + +#define QOS_NUM_CLASSES 4 +#define QOS_DEFAULT_VALUE -1 + +int qos_add_requirement (int qos,char *name,s32 value); +int qos_update_requirement (int qos,char *name,s32 new_value); +void qos_remove_requirement (int qos,char *name); + +int qos_requirement (int qos); + +int qos_add_notifier (int qos,struct notifier_block *notifier); +int qos_remove_notifier (int qos,struct notifier_block *notifier); +
kernel/Makefile +1 -1
@@ -9,7 +9,7 @@ rcupdate.o extable.o params.o posix-timers.o \ kthread.o wait.o kfifo.o sys_ni.o posix-cpu-timers.o mutex.o \ hrtimer.o rwsem.o latency.o nsproxy.o srcu.o die_notifier.o \ - utsname.o + utsname.o qos_params.o obj-$(CONFIG_STACKTRACE) += stacktrace.o obj-y += time/
kernel/qos_params.c +354 -0
354 lines not on this page · raw message
Re: [RFC] QoS params patch
Quoted 3 lines · On Wed, 26 Sep 2007 15:40:26 -0700 Mark Gross wrote:
The following is the
qos_parampatch that implements a genralization oflatency.c.
Just some general comments (as on irc):
- use 'diffstat -p1 -w70' to summarize each patch
- use checkpatch.pl to check for coding style and other buglets
- has no API docs :(
kernel/qos_params.c line 91
Quoted 2 lines
kernel/qos_params.c +2
-0+/* assumes qos_lock is held */ +static void update_target (int i)
I'd prefer a better arg name than 'i'.
Quoted 1 line
+{
---
~Randy
Phaedrus says that Quality is about caring.
Re: [RFC] QoS params patch
In reply to Mark GrossQuoted 3 lines · On Wed, 26 Sep 2007 15:40:26 -0700 Mark Gross wrote:
The following is the
qos_parampatch that implements a genralization oflatency.c.
Just some general comments (as on irc):
- use 'diffstat -p1 -w70' to summarize each patch
- use checkpatch.pl to check for coding style and other buglets
- has no API docs :(
kernel/qos_params.c line 91
Quoted 2 lines
kernel/qos_params.c +2
-0+/* assumes qos_lock is held */ +static void update_target (int i)
I'd prefer a better arg name than 'i'.
Quoted 1 line
+{
---
~Randy
Phaedrus says that Quality is about caring.
Re: [RFC] QoS params patch
Quoted 10 lines · Randy Dunlap
Quoted 3 lines · On Wed, 26 Sep 2007 15:40:26 -0700 Mark Gross wrote:
The following is the
qos_parampatch that implements a genralization oflatency.c.Just some general comments (as on irc):
- use 'diffstat -p1 -w70' to summarize each patch
- use checkpatch.pl to check for coding style and other buglets
done
Quoted 1 line
- has no API docs :(
not done yet.
Quoted 8 lines
Review
Quoted 6 lines
(quoted diff) +2
-0+/* assumes qos_lock is held */ +static void update_target(int i)I'd prefer a better arg name than 'i'.
I do too, but i in this case is an Index.
Quoted 5 lines
Quoted 1 line
+{
---
~Randy
Phaedrus says that Quality is about caring.
updated patch:
Makefile | 2 linux/qos_params.h | 35 +++++ qos_params.c | 354 +++++++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 390 insertions(+), 1 deletion(-)
include/linux/qos_params.h +35 -0
@@ -0,0 +1,35 @@ +/* interface for the qos_power infrastructure of the linux kernel. +* +* Mark Gross +*/ +#include <linux/list.h> +#include <linux/notifier.h> +#include <linux/miscdevice.h> + +struct requirement_list { +struct list_head list; +union { +s32 value; +s32 usec; +s32 kbps; + }; +char *name; +}; + +#define QOS_RESERVED 0 +#define QOS_CPU_DMA_LATENCY 1 +#define QOS_NETWORK_LATENCY 2 +#define QOS_NETWORK_THROUGHPUT 3 + +#define QOS_NUM_CLASSES 4 +#define QOS_DEFAULT_VALUE -1 + +int qos_add_requirement (int qos,char *name,s32 value); +int qos_update_requirement (int qos,char *name,s32 new_value); +void qos_remove_requirement (int qos,char *name); + +int qos_requirement (int qos); + +int qos_add_notifier (int qos,struct notifier_block *notifier); +int qos_remove_notifier (int qos,struct notifier_block *notifier); +
kernel/Makefile +1 -1
@@ -9,7 +9,7 @@ rcupdate.o extable.o params.o posix-timers.o \ kthread.o wait.o kfifo.o sys_ni.o posix-cpu-timers.o mutex.o \ hrtimer.o rwsem.o latency.o nsproxy.o srcu.o die_notifier.o \ - utsname.o + utsname.o qos_params.o obj-$(CONFIG_STACKTRACE) += stacktrace.o obj-y += time/
kernel/qos_params.c +354 -0
354 lines not on this page · raw message
Re: [RFC] QoS params patch
In reply to Randy DunlapQuoted 10 lines · Randy Dunlap
Quoted 3 lines · On Wed, 26 Sep 2007 15:40:26 -0700 Mark Gross wrote:
The following is the
qos_parampatch that implements a genralization oflatency.c.Just some general comments (as on irc):
- use 'diffstat -p1 -w70' to summarize each patch
- use checkpatch.pl to check for coding style and other buglets
done
Quoted 1 line
- has no API docs :(
not done yet.
Quoted 8 lines
Review
Quoted 6 lines
(quoted diff) +2
-0+/* assumes qos_lock is held */ +static void update_target(int i)I'd prefer a better arg name than 'i'.
I do too, but i in this case is an Index.
Quoted 5 lines
Quoted 1 line
+{
---
~Randy
Phaedrus says that Quality is about caring.
updated patch:
Makefile | 2 linux/qos_params.h | 35 +++++ qos_params.c | 354 +++++++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 390 insertions(+), 1 deletion(-)
include/linux/qos_params.h +35 -0
@@ -0,0 +1,35 @@ +/* interface for the qos_power infrastructure of the linux kernel. +* +* Mark Gross +*/ +#include <linux/list.h> +#include <linux/notifier.h> +#include <linux/miscdevice.h> + +struct requirement_list { +struct list_head list; +union { +s32 value; +s32 usec; +s32 kbps; + }; +char *name; +}; + +#define QOS_RESERVED 0 +#define QOS_CPU_DMA_LATENCY 1 +#define QOS_NETWORK_LATENCY 2 +#define QOS_NETWORK_THROUGHPUT 3 + +#define QOS_NUM_CLASSES 4 +#define QOS_DEFAULT_VALUE -1 + +int qos_add_requirement (int qos,char *name,s32 value); +int qos_update_requirement (int qos,char *name,s32 new_value); +void qos_remove_requirement (int qos,char *name); + +int qos_requirement (int qos); + +int qos_add_notifier (int qos,struct notifier_block *notifier); +int qos_remove_notifier (int qos,struct notifier_block *notifier); +
kernel/Makefile +1 -1
@@ -9,7 +9,7 @@ rcupdate.o extable.o params.o posix-timers.o \ kthread.o wait.o kfifo.o sys_ni.o posix-cpu-timers.o mutex.o \ hrtimer.o rwsem.o latency.o nsproxy.o srcu.o die_notifier.o \ - utsname.o + utsname.o qos_params.o obj-$(CONFIG_STACKTRACE) += stacktrace.o obj-y += time/
kernel/qos_params.c +354 -0
354 lines not on this page · raw message
Re: [RFC] QoS params patch
On Wed, 26 Sep 2007 17:40:20 PDT, Mark Gross said:
(others here are probably better at spotting leaks and races than I am, so I'm skipping those and picking other nits. ;)
kernel/Makefile
Quoted 2 lines
kernel/Makefile +0
-0
0700
kernel/Makefile line 12
Quoted 6 lines
kernel/Makefile +1
-1@@ -9,7 +9,7 @@ rcupdate.o extable.o params.o posix-timers.o \ kthread.o wait.o kfifo.o sys_ni.o posix-cpu-timers.o mutex.o \ hrtimer.o rwsem.o latency.o nsproxy.o srcu.o die_notifier.o \ - utsname.o + utsname.o qos_params.o
So I don't get a choice in the matter if I will be dragging this thing around in my kernel, even if I have no intention of using the functionality?
kernel/Makefile
Quoted 8 lines
kernel/Makefile +8
-0+ * This QoS design is best effort based. Dependents register their QoS needs. + * Watchers register to keep track of the current QoS needs of the system. + * + * There are 3 basic classes of QoS parameter: latency, timeout, throughput + * each have defined units: + * latency: usec + * timeout: usec <-- currently not used. + * throughput: kbs (kilo byte / sec)
It's unclear whether these are registering a differing QoS request for each process/container/whatever that asks for one, or if they're global across the system. Also, even though it's "best effort", it would be good to document what the failure mode is if we get conflicting requests, or an overcommit situation - do new requests get refused, or allowed and ignored, or allowed and only sometimes fulfilled. For instance, assume a gigabit ethernet, and 3 processes ask for 400 mbits/sec each - who wins, who gets part of what they asked for, and who loses and gets starved?
Quoted 3 lines
+ * User mode requirements on a QOS parameter register themselves to the
+ * subsystem by opening the device node/dev/... and writing there request to
+ * the node. As long as the process holds a file handle open to the node the
/dev? What /dev entry do you use for a network interface? Should this be a configfs creature instead, or maybe something else?
kernel/Makefile
Quoted 2 lines
kernel/Makefile +2
-0+/* static helper functions */ +static s32 max_compare(s32 v1, s32 v2)
Blech. Is it time for the yearly stamp-out-reinvention-of-max() already? The use of pointer functions is interesting, but I have to wonder if there's not a better way...
kernel/Makefile
Quoted 2 lines
kernel/Makefile +2
-0+#define PID_NAME_LEN sizeof("process_1234567890") +static char name[PID_NAME_LEN];
And then you just pass a pointer to this and kstrdup() it. Why not kmalloc() the space initially and just 'dep->name = name;' and be done with it?
General nit - why qos_power_*, when none of the supported QoS parameters seem to be power-related?
Re: [RFC] QoS params patch
In reply to Mark GrossOn Wed, 26 Sep 2007 17:40:20 PDT, Mark Gross said:
(others here are probably better at spotting leaks and races than I am, so I'm skipping those and picking other nits. ;)
kernel/Makefile
Quoted 2 lines
kernel/Makefile +0
-0
0700
kernel/Makefile line 12
Quoted 6 lines
kernel/Makefile +1
-1@@ -9,7 +9,7 @@ rcupdate.o extable.o params.o posix-timers.o \ kthread.o wait.o kfifo.o sys_ni.o posix-cpu-timers.o mutex.o \ hrtimer.o rwsem.o latency.o nsproxy.o srcu.o die_notifier.o \ - utsname.o + utsname.o qos_params.o
So I don't get a choice in the matter if I will be dragging this thing around in my kernel, even if I have no intention of using the functionality?
kernel/Makefile
Quoted 8 lines
kernel/Makefile +8
-0+ * This QoS design is best effort based. Dependents register their QoS needs. + * Watchers register to keep track of the current QoS needs of the system. + * + * There are 3 basic classes of QoS parameter: latency, timeout, throughput + * each have defined units: + * latency: usec + * timeout: usec <-- currently not used. + * throughput: kbs (kilo byte / sec)
It's unclear whether these are registering a differing QoS request for each process/container/whatever that asks for one, or if they're global across the system. Also, even though it's "best effort", it would be good to document what the failure mode is if we get conflicting requests, or an overcommit situation - do new requests get refused, or allowed and ignored, or allowed and only sometimes fulfilled. For instance, assume a gigabit ethernet, and 3 processes ask for 400 mbits/sec each - who wins, who gets part of what they asked for, and who loses and gets starved?
Quoted 3 lines
+ * User mode requirements on a QOS parameter register themselves to the
+ * subsystem by opening the device node/dev/... and writing there request to
+ * the node. As long as the process holds a file handle open to the node the
/dev? What /dev entry do you use for a network interface? Should this be a configfs creature instead, or maybe something else?
kernel/Makefile
Quoted 2 lines
kernel/Makefile +2
-0+/* static helper functions */ +static s32 max_compare(s32 v1, s32 v2)
Blech. Is it time for the yearly stamp-out-reinvention-of-max() already? The use of pointer functions is interesting, but I have to wonder if there's not a better way...
kernel/Makefile
Quoted 2 lines
kernel/Makefile +2
-0+#define PID_NAME_LEN sizeof("process_1234567890") +static char name[PID_NAME_LEN];
And then you just pass a pointer to this and kstrdup() it. Why not kmalloc() the space initially and just 'dep->name = name;' and be done with it?
General nit - why qos_power_*, when none of the supported QoS parameters seem to be power-related?
Re: [RFC] QoS params patch
Quoted 4 lines · On Wed, Sep 26, 2007 at 10:53:03PM -0400, [email protected] wrote:
kernel/Makefile
Quoted 2 lines · Mark Gross
kernel/Makefile +0
-00700
Quoted 10 lines
kernel/Makefile line 12
Quoted 6 lines
kernel/Makefile +1
-1@@ -9,7 +9,7 @@ rcupdate.o extable.o params.o posix-timers.o \ kthread.o wait.o kfifo.o sys_ni.o posix-cpu-timers.o mutex.o \ hrtimer.o rwsem.o latency.o nsproxy.o srcu.o die_notifier.o \ - utsname.o + utsname.o qos_params.oSo I don't get a choice in the matter if I will be dragging this thing around in my kernel, even if I have no intention of using the functionality?
You don't get that option with latency.c either at the moment, and it's arguable whether it's even worth it. The more curious thing is that while this qos params seems to be an evolution of Arjan's latency.c (and the drivers that are using it are updated in the rest of the patch set), latency.c itself is still compiled in. Is this an oversight, or was it intentional? One set of latency hinting APIs only, please :-)
Re: [RFC] QoS params patch
In reply to [email protected]Quoted 4 lines · On Wed, Sep 26, 2007 at 10:53:03PM -0400, [email protected] wrote:
kernel/Makefile
Quoted 2 lines · Mark Gross
kernel/Makefile +0
-00700
Quoted 10 lines
kernel/Makefile line 12
Quoted 6 lines
kernel/Makefile +1
-1@@ -9,7 +9,7 @@ rcupdate.o extable.o params.o posix-timers.o \ kthread.o wait.o kfifo.o sys_ni.o posix-cpu-timers.o mutex.o \ hrtimer.o rwsem.o latency.o nsproxy.o srcu.o die_notifier.o \ - utsname.o + utsname.o qos_params.oSo I don't get a choice in the matter if I will be dragging this thing around in my kernel, even if I have no intention of using the functionality?
You don't get that option with latency.c either at the moment, and it's arguable whether it's even worth it. The more curious thing is that while this qos params seems to be an evolution of Arjan's latency.c (and the drivers that are using it are updated in the rest of the patch set), latency.c itself is still compiled in. Is this an oversight, or was it intentional? One set of latency hinting APIs only, please :-)
Re: [RFC] QoS params patch
Quoted 15 lines · Paul Mundt
Quoted 4 lines · On Wed, Sep 26, 2007 at 10:53:03PM -0400, [email protected] wrote:
kernel/Makefile
Quoted 2 lines · Mark Gross
kernel/Makefile +0
-00700
Quoted 10 lines
kernel/Makefile line 12
Quoted 6 lines
kernel/Makefile +1
-1@@ -9,7 +9,7 @@ rcupdate.o extable.o params.o posix-timers.o \ kthread.o wait.o kfifo.o sys_ni.o posix-cpu-timers.o mutex.o \ hrtimer.o rwsem.o latency.o nsproxy.o srcu.o die_notifier.o \ - utsname.o + utsname.o qos_params.oSo I don't get a choice in the matter if I will be dragging this thing around in my kernel, even if I have no intention of using the functionality?
Quoted 6 lines
You don't get that option with
latency.ceither at the moment, and it's arguable whether it's even worth it. The more curious thing is that while this qos params seems to be an evolution of Arjan'slatency.c(and the drivers that are using it are updated in the rest of the patch set),latency.citself is still compiled in. Is this an oversight, or was it intentional? One set of latency hinting APIs only, please :-)
It was intentional to compile latnecy.c in (and hence qos_parms is starting off doing the same thing)
I agree, "there can be only be one". (couldn't resist)
I split the patch set up this first patch puts in the qos_param.c and the second patch yanks latency.c replacing things with its interfaces.
--mgross
Re: [RFC] QoS params patch
In reply to Paul MundtQuoted 15 lines · Paul Mundt
Quoted 4 lines · On Wed, Sep 26, 2007 at 10:53:03PM -0400, [email protected] wrote:
kernel/Makefile
Quoted 2 lines · Mark Gross
kernel/Makefile +0
-00700
Quoted 10 lines
kernel/Makefile line 12
Quoted 6 lines
kernel/Makefile +1
-1@@ -9,7 +9,7 @@ rcupdate.o extable.o params.o posix-timers.o \ kthread.o wait.o kfifo.o sys_ni.o posix-cpu-timers.o mutex.o \ hrtimer.o rwsem.o latency.o nsproxy.o srcu.o die_notifier.o \ - utsname.o + utsname.o qos_params.oSo I don't get a choice in the matter if I will be dragging this thing around in my kernel, even if I have no intention of using the functionality?
Quoted 6 lines
You don't get that option with
latency.ceither at the moment, and it's arguable whether it's even worth it. The more curious thing is that while this qos params seems to be an evolution of Arjan'slatency.c(and the drivers that are using it are updated in the rest of the patch set),latency.citself is still compiled in. Is this an oversight, or was it intentional? One set of latency hinting APIs only, please :-)
It was intentional to compile latnecy.c in (and hence qos_parms is starting off doing the same thing)
I agree, "there can be only be one". (couldn't resist)
I split the patch set up this first patch puts in the qos_param.c and the second patch yanks latency.c replacing things with its interfaces.
--mgross
Re: [RFC] QoS params patch
In reply to [email protected]Quoted 7 lines · On Wed, Sep 26, 2007 at 10:53:03PM -0400, [email protected] wrote:
On Wed, 26 Sep 2007 17:40:20 PDT, Mark Gross said:
(others here are probably better at spotting leaks and races than I am, so I'm skipping those and picking other nits. ;)kernel/Makefile
Quoted 2 lines
kernel/Makefile +0
-00700
Quoted 9 lines
kernel/Makefile line 12
Quoted 6 lines
kernel/Makefile +1
-1@@ -9,7 +9,7 @@ rcupdate.o extable.o params.o posix-timers.o \ kthread.o wait.o kfifo.o sys_ni.o posix-cpu-timers.o mutex.o \ hrtimer.o rwsem.o latency.o nsproxy.o srcu.o die_notifier.o \ - utsname.o + utsname.o qos_params.oSo I don't get a choice in the matter if I will be dragging this thing around in my kernel, even if I have no intention of using the functionality?
nope.
Quoted 10 lines
kernel/Makefile
Quoted 9 lines
kernel/Makefile +8
-0+ * This QoS design is best effort based. Dependents register their QoS needs. + * Watchers register to keep track of the current QoS needs of the system. + * + * There are 3 basic classes of QoS parameter: latency, timeout, throughput + * each have defined units: + * latency: usec + * timeout: usec <-- currently not used. + * throughput: kbs (kilo byte / sec)
Quoted 8 lines
It's unclear whether these are registering a differing QoS request for each process/container/whatever that asks for one, or if they're global across the system. Also, even though it's "best effort", it would be good to document what the failure mode is if we get conflicting requests, or an overcommit situation - do new requests get refused, or allowed and ignored, or allowed and only sometimes fulfilled. For instance, assume a gigabit ethernet, and 3 processes ask for 400 mbits/sec each - who wins, who gets part of what they asked for, and who loses and gets starved?
There are no overcommit failure modes. These QoS parameters are for constraining aggressive power management throttling from breaking things thereby enabling better power management solutions.
The QoS values are system wide parameters, not per process or driver. So the code takes the max or min depending on what class of parameter is in question. So if you have multiple applications asking for network throughput of 400Mbs the current code will let the nic driver find out that it shouldn't throttle itself to the point where it can provide 400Mbs to any one.
You raise a perspective I overlooked. For each of the parameter classes I had been thinking of computing some extrema out of the set of requests. However; for throughput it may make more sense to sum up a collative value.
I'm not sure which way would make more sense.
--mgross
Quoted 23 lines
Quoted 4 lines
+ * User mode requirements on a QOS parameter register themselves to the
+ * subsystem by opening the device node/dev/... and writing there request to
+ * the node. As long as the process holds a file handle open to the node the/dev? What /dev entry do you use for a network interface? Should this be a configfs creature instead, or maybe something else?
kernel/Makefile
Quoted 2 lines
kernel/Makefile +2
-0+/* static helper functions */ +static s32 max_compare(s32 v1, s32 v2)Blech. Is it time for the yearly stamp-out-reinvention-of-max() already? The use of pointer functions is interesting, but I have to wonder if there's not a better way...
kernel/Makefile
Quoted 2 lines
kernel/Makefile +2
-0+#define PID_NAME_LEN sizeof("process_1234567890") +static char name[PID_NAME_LEN];And then you just pass a pointer to this and
kstrdup()it. Why notkmalloc()the space initially and just 'dep->name= name;' and be done with it?General nit - why qos_power_*, when none of the supported QoS parameters seem to be power-related?
Re: [RFC] QoS params patch
In reply to [email protected]Quoted 7 lines · On Wed, Sep 26, 2007 at 10:53:03PM -0400, [email protected] wrote:
On Wed, 26 Sep 2007 17:40:20 PDT, Mark Gross said:
(others here are probably better at spotting leaks and races than I am, so I'm skipping those and picking other nits. ;)kernel/Makefile
Quoted 2 lines
kernel/Makefile +0
-00700
Quoted 9 lines
kernel/Makefile line 12
Quoted 6 lines
kernel/Makefile +1
-1@@ -9,7 +9,7 @@ rcupdate.o extable.o params.o posix-timers.o \ kthread.o wait.o kfifo.o sys_ni.o posix-cpu-timers.o mutex.o \ hrtimer.o rwsem.o latency.o nsproxy.o srcu.o die_notifier.o \ - utsname.o + utsname.o qos_params.oSo I don't get a choice in the matter if I will be dragging this thing around in my kernel, even if I have no intention of using the functionality?
nope.
Quoted 10 lines
kernel/Makefile
Quoted 9 lines
kernel/Makefile +8
-0+ * This QoS design is best effort based. Dependents register their QoS needs. + * Watchers register to keep track of the current QoS needs of the system. + * + * There are 3 basic classes of QoS parameter: latency, timeout, throughput + * each have defined units: + * latency: usec + * timeout: usec <-- currently not used. + * throughput: kbs (kilo byte / sec)
Quoted 8 lines
It's unclear whether these are registering a differing QoS request for each process/container/whatever that asks for one, or if they're global across the system. Also, even though it's "best effort", it would be good to document what the failure mode is if we get conflicting requests, or an overcommit situation - do new requests get refused, or allowed and ignored, or allowed and only sometimes fulfilled. For instance, assume a gigabit ethernet, and 3 processes ask for 400 mbits/sec each - who wins, who gets part of what they asked for, and who loses and gets starved?
There are no overcommit failure modes. These QoS parameters are for constraining aggressive power management throttling from breaking things thereby enabling better power management solutions.
The QoS values are system wide parameters, not per process or driver. So the code takes the max or min depending on what class of parameter is in question. So if you have multiple applications asking for network throughput of 400Mbs the current code will let the nic driver find out that it shouldn't throttle itself to the point where it can provide 400Mbs to any one.
You raise a perspective I overlooked. For each of the parameter classes I had been thinking of computing some extrema out of the set of requests. However; for throughput it may make more sense to sum up a collative value.
I'm not sure which way would make more sense.
--mgross
Quoted 23 lines
Quoted 4 lines
+ * User mode requirements on a QOS parameter register themselves to the
+ * subsystem by opening the device node/dev/... and writing there request to
+ * the node. As long as the process holds a file handle open to the node the/dev? What /dev entry do you use for a network interface? Should this be a configfs creature instead, or maybe something else?
kernel/Makefile
Quoted 2 lines
kernel/Makefile +2
-0+/* static helper functions */ +static s32 max_compare(s32 v1, s32 v2)Blech. Is it time for the yearly stamp-out-reinvention-of-max() already? The use of pointer functions is interesting, but I have to wonder if there's not a better way...
kernel/Makefile
Quoted 2 lines
kernel/Makefile +2
-0+#define PID_NAME_LEN sizeof("process_1234567890") +static char name[PID_NAME_LEN];And then you just pass a pointer to this and
kstrdup()it. Why notkmalloc()the space initially and just 'dep->name= name;' and be done with it?General nit - why qos_power_*, when none of the supported QoS parameters seem to be power-related?
Re: [RFC] QoS params patch
In reply to Mark GrossQuoted 18 lines · Mark Gross
Quoted 9 lines · Randy Dunlap
Quoted 3 lines · On Wed, 26 Sep 2007 15:40:26 -0700 Mark Gross wrote:
The following is the
qos_parampatch that implements a genralization oflatency.c.Just some general comments (as on irc):
- use 'diffstat -p1 -w70' to summarize each patch
- use checkpatch.pl to check for coding style and other bugletsdone
Quoted 1 line
- has no API docs :(
not done yet.
Quoted 3 lines
Quoted 5 lines
Quoted 3 lines
kernel/qos_params.c line 91
Quoted 2 lines
kernel/qos_params.c +2
-0+/* assumes qos_lock is held */ +static void update_target (int i)I'd prefer a better arg name than 'i'.
I do too, but i in this case is an Index.
I think in many cases you could use a pointer to qos_array[i] instead of passing this index 'i' as a function argument.
Roel
Re: [RFC] QoS params patch
In reply to Mark GrossQuoted 18 lines · Mark Gross
Quoted 9 lines · Randy Dunlap
Quoted 3 lines · On Wed, 26 Sep 2007 15:40:26 -0700 Mark Gross wrote:
The following is the
qos_parampatch that implements a genralization oflatency.c.Just some general comments (as on irc):
- use 'diffstat -p1 -w70' to summarize each patch
- use checkpatch.pl to check for coding style and other bugletsdone
Quoted 1 line
- has no API docs :(
not done yet.
Quoted 3 lines
Quoted 5 lines
Quoted 3 lines
kernel/qos_params.c line 91
Quoted 2 lines
kernel/qos_params.c +2
-0+/* assumes qos_lock is held */ +static void update_target (int i)I'd prefer a better arg name than 'i'.
I do too, but i in this case is an Index.
I think in many cases you could use a pointer to qos_array[i] instead of passing this index 'i' as a function argument.
Roel
Re: [RFC] QoS params patch
In reply to Mark Grossinclude/linux/qos_params.h line 16
Quoted 7 lines · Mark Gross
include/linux/qos_params.h +7
-0+struct list_head list; +union { +s32 value; +s32 usec; +s32 kbps; + }; +char *name;
Your } is in a strange place. It looks like it wants to join its friends closer to the left margin ;-)
include/linux/qos_params.h line 25
Quoted 9 lines
include/linux/qos_params.h +9
-0+}; + +#define QOS_RESERVED 0 +#define QOS_CPU_DMA_LATENCY 1 +#define QOS_NETWORK_LATENCY 2 +#define QOS_NETWORK_THROUGHPUT 3 + +#define QOS_NUM_CLASSES 4 +#define QOS_DEFAULT_VALUE -1
an enum would be better for this, especially as people are likely to add new types, having to update QOS_NUM_CLASSES each time sucks.
Quoted 7 lines
kernel/qos_params.c +7
-0+/* static helper functions */ +static s32 max_compare (s32 v1,s32 v2) +{ +if (v1 < v2) +return v2; +else +return v1;
kernel/qos_params.c line 89
Quoted 10 lines
kernel/qos_params.c +10
-0+} + +static s32 min_compare (s32 v1,s32 v2) +{ +if (v1 < v2) +return v1; +else +return v2; +} +
min()/max() instead?
kernel/qos_params.c line 92
Quoted 3 lines
kernel/qos_params.c +3
-0+/* assumes qos_lock is held */ +static void update_target (int i) +{
'target' might be a more meaningful variable name.
kernel/qos_params.c line 124
Quoted 9 lines
kernel/qos_params.c +9
-0+ qos->qos_power_miscdev.fops = &qos_power_fops; + + ret =misc_register (& qos->qos_power_miscdev); +if (ret <0 ) +printk (KERN_ERR "qos_power: misc_register returns %d.\n" , ret); + +return ret; +} +
Just return misc_register(...); ?
Quoted 8 lines
kernel/qos_params.c +8
-0+if ( i <QOS_NUM_CLASSES ) { + qos = &qos_array[i]; + qos->name =kstrdup (name,GFP_KERNEL ); +if (!qos->name) + goto cleanup; + + qos->default_value = default_value; + qos->target_value = default_value;
kernel/qos_params.c line 149
Quoted 10 lines
kernel/qos_params.c +10
-0+ qos->comparitor = comparitor; +srcu_init_notifier_head (&qos->notifiers); +INIT_LIST_HEAD (&qos->requirements.list); +if (register_new_qos_misc (qos) <0 ) + goto cleanup; + } +return ; +cleanup: +kfree (qos->name); +}
This leaks. You'll have to scan down from i and clean up the kstrdup() per qos_array element. Presently this will only free the first one to fail registration.
kernel/qos_params.c line 195
Quoted 8 lines
kernel/qos_params.c +8
-0+ +int qos_add_requirement (int i,char *name,s32 value) +{ +struct requirement_list * dep; +unsigned long flags; + +spin_lock_irqsave (&qos_lock, flags); + dep =kzalloc (sizeof (struct requirement_list ),GFP_KERNEL );
kmalloc() under a spinlock. GFP_KERNEL implies __GFP_WAIT, which can sleep. Slab debugging would have caught this, too.
kernel/qos_params.c line 201
Quoted 6 lines
kernel/qos_params.c +6
-0+if (dep) { +if (value ==QOS_DEFAULT_VALUE ) + dep->value = qos_array[i].default_value; +else + dep->value = value; + dep->name =kstrdup (name,GFP_KERNEL );
And here also, still under the spinlock. You can probably rework the locking just to protect the list, if you really need it at all, it doesn't seem to matter anywhere else here.
Quoted 4 lines
kernel/qos_params.c +4
-0+if (!dep->name) + goto cleanup; + +list_add (&dep->list, &qos_array[i].requirements.list);
kernel/qos_params.c line 215
Quoted 10 lines
kernel/qos_params.c +10
-0+update_target (i); +spin_unlock_irqrestore (&qos_lock, flags); + +return 0 ; + } +spin_unlock_irqrestore (&qos_lock, flags); + +cleanup: +if (dep) +kfree (dep);
no if() needed.
kernel/qos_params.c line 218
Quoted 3 lines
kernel/qos_params.c +3
-0+return -ENOMEM ; +} +EXPORT_SYMBOL_GPL (qos_add_requirement);
You also didn't spin_unlock_irqrestore() in the error path, so this bails out with the lock held and IRQs disabled.
Re: [RFC] QoS params patch
In reply to Mark Grossinclude/linux/qos_params.h line 16
Quoted 7 lines · Mark Gross
include/linux/qos_params.h +7
-0+struct list_head list; +union { +s32 value; +s32 usec; +s32 kbps; + }; +char *name;
Your } is in a strange place. It looks like it wants to join its friends closer to the left margin ;-)
include/linux/qos_params.h line 25
Quoted 9 lines
include/linux/qos_params.h +9
-0+}; + +#define QOS_RESERVED 0 +#define QOS_CPU_DMA_LATENCY 1 +#define QOS_NETWORK_LATENCY 2 +#define QOS_NETWORK_THROUGHPUT 3 + +#define QOS_NUM_CLASSES 4 +#define QOS_DEFAULT_VALUE -1
an enum would be better for this, especially as people are likely to add new types, having to update QOS_NUM_CLASSES each time sucks.
Quoted 7 lines
kernel/qos_params.c +7
-0+/* static helper functions */ +static s32 max_compare (s32 v1,s32 v2) +{ +if (v1 < v2) +return v2; +else +return v1;
kernel/qos_params.c line 89
Quoted 10 lines
kernel/qos_params.c +10
-0+} + +static s32 min_compare (s32 v1,s32 v2) +{ +if (v1 < v2) +return v1; +else +return v2; +} +
min()/max() instead?
kernel/qos_params.c line 92
Quoted 3 lines
kernel/qos_params.c +3
-0+/* assumes qos_lock is held */ +static void update_target (int i) +{
'target' might be a more meaningful variable name.
kernel/qos_params.c line 124
Quoted 9 lines
kernel/qos_params.c +9
-0+ qos->qos_power_miscdev.fops = &qos_power_fops; + + ret =misc_register (& qos->qos_power_miscdev); +if (ret <0 ) +printk (KERN_ERR "qos_power: misc_register returns %d.\n" , ret); + +return ret; +} +
Just return misc_register(...); ?
Quoted 8 lines
kernel/qos_params.c +8
-0+if ( i <QOS_NUM_CLASSES ) { + qos = &qos_array[i]; + qos->name =kstrdup (name,GFP_KERNEL ); +if (!qos->name) + goto cleanup; + + qos->default_value = default_value; + qos->target_value = default_value;
kernel/qos_params.c line 149
Quoted 10 lines
kernel/qos_params.c +10
-0+ qos->comparitor = comparitor; +srcu_init_notifier_head (&qos->notifiers); +INIT_LIST_HEAD (&qos->requirements.list); +if (register_new_qos_misc (qos) <0 ) + goto cleanup; + } +return ; +cleanup: +kfree (qos->name); +}
This leaks. You'll have to scan down from i and clean up the kstrdup() per qos_array element. Presently this will only free the first one to fail registration.
kernel/qos_params.c line 195
Quoted 8 lines
kernel/qos_params.c +8
-0+ +int qos_add_requirement (int i,char *name,s32 value) +{ +struct requirement_list * dep; +unsigned long flags; + +spin_lock_irqsave (&qos_lock, flags); + dep =kzalloc (sizeof (struct requirement_list ),GFP_KERNEL );
kmalloc() under a spinlock. GFP_KERNEL implies __GFP_WAIT, which can sleep. Slab debugging would have caught this, too.
kernel/qos_params.c line 201
Quoted 6 lines
kernel/qos_params.c +6
-0+if (dep) { +if (value ==QOS_DEFAULT_VALUE ) + dep->value = qos_array[i].default_value; +else + dep->value = value; + dep->name =kstrdup (name,GFP_KERNEL );
And here also, still under the spinlock. You can probably rework the locking just to protect the list, if you really need it at all, it doesn't seem to matter anywhere else here.
Quoted 4 lines
kernel/qos_params.c +4
-0+if (!dep->name) + goto cleanup; + +list_add (&dep->list, &qos_array[i].requirements.list);
kernel/qos_params.c line 215
Quoted 10 lines
kernel/qos_params.c +10
-0+update_target (i); +spin_unlock_irqrestore (&qos_lock, flags); + +return 0 ; + } +spin_unlock_irqrestore (&qos_lock, flags); + +cleanup: +if (dep) +kfree (dep);
no if() needed.
kernel/qos_params.c line 218
Quoted 3 lines
kernel/qos_params.c +3
-0+return -ENOMEM ; +} +EXPORT_SYMBOL_GPL (qos_add_requirement);
You also didn't spin_unlock_irqrestore() in the error path, so this bails out with the lock held and IRQs disabled.
Re: [RFC] QoS params patch
Quoted 10 lines · On Thu, 27 Sep 2007 11:24:40 +0900 Paul Mundt wrote:
Quoted 10 lines
kernel/qos_params.c +10
-0+/* static helper functions */ +static s32 max_compare (s32 v1,s32 v2) +{ +if (v1 < v2) +return v2; +else +return v1; +} + +static s32 min_compare (s32 v1,s32 v2)
Quoted 8 lines
kernel/qos_params.c line 89
Quoted 7 lines
kernel/qos_params.c +7
-0+{ +if (v1 < v2) +return v1; +else +return v2; +} +min()/max() instead?
Other code wants function pointers to the min & max functions. That's why they are here AFAICT.
Quoted 4 lines
kernel/qos_params.c line 92
Quoted 3 lines
kernel/qos_params.c +3
-0+/* assumes qos_lock is held */ +static void update_target (int i) +{'target' might be a more meaningful variable name.
Anything but 'i'.
---
~Randy
Phaedrus says that Quality is about caring.
Re: [RFC] QoS params patch
In reply to Paul MundtQuoted 10 lines · On Thu, 27 Sep 2007 11:24:40 +0900 Paul Mundt wrote:
Quoted 10 lines
kernel/qos_params.c +10
-0+/* static helper functions */ +static s32 max_compare (s32 v1,s32 v2) +{ +if (v1 < v2) +return v2; +else +return v1; +} + +static s32 min_compare (s32 v1,s32 v2)
Quoted 8 lines
kernel/qos_params.c line 89
Quoted 7 lines
kernel/qos_params.c +7
-0+{ +if (v1 < v2) +return v1; +else +return v2; +} +min()/max() instead?
Other code wants function pointers to the min & max functions. That's why they are here AFAICT.
Quoted 4 lines
kernel/qos_params.c line 92
Quoted 3 lines
kernel/qos_params.c +3
-0+/* assumes qos_lock is held */ +static void update_target (int i) +{'target' might be a more meaningful variable name.
Anything but 'i'.
---
~Randy
Phaedrus says that Quality is about caring.
Re: [RFC] QoS params patch
Quoted 25 lines · Randy Dunlap
Quoted 10 lines · On Thu, 27 Sep 2007 11:24:40 +0900 Paul Mundt wrote:
Quoted 10 lines
kernel/qos_params.c +10
-0+/* static helper functions */ +static s32 max_compare (s32 v1,s32 v2) +{ +if (v1 < v2) +return v2; +else +return v1; +} + +static s32 min_compare (s32 v1,s32 v2)Quoted 8 lines
kernel/qos_params.c line 89
Quoted 7 lines
kernel/qos_params.c +7
-0+{ +if (v1 < v2) +return v1; +else +return v2; +} +min()/max() instead?
Other code wants function pointers to the min & max functions. That's why they are here AFAICT.
Quoted 6 lines
Quoted 4 lines
kernel/qos_params.c line 92
Quoted 3 lines
kernel/qos_params.c +3
-0+/* assumes qos_lock is held */ +static void update_target (int i) +{'target' might be a more meaningful variable name.
Anything but 'i'.
The 'i' is gone. Will post new patch later today.
--mgross
Re: [RFC] QoS params patch
In reply to Randy DunlapQuoted 25 lines · Randy Dunlap
Quoted 10 lines · On Thu, 27 Sep 2007 11:24:40 +0900 Paul Mundt wrote:
Quoted 10 lines
kernel/qos_params.c +10
-0+/* static helper functions */ +static s32 max_compare (s32 v1,s32 v2) +{ +if (v1 < v2) +return v2; +else +return v1; +} + +static s32 min_compare (s32 v1,s32 v2)Quoted 8 lines
kernel/qos_params.c line 89
Quoted 7 lines
kernel/qos_params.c +7
-0+{ +if (v1 < v2) +return v1; +else +return v2; +} +min()/max() instead?
Other code wants function pointers to the min & max functions. That's why they are here AFAICT.
Quoted 6 lines
Quoted 4 lines
kernel/qos_params.c line 92
Quoted 3 lines
kernel/qos_params.c +3
-0+/* assumes qos_lock is held */ +static void update_target (int i) +{'target' might be a more meaningful variable name.
Anything but 'i'.
The 'i' is gone. Will post new patch later today.
--mgross
Re: [RFC] QoS params patch update.
In reply to Randy DunlapQuoted 25 lines · Randy Dunlap
Quoted 10 lines · On Thu, 27 Sep 2007 11:24:40 +0900 Paul Mundt wrote:
Quoted 10 lines
(quoted diff) +10
-0+/* static helper functions */ +static s32 max_compare(s32 v1, s32 v2) +{ + if (v1 < v2) + return v2; + else + return v1; +} + +static s32 min_compare(s32 v1, s32 v2)Quoted 8 lines
Review
Quoted 7 lines
(quoted diff) +7
-0+{ + if (v1 < v2) + return v1; + else + return v2; +} +min()/max() instead?
Other code wants function pointers to the min & max functions. That's why they are here AFAICT.
Quoted 8 lines
Quoted 4 lines
Review
Quoted 3 lines
(quoted diff) +3
-0+/* assumes qos_lock is held */ +static void update_target(int i) +{'target' might be a more meaningful variable name.
Anything but 'i'.
---
Updated qos PM parameter patch:
Note: the replacing of latency.c with this is a separate patch.
this patch attempts to address the issues raised so far.
--mgross
mgross@mgross-t43:~/workstuff/power_qos$ diffstat -p1 qos_params_sept_27.patch include/linux/qos_params.h | 35 +++ kernel/Makefile | 2 kernel/qos_params.c | 413 +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 449 insertions(+), 1 deletion(-)
include/linux/qos_params.h +35 -0
@@ -0,0 +1,35 @@ +/* interface for the qos_power infrastructure of the linux kernel. +* +* Mark Gross +*/ +#include <linux/list.h> +#include <linux/notifier.h> +#include <linux/miscdevice.h> + +struct requirement_list { +struct list_head list; +union { +s32 value; +s32 usec; +s32 kbps; + }; +char *name; +}; + +#define QOS_RESERVED 0 +#define QOS_CPU_DMA_LATENCY 1 +#define QOS_NETWORK_LATENCY 2 +#define QOS_NETWORK_THROUGHPUT 3 + +#define QOS_NUM_CLASSES 4 +#define QOS_DEFAULT_VALUE -1 + +int qos_add_requirement (int qos,char *name,s32 value); +int qos_update_requirement (int qos,char *name,s32 new_value); +void qos_remove_requirement (int qos,char *name); + +int qos_requirement (int qos); + +int qos_add_notifier (int qos,struct notifier_block *notifier); +int qos_remove_notifier (int qos,struct notifier_block *notifier); +
kernel/Makefile +1 -1
@@ -9,7 +9,7 @@ rcupdate.o extable.o params.o posix-timers.o \ kthread.o wait.o kfifo.o sys_ni.o posix-cpu-timers.o mutex.o \ hrtimer.o rwsem.o latency.o nsproxy.o srcu.o die_notifier.o \ - utsname.o + utsname.o qos_params.o obj-$(CONFIG_STACKTRACE) += stacktrace.o obj-y += time/
kernel/qos_params.c +413 -0
413 lines not on this page · raw message
Re: [RFC] QoS params patch update.
In reply to Randy DunlapQuoted 25 lines · Randy Dunlap
Quoted 10 lines · On Thu, 27 Sep 2007 11:24:40 +0900 Paul Mundt wrote:
Quoted 10 lines
(quoted diff) +10
-0+/* static helper functions */ +static s32 max_compare(s32 v1, s32 v2) +{ + if (v1 < v2) + return v2; + else + return v1; +} + +static s32 min_compare(s32 v1, s32 v2)Quoted 8 lines
Review
Quoted 7 lines
(quoted diff) +7
-0+{ + if (v1 < v2) + return v1; + else + return v2; +} +min()/max() instead?
Other code wants function pointers to the min & max functions. That's why they are here AFAICT.
Quoted 8 lines
Quoted 4 lines
Review
Quoted 3 lines
(quoted diff) +3
-0+/* assumes qos_lock is held */ +static void update_target(int i) +{'target' might be a more meaningful variable name.
Anything but 'i'.
---
Updated qos PM parameter patch:
Note: the replacing of latency.c with this is a separate patch.
this patch attempts to address the issues raised so far.
--mgross
mgross@mgross-t43:~/workstuff/power_qos$ diffstat -p1 qos_params_sept_27.patch include/linux/qos_params.h | 35 +++ kernel/Makefile | 2 kernel/qos_params.c | 413 +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 449 insertions(+), 1 deletion(-)
include/linux/qos_params.h +35 -0
@@ -0,0 +1,35 @@ +/* interface for the qos_power infrastructure of the linux kernel. +* +* Mark Gross +*/ +#include <linux/list.h> +#include <linux/notifier.h> +#include <linux/miscdevice.h> + +struct requirement_list { +struct list_head list; +union { +s32 value; +s32 usec; +s32 kbps; + }; +char *name; +}; + +#define QOS_RESERVED 0 +#define QOS_CPU_DMA_LATENCY 1 +#define QOS_NETWORK_LATENCY 2 +#define QOS_NETWORK_THROUGHPUT 3 + +#define QOS_NUM_CLASSES 4 +#define QOS_DEFAULT_VALUE -1 + +int qos_add_requirement (int qos,char *name,s32 value); +int qos_update_requirement (int qos,char *name,s32 new_value); +void qos_remove_requirement (int qos,char *name); + +int qos_requirement (int qos); + +int qos_add_notifier (int qos,struct notifier_block *notifier); +int qos_remove_notifier (int qos,struct notifier_block *notifier); +
kernel/Makefile +1 -1
@@ -9,7 +9,7 @@ rcupdate.o extable.o params.o posix-timers.o \ kthread.o wait.o kfifo.o sys_ni.o posix-cpu-timers.o mutex.o \ hrtimer.o rwsem.o latency.o nsproxy.o srcu.o die_notifier.o \ - utsname.o + utsname.o qos_params.o obj-$(CONFIG_STACKTRACE) += stacktrace.o obj-y += time/
kernel/qos_params.c +413 -0
413 lines not on this page · raw message
Re: [RFC] QoS params patch update.
Quoted 5 lines · Mark Gross
Updated qos PM parameter patch:
Note: the replacing oflatency.cwith this is a separate patch.this patch attempts to address the issues raised so far.
[snip]
kernel/qos_params.c line 116
Quoted 13 lines
kernel/qos_params.c +13
-0+static int register_new_qos_misc (struct qos_object *qos) +{ +int ret; + + qos->qos_power_miscdev.minor =MISC_DYNAMIC_MINOR ; + qos->qos_power_miscdev.name = qos->name; + qos->qos_power_miscdev.fops = &qos_power_fops; + + ret =misc_register (&qos->qos_power_miscdev); + +return ret; +} +
Minor nit, ret is a pointless variable here, you can just return misc_register directly.
Other than that, this looks much better!
Re: [RFC] QoS params patch update.
In reply to Mark GrossQuoted 5 lines · Mark Gross
Updated qos PM parameter patch:
Note: the replacing oflatency.cwith this is a separate patch.this patch attempts to address the issues raised so far.
[snip]
kernel/qos_params.c line 116
Quoted 13 lines
kernel/qos_params.c +13
-0+static int register_new_qos_misc (struct qos_object *qos) +{ +int ret; + + qos->qos_power_miscdev.minor =MISC_DYNAMIC_MINOR ; + qos->qos_power_miscdev.name = qos->name; + qos->qos_power_miscdev.fops = &qos_power_fops; + + ret =misc_register (&qos->qos_power_miscdev); + +return ret; +} +
Minor nit, ret is a pointless variable here, you can just return misc_register directly.
Other than that, this looks much better!
Re: [RFC] QoS params patch
In reply to Paul MundtQuoted 11 lines · Paul Mundt
include/linux/qos_params.h line 16
Quoted 7 lines · Mark Gross
include/linux/qos_params.h +7
-0+struct list_head list; +union { +s32 value; +s32 usec; +s32 kbps; + }; +char *name;Your } is in a strange place. It looks like it wants to join its friends closer to the left margin ;-)
fixed.
Quoted 13 lines
include/linux/qos_params.h line 25
Quoted 10 lines
include/linux/qos_params.h +9
-0+}; + +#define QOS_RESERVED 0 +#define QOS_CPU_DMA_LATENCY 1 +#define QOS_NETWORK_LATENCY 2 +#define QOS_NETWORK_THROUGHPUT 3 + +#define QOS_NUM_CLASSES 4 +#define QOS_DEFAULT_VALUE -1an enum would be better for this, especially as people are likely to add new types, having to update
QOS_NUM_CLASSESeach time sucks.
I'm partial to the simplicity of not using an enum in this case. We do want it to be somewhat non-trivial to add qos classes.
Quoted 11 lines
Quoted 11 lines
kernel/qos_params.c +10
-0+/* static helper functions */ +static s32 max_compare (s32 v1,s32 v2) +{ +if (v1 < v2) +return v2; +else +return v1; +} + +static s32 min_compare (s32 v1,s32 v2)
Quoted 8 lines
kernel/qos_params.c line 89
Quoted 7 lines
kernel/qos_params.c +7
-0+{ +if (v1 < v2) +return v1; +else +return v2; +} +min()/max() instead?
done.
Quoted 5 lines
kernel/qos_params.c line 92
Quoted 4 lines
kernel/qos_params.c +3
-0+/* assumes qos_lock is held */ +static void update_target (int i) +{'target' might be a more meaningful variable name.
done.
Quoted 11 lines
kernel/qos_params.c line 124
Quoted 10 lines
kernel/qos_params.c +9
-0+ qos->qos_power_miscdev.fops = &qos_power_fops; + + ret =misc_register (& qos->qos_power_miscdev); +if (ret <0 ) +printk (KERN_ERR "qos_power: misc_register returns %d.\n" , ret); + +return ret; +} +Just return
misc_register(...); ?
ok.
Quoted 15 lines
Quoted 15 lines
kernel/qos_params.c +14
-0+if ( i <QOS_NUM_CLASSES ) { + qos = &qos_array[i]; + qos->name =kstrdup (name,GFP_KERNEL ); +if (!qos->name) + goto cleanup; + + qos->default_value = default_value; + qos->target_value = default_value; + qos->comparitor = comparitor; +srcu_init_notifier_head (&qos->notifiers); +INIT_LIST_HEAD (&qos->requirements.list); +if (register_new_qos_misc (qos) <0 ) + goto cleanup; + }
Quoted 8 lines
kernel/qos_params.c line 149
Quoted 4 lines
kernel/qos_params.c +4
-0+return ; +cleanup: +kfree (qos->name); +}This leaks. You'll have to scan down from i and clean up the
kstrdup()perqos_arrayelement. Presently this will only free the first one to fail registration.
Not seeing the leak here. this is an if block not a for loop.
Quoted 13 lines
kernel/qos_params.c line 195
Quoted 9 lines
kernel/qos_params.c +8
-0+ +int qos_add_requirement (int i,char *name,s32 value) +{ +struct requirement_list * dep; +unsigned long flags; + +spin_lock_irqsave (&qos_lock, flags); + dep =kzalloc (sizeof (struct requirement_list ),GFP_KERNEL );
kmalloc()under a spinlock.GFP_KERNELimplies__GFP_WAIT, which can sleep. Slab debugging would have caught this, too.
/me turns on slab debugging.
Quoted 11 lines
kernel/qos_params.c line 201
Quoted 6 lines
kernel/qos_params.c +6
-0+if (dep) { +if (value ==QOS_DEFAULT_VALUE ) + dep->value = qos_array[i].default_value; +else + dep->value = value; + dep->name =kstrdup (name,GFP_KERNEL );And here also, still under the spinlock. You can probably rework the locking just to protect the list, if you really need it at all, it doesn't seem to matter anywhere else here.
Re-worked. I didn't need the lock to be held as long as I had it. Thanks for catching this!
Quoted 8 lines
Quoted 8 lines
kernel/qos_params.c +8
-0+if (!dep->name) + goto cleanup; + +list_add (&dep->list, &qos_array[i].requirements.list); +update_target (i); +spin_unlock_irqrestore (&qos_lock, flags); + +return 0 ;
Quoted 8 lines
kernel/qos_params.c line 215
Quoted 6 lines
kernel/qos_params.c +6
-0+ } +spin_unlock_irqrestore (&qos_lock, flags); + +cleanup: +if (dep) +kfree (dep);no
if()needed.
fixed.
Quoted 7 lines
kernel/qos_params.c line 218
Quoted 4 lines
kernel/qos_params.c +3
-0+return -ENOMEM ; +} +EXPORT_SYMBOL_GPL (qos_add_requirement);You also didn't
spin_unlock_irqrestore()in the error path, so this bails out with the lock held and IRQs disabled.
fixed by the re-work.
--mgross
Re: [RFC] QoS params patch
In reply to Paul MundtQuoted 11 lines · Paul Mundt
include/linux/qos_params.h line 16
Quoted 7 lines · Mark Gross
include/linux/qos_params.h +7
-0+struct list_head list; +union { +s32 value; +s32 usec; +s32 kbps; + }; +char *name;Your } is in a strange place. It looks like it wants to join its friends closer to the left margin ;-)
fixed.
Quoted 13 lines
include/linux/qos_params.h line 25
Quoted 10 lines
include/linux/qos_params.h +9
-0+}; + +#define QOS_RESERVED 0 +#define QOS_CPU_DMA_LATENCY 1 +#define QOS_NETWORK_LATENCY 2 +#define QOS_NETWORK_THROUGHPUT 3 + +#define QOS_NUM_CLASSES 4 +#define QOS_DEFAULT_VALUE -1an enum would be better for this, especially as people are likely to add new types, having to update
QOS_NUM_CLASSESeach time sucks.
I'm partial to the simplicity of not using an enum in this case. We do want it to be somewhat non-trivial to add qos classes.
Quoted 11 lines
Quoted 11 lines
kernel/qos_params.c +10
-0+/* static helper functions */ +static s32 max_compare (s32 v1,s32 v2) +{ +if (v1 < v2) +return v2; +else +return v1; +} + +static s32 min_compare (s32 v1,s32 v2)
Quoted 8 lines
kernel/qos_params.c line 89
Quoted 7 lines
kernel/qos_params.c +7
-0+{ +if (v1 < v2) +return v1; +else +return v2; +} +min()/max() instead?
done.
Quoted 5 lines
kernel/qos_params.c line 92
Quoted 4 lines
kernel/qos_params.c +3
-0+/* assumes qos_lock is held */ +static void update_target (int i) +{'target' might be a more meaningful variable name.
done.
Quoted 11 lines
kernel/qos_params.c line 124
Quoted 10 lines
kernel/qos_params.c +9
-0+ qos->qos_power_miscdev.fops = &qos_power_fops; + + ret =misc_register (& qos->qos_power_miscdev); +if (ret <0 ) +printk (KERN_ERR "qos_power: misc_register returns %d.\n" , ret); + +return ret; +} +Just return
misc_register(...); ?
ok.
Quoted 15 lines
Quoted 15 lines
kernel/qos_params.c +14
-0+if ( i <QOS_NUM_CLASSES ) { + qos = &qos_array[i]; + qos->name =kstrdup (name,GFP_KERNEL ); +if (!qos->name) + goto cleanup; + + qos->default_value = default_value; + qos->target_value = default_value; + qos->comparitor = comparitor; +srcu_init_notifier_head (&qos->notifiers); +INIT_LIST_HEAD (&qos->requirements.list); +if (register_new_qos_misc (qos) <0 ) + goto cleanup; + }
Quoted 8 lines
kernel/qos_params.c line 149
Quoted 4 lines
kernel/qos_params.c +4
-0+return ; +cleanup: +kfree (qos->name); +}This leaks. You'll have to scan down from i and clean up the
kstrdup()perqos_arrayelement. Presently this will only free the first one to fail registration.
Not seeing the leak here. this is an if block not a for loop.
Quoted 13 lines
kernel/qos_params.c line 195
Quoted 9 lines
kernel/qos_params.c +8
-0+ +int qos_add_requirement (int i,char *name,s32 value) +{ +struct requirement_list * dep; +unsigned long flags; + +spin_lock_irqsave (&qos_lock, flags); + dep =kzalloc (sizeof (struct requirement_list ),GFP_KERNEL );
kmalloc()under a spinlock.GFP_KERNELimplies__GFP_WAIT, which can sleep. Slab debugging would have caught this, too.
/me turns on slab debugging.
Quoted 11 lines
kernel/qos_params.c line 201
Quoted 6 lines
kernel/qos_params.c +6
-0+if (dep) { +if (value ==QOS_DEFAULT_VALUE ) + dep->value = qos_array[i].default_value; +else + dep->value = value; + dep->name =kstrdup (name,GFP_KERNEL );And here also, still under the spinlock. You can probably rework the locking just to protect the list, if you really need it at all, it doesn't seem to matter anywhere else here.
Re-worked. I didn't need the lock to be held as long as I had it. Thanks for catching this!
Quoted 8 lines
Quoted 8 lines
kernel/qos_params.c +8
-0+if (!dep->name) + goto cleanup; + +list_add (&dep->list, &qos_array[i].requirements.list); +update_target (i); +spin_unlock_irqrestore (&qos_lock, flags); + +return 0 ;
Quoted 8 lines
kernel/qos_params.c line 215
Quoted 6 lines
kernel/qos_params.c +6
-0+ } +spin_unlock_irqrestore (&qos_lock, flags); + +cleanup: +if (dep) +kfree (dep);no
if()needed.
fixed.
Quoted 7 lines
kernel/qos_params.c line 218
Quoted 4 lines
kernel/qos_params.c +3
-0+return -ENOMEM ; +} +EXPORT_SYMBOL_GPL (qos_add_requirement);You also didn't
spin_unlock_irqrestore()in the error path, so this bails out with the lock held and IRQs disabled.
fixed by the re-work.
--mgross
Re: [RFC] QoS params patch
In reply to Mark Grossinclude/linux/qos_params.h line 29
Quoted 11 lines · On Wed, 26 Sep 2007 15:40:26 -0700 Mark Gross <[email protected]> wrote:
include/linux/qos_params.h +11
-0+#define QOS_RESERVED 0 +#define QOS_CPU_DMA_LATENCY 1 +#define QOS_NETWORK_LATENCY 2 +#define QOS_NETWORK_THROUGHPUT 3 + +#define QOS_NUM_CLASSES 4 +#define QOS_DEFAULT_VALUE -1 + +int qos_add_requirement (int qos,char *name,s32 value); +int qos_update_requirement (int qos,char *name,s32 new_value); +void qos_remove_requirement (int qos,char *name);
It's a bit rude stealing the entire "qos" namespace like this - there are many different forms of QoS, some already in-kernel.
s/qos/pm_qos/g ?
Re: [RFC] QoS params patch
In reply to Mark Grossinclude/linux/qos_params.h line 29
Quoted 11 lines · On Wed, 26 Sep 2007 15:40:26 -0700 Mark Gross <[email protected]> wrote:
include/linux/qos_params.h +11
-0+#define QOS_RESERVED 0 +#define QOS_CPU_DMA_LATENCY 1 +#define QOS_NETWORK_LATENCY 2 +#define QOS_NETWORK_THROUGHPUT 3 + +#define QOS_NUM_CLASSES 4 +#define QOS_DEFAULT_VALUE -1 + +int qos_add_requirement (int qos,char *name,s32 value); +int qos_update_requirement (int qos,char *name,s32 new_value); +void qos_remove_requirement (int qos,char *name);
It's a bit rude stealing the entire "qos" namespace like this - there are many different forms of QoS, some already in-kernel.
s/qos/pm_qos/g ?
Re: [RFC] QoS params patch
Quoted 9 lines · Andrew Morton
include/linux/qos_params.h line 29
Quoted 3 lines · On Wed, 26 Sep 2007 15:40:26 -0700 Mark Gross <[email protected]> wrote:
include/linux/qos_params.h +3
-0+int qos_add_requirement (int qos,char *name,s32 value); +int qos_update_requirement (int qos,char *name,s32 new_value); +void qos_remove_requirement (int qos,char *name);It's a bit rude stealing the entire "qos" namespace like this - there are many different forms of QoS, some already in-kernel.
s/qos/pm_qos/g ?
lat_qos or something might be more suitable.. it's a latency property, not a power management one (even if pm ends up being the primary user of it).
Re: [RFC] QoS params patch
In reply to Andrew MortonQuoted 9 lines · Andrew Morton
include/linux/qos_params.h line 29
Quoted 3 lines · On Wed, 26 Sep 2007 15:40:26 -0700 Mark Gross <[email protected]> wrote:
include/linux/qos_params.h +3
-0+int qos_add_requirement (int qos,char *name,s32 value); +int qos_update_requirement (int qos,char *name,s32 new_value); +void qos_remove_requirement (int qos,char *name);It's a bit rude stealing the entire "qos" namespace like this - there are many different forms of QoS, some already in-kernel.
s/qos/pm_qos/g ?
lat_qos or something might be more suitable.. it's a latency property, not a power management one (even if pm ends up being the primary user of it).
Re: [RFC] QoS params patch
Quoted 9 lines · Paul Mundt
Quoted 8 lines · Andrew Morton
include/linux/qos_params.h line 29
Quoted 3 lines · On Wed, 26 Sep 2007 15:40:26 -0700 Mark Gross <[email protected]> wrote:
include/linux/qos_params.h +3
-0+int qos_add_requirement (int qos,char *name,s32 value); +int qos_update_requirement (int qos,char *name,s32 new_value); +void qos_remove_requirement (int qos,char *name);It's a bit rude stealing the entire "qos" namespace like this - there are many different forms of QoS, some already in-kernel.
Quoted 4 lines
Quoted 1 line
s/qos/pm_qos/g ?
lat_qosor something might be more suitable.. it's a latency property, not a power management one (even if pm ends up being the primary user of it).
Its not just latency otherwise I'd agree with you. right now I'm thinking of changing things to "qospm" unless there folks feel strongly about pm_qos.
--mgross
Re: [RFC] QoS params patch
In reply to Paul MundtQuoted 9 lines · Paul Mundt
Quoted 8 lines · Andrew Morton
include/linux/qos_params.h line 29
Quoted 3 lines · On Wed, 26 Sep 2007 15:40:26 -0700 Mark Gross <[email protected]> wrote:
include/linux/qos_params.h +3
-0+int qos_add_requirement (int qos,char *name,s32 value); +int qos_update_requirement (int qos,char *name,s32 new_value); +void qos_remove_requirement (int qos,char *name);It's a bit rude stealing the entire "qos" namespace like this - there are many different forms of QoS, some already in-kernel.
Quoted 4 lines
Quoted 1 line
s/qos/pm_qos/g ?
lat_qosor something might be more suitable.. it's a latency property, not a power management one (even if pm ends up being the primary user of it).
Its not just latency otherwise I'd agree with you. right now I'm thinking of changing things to "qospm" unless there folks feel strongly about pm_qos.
--mgross
Re: [RFC] QoS params patch
In reply to Andrew MortonQuoted 17 lines · Andrew Morton
include/linux/qos_params.h line 29
Quoted 11 lines · On Wed, 26 Sep 2007 15:40:26 -0700 Mark Gross <[email protected]> wrote:
include/linux/qos_params.h +11
-0+#define QOS_RESERVED 0 +#define QOS_CPU_DMA_LATENCY 1 +#define QOS_NETWORK_LATENCY 2 +#define QOS_NETWORK_THROUGHPUT 3 + +#define QOS_NUM_CLASSES 4 +#define QOS_DEFAULT_VALUE -1 + +int qos_add_requirement (int qos,char *name,s32 value); +int qos_update_requirement (int qos,char *name,s32 new_value); +void qos_remove_requirement (int qos,char *name);It's a bit rude stealing the entire "qos" namespace like this - there are many different forms of QoS, some already in-kernel.
Quoted 1 line
s/qos/pm_qos/g ?
I suppose it is a bit inconiderate. I could grow to like pm_qos, performance_throttling_constraint_hint_infrastructure is a bit too wordy.
I suppose I should use qospm as thats the way it was put up on that lesswatts.org web page.
Would qospm be good enough?
--mgross
Re: [RFC] QoS params patch
In reply to Andrew MortonQuoted 17 lines · Andrew Morton
include/linux/qos_params.h line 29
Quoted 11 lines · On Wed, 26 Sep 2007 15:40:26 -0700 Mark Gross <[email protected]> wrote:
include/linux/qos_params.h +11
-0+#define QOS_RESERVED 0 +#define QOS_CPU_DMA_LATENCY 1 +#define QOS_NETWORK_LATENCY 2 +#define QOS_NETWORK_THROUGHPUT 3 + +#define QOS_NUM_CLASSES 4 +#define QOS_DEFAULT_VALUE -1 + +int qos_add_requirement (int qos,char *name,s32 value); +int qos_update_requirement (int qos,char *name,s32 new_value); +void qos_remove_requirement (int qos,char *name);It's a bit rude stealing the entire "qos" namespace like this - there are many different forms of QoS, some already in-kernel.
Quoted 1 line
s/qos/pm_qos/g ?
I suppose it is a bit inconiderate. I could grow to like pm_qos, performance_throttling_constraint_hint_infrastructure is a bit too wordy.
I suppose I should use qospm as thats the way it was put up on that lesswatts.org web page.
Would qospm be good enough?
--mgross
Re: [RFC] QoS params patch
Quoted 20 lines · On Fri, 28 Sep 2007 10:19:21 -0700 Mark Gross <[email protected]> wrote:
Quoted 17 lines · Andrew Morton
include/linux/qos_params.h line 29
Quoted 11 lines · On Wed, 26 Sep 2007 15:40:26 -0700 Mark Gross <[email protected]> wrote:
include/linux/qos_params.h +11
-0+#define QOS_RESERVED 0 +#define QOS_CPU_DMA_LATENCY 1 +#define QOS_NETWORK_LATENCY 2 +#define QOS_NETWORK_THROUGHPUT 3 + +#define QOS_NUM_CLASSES 4 +#define QOS_DEFAULT_VALUE -1 + +int qos_add_requirement (int qos,char *name,s32 value); +int qos_update_requirement (int qos,char *name,s32 new_value); +void qos_remove_requirement (int qos,char *name);It's a bit rude stealing the entire "qos" namespace like this - there are many different forms of QoS, some already in-kernel.
Quoted 1 line
s/qos/pm_qos/g ?
Quoted 9 lines
I suppose it is a bit inconiderate. I could grow to like
pm_qos,performance_throttling_constraint_hint_infrastructureis a bit too wordy.I suppose I should use qospm as thats the way it was put up on that lesswatts.org web page.
Would qospm be good enough?
Don't think it matters a lot, but kernel naming tends to be big-endian (ie:
we have net_ratelimit, not ratelimit_net), so the major part (pm) would come first under that scheme.
Re: [RFC] QoS params patch
In reply to Mark GrossQuoted 20 lines · On Fri, 28 Sep 2007 10:19:21 -0700 Mark Gross <[email protected]> wrote:
Quoted 17 lines · Andrew Morton
include/linux/qos_params.h line 29
Quoted 11 lines · On Wed, 26 Sep 2007 15:40:26 -0700 Mark Gross <[email protected]> wrote:
include/linux/qos_params.h +11
-0+#define QOS_RESERVED 0 +#define QOS_CPU_DMA_LATENCY 1 +#define QOS_NETWORK_LATENCY 2 +#define QOS_NETWORK_THROUGHPUT 3 + +#define QOS_NUM_CLASSES 4 +#define QOS_DEFAULT_VALUE -1 + +int qos_add_requirement (int qos,char *name,s32 value); +int qos_update_requirement (int qos,char *name,s32 new_value); +void qos_remove_requirement (int qos,char *name);It's a bit rude stealing the entire "qos" namespace like this - there are many different forms of QoS, some already in-kernel.
Quoted 1 line
s/qos/pm_qos/g ?
Quoted 9 lines
I suppose it is a bit inconiderate. I could grow to like
pm_qos,performance_throttling_constraint_hint_infrastructureis a bit too wordy.I suppose I should use qospm as thats the way it was put up on that lesswatts.org web page.
Would qospm be good enough?
Don't think it matters a lot, but kernel naming tends to be big-endian (ie:
we have net_ratelimit, not ratelimit_net), so the major part (pm) would come first under that scheme.
Re: [RFC] QoS params patch
Quoted 29 lines · Andrew Morton
Quoted 27 lines · On Fri, 28 Sep 2007 10:19:21 -0700 Mark Gross <[email protected]> wrote:
Quoted 17 lines · Andrew Morton
include/linux/qos_params.h line 29
Quoted 11 lines · On Wed, 26 Sep 2007 15:40:26 -0700 Mark Gross <[email protected]> wrote:
include/linux/qos_params.h +11
-0+#define QOS_RESERVED 0 +#define QOS_CPU_DMA_LATENCY 1 +#define QOS_NETWORK_LATENCY 2 +#define QOS_NETWORK_THROUGHPUT 3 + +#define QOS_NUM_CLASSES 4 +#define QOS_DEFAULT_VALUE -1 + +int qos_add_requirement (int qos,char *name,s32 value); +int qos_update_requirement (int qos,char *name,s32 new_value); +void qos_remove_requirement (int qos,char *name);It's a bit rude stealing the entire "qos" namespace like this - there are many different forms of QoS, some already in-kernel.
Quoted 1 line
s/qos/pm_qos/g ?
I suppose it is a bit inconiderate. I could grow to like
pm_qos,performance_throttling_constraint_hint_infrastructureis a bit too wordy.I suppose I should use qospm as thats the way it was put up on that lesswatts.org web page.
Quoted 6 lines
Quoted 2 lines
Would qospm be good enough?
Don't think it matters a lot, but kernel naming tends to be big-endian (ie:
we havenet_ratelimit, notratelimit_net), so the major part (pm) would come first under that scheme.
this makes sense. pm_qos it is.
--mgross
Re: [RFC] QoS params patch
In reply to Andrew MortonQuoted 29 lines · Andrew Morton
Quoted 27 lines · On Fri, 28 Sep 2007 10:19:21 -0700 Mark Gross <[email protected]> wrote:
Quoted 17 lines · Andrew Morton
include/linux/qos_params.h line 29
Quoted 11 lines · On Wed, 26 Sep 2007 15:40:26 -0700 Mark Gross <[email protected]> wrote:
include/linux/qos_params.h +11
-0+#define QOS_RESERVED 0 +#define QOS_CPU_DMA_LATENCY 1 +#define QOS_NETWORK_LATENCY 2 +#define QOS_NETWORK_THROUGHPUT 3 + +#define QOS_NUM_CLASSES 4 +#define QOS_DEFAULT_VALUE -1 + +int qos_add_requirement (int qos,char *name,s32 value); +int qos_update_requirement (int qos,char *name,s32 new_value); +void qos_remove_requirement (int qos,char *name);It's a bit rude stealing the entire "qos" namespace like this - there are many different forms of QoS, some already in-kernel.
Quoted 1 line
s/qos/pm_qos/g ?
I suppose it is a bit inconiderate. I could grow to like
pm_qos,performance_throttling_constraint_hint_infrastructureis a bit too wordy.I suppose I should use qospm as thats the way it was put up on that lesswatts.org web page.
Quoted 6 lines
Quoted 2 lines
Would qospm be good enough?
Don't think it matters a lot, but kernel naming tends to be big-endian (ie:
we havenet_ratelimit, notratelimit_net), so the major part (pm) would come first under that scheme.
this makes sense. pm_qos it is.
--mgross
The following patch replaces latency.c with qos_params.c and fixes up users of latency to use qos_params
drivers/acpi/processor_idle.c +6 -6
@@ -38,7 +38,7 @@#include <linux/dmi.h> #include <linux/moduleparam.h> #include <linux/sched.h> /* need_resched() */ -#include <linux/latency.h> +#include <linux/qos_params.h> #include <linux/clockchips.h> /*
@@ -605,7 +605,7 @@if (cx->promotion.state && ((cx->promotion.state - pr->power.states) <= max_cstate)) {if (sleep_ticks > cx->promotion.threshold.ticks && - cx->promotion.state->latency <=system_latency_constraint ()) { + cx->promotion.state->latency <=qos_requirement (QOS_CPU_DMA_LATENCY )) { cx->promotion.count++; cx->demotion.count =0 ; if (cx->promotion.count >=
@@ -649,7 +649,7 @@* or if the latency of the current state is unacceptable */ if ((pr->power.state - pr->power.states) > max_cstate || - pr->power.state->latency >system_latency_constraint ()) { + pr->power.state->latency >qos_requirement (QOS_CPU_DMA_LATENCY )) {if (cx->demotion.state) next_state = cx->demotion.state; }
@@ -1173,7 +1173,7 @@"maximum allowed latency: %d usec\n" , pr->power.state ? pr->power.state - pr->power.states :0 , max_cstate, (unsigned )pr->power.bm_activity, - system_latency_constraint()); + qos_requirement(QOS_CPU_DMA_LATENCY));seq_puts (seq,"states:\n" );
@@ -1280,7 +1280,7 @@ max_cstate); first_run++;#ifdef CONFIG_SMP -register_latency_notifier (&acpi_processor_latency_notifier); +qos_add_notifier (QOS_CPU_DMA_LATENCY , &acpi_processor_latency_notifier);#endif }
@@ -1354,7 +1354,7 @@*/ cpu_idle_wait ();#ifdef CONFIG_SMP -unregister_latency_notifier (&acpi_processor_latency_notifier); +qos_remove_notifier (QOS_CPU_DMA_LATENCY , &acpi_processor_latency_notifier);#endif }
drivers/net/wireless/ipw2100.c +5 -5
@@ -162,7 +162,7 @@#include <linux/firmware.h> #include <linux/acpi.h> #include <linux/ctype.h> -#include <linux/latency.h> +#include <linux/qos_params.h> #include "ipw2100.h"
@@ -1701,7 +1701,7 @@/* the ipw2100 hardware really doesn't want power management delays * longer than 175usec */ -modify_acceptable_latency ("ipw2100" ,175 ); +qos_update_requirement (QOS_CPU_DMA_LATENCY ,"ipw2100" ,175 );/* If the interrupt is enabled, turn it off... */ spin_lock_irqsave (&priv->low_lock, flags);
@@ -1856,7 +1856,7 @@ipw2100_disable_interrupts (priv);spin_unlock_irqrestore (&priv->low_lock, flags); -modify_acceptable_latency ("ipw2100" ,INFINITE_LATENCY ); +qos_update_requirement (QOS_CPU_DMA_LATENCY ,"ipw2100" ,QOS_DEFAULT_VALUE );#ifdef ACPI_CSTATE_LIMIT_DEFINED if (priv->config &CFG_C3_DISABLED ) {
@@ -6544,7 +6544,7 @@if (ret) goto out; -set_acceptable_latency ("ipw2100" ,INFINITE_LATENCY ); +qos_add_requirement (QOS_CPU_DMA_LATENCY ,"ipw2100" ,QOS_DEFAULT_VALUE );#ifdef CONFIG_IPW2100_DEBUG ipw2100_debug_level = debug; ret = driver_create_file(&ipw2100_pci_driver.driver,
@@ -6566,7 +6566,7 @@ &driver_attr_debug_level);#endif pci_unregister_driver (&ipw2100_pci_driver); -remove_acceptable_latency ("ipw2100" ); +qos_remove_requirement (QOS_CPU_DMA_LATENCY ,"ipw2100" ); }module_init (ipw2100_init);
include/linux/latency.h +0 -25
@@ -1,25 +0,0 @@ -/* -* latency.h: Explicit system-wide latency-expectation infrastructure -* -* (C) Copyright 2006 Intel Corporation -* Author: Arjan van de Ven <[email protected]> -* -*/ - -#ifndef _INCLUDE_GUARD_LATENCY_H_ -#define _INCLUDE_GUARD_LATENCY_H_ - -#include <linux/notifier.h> - -void set_acceptable_latency (char *identifier,int usecs); -void modify_acceptable_latency (char *identifier,int usecs); -void remove_acceptable_latency (char *identifier); -void synchronize_acceptable_latency (void ); -int system_latency_constraint (void ); - -int register_latency_notifier (struct notifier_block * nb); -int unregister_latency_notifier (struct notifier_block * nb); - -#define INFINITE_LATENCY 1000000 - -#endif
kernel/latency.c +0 -280
280 lines not on this page · raw message
kernel/Makefile +1 -1
@@ -8,7 +8,7 @@ signal.o sys.o kmod.o workqueue.o pid.o \ rcupdate.o extable.o params.o posix-timers.o \ kthread.o wait.o kfifo.o sys_ni.o posix-cpu-timers.o mutex.o \ - hrtimer.o rwsem.o latency.o nsproxy.o srcu.o die_notifier.o \ + hrtimer.o rwsem.o nsproxy.o srcu.o die_notifier.o \ utsname.o qos_params.o obj-$(CONFIG_STACKTRACE) += stacktrace.o
sound/core/pcm_native.c +4 -4
@@ -24,7 +24,7 @@#include <linux/file.h> #include <linux/slab.h> #include <linux/time.h> -#include <linux/latency.h> +#include <linux/qos_params.h> #include <linux/uio.h> #include <sound/core.h> #include <sound/control.h>
@@ -447,9 +447,9 @@snd_pcm_timer_resolution_change (substream); runtime->status->state =SNDRV_PCM_STATE_SETUP ; -remove_acceptable_latency (substream->latency_id); +qos_remove_requirement (QOS_CPU_DMA_LATENCY , substream->latency_id);if ((usecs =period_to_usecs (runtime)) >=0 ) -set_acceptable_latency (substream->latency_id, usecs); +qos_add_requirement (QOS_CPU_DMA_LATENCY , substream->latency_id, usecs);return 0 ; _error:/* hardware might be unuseable from this time,
@@ -509,7 +509,7 @@if (substream->ops->hw_free) result = substream->ops->hw_free (substream); runtime->status->state =SNDRV_PCM_STATE_OPEN ; -remove_acceptable_latency (substream->latency_id); +qos_remove_requirement (QOS_CPU_DMA_LATENCY , substream->latency_id);return result; }
The following patch replaces latency.c with qos_params.c and fixes up users of latency to use qos_params
drivers/acpi/processor_idle.c +6 -6
@@ -38,7 +38,7 @@#include <linux/dmi.h> #include <linux/moduleparam.h> #include <linux/sched.h> /* need_resched() */ -#include <linux/latency.h> +#include <linux/qos_params.h> #include <linux/clockchips.h> /*
@@ -605,7 +605,7 @@if (cx->promotion.state && ((cx->promotion.state - pr->power.states) <= max_cstate)) {if (sleep_ticks > cx->promotion.threshold.ticks && - cx->promotion.state->latency <=system_latency_constraint ()) { + cx->promotion.state->latency <=qos_requirement (QOS_CPU_DMA_LATENCY )) { cx->promotion.count++; cx->demotion.count =0 ; if (cx->promotion.count >=
@@ -649,7 +649,7 @@* or if the latency of the current state is unacceptable */ if ((pr->power.state - pr->power.states) > max_cstate || - pr->power.state->latency >system_latency_constraint ()) { + pr->power.state->latency >qos_requirement (QOS_CPU_DMA_LATENCY )) {if (cx->demotion.state) next_state = cx->demotion.state; }
@@ -1173,7 +1173,7 @@"maximum allowed latency: %d usec\n" , pr->power.state ? pr->power.state - pr->power.states :0 , max_cstate, (unsigned )pr->power.bm_activity, - system_latency_constraint()); + qos_requirement(QOS_CPU_DMA_LATENCY));seq_puts (seq,"states:\n" );
@@ -1280,7 +1280,7 @@ max_cstate); first_run++;#ifdef CONFIG_SMP -register_latency_notifier (&acpi_processor_latency_notifier); +qos_add_notifier (QOS_CPU_DMA_LATENCY , &acpi_processor_latency_notifier);#endif }
@@ -1354,7 +1354,7 @@*/ cpu_idle_wait ();#ifdef CONFIG_SMP -unregister_latency_notifier (&acpi_processor_latency_notifier); +qos_remove_notifier (QOS_CPU_DMA_LATENCY , &acpi_processor_latency_notifier);#endif }
drivers/net/wireless/ipw2100.c +5 -5
@@ -162,7 +162,7 @@#include <linux/firmware.h> #include <linux/acpi.h> #include <linux/ctype.h> -#include <linux/latency.h> +#include <linux/qos_params.h> #include "ipw2100.h"
@@ -1701,7 +1701,7 @@/* the ipw2100 hardware really doesn't want power management delays * longer than 175usec */ -modify_acceptable_latency ("ipw2100" ,175 ); +qos_update_requirement (QOS_CPU_DMA_LATENCY ,"ipw2100" ,175 );/* If the interrupt is enabled, turn it off... */ spin_lock_irqsave (&priv->low_lock, flags);
@@ -1856,7 +1856,7 @@ipw2100_disable_interrupts (priv);spin_unlock_irqrestore (&priv->low_lock, flags); -modify_acceptable_latency ("ipw2100" ,INFINITE_LATENCY ); +qos_update_requirement (QOS_CPU_DMA_LATENCY ,"ipw2100" ,QOS_DEFAULT_VALUE );#ifdef ACPI_CSTATE_LIMIT_DEFINED if (priv->config &CFG_C3_DISABLED ) {
@@ -6544,7 +6544,7 @@if (ret) goto out; -set_acceptable_latency ("ipw2100" ,INFINITE_LATENCY ); +qos_add_requirement (QOS_CPU_DMA_LATENCY ,"ipw2100" ,QOS_DEFAULT_VALUE );#ifdef CONFIG_IPW2100_DEBUG ipw2100_debug_level = debug; ret = driver_create_file(&ipw2100_pci_driver.driver,
@@ -6566,7 +6566,7 @@ &driver_attr_debug_level);#endif pci_unregister_driver (&ipw2100_pci_driver); -remove_acceptable_latency ("ipw2100" ); +qos_remove_requirement (QOS_CPU_DMA_LATENCY ,"ipw2100" ); }module_init (ipw2100_init);
include/linux/latency.h +0 -25
@@ -1,25 +0,0 @@ -/* -* latency.h: Explicit system-wide latency-expectation infrastructure -* -* (C) Copyright 2006 Intel Corporation -* Author: Arjan van de Ven <[email protected]> -* -*/ - -#ifndef _INCLUDE_GUARD_LATENCY_H_ -#define _INCLUDE_GUARD_LATENCY_H_ - -#include <linux/notifier.h> - -void set_acceptable_latency (char *identifier,int usecs); -void modify_acceptable_latency (char *identifier,int usecs); -void remove_acceptable_latency (char *identifier); -void synchronize_acceptable_latency (void ); -int system_latency_constraint (void ); - -int register_latency_notifier (struct notifier_block * nb); -int unregister_latency_notifier (struct notifier_block * nb); - -#define INFINITE_LATENCY 1000000 - -#endif
kernel/latency.c +0 -280
280 lines not on this page · raw message
kernel/Makefile +1 -1
@@ -8,7 +8,7 @@ signal.o sys.o kmod.o workqueue.o pid.o \ rcupdate.o extable.o params.o posix-timers.o \ kthread.o wait.o kfifo.o sys_ni.o posix-cpu-timers.o mutex.o \ - hrtimer.o rwsem.o latency.o nsproxy.o srcu.o die_notifier.o \ + hrtimer.o rwsem.o nsproxy.o srcu.o die_notifier.o \ utsname.o qos_params.o obj-$(CONFIG_STACKTRACE) += stacktrace.o
sound/core/pcm_native.c +4 -4
@@ -24,7 +24,7 @@#include <linux/file.h> #include <linux/slab.h> #include <linux/time.h> -#include <linux/latency.h> +#include <linux/qos_params.h> #include <linux/uio.h> #include <sound/core.h> #include <sound/control.h>
@@ -447,9 +447,9 @@snd_pcm_timer_resolution_change (substream); runtime->status->state =SNDRV_PCM_STATE_SETUP ; -remove_acceptable_latency (substream->latency_id); +qos_remove_requirement (QOS_CPU_DMA_LATENCY , substream->latency_id);if ((usecs =period_to_usecs (runtime)) >=0 ) -set_acceptable_latency (substream->latency_id, usecs); +qos_add_requirement (QOS_CPU_DMA_LATENCY , substream->latency_id, usecs);return 0 ; _error:/* hardware might be unuseable from this time,
@@ -509,7 +509,7 @@if (substream->ops->hw_free) result = substream->ops->hw_free (substream); runtime->status->state =SNDRV_PCM_STATE_OPEN ; -remove_acceptable_latency (substream->latency_id); +qos_remove_requirement (QOS_CPU_DMA_LATENCY , substream->latency_id);return result; }
Re: [RFC] QoS power example / hack
In reply to Mark GrossThe following patch is a bit of a hack to illustrate how the qos parameter infrastructure can communication information to the e1000 driver to use to set interrupt consolidation policy as a function of acceptable network latency.
Its just an example.
drivers/net/e1000/e1000_main.c +9 -0
@@ -27,6 +27,7 @@*******************************************************************************/ #include "e1000.h" +#include <linux/qos_params.h> #include <net/ip6_checksum.h> char e1000_driver_name[] ="e1000" ;
@@ -2764,6 +2765,7 @@ {unsigned int retval = itr_setting;struct e1000_hw *hw = &adapter->hw; +int requested_latency =qos_requirement (QOS_NETWORK_LATENCY );if (unlikely (hw->mac_type < e1000_82540)) goto update_itr_done;
@@ -2803,6 +2805,13 @@break ; } +if (requested_latency <50 ) + retval = lowest_latency; +else if (requested_latency <250 ) + retval = low_latency; +else + ;//don't change the current algorithm + update_itr_done:return retval; }
Re: [RFC] QoS power example / hack
In reply to Mark GrossThe following patch is a bit of a hack to illustrate how the qos parameter infrastructure can communication information to the e1000 driver to use to set interrupt consolidation policy as a function of acceptable network latency.
Its just an example.
drivers/net/e1000/e1000_main.c +9 -0
@@ -27,6 +27,7 @@*******************************************************************************/ #include "e1000.h" +#include <linux/qos_params.h> #include <net/ip6_checksum.h> char e1000_driver_name[] ="e1000" ;
@@ -2764,6 +2765,7 @@ {unsigned int retval = itr_setting;struct e1000_hw *hw = &adapter->hw; +int requested_latency =qos_requirement (QOS_NETWORK_LATENCY );if (unlikely (hw->mac_type < e1000_82540)) goto update_itr_done;
@@ -2803,6 +2805,13 @@break ; } +if (requested_latency <50 ) + retval = lowest_latency; +else if (requested_latency <250 ) + retval = low_latency; +else + ;//don't change the current algorithm + update_itr_done:return retval; }