Re: [RFC] QoS params patch
On Fri, Sep 28, 2007 at 11:51:41AM -0700, Andrew Morton wrote: > On Fri, 28 Sep 2007 10:19:21 -0700 Mark Gross <[EMAIL PROTECTED]> wrote: > > > On Thu, Sep 27, 2007 at 11:25:01PM -0700, Andrew Morton wrote: > > > On Wed, 26 Sep 2007 15:40:26 -0700 Mark Gross <[EMAIL PROTECTED]> wrote: > > > > > > > +#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 ? > > > > 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? > > > > 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. this makes sense. pm_qos it is. --mgross - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] QoS params patch
On Fri, Sep 28, 2007 at 11:51:41AM -0700, Andrew Morton wrote: On Fri, 28 Sep 2007 10:19:21 -0700 Mark Gross [EMAIL PROTECTED] wrote: On Thu, Sep 27, 2007 at 11:25:01PM -0700, Andrew Morton wrote: On Wed, 26 Sep 2007 15:40:26 -0700 Mark Gross [EMAIL PROTECTED] wrote: +#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 ? 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? 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. this makes sense. pm_qos it is. --mgross - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] QoS params patch
On Fri, 28 Sep 2007 10:19:21 -0700 Mark Gross <[EMAIL PROTECTED]> wrote: > On Thu, Sep 27, 2007 at 11:25:01PM -0700, Andrew Morton wrote: > > On Wed, 26 Sep 2007 15:40:26 -0700 Mark Gross <[EMAIL PROTECTED]> wrote: > > > > > +#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 ? > > 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? > 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. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] QoS params patch
On Fri, Sep 28, 2007 at 03:41:13PM +0900, Paul Mundt wrote: > On Thu, Sep 27, 2007 at 11:25:01PM -0700, Andrew Morton wrote: > > On Wed, 26 Sep 2007 15:40:26 -0700 Mark Gross <[EMAIL PROTECTED]> wrote: > > > +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). 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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] QoS params patch
On Thu, Sep 27, 2007 at 11:25:01PM -0700, Andrew Morton wrote: > On Wed, 26 Sep 2007 15:40:26 -0700 Mark Gross <[EMAIL PROTECTED]> wrote: > > > +#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 ? 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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] QoS params patch
On Thu, Sep 27, 2007 at 11:25:01PM -0700, Andrew Morton wrote: > On Wed, 26 Sep 2007 15:40:26 -0700 Mark Gross <[EMAIL PROTECTED]> wrote: > > +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). - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] QoS params patch
On Wed, 26 Sep 2007 15:40:26 -0700 Mark Gross <[EMAIL PROTECTED]> wrote: > +#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 ? - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] QoS params patch
On Wed, 26 Sep 2007 15:40:26 -0700 Mark Gross [EMAIL PROTECTED] wrote: +#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 ? - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] QoS params patch
On Thu, Sep 27, 2007 at 11:25:01PM -0700, Andrew Morton wrote: On Wed, 26 Sep 2007 15:40:26 -0700 Mark Gross [EMAIL PROTECTED] wrote: +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). - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] QoS params patch
On Thu, Sep 27, 2007 at 11:25:01PM -0700, Andrew Morton wrote: On Wed, 26 Sep 2007 15:40:26 -0700 Mark Gross [EMAIL PROTECTED] wrote: +#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 ? 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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] QoS params patch
On Fri, Sep 28, 2007 at 03:41:13PM +0900, Paul Mundt wrote: On Thu, Sep 27, 2007 at 11:25:01PM -0700, Andrew Morton wrote: On Wed, 26 Sep 2007 15:40:26 -0700 Mark Gross [EMAIL PROTECTED] wrote: +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). 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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] QoS params patch
On Fri, 28 Sep 2007 10:19:21 -0700 Mark Gross [EMAIL PROTECTED] wrote: On Thu, Sep 27, 2007 at 11:25:01PM -0700, Andrew Morton wrote: On Wed, 26 Sep 2007 15:40:26 -0700 Mark Gross [EMAIL PROTECTED] wrote: +#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 ? 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? 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. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] QoS params patch update.
On Thu, Sep 27, 2007 at 01:17:39PM -0700, Mark Gross wrote:
> 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.
>
[snip]
> +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 = _power_fops;
> +
> + ret = misc_register(>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!
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] QoS params patch update.
On Wed, Sep 26, 2007 at 09:05:15PM -0700, Randy Dunlap wrote:
> On Thu, 27 Sep 2007 11:24:40 +0900 Paul Mundt wrote:
>
> > > +/* 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)
> > > +{
> > > + 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.
>
>
> > > +/* 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
Signed-off-by: mark gross <[EMAIL PROTECTED]>
[EMAIL PROTECTED]:~/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(-)
diff -urN -X linux-2.6.23-rc8/Documentation/dontdiff
linux-2.6.23-rc8/include/linux/qos_params.h
linux-2.6.23-rc8-qos/include/linux/qos_params.h
--- linux-2.6.23-rc8/include/linux/qos_params.h 1969-12-31 16:00:00.0
-0800
+++ linux-2.6.23-rc8-qos/include/linux/qos_params.h 2007-09-27
08:42:54.0 -0700
@@ -0,0 +1,35 @@
+/* interface for the qos_power infrastructure of the linux kernel.
+ *
+ * Mark Gross
+ */
+#include
+#include
+#include
+
+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);
+
diff -urN -X linux-2.6.23-rc8/Documentation/dontdiff
linux-2.6.23-rc8/kernel/Makefile linux-2.6.23-rc8-qos/kernel/Makefile
--- linux-2.6.23-rc8/kernel/Makefile2007-09-26 13:54:54.0 -0700
+++ linux-2.6.23-rc8-qos/kernel/Makefile2007-09-26 14:06:38.0
-0700
@@ -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/
diff -urN -X linux-2.6.23-rc8/Documentation/dontdiff
linux-2.6.23-rc8/kernel/qos_params.c linux-2.6.23-rc8-qos/kernel/qos_params.c
--- linux-2.6.23-rc8/kernel/qos_params.c1969-12-31 16:00:00.0
-0800
+++ linux-2.6.23-rc8-qos/kernel/qos_params.c2007-09-27 12:46:13.0
-0700
@@ -0,0 +1,413 @@
+/*
+ * This module exposes the interface to kernel space for specifying
+ * QoS dependencies. It provides infrastructure for registration of:
+ *
+ * Dependents on a QoS value : register requirements
+ * Watchers of QoS value : get notified when target QoS value changes
+ *
+ * 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)
+ *
+ * There are lists of qos_objects each one wrapping requirements, notifiers
+ *
+ * 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
+ * client continues to be accounted for. Upon file release the usermode
+ * requirement is removed and a new qos target is computed. This way when the
+ * requirement that the application has is cleaned up when closes the file
+ * pointer or exits the qos_object will get an opportunity to clean up.
+ *
+ * mark gross [EMAIL PROTECTED]
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include
+
+/*
+ * locking rule: all changes to target_value or requirements or notifiers lists
+ * or qos_object list and qos_objects need to happen
Re: [RFC] QoS params patch
On Thu, Sep 27, 2007 at 11:24:40AM +0900, Paul Mundt wrote:
> On Wed, Sep 26, 2007 at 03:40:26PM -0700, Mark Gross wrote:
> > + 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.
>
> > +};
> > +
> > +#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.
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.
>
> > +/* 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)
> > +{
> > + if (v1 < v2)
> > + return v1;
> > + else
> > + return v2;
> > +}
> > +
> min()/max() instead?
done.
>
> > +/* assumes qos_lock is held */
> > +static void update_target(int i)
> > +{
> 'target' might be a more meaningful variable name.
done.
>
> > + qos->qos_power_miscdev.fops = _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.
>
> > + if( i < QOS_NUM_CLASSES) {
> > + 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(>notifiers);
> > + INIT_LIST_HEAD(>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.
Not seeing the leak here. this is an if block not a for loop.
>
> > +
> > +int qos_add_requirement(int i, char *name, s32 value)
> > +{
> > + struct requirement_list * dep;
> > + unsigned long flags;
> > +
> > + spin_lock_irqsave(_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.
>
/me turns on slab debugging.
> > + 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!
> > + if(!dep->name)
> > + goto cleanup;
> > +
> > + list_add(>list, _array[i].requirements.list);
> > + update_target(i);
> > + spin_unlock_irqrestore(_lock, flags);
> > +
> > + return 0;
> > + }
> > + spin_unlock_irqrestore(_lock, flags);
> > +
> > +cleanup:
> > + if(dep)
> > + kfree(dep);
>
> no if() needed.
fixed.
>
> > + 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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] QoS params patch
On Wed, Sep 26, 2007 at 09:05:15PM -0700, Randy Dunlap wrote:
> On Thu, 27 Sep 2007 11:24:40 +0900 Paul Mundt wrote:
>
> > > +/* 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)
> > > +{
> > > + 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.
>
>
> > > +/* 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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] QoS params patch
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. ;)
>
> > --- linux-2.6.23-rc8/kernel/Makefile2007-09-26 13:54:54.0
> > -0700
> > +++ linux-2.6.23-rc8-qos/kernel/Makefile2007-09-26 14:06:38.0 -
> 0700
> > @@ -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?
nope.
>
> > + * 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?
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
>
> > + * 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?
>
> > +/* 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...
>
> > +#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?
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] QoS params patch
On Thu, Sep 27, 2007 at 12:18:21PM +0900, Paul Mundt wrote: > 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: > > > --- linux-2.6.23-rc8/kernel/Makefile 2007-09-26 13:54:54.0 > > > -0700 > > > +++ linux-2.6.23-rc8-qos/kernel/Makefile 2007-09-26 14:06:38.0 - > > 0700 > > > @@ -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? > > > 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 :-) 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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] QoS params patch
Mark Gross wrote: > On Wed, Sep 26, 2007 at 04:41:59PM -0700, Randy Dunlap wrote: >> On Wed, 26 Sep 2007 15:40:26 -0700 Mark Gross wrote: >> >>> The following is the qos_param patch that implements a genralization of >>> latency.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 > >> - has no API docs :( > not done yet. >> >> >> >>> +/* 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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] QoS params patch
Mark Gross wrote: On Wed, Sep 26, 2007 at 04:41:59PM -0700, Randy Dunlap wrote: On Wed, 26 Sep 2007 15:40:26 -0700 Mark Gross wrote: The following is the qos_param patch that implements a genralization of latency.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 - has no API docs :( not done yet. +/* 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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] QoS params patch
On Thu, Sep 27, 2007 at 12:18:21PM +0900, Paul Mundt wrote: 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: --- linux-2.6.23-rc8/kernel/Makefile 2007-09-26 13:54:54.0 -0700 +++ linux-2.6.23-rc8-qos/kernel/Makefile 2007-09-26 14:06:38.0 - 0700 @@ -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? 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 :-) 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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] QoS params patch
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. ;) --- linux-2.6.23-rc8/kernel/Makefile2007-09-26 13:54:54.0 -0700 +++ linux-2.6.23-rc8-qos/kernel/Makefile2007-09-26 14:06:38.0 - 0700 @@ -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? nope. + * 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? 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 + * 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? +/* 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... +#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? - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] QoS params patch
On Wed, Sep 26, 2007 at 09:05:15PM -0700, Randy Dunlap wrote:
On Thu, 27 Sep 2007 11:24:40 +0900 Paul Mundt wrote:
+/* 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)
+{
+ 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.
+/* 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
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] QoS params patch
On Thu, Sep 27, 2007 at 11:24:40AM +0900, Paul Mundt wrote:
On Wed, Sep 26, 2007 at 03:40:26PM -0700, Mark Gross wrote:
+ 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.
+};
+
+#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.
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.
+/* 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)
+{
+ if (v1 v2)
+ return v1;
+ else
+ return v2;
+}
+
min()/max() instead?
done.
+/* assumes qos_lock is held */
+static void update_target(int i)
+{
'target' might be a more meaningful variable name.
done.
+ 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.
+ 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;
+ }
+ 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.
Not seeing the leak here. this is an if block not a for loop.
+
+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.
/me turns on slab debugging.
+ 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!
+ 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;
+ }
+ spin_unlock_irqrestore(qos_lock, flags);
+
+cleanup:
+ if(dep)
+ kfree(dep);
no if() needed.
fixed.
+ 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
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] QoS params patch update.
On Wed, Sep 26, 2007 at 09:05:15PM -0700, Randy Dunlap wrote:
On Thu, 27 Sep 2007 11:24:40 +0900 Paul Mundt wrote:
+/* 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)
+{
+ 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.
+/* 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
Signed-off-by: mark gross [EMAIL PROTECTED]
[EMAIL PROTECTED]:~/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(-)
diff -urN -X linux-2.6.23-rc8/Documentation/dontdiff
linux-2.6.23-rc8/include/linux/qos_params.h
linux-2.6.23-rc8-qos/include/linux/qos_params.h
--- linux-2.6.23-rc8/include/linux/qos_params.h 1969-12-31 16:00:00.0
-0800
+++ linux-2.6.23-rc8-qos/include/linux/qos_params.h 2007-09-27
08:42:54.0 -0700
@@ -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);
+
diff -urN -X linux-2.6.23-rc8/Documentation/dontdiff
linux-2.6.23-rc8/kernel/Makefile linux-2.6.23-rc8-qos/kernel/Makefile
--- linux-2.6.23-rc8/kernel/Makefile2007-09-26 13:54:54.0 -0700
+++ linux-2.6.23-rc8-qos/kernel/Makefile2007-09-26 14:06:38.0
-0700
@@ -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/
diff -urN -X linux-2.6.23-rc8/Documentation/dontdiff
linux-2.6.23-rc8/kernel/qos_params.c linux-2.6.23-rc8-qos/kernel/qos_params.c
--- linux-2.6.23-rc8/kernel/qos_params.c1969-12-31 16:00:00.0
-0800
+++ linux-2.6.23-rc8-qos/kernel/qos_params.c2007-09-27 12:46:13.0
-0700
@@ -0,0 +1,413 @@
+/*
+ * This module exposes the interface to kernel space for specifying
+ * QoS dependencies. It provides infrastructure for registration of:
+ *
+ * Dependents on a QoS value : register requirements
+ * Watchers of QoS value : get notified when target QoS value changes
+ *
+ * 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)
+ *
+ * There are lists of qos_objects each one wrapping requirements, notifiers
+ *
+ * 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
+ * client continues to be accounted for. Upon file release the usermode
+ * requirement is removed and a new qos target is computed. This way when the
+ * requirement that the application has is cleaned up when closes the file
+ * pointer or exits the qos_object will get an opportunity to clean up.
+ *
+ * mark gross [EMAIL PROTECTED]
+ */
+
+#include linux/qos_params.h
+#include linux/sched.h
+#include linux/spinlock.h
+#include linux/slab.h
+#include linux/time.h
+#include linux/fs.h
+#include linux/device.h
+#include linux/miscdevice.h
+#include linux/string.h
+#include linux/platform_device.h
+#include linux/init.h
+
+#include
Re: [RFC] QoS params patch update.
On Thu, Sep 27, 2007 at 01:17:39PM -0700, Mark Gross wrote:
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.
[snip]
+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!
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] QoS params patch
On Thu, 27 Sep 2007 11:24:40 +0900 Paul Mundt wrote:
> > +/* 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)
> > +{
> > + 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.
> > +/* 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.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] QoS params patch
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: > > --- linux-2.6.23-rc8/kernel/Makefile2007-09-26 13:54:54.0 > > -0700 > > +++ linux-2.6.23-rc8-qos/kernel/Makefile2007-09-26 14:06:38.0 - > 0700 > > @@ -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? > 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 :-) - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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. ;)
> --- linux-2.6.23-rc8/kernel/Makefile 2007-09-26 13:54:54.0 -0700
> +++ linux-2.6.23-rc8-qos/kernel/Makefile 2007-09-26 14:06:38.0 -
0700
> @@ -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?
> + * 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?
> + * 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?
> +/* 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...
> +#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?
pgpe8ho0wgIaP.pgp
Description: PGP signature
Re: [RFC] QoS params patch
On Wed, Sep 26, 2007 at 03:40:26PM -0700, Mark Gross wrote:
> + 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 ;-)
> +};
> +
> +#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.
> +/* 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)
> +{
> + if (v1 < v2)
> + return v1;
> + else
> + return v2;
> +}
> +
min()/max() instead?
> +/* assumes qos_lock is held */
> +static void update_target(int i)
> +{
'target' might be a more meaningful variable name.
> + qos->qos_power_miscdev.fops = _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(...); ?
> + if( i < QOS_NUM_CLASSES) {
> + 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(>notifiers);
> + INIT_LIST_HEAD(>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.
> +
> +int qos_add_requirement(int i, char *name, s32 value)
> +{
> + struct requirement_list * dep;
> + unsigned long flags;
> +
> + spin_lock_irqsave(_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.
> + 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.
> + if(!dep->name)
> + goto cleanup;
> +
> + list_add(>list, _array[i].requirements.list);
> + update_target(i);
> + spin_unlock_irqrestore(_lock, flags);
> +
> + return 0;
> + }
> + spin_unlock_irqrestore(_lock, flags);
> +
> +cleanup:
> + if(dep)
> + kfree(dep);
no if() needed.
> + 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.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] QoS params patch
On Wed, Sep 26, 2007 at 04:41:59PM -0700, Randy Dunlap wrote:
> On Wed, 26 Sep 2007 15:40:26 -0700 Mark Gross wrote:
>
> > The following is the qos_param patch that implements a genralization of
> > latency.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
> - has no API docs :(
not done yet.
>
>
>
>
> > +/* 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.
> > +{
>
> ---
> ~Randy
> Phaedrus says that Quality is about caring.
updated patch:
Signed-off-by: mark gross <[EMAIL PROTECTED]>
Makefile |2
linux/qos_params.h | 35 +
qos_params.c | 354 +
3 files changed, 390 insertions(+), 1 deletion(-)
diff -urN -X linux-2.6.23-rc8/Documentation/dontdiff
linux-2.6.23-rc8/include/linux/qos_params.h
linux-2.6.23-rc8-qos/include/linux/qos_params.h
--- linux-2.6.23-rc8/include/linux/qos_params.h 1969-12-31 16:00:00.0
-0800
+++ linux-2.6.23-rc8-qos/include/linux/qos_params.h 2007-09-26
14:05:20.0 -0700
@@ -0,0 +1,35 @@
+/* interface for the qos_power infrastructure of the linux kernel.
+ *
+ * Mark Gross
+ */
+#include
+#include
+#include
+
+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);
+
diff -urN -X linux-2.6.23-rc8/Documentation/dontdiff
linux-2.6.23-rc8/kernel/Makefile linux-2.6.23-rc8-qos/kernel/Makefile
--- linux-2.6.23-rc8/kernel/Makefile2007-09-26 13:54:54.0 -0700
+++ linux-2.6.23-rc8-qos/kernel/Makefile2007-09-26 14:06:38.0
-0700
@@ -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/
diff -urN -X linux-2.6.23-rc8/Documentation/dontdiff
linux-2.6.23-rc8/kernel/qos_params.c linux-2.6.23-rc8-qos/kernel/qos_params.c
--- linux-2.6.23-rc8/kernel/qos_params.c1969-12-31 16:00:00.0
-0800
+++ linux-2.6.23-rc8-qos/kernel/qos_params.c2007-09-26 17:09:44.0
-0700
@@ -0,0 +1,354 @@
+/*
+ * This module exposes the interface to kernel space for specifying
+ * QoS dependencies. It provides infrastructure for registration of:
+ *
+ * Dependents on a QoS value : register requirements
+ * Watchers of QoS value : get notified when target QoS value changes
+ *
+ * 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)
+ *
+ * There are lists of qos_objects each one wrapping requirements, notifiers
+ *
+ * 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
+ * client continues to be accounted for. Upon file release the usermode
+ * requirement is removed and a new qos target is computed. This way when the
+ * requirement that the application has is cleaned up when closes the file
+ * pointer or exits the qos_object will get an opportunity to clean up.
+ *
+ * mark gross [EMAIL PROTECTED]
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include
+
+/*
+ * locking rule: all changes to target_value or requirements or notifiers lists
+ * or qos_object list and qos_objects need to happen with qos_lock held, taken
+ * with _irqsave. One lock to rule them all
+ */
+
+struct qos_object {
+ struct requirement_list requirements;
+ struct srcu_notifier_head notifiers;
+ struct miscdevice qos_power_miscdev;
+ char *name;
+ s32 default_value;
+ s32 target_value;
+ s32 (*comparitor)(s32, s32);
+};
Re: [RFC] QoS params patch
On Wed, 26 Sep 2007 15:40:26 -0700 Mark Gross wrote:
> The following is the qos_param patch that implements a genralization of
> latency.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 :(
> +/* assumes qos_lock is held */
> +static void update_target(int i)
I'd prefer a better arg name than 'i'.
> +{
---
~Randy
Phaedrus says that Quality is about caring.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] QoS params patch
The following is the qos_param patch that implements a genralization of
latency.c.
Signed-off-by: Mark Gross <[EMAIL PROTECTED]>
diff -urN -X linux-2.6.23-rc8/Documentation/dontdiff
linux-2.6.23-rc8/include/linux/qos_params.h
linux-2.6.23-rc8-qos/include/linux/qos_params.h
--- linux-2.6.23-rc8/include/linux/qos_params.h 1969-12-31 16:00:00.0
-0800
+++ linux-2.6.23-rc8-qos/include/linux/qos_params.h 2007-09-26
14:05:20.0 -0700
@@ -0,0 +1,35 @@
+/* interface for the qos_power infrastructure of the linux kernel.
+ *
+ * Mark Gross
+ */
+#include
+#include
+#include
+
+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);
+
diff -urN -X linux-2.6.23-rc8/Documentation/dontdiff
linux-2.6.23-rc8/kernel/Makefile linux-2.6.23-rc8-qos/kernel/Makefile
--- linux-2.6.23-rc8/kernel/Makefile2007-09-26 13:54:54.0 -0700
+++ linux-2.6.23-rc8-qos/kernel/Makefile2007-09-26 14:06:38.0
-0700
@@ -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/
diff -urN -X linux-2.6.23-rc8/Documentation/dontdiff
linux-2.6.23-rc8/kernel/qos_params.c linux-2.6.23-rc8-qos/kernel/qos_params.c
--- linux-2.6.23-rc8/kernel/qos_params.c1969-12-31 16:00:00.0
-0800
+++ linux-2.6.23-rc8-qos/kernel/qos_params.c2007-09-26 15:21:51.0
-0700
@@ -0,0 +1,354 @@
+/*
+ * This module exposes the interface to kernel space for specifying
+ * QoS dependencies. It provides infrastructure for registration of:
+ *
+ * Dependents on a QoS value : register requirements
+ * Watchers of QoS value : get notified when target QoS value changes
+ *
+ * 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)
+ *
+ * There are lists of qos_objects each one wrapping requirements, notifiers
+ *
+ * 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
+ * client continues to be accounted for. Upon file release the usermode
+ * requirement is removed and a new qos target is computed. This way when the
+ * requirement that the application has is cleaned up when closes the file
+ * pointer or exits the qos_object will get an opportunity to clean up.
+ *
+ * mark gross [EMAIL PROTECTED]
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include
+
+/*
+ * locking rule: all changes to target_value or requirements or notifiers
lists or
+ * qos_object list and qos_objects need to happen with qos_lock held, taken
+ * with _irqsave. One lock to rule them all
+ */
+
+struct qos_object {
+ struct requirement_list requirements;
+ struct srcu_notifier_head notifiers;
+ struct miscdevice qos_power_miscdev;
+ char * name;
+ s32 default_value;
+ s32 target_value;
+ s32 (* comparitor)(s32,s32);
+};
+static struct qos_object qos_array[QOS_NUM_CLASSES];
+static DEFINE_SPINLOCK(qos_lock);
+
+static ssize_t qos_power_write(struct file *filp, const char __user *buf,
+ size_t count, loff_t *f_pos);
+static int qos_power_open(struct inode *inode, struct file *filp);
+static int qos_power_release(struct inode *inode, struct file *filp);
+
+static const struct file_operations qos_power_fops = {
+ .write = qos_power_write,
+ .open = qos_power_open,
+ .release = qos_power_release,
+};
+
+/* 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)
+{
+ if (v1 < v2)
+ return v1;
+ else
Re: [RFC] QoS params patch
The following is the qos_param patch that implements a genralization of
latency.c.
Signed-off-by: Mark Gross [EMAIL PROTECTED]
diff -urN -X linux-2.6.23-rc8/Documentation/dontdiff
linux-2.6.23-rc8/include/linux/qos_params.h
linux-2.6.23-rc8-qos/include/linux/qos_params.h
--- linux-2.6.23-rc8/include/linux/qos_params.h 1969-12-31 16:00:00.0
-0800
+++ linux-2.6.23-rc8-qos/include/linux/qos_params.h 2007-09-26
14:05:20.0 -0700
@@ -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);
+
diff -urN -X linux-2.6.23-rc8/Documentation/dontdiff
linux-2.6.23-rc8/kernel/Makefile linux-2.6.23-rc8-qos/kernel/Makefile
--- linux-2.6.23-rc8/kernel/Makefile2007-09-26 13:54:54.0 -0700
+++ linux-2.6.23-rc8-qos/kernel/Makefile2007-09-26 14:06:38.0
-0700
@@ -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/
diff -urN -X linux-2.6.23-rc8/Documentation/dontdiff
linux-2.6.23-rc8/kernel/qos_params.c linux-2.6.23-rc8-qos/kernel/qos_params.c
--- linux-2.6.23-rc8/kernel/qos_params.c1969-12-31 16:00:00.0
-0800
+++ linux-2.6.23-rc8-qos/kernel/qos_params.c2007-09-26 15:21:51.0
-0700
@@ -0,0 +1,354 @@
+/*
+ * This module exposes the interface to kernel space for specifying
+ * QoS dependencies. It provides infrastructure for registration of:
+ *
+ * Dependents on a QoS value : register requirements
+ * Watchers of QoS value : get notified when target QoS value changes
+ *
+ * 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)
+ *
+ * There are lists of qos_objects each one wrapping requirements, notifiers
+ *
+ * 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
+ * client continues to be accounted for. Upon file release the usermode
+ * requirement is removed and a new qos target is computed. This way when the
+ * requirement that the application has is cleaned up when closes the file
+ * pointer or exits the qos_object will get an opportunity to clean up.
+ *
+ * mark gross [EMAIL PROTECTED]
+ */
+
+#include linux/qos_params.h
+#include linux/sched.h
+#include linux/spinlock.h
+#include linux/slab.h
+#include linux/time.h
+#include linux/fs.h
+#include linux/device.h
+#include linux/miscdevice.h
+#include linux/string.h
+#include linux/platform_device.h
+#include linux/init.h
+
+#include asm/uaccess.h
+
+/*
+ * locking rule: all changes to target_value or requirements or notifiers
lists or
+ * qos_object list and qos_objects need to happen with qos_lock held, taken
+ * with _irqsave. One lock to rule them all
+ */
+
+struct qos_object {
+ struct requirement_list requirements;
+ struct srcu_notifier_head notifiers;
+ struct miscdevice qos_power_miscdev;
+ char * name;
+ s32 default_value;
+ s32 target_value;
+ s32 (* comparitor)(s32,s32);
+};
+static struct qos_object qos_array[QOS_NUM_CLASSES];
+static DEFINE_SPINLOCK(qos_lock);
+
+static ssize_t qos_power_write(struct file *filp, const char __user *buf,
+ size_t count, loff_t *f_pos);
+static int qos_power_open(struct inode *inode, struct file *filp);
+static int qos_power_release(struct inode *inode, struct file *filp);
+
+static const struct file_operations qos_power_fops = {
+ .write = qos_power_write,
+ .open = qos_power_open,
+ .release = qos_power_release,
+};
+
+/* static helper functions */
+static s32
Re: [RFC] QoS params patch
On Wed, 26 Sep 2007 15:40:26 -0700 Mark Gross wrote:
The following is the qos_param patch that implements a genralization of
latency.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 :(
+/* assumes qos_lock is held */
+static void update_target(int i)
I'd prefer a better arg name than 'i'.
+{
---
~Randy
Phaedrus says that Quality is about caring.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] QoS params patch
On Wed, Sep 26, 2007 at 04:41:59PM -0700, Randy Dunlap wrote:
On Wed, 26 Sep 2007 15:40:26 -0700 Mark Gross wrote:
The following is the qos_param patch that implements a genralization of
latency.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
- has no API docs :(
not done yet.
+/* 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.
+{
---
~Randy
Phaedrus says that Quality is about caring.
updated patch:
Signed-off-by: mark gross [EMAIL PROTECTED]
Makefile |2
linux/qos_params.h | 35 +
qos_params.c | 354 +
3 files changed, 390 insertions(+), 1 deletion(-)
diff -urN -X linux-2.6.23-rc8/Documentation/dontdiff
linux-2.6.23-rc8/include/linux/qos_params.h
linux-2.6.23-rc8-qos/include/linux/qos_params.h
--- linux-2.6.23-rc8/include/linux/qos_params.h 1969-12-31 16:00:00.0
-0800
+++ linux-2.6.23-rc8-qos/include/linux/qos_params.h 2007-09-26
14:05:20.0 -0700
@@ -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);
+
diff -urN -X linux-2.6.23-rc8/Documentation/dontdiff
linux-2.6.23-rc8/kernel/Makefile linux-2.6.23-rc8-qos/kernel/Makefile
--- linux-2.6.23-rc8/kernel/Makefile2007-09-26 13:54:54.0 -0700
+++ linux-2.6.23-rc8-qos/kernel/Makefile2007-09-26 14:06:38.0
-0700
@@ -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/
diff -urN -X linux-2.6.23-rc8/Documentation/dontdiff
linux-2.6.23-rc8/kernel/qos_params.c linux-2.6.23-rc8-qos/kernel/qos_params.c
--- linux-2.6.23-rc8/kernel/qos_params.c1969-12-31 16:00:00.0
-0800
+++ linux-2.6.23-rc8-qos/kernel/qos_params.c2007-09-26 17:09:44.0
-0700
@@ -0,0 +1,354 @@
+/*
+ * This module exposes the interface to kernel space for specifying
+ * QoS dependencies. It provides infrastructure for registration of:
+ *
+ * Dependents on a QoS value : register requirements
+ * Watchers of QoS value : get notified when target QoS value changes
+ *
+ * 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)
+ *
+ * There are lists of qos_objects each one wrapping requirements, notifiers
+ *
+ * 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
+ * client continues to be accounted for. Upon file release the usermode
+ * requirement is removed and a new qos target is computed. This way when the
+ * requirement that the application has is cleaned up when closes the file
+ * pointer or exits the qos_object will get an opportunity to clean up.
+ *
+ * mark gross [EMAIL PROTECTED]
+ */
+
+#include linux/qos_params.h
+#include linux/sched.h
+#include linux/spinlock.h
+#include linux/slab.h
+#include linux/time.h
+#include linux/fs.h
+#include linux/device.h
+#include linux/miscdevice.h
+#include linux/string.h
+#include linux/platform_device.h
+#include linux/init.h
+
+#include linux/uaccess.h
+
+/*
+ * locking rule: all changes to target_value or requirements or notifiers lists
+ * or qos_object list and qos_objects need to happen with qos_lock held, taken
+ * with _irqsave. One lock to rule them all
+ */
+
+struct qos_object {
+ struct requirement_list requirements;
+ struct
Re: [RFC] QoS params patch
On Wed, Sep 26, 2007 at 03:40:26PM -0700, Mark Gross wrote:
+ 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 ;-)
+};
+
+#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.
+/* 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)
+{
+ if (v1 v2)
+ return v1;
+ else
+ return v2;
+}
+
min()/max() instead?
+/* assumes qos_lock is held */
+static void update_target(int i)
+{
'target' might be a more meaningful variable name.
+ 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(...); ?
+ 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;
+ }
+ 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.
+
+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.
+ 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.
+ 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;
+ }
+ spin_unlock_irqrestore(qos_lock, flags);
+
+cleanup:
+ if(dep)
+ kfree(dep);
no if() needed.
+ 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.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
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. ;) --- linux-2.6.23-rc8/kernel/Makefile 2007-09-26 13:54:54.0 -0700 +++ linux-2.6.23-rc8-qos/kernel/Makefile 2007-09-26 14:06:38.0 - 0700 @@ -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? + * 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? + * 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? +/* 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... +#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? pgpe8ho0wgIaP.pgp Description: PGP signature
Re: [RFC] QoS params patch
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: --- linux-2.6.23-rc8/kernel/Makefile2007-09-26 13:54:54.0 -0700 +++ linux-2.6.23-rc8-qos/kernel/Makefile2007-09-26 14:06:38.0 - 0700 @@ -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? 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 :-) - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] QoS params patch
On Thu, 27 Sep 2007 11:24:40 +0900 Paul Mundt wrote:
+/* 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)
+{
+ 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.
+/* 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.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
