[PATCH 1/4] tracing: introduce for_each_token macro

[PATCH 1/4] tracing: introduce for_each_token macro

[Yuanhan Liu]

Introduce for_each_token macro to parse the trace event list.

Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
---
 kernel/trace/trace_events.c |   14 ++++++--------
 1 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index 0725eea..fe15b7c 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -1218,6 +1218,11 @@ void trace_remove_event_call(struct ftrace_event_call *call)
 	mutex_unlock(&event_mutex);
 }
 
+#define for_each_token(token, buf) 				\
+	for (token = strsep(&buf, ","); 			\
+	     token && *token; 					\
+	     token = strsep(&buf, ","))
+
 #define for_each_event(event, start, end)			\
 	for (event = start;					\
 	     (unsigned long)event < (unsigned long)end;		\
@@ -1430,14 +1435,7 @@ static __init int event_trace_init(void)
 				       &ftrace_event_format_fops);
 	}
 
-	while (true) {
-		token = strsep(&buf, ",");
-
-		if (!token)
-			break;
-		if (!*token)
-			continue;
-
+	for_each_token(token, buf) {
 		ret = ftrace_set_clr_event(token, 1);
 		if (ret)
 			pr_warning("Failed to enable trace event: %s\n", token);
-- 
1.7.2.3

[PATCH 2/4] tracing: introduce trace_set_clr_module_event

[Yuanhan Liu]

Trace events belong to a module does exist only when that module is
loaded. While, when the module loaded, we may miss some trace event
happened at the module load time.

This is so true for gpu driver: when X(or shell) is started, you would
be going to miss lots of events. This would be worse when the KMS is
failed.

So, introduce trace_set_clr_module_event function, and do export it.
Trace event then can be enabled on module load by doing something like
follows at your module init function:

char *xxx_trace = NULL;
module_param_named(trace, xxx_trace, charp, 0400);

xxx_init()
{
	....
	if (xxx_trace)
		ret = trace_set_clr_module_event(THIS_MODULE, xxx_trace, 1);
	....
}

where, xxx_trace is a comm separated event list, * for all event in that
module, NO subsystem is needed.

Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
---
 include/linux/ftrace_event.h |    3 ++
 kernel/trace/trace_events.c  |   45 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 48 insertions(+), 0 deletions(-)

diff --git a/include/linux/ftrace_event.h b/include/linux/ftrace_event.h
index 8beabb9..8fbdbbd 100644
--- a/include/linux/ftrace_event.h
+++ b/include/linux/ftrace_event.h
@@ -226,6 +226,9 @@ extern void trace_remove_event_call(struct ftrace_event_call *call);
 
 int trace_set_clr_event(const char *system, const char *event, int set);
 
+int trace_set_clr_module_event(struct module *mod, 
+			       const char *event_list, int set);
+
 /*
  * The double __builtin_constant_p is because gcc will give us an error
  * if we try to allocate the static variable to fmt if it is not a
diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index fe15b7c..711a1d1 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -1353,6 +1353,51 @@ static int trace_module_notify(struct notifier_block *self,
 
 	return 0;
 }
+
+/**
+ * trace_set_clr_module_event - enable or disable events for specified module
+ * @mod: the module, the caller should mostly pass by THIS_MODULE
+ * @event_list: the event list, separated by comma.
+ * @set: 1 to enable, 0 to disable
+ *
+ * This function is used by a module to enable or disable events belong to that 
+ * module. This is a way for module to enable some events on module loading.
+ */
+int trace_set_clr_module_event(struct module *mod, 
+				const char *event_list, int set)
+{
+	struct ftrace_event_call *call, *start, *end;
+	char *buf, *p;
+	char *token;
+
+	start = mod->trace_events;
+	end = mod->trace_events + mod->num_trace_events;
+	if (start == end)
+		return 0;
+	
+	if (!event_list)
+		return 0;
+	buf = p = kstrdup(event_list, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	for_each_token(token, p) {
+		for_each_event(call, start, end) {
+			if (strcmp(token, "*") == 0 ||
+			    strcmp(token, call->name) == 0) 
+				ftrace_event_enable_disable(call, set);
+			else
+				pr_warning("Failed to find trace event: %s "
+					   "in module %s\n", token, mod->name);
+		
+		}
+	}
+	kfree(buf);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(trace_set_clr_module_event);
+
+
 #else
 static int trace_module_notify(struct notifier_block *self,
 			       unsigned long val, void *data)
-- 
1.7.2.3

[PATCH 3/4] tracing: update trace-events-sample.c

[Yuanhan Liu]

Update the samples/trace_events/trace-events-sample.c to show how to use
trace_set_clr_module_event to enable some events at module load time.

Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
---
 samples/trace_events/trace-events-sample.c |   16 +++++++++++++---
 1 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/samples/trace_events/trace-events-sample.c b/samples/trace_events/trace-events-sample.c
index aabc4e9..b7212e1 100644
--- a/samples/trace_events/trace-events-sample.c
+++ b/samples/trace_events/trace-events-sample.c
@@ -9,7 +9,10 @@
  */
 #define CREATE_TRACE_POINTS
 #include "trace-events-sample.h"
+#include <linux/ftrace_event.h>
 
+char *sample_trace = NULL;
+module_param_named(trace, sample_trace, charp, 0400);
 
 static void simple_thread_func(int cnt)
 {
@@ -32,11 +35,18 @@ static struct task_struct *simple_tsk;
 
 static int __init trace_event_init(void)
 {
+	int ret = 0;
+
+        if (sample_trace) {
+                ret = trace_set_clr_module_event(THIS_MODULE, sample_trace, 1);
+		if (ret)
+			return ret;
+	}
+
 	simple_tsk = kthread_run(simple_thread, NULL, "event-sample");
-	if (IS_ERR(simple_tsk))
-		return -1;
+	ret = PTR_ERR(simple_tsk);
 
-	return 0;
+	return ret;
 }
 
 static void __exit trace_event_exit(void)
-- 
1.7.2.3

[PATCH 4/4] tracing: update the trace/events.txt documentation

[Yuanhan Liu]

Describe the usage of trace_set_clr_module_event in events.txt

Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
---
 Documentation/trace/events.txt |   25 +++++++++++++++++++++++++
 1 files changed, 25 insertions(+), 0 deletions(-)

diff --git a/Documentation/trace/events.txt b/Documentation/trace/events.txt
index 09bd8e9..120c1fe 100644
--- a/Documentation/trace/events.txt
+++ b/Documentation/trace/events.txt
@@ -93,6 +93,31 @@ In order to facilitate early boot debugging, use boot option:
 event-list is a comma separated list of events. See section 2.1 for event
 format.
 
+2.4 Module load option
+----------------------
+
+In order to track the events at the early module load phase, add the
+following codes in your module:
+
+    char *xxx_trace = NULL;
+    module_param_named(trace, xxx_trace, charp, 0400);
+
+
+    /* Your module init function */
+    xxx_init()
+    {
+        ....
+        if (xxx_trace)
+                trace_set_clr_module_event(THIS_MODULE, xxx_trace, 1);
+        ....
+    }
+
+where the xxx_trace is same as event-list described above, besides DO
+NOT include the subsystem.
+
+You can check the sample code for more information.
+
+
 3. Defining an event-enabled tracepoint
 =======================================
 
-- 
1.7.2.3

Re: [PATCH 2/4] tracing: introduce trace_set_clr_module_event

[Yuanhan Liu]

On Tue, Nov 09, 2010 at 05:12:45PM +0800, Yuanhan Liu wrote:

[ snip ]

> +	for_each_token(token, p) {
> +		for_each_event(call, start, end) {
> +			if (strcmp(token, "*") == 0 ||
> +			    strcmp(token, call->name) == 0) 
> +				ftrace_event_enable_disable(call, set);
> +			else
> +				pr_warning("Failed to find trace event: %s "
> +					   "in module %s\n", token, mod->name);
Oops, sorry for this obvious error, will fix it.

Besides that, any comments? 

Thanks.

--
Yuanhan Liu
> +		
> +		}
> +	}
> +	kfree(buf);
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(trace_set_clr_module_event);
> +
> +
>  #else
>  static int trace_module_notify(struct notifier_block *self,
>  			       unsigned long val, void *data)
> -- 
> 1.7.2.3

Re: [PATCH 2/4] tracing: introduce trace_set_clr_module_event

[Chris Wilson]

On Tue, 9 Nov 2010 19:23:20 +0800, Yuanhan Liu <yuanhan.liu@linux.intel.com> wrote:
> Besides that, any comments? 

I've not looked closely at this yet, but it seems like we should be able
to hook into a trace_module_notify and implement a generic option parser
from within trace/ as opposed to each individual module.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 2/4] tracing: introduce trace_set_clr_module_event

[Yuanhan Liu]

On Tue, Nov 09, 2010 at 11:27:18AM +0000, Chris Wilson wrote:
> I've not looked closely at this yet, but it seems like we should be able
> to hook into a trace_module_notify 

But there should be someplace to enable these events, right?  And how do
you tell trace_module_notify to just enable one or several specific
event?

>and implement a generic option parser
> from within trace/ as opposed to each individual module.
Nope, this patch does implement a generic option parser within trace/, and
we just call this function from a module.

Thanks.

--
Yuanhan Liu

Re: [PATCH 2/4] tracing: introduce trace_set_clr_module_event

[Chris Wilson]

On Tue, 9 Nov 2010 21:20:59 +0800, Yuanhan Liu <yuanhan.liu@linux.intel.com> wrote:
> On Tue, Nov 09, 2010 at 11:27:18AM +0000, Chris Wilson wrote:
> > I've not looked closely at this yet, but it seems like we should be able
> > to hook into a trace_module_notify 
> 
> But there should be someplace to enable these events, right?  And how do
> you tell trace_module_notify to just enable one or several specific
> event?

Yes, I was thinking of extending the module loader to permit core to
hook into parse_args() and so the trace= facility could work with any
module without exporting any new functions.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 2/4] tracing: introduce trace_set_clr_module_event

[Steven Rostedt]

On Tue, 2010-11-09 at 13:31 +0000, Chris Wilson wrote:
> On Tue, 9 Nov 2010 21:20:59 +0800, Yuanhan Liu <yuanhan.liu@linux.intel.com> wrote:
> > On Tue, Nov 09, 2010 at 11:27:18AM +0000, Chris Wilson wrote:
> > > I've not looked closely at this yet, but it seems like we should be able
> > > to hook into a trace_module_notify 
> > 
> > But there should be someplace to enable these events, right?  And how do
> > you tell trace_module_notify to just enable one or several specific
> > event?
> 
> Yes, I was thinking of extending the module loader to permit core to
> hook into parse_args() and so the trace= facility could work with any
> module without exporting any new functions.

Exactly, if you want, I probably could whip up a patch.

-- Steve

Re: [PATCH 2/4] tracing: introduce trace_set_clr_module_event

[Yuanhan Liu]

On Tue, Nov 09, 2010 at 01:31:27PM +0000, Chris Wilson wrote:
> On Tue, 9 Nov 2010 21:20:59 +0800, Yuanhan Liu <yuanhan.liu@linux.intel.com> wrote:
> > On Tue, Nov 09, 2010 at 11:27:18AM +0000, Chris Wilson wrote:
> > > I've not looked closely at this yet, but it seems like we should be able
> > > to hook into a trace_module_notify 
> > 
> > But there should be someplace to enable these events, right?  And how do
> > you tell trace_module_notify to just enable one or several specific
> > event?
> 
> Yes, I was thinking of extending the module loader to permit core to
> hook into parse_args() and so the trace= facility could work with any
> module without exporting any new functions.

Got your idea. But is it OK? 

Thanks.

--
Yuanhan Liu

Re: [PATCH 2/4] tracing: introduce trace_set_clr_module_event

[Steven Rostedt]

On Tue, 2010-11-09 at 21:59 +0800, Yuanhan Liu wrote:
> On Tue, Nov 09, 2010 at 01:31:27PM +0000, Chris Wilson wrote:
> > On Tue, 9 Nov 2010 21:20:59 +0800, Yuanhan Liu <yuanhan.liu@linux.intel.com> wrote:
> > > On Tue, Nov 09, 2010 at 11:27:18AM +0000, Chris Wilson wrote:
> > > > I've not looked closely at this yet, but it seems like we should be able
> > > > to hook into a trace_module_notify 
> > > 
> > > But there should be someplace to enable these events, right?  And how do
> > > you tell trace_module_notify to just enable one or several specific
> > > event?
> > 
> > Yes, I was thinking of extending the module loader to permit core to
> > hook into parse_args() and so the trace= facility could work with any
> > module without exporting any new functions.
> 
> Got your idea. But is it OK? 

Of course it is. It's the one that makes the most sense.

-- Steve

Re: [PATCH 2/4] tracing: introduce trace_set_clr_module_event

[Yuanhan Liu]

On Tue, Nov 09, 2010 at 08:56:11AM -0500, Steven Rostedt wrote:
> On Tue, 2010-11-09 at 13:31 +0000, Chris Wilson wrote:
> > On Tue, 9 Nov 2010 21:20:59 +0800, Yuanhan Liu <yuanhan.liu@linux.intel.com> wrote:
> > > On Tue, Nov 09, 2010 at 11:27:18AM +0000, Chris Wilson wrote:
> > > > I've not looked closely at this yet, but it seems like we should be able
> > > > to hook into a trace_module_notify 
> > > 
> > > But there should be someplace to enable these events, right?  And how do
> > > you tell trace_module_notify to just enable one or several specific
> > > event?
> > 
> > Yes, I was thinking of extending the module loader to permit core to
> > hook into parse_args() and so the trace= facility could work with any
> > module without exporting any new functions.
> 
> Exactly, if you want, I probably could whip up a patch.

Yes, please if you are free;)

Thanks.

--
Yuanhan Liu
> 

[PATCH] Delegate unknown module parameters to interested parties

[Chris Wilson]

This is a sketch of what I had in mind for trace to do the event
initialisation on the modules behalf.

---
 include/linux/module.h      |    8 ++++++++
 include/linux/moduleparam.h |    3 ++-
 init/main.c                 |    8 ++++----
 kernel/module.c             |   21 ++++++++++++++++++++-
 kernel/params.c             |   10 ++++++----
 kernel/trace/trace_events.c |   15 ++++++++++++++-
 6 files changed, 54 insertions(+), 11 deletions(-)

diff --git a/include/linux/module.h b/include/linux/module.h
index b29e745..84daad8 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -241,6 +241,14 @@ enum module_state
 	MODULE_STATE_GOING,
 };
 
+#define MODULE_UNKNOWN_PARAM (MODULE_STATE_GOING + 1)
+
+struct module_unknown_param {
+	struct module *mod;
+	char *param;
+	char *val;
+};
+
 struct module
 {
 	enum module_state state;
diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h
index 112adf8..5e24ec1 100644
--- a/include/linux/moduleparam.h
+++ b/include/linux/moduleparam.h
@@ -265,7 +265,8 @@ extern int parse_args(const char *name,
 		      char *args,
 		      const struct kernel_param *params,
 		      unsigned num,
-		      int (*unknown)(char *param, char *val));
+		      int (*unknown)(char *param, char *val, void *data),
+		      void *data);
 
 /* Called by module remove. */
 #ifdef CONFIG_SYSFS
diff --git a/init/main.c b/init/main.c
index e59af24..782780e 100644
--- a/init/main.c
+++ b/init/main.c
@@ -267,7 +267,7 @@ early_param("loglevel", loglevel);
  * Unknown boot options get handed to init, unless they look like
  * unused parameters (modprobe will find them in /proc/cmdline).
  */
-static int __init unknown_bootoption(char *param, char *val)
+static int __init unknown_bootoption(char *param, char *val, void *data)
 {
 	/* Change NUL term back to "=", to make "param" the whole string. */
 	if (val) {
@@ -455,7 +455,7 @@ static noinline void __init_refok rest_init(void)
 }
 
 /* Check for early params. */
-static int __init do_early_param(char *param, char *val)
+static int __init do_early_param(char *param, char *val, void *data)
 {
 	const struct obs_kernel_param *p;
 
@@ -475,7 +475,7 @@ static int __init do_early_param(char *param, char *val)
 
 void __init parse_early_options(char *cmdline)
 {
-	parse_args("early options", cmdline, NULL, 0, do_early_param);
+	parse_args("early options", cmdline, NULL, 0, do_early_param, NULL);
 }
 
 /* Arch code calls this early on, or if not, just before other parsing. */
@@ -578,7 +578,7 @@ asmlinkage void __init start_kernel(void)
 	parse_early_param();
 	parse_args("Booting kernel", static_command_line, __start___param,
 		   __stop___param - __start___param,
-		   &unknown_bootoption);
+		   &unknown_bootoption, NULL);
 	/*
 	 * These use large bootmem allocations and must precede
 	 * kmem_cache_init()
diff --git a/kernel/module.c b/kernel/module.c
index 2df4630..87ce541 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2544,6 +2544,24 @@ static int post_relocation(struct module *mod, const struct load_info *info)
 	return module_finalize(info->hdr, info->sechdrs, mod);
 }
 
+static int unknown_param(char *param, char *val, void *data)
+{
+	struct module_unknown_param arg;
+	int ret;
+
+	arg.mod = data;
+	arg.param = param;
+	arg.val = val;
+
+	ret = blocking_notifier_call_chain(&module_notify_list,
+					  MODULE_UNKNOWN_PARAM,
+					  &arg);
+	if (ret & NOTIFY_STOP_MASK)
+		return notifier_to_errno(ret);
+
+	return -ENOENT;
+}
+
 /* Allocate and load the module: note that size of section 0 is always
    zero, and we rely on this for optional sections. */
 static struct module *load_module(void __user *umod,
@@ -2637,7 +2655,8 @@ static struct module *load_module(void __user *umod,
 	mutex_unlock(&module_mutex);
 
 	/* Module is ready to execute: parsing args may do that. */
-	err = parse_args(mod->name, mod->args, mod->kp, mod->num_kp, NULL);
+	err = parse_args(mod->name, mod->args, mod->kp, mod->num_kp,
+			 unknown_param, mod);
 	if (err < 0)
 		goto unlink;
 
diff --git a/kernel/params.c b/kernel/params.c
index 08107d1..d8c707c 100644
--- a/kernel/params.c
+++ b/kernel/params.c
@@ -87,7 +87,8 @@ static int parse_one(char *param,
 		     char *val,
 		     const struct kernel_param *params,
 		     unsigned num_params,
-		     int (*handle_unknown)(char *param, char *val))
+		     int (*handle_unknown)(char *param, char *val, void *data),
+		     void *data)
 {
 	unsigned int i;
 	int err;
@@ -109,7 +110,7 @@ static int parse_one(char *param,
 
 	if (handle_unknown) {
 		DEBUGP("Unknown argument: calling %p\n", handle_unknown);
-		return handle_unknown(param, val);
+		return handle_unknown(param, val, data);
 	}
 
 	DEBUGP("Unknown argument `%s'\n", param);
@@ -173,7 +174,8 @@ int parse_args(const char *name,
 	       char *args,
 	       const struct kernel_param *params,
 	       unsigned num,
-	       int (*unknown)(char *param, char *val))
+	       int (*unknown)(char *param, char *val, void *data),
+	       void *data)
 {
 	char *param, *val;
 
@@ -188,7 +190,7 @@ int parse_args(const char *name,
 
 		args = next_arg(args, &param, &val);
 		irq_was_disabled = irqs_disabled();
-		ret = parse_one(param, val, params, num, unknown);
+		ret = parse_one(param, val, params, num, unknown, data);
 		if (irq_was_disabled && !irqs_disabled()) {
 			printk(KERN_WARNING "parse_args(): option '%s' enabled "
 					"irq's!\n", param);
diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index 0725eea..d23cbf3 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -1330,10 +1330,21 @@ static void trace_module_remove_events(struct module *mod)
 	up_write(&trace_event_mutex);
 }
 
+static int trace_module_unknown_param(struct module_unknown_param *arg)
+{
+	if (strcmp(arg->param, "trace") == 0) {
+		int ret = trace_set_clr_module_event(arg->mod, arg->val, 1);
+		return NOTIFY_STOP_MASK | (NOTIFY_OK - ret);
+	}
+
+	return NOTIFY_OK;
+}
+
 static int trace_module_notify(struct notifier_block *self,
 			       unsigned long val, void *data)
 {
 	struct module *mod = data;
+	int ret = 0;
 
 	mutex_lock(&event_mutex);
 	switch (val) {
@@ -1343,10 +1354,12 @@ static int trace_module_notify(struct notifier_block *self,
 	case MODULE_STATE_GOING:
 		trace_module_remove_events(mod);
 		break;
+	case MODULE_UNKNOWN_PARAM:
+		ret = trace_module_unknown_param(data);
 	}
 	mutex_unlock(&event_mutex);
 
-	return 0;
+	return ret;
 }
 #else
 static int trace_module_notify(struct notifier_block *self,
-- 
1.7.2.3

Re: [PATCH] Delegate unknown module parameters to interested parties

[Steven Rostedt]

[ Added Rusty "Module God" Russell ]


On Tue, 2010-11-09 at 14:11 +0000, Chris Wilson wrote:
> This is a sketch of what I had in mind for trace to do the event
> initialisation on the modules behalf.
> 
> ---
>  include/linux/module.h      |    8 ++++++++
>  include/linux/moduleparam.h |    3 ++-
>  init/main.c                 |    8 ++++----
>  kernel/module.c             |   21 ++++++++++++++++++++-
>  kernel/params.c             |   10 ++++++----
>  kernel/trace/trace_events.c |   15 ++++++++++++++-
>  6 files changed, 54 insertions(+), 11 deletions(-)
> 
> diff --git a/include/linux/module.h b/include/linux/module.h
> index b29e745..84daad8 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -241,6 +241,14 @@ enum module_state
>  	MODULE_STATE_GOING,
>  };
>  
> +#define MODULE_UNKNOWN_PARAM (MODULE_STATE_GOING + 1)
> +
> +struct module_unknown_param {
> +	struct module *mod;
> +	char *param;
> +	char *val;
> +};
> +
>  struct module
>  {
>  	enum module_state state;
> diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h
> index 112adf8..5e24ec1 100644
> --- a/include/linux/moduleparam.h
> +++ b/include/linux/moduleparam.h
> @@ -265,7 +265,8 @@ extern int parse_args(const char *name,
>  		      char *args,
>  		      const struct kernel_param *params,
>  		      unsigned num,
> -		      int (*unknown)(char *param, char *val));
> +		      int (*unknown)(char *param, char *val, void *data),
> +		      void *data);
>  
>  /* Called by module remove. */
>  #ifdef CONFIG_SYSFS
> diff --git a/init/main.c b/init/main.c
> index e59af24..782780e 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -267,7 +267,7 @@ early_param("loglevel", loglevel);
>   * Unknown boot options get handed to init, unless they look like
>   * unused parameters (modprobe will find them in /proc/cmdline).
>   */
> -static int __init unknown_bootoption(char *param, char *val)
> +static int __init unknown_bootoption(char *param, char *val, void *data)
>  {
>  	/* Change NUL term back to "=", to make "param" the whole string. */
>  	if (val) {
> @@ -455,7 +455,7 @@ static noinline void __init_refok rest_init(void)
>  }
>  
>  /* Check for early params. */
> -static int __init do_early_param(char *param, char *val)
> +static int __init do_early_param(char *param, char *val, void *data)
>  {
>  	const struct obs_kernel_param *p;
>  
> @@ -475,7 +475,7 @@ static int __init do_early_param(char *param, char *val)
>  
>  void __init parse_early_options(char *cmdline)
>  {
> -	parse_args("early options", cmdline, NULL, 0, do_early_param);
> +	parse_args("early options", cmdline, NULL, 0, do_early_param, NULL);
>  }
>  
>  /* Arch code calls this early on, or if not, just before other parsing. */
> @@ -578,7 +578,7 @@ asmlinkage void __init start_kernel(void)
>  	parse_early_param();
>  	parse_args("Booting kernel", static_command_line, __start___param,
>  		   __stop___param - __start___param,
> -		   &unknown_bootoption);
> +		   &unknown_bootoption, NULL);
>  	/*
>  	 * These use large bootmem allocations and must precede
>  	 * kmem_cache_init()
> diff --git a/kernel/module.c b/kernel/module.c
> index 2df4630..87ce541 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -2544,6 +2544,24 @@ static int post_relocation(struct module *mod, const struct load_info *info)
>  	return module_finalize(info->hdr, info->sechdrs, mod);
>  }
>  
> +static int unknown_param(char *param, char *val, void *data)
> +{
> +	struct module_unknown_param arg;
> +	int ret;
> +
> +	arg.mod = data;
> +	arg.param = param;
> +	arg.val = val;
> +
> +	ret = blocking_notifier_call_chain(&module_notify_list,
> +					  MODULE_UNKNOWN_PARAM,
> +					  &arg);
> +	if (ret & NOTIFY_STOP_MASK)
> +		return notifier_to_errno(ret);
> +
> +	return -ENOENT;
> +}
> +
>  /* Allocate and load the module: note that size of section 0 is always
>     zero, and we rely on this for optional sections. */
>  static struct module *load_module(void __user *umod,
> @@ -2637,7 +2655,8 @@ static struct module *load_module(void __user *umod,
>  	mutex_unlock(&module_mutex);
>  
>  	/* Module is ready to execute: parsing args may do that. */
> -	err = parse_args(mod->name, mod->args, mod->kp, mod->num_kp, NULL);
> +	err = parse_args(mod->name, mod->args, mod->kp, mod->num_kp,
> +			 unknown_param, mod);
>  	if (err < 0)
>  		goto unlink;
>  
> diff --git a/kernel/params.c b/kernel/params.c
> index 08107d1..d8c707c 100644
> --- a/kernel/params.c
> +++ b/kernel/params.c
> @@ -87,7 +87,8 @@ static int parse_one(char *param,
>  		     char *val,
>  		     const struct kernel_param *params,
>  		     unsigned num_params,
> -		     int (*handle_unknown)(char *param, char *val))
> +		     int (*handle_unknown)(char *param, char *val, void *data),
> +		     void *data)
>  {
>  	unsigned int i;
>  	int err;
> @@ -109,7 +110,7 @@ static int parse_one(char *param,
>  
>  	if (handle_unknown) {
>  		DEBUGP("Unknown argument: calling %p\n", handle_unknown);
> -		return handle_unknown(param, val);
> +		return handle_unknown(param, val, data);
>  	}
>  
>  	DEBUGP("Unknown argument `%s'\n", param);
> @@ -173,7 +174,8 @@ int parse_args(const char *name,
>  	       char *args,
>  	       const struct kernel_param *params,
>  	       unsigned num,
> -	       int (*unknown)(char *param, char *val))
> +	       int (*unknown)(char *param, char *val, void *data),
> +	       void *data)
>  {
>  	char *param, *val;
>  
> @@ -188,7 +190,7 @@ int parse_args(const char *name,
>  
>  		args = next_arg(args, &param, &val);
>  		irq_was_disabled = irqs_disabled();
> -		ret = parse_one(param, val, params, num, unknown);
> +		ret = parse_one(param, val, params, num, unknown, data);
>  		if (irq_was_disabled && !irqs_disabled()) {
>  			printk(KERN_WARNING "parse_args(): option '%s' enabled "
>  					"irq's!\n", param);
> diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
> index 0725eea..d23cbf3 100644
> --- a/kernel/trace/trace_events.c
> +++ b/kernel/trace/trace_events.c
> @@ -1330,10 +1330,21 @@ static void trace_module_remove_events(struct module *mod)
>  	up_write(&trace_event_mutex);
>  }
>  
> +static int trace_module_unknown_param(struct module_unknown_param *arg)
> +{
> +	if (strcmp(arg->param, "trace") == 0) {
> +		int ret = trace_set_clr_module_event(arg->mod, arg->val, 1);

This of course assumes that arg->mod is the system. Perhaps I could
rewrite this to only enable events in this module.

> +		return NOTIFY_STOP_MASK | (NOTIFY_OK - ret);
> +	}
> +
> +	return NOTIFY_OK;
> +}


Wow! I was just going to add a direct hook into the module code, but
this is much more generic!

-- Steve

> +
>  static int trace_module_notify(struct notifier_block *self,
>  			       unsigned long val, void *data)
>  {
>  	struct module *mod = data;
> +	int ret = 0;
>  
>  	mutex_lock(&event_mutex);
>  	switch (val) {
> @@ -1343,10 +1354,12 @@ static int trace_module_notify(struct notifier_block *self,
>  	case MODULE_STATE_GOING:
>  		trace_module_remove_events(mod);
>  		break;
> +	case MODULE_UNKNOWN_PARAM:
> +		ret = trace_module_unknown_param(data);
>  	}
>  	mutex_unlock(&event_mutex);
>  
> -	return 0;
> +	return ret;
>  }
>  #else
>  static int trace_module_notify(struct notifier_block *self,

Re: [PATCH] Delegate unknown module parameters to interested parties

[Rusty Russell]

On Wed, 10 Nov 2010 01:09:21 am Steven Rostedt wrote:
> [ Added Rusty "Module God" Russell ]

And I also wrote the parameter parsing code, so sending to me is probably
a good idea.

So, what's this for?  You want trace= as a standard module parameter
or something?  If this is a once-off I'd prefer a custom hack I think.
If you can think of other users, this might be a good idea; though don't
use "void *data" use an explicit struct module *...

Cheers,
Rusty.

Re: [PATCH] Delegate unknown module parameters to interested parties

[Steven Rostedt]

On Wed, 2010-11-10 at 16:21 +1030, Rusty Russell wrote:
> On Wed, 10 Nov 2010 01:09:21 am Steven Rostedt wrote:
> > [ Added Rusty "Module God" Russell ]
> 
> And I also wrote the parameter parsing code, so sending to me is probably
> a good idea.
> 
> So, what's this for?  You want trace= as a standard module parameter

Yep, this way we could even enable tracepoints that are in the init
section.

> or something?  If this is a once-off I'd prefer a custom hack I think.
> If you can think of other users, this might be a good idea; though don't
> use "void *data" use an explicit struct module *...

If I wrote the code, it would have been a once-off custom hack. But,
personally, I like the generic addition. Perhaps others will hook into
it without fear of having to hack the module code, which can be quite
intimidating to some.

I also agree to not use the void *data.

-- Steve

Re: [PATCH] Delegate unknown module parameters to interested parties

[Rusty Russell]

On Thu, 11 Nov 2010 12:26:00 am Steven Rostedt wrote:
> On Wed, 2010-11-10 at 16:21 +1030, Rusty Russell wrote:
> > On Wed, 10 Nov 2010 01:09:21 am Steven Rostedt wrote:
> > > [ Added Rusty "Module God" Russell ]
> > 
> > And I also wrote the parameter parsing code, so sending to me is probably
> > a good idea.
> > 
> > So, what's this for?  You want trace= as a standard module parameter
> 
> Yep, this way we could even enable tracepoints that are in the init
> section.

*Exactly* how would it be used though?  Please provide a synopsis for
someone unaware of what tracing does these days?

Because we could compile an extra module_parm() into the module using
modpost, for example, at a cost of an extra 16/32 bytes per module.

> But, personally, I like the generic addition. Perhaps others will hook into
> it without fear of having to hack the module code, which can be quite
> intimidating to some.

We *all* want to build infrastructure; when other coders are forced to use
it we rise up the kernel dominance hierarchy. Ook ook!  (Every Unix app has
its own config language for the same reason: the author distils the mental
sweat of the users into some kind of Elixer of Coder Hubris).

Yet abstractions obfuscate: let's resist our primal urges to add another
speed hump on the lengthening road to kernel expertese.

And this one's classicly easy: in single uses cases we always get the
infrastructure wrong for future users anyway, so let's not do it until
we have more than one user.

Cheers,
Rusty.

Re: [PATCH] Delegate unknown module parameters to interested parties

[Steven Rostedt]

On Thu, 2010-11-11 at 11:05 +1030, Rusty Russell wrote:
>  
> > Yep, this way we could even enable tracepoints that are in the init
> > section.
> 
> *Exactly* how would it be used though?  Please provide a synopsis for
> someone unaware of what tracing does these days?

Well, if you have a tracepoint in the module, and you want to enable it
as soon as the module is loaded, it would be nice to have that ability.

> 
> Because we could compile an extra module_parm() into the module using
> modpost, for example, at a cost of an extra 16/32 bytes per module.
> 
> > But, personally, I like the generic addition. Perhaps others will hook into
> > it without fear of having to hack the module code, which can be quite
> > intimidating to some.
> 
> We *all* want to build infrastructure; when other coders are forced to use
> it we rise up the kernel dominance hierarchy. Ook ook!  (Every Unix app has
> its own config language for the same reason: the author distils the mental
> sweat of the users into some kind of Elixer of Coder Hubris).

Note, I would have just done the hack, but I liked Chris's generic
approach. /me ook ooks Chris!


> 
> Yet abstractions obfuscate: let's resist our primal urges to add another
> speed hump on the lengthening road to kernel expertese.

Adding a generic way to do global module options is a primal urge?

> 
> And this one's classicly easy: in single uses cases we always get the
> infrastructure wrong for future users anyway, so let's not do it until
> we have more than one user.

OK, we can add the hack for now. But perhaps have an archive of this
patch (maybe even sneak a link to it in the change log).

Hmm, we can just add:

LKML-Reference: <1289311867-10096-1-git-send-email-chris@chris-wilson.co.uk>


-- Steve