[Patch 1/3] ksym_tracer: Eliminate trace concatenation and machine stall issues post removal

[Patch 1/3] ksym_tracer: Eliminate trace concatenation and machine stall issues post removal

[K.Prasad]

[-- Attachment #1: new_ksym_fix_NEW01 --]
[-- Type: text/plain, Size: 3325 bytes --]

This patch fixes two issues reported with ksym tracer, seen after
removal of a symbol from the tracer i) Concatenation of trace logs
into a single line ii) Machine stall seen when logs are viewed
using 'trace_pipe'.

Known issue: Upon removal, 'cat trace_pipe' (without any preceding
command and any further output in the trace buffer) responds to
SIGTERM quickly but only after an indeterminate amount of delay
for SIGINT.

Signed-off-by: K.Prasad <prasad@linux.vnet.ibm.com>
---
 kernel/trace/trace.h      |   12 +++++++++---
 kernel/trace/trace_ksym.c |   11 ++++-------
 2 files changed, 13 insertions(+), 10 deletions(-)

Index: linux-2.6-tip.hbkpt/kernel/trace/trace_ksym.c
===================================================================
--- linux-2.6-tip.hbkpt.orig/kernel/trace/trace_ksym.c
+++ linux-2.6-tip.hbkpt/kernel/trace/trace_ksym.c
@@ -71,7 +71,7 @@ void ksym_hbp_handler(struct hw_breakpoi
 {
 	struct ring_buffer_event *event;
 	struct trace_array *tr;
-	struct trace_ksym *entry;
+	struct trace_ksym_rb *entry;
 	int pc;
 
 	if (!ksym_tracing_enabled)
@@ -87,7 +87,7 @@ void ksym_hbp_handler(struct hw_breakpoi
 
 	entry = ring_buffer_event_data(event);
 	strlcpy(entry->ksym_name, hbp->info.name, KSYM_SYMBOL_LEN);
-	entry->ksym_hbp = hbp;
+	memcpy(&(entry->ksym_hbp), hbp, sizeof(struct hw_breakpoint));
 	entry->ip = instruction_pointer(regs);
 	strlcpy(entry->p_name, current->comm, TASK_COMM_LEN);
 #ifdef CONFIG_PROFILE_KSYM_TRACER
@@ -380,7 +380,7 @@ static enum print_line_t ksym_trace_outp
 {
 	struct trace_entry *entry = iter->ent;
 	struct trace_seq *s = &iter->seq;
-	struct trace_ksym *field;
+	struct trace_ksym_rb *field;
 	char str[KSYM_SYMBOL_LEN];
 	int ret;
 
@@ -394,17 +394,14 @@ static enum print_line_t ksym_trace_outp
 	if (!ret)
 		return TRACE_TYPE_PARTIAL_LINE;
 
-	switch (field->ksym_hbp->info.type) {
+	switch (field->ksym_hbp.info.type) {
 	case HW_BREAKPOINT_WRITE:
 		ret = trace_seq_printf(s, " W  ");
 		break;
 	case HW_BREAKPOINT_RW:
 		ret = trace_seq_printf(s, " RW ");
 		break;
-	default:
-		return TRACE_TYPE_PARTIAL_LINE;
 	}
-
 	if (!ret)
 		return TRACE_TYPE_PARTIAL_LINE;
 
Index: linux-2.6-tip.hbkpt/kernel/trace/trace.h
===================================================================
--- linux-2.6-tip.hbkpt.orig/kernel/trace/trace.h
+++ linux-2.6-tip.hbkpt/kernel/trace/trace.h
@@ -216,15 +216,21 @@ struct syscall_trace_exit {
 extern int process_new_ksym_entry(char *ksymname, int op, unsigned long addr);
 
 struct trace_ksym {
-	struct trace_entry	ent;
 	struct hw_breakpoint	*ksym_hbp;
 	unsigned long		ksym_addr;
-	unsigned long		ip;
 #ifdef CONFIG_PROFILE_KSYM_TRACER
 	unsigned long		counter;
 #endif
 	struct hlist_node	ksym_hlist;
 	char			ksym_name[KSYM_NAME_LEN];
+};
+
+/* Ring buffer's copy of the breakpoint data */
+struct trace_ksym_rb {
+	struct trace_entry	ent;
+	struct hw_breakpoint	ksym_hbp;
+	unsigned long		ip;
+	char			ksym_name[KSYM_NAME_LEN];
 	char			p_name[TASK_COMM_LEN];
 };
 
@@ -343,7 +349,7 @@ extern void __ftrace_bad_type(void);
 			  TRACE_SYSCALL_ENTER);				\
 		IF_ASSIGN(var, ent, struct syscall_trace_exit,		\
 			  TRACE_SYSCALL_EXIT);				\
-		IF_ASSIGN(var, ent, struct trace_ksym, TRACE_KSYM);	\
+		IF_ASSIGN(var, ent, struct trace_ksym_rb, TRACE_KSYM);	\
 		__ftrace_bad_type();					\
 	} while (0)
 

[Patch 2/3] ksym_tracer: Allow bulk removal using empty or wildcard string input

[K.Prasad]

[-- Attachment #1: enable_new_features_02 --]
[-- Type: text/plain, Size: 4148 bytes --]

Accept an empty string or wildcard input (of the form *:---) as input for
ksym_trace_filter to perform bulk removal of all monitored entries.

Signed-off-by: K.Prasad <prasad@linux.vnet.ibm.com>
---
 kernel/trace/trace_ksym.c |   75 +++++++++++++++++++++++++---------------------
 1 file changed, 42 insertions(+), 33 deletions(-)

Index: linux-2.6-tip.hbkpt/kernel/trace/trace_ksym.c
===================================================================
--- linux-2.6-tip.hbkpt.orig/kernel/trace/trace_ksym.c
+++ linux-2.6-tip.hbkpt/kernel/trace/trace_ksym.c
@@ -138,6 +138,34 @@ static int ksym_trace_get_access_type(ch
 	return access;
 }
 
+static void ksym_trace_reset(struct trace_array *tr)
+{
+	struct trace_ksym *entry;
+	struct hlist_node *node, *node1;
+
+	ksym_tracing_enabled = 0;
+
+	mutex_lock(&ksym_tracer_mutex);
+	hlist_for_each_entry_safe(entry, node, node1, &ksym_filter_head,
+								ksym_hlist) {
+		unregister_kernel_hw_breakpoint(entry->ksym_hbp);
+		ksym_filter_entry_count--;
+		hlist_del_rcu(&(entry->ksym_hlist));
+		synchronize_rcu();
+		/* Free the 'input_string' only if reset
+		 * after startup self-test
+		 */
+#ifdef CONFIG_FTRACE_SELFTEST
+		if (strncmp(entry->ksym_hbp->info.name, KSYM_SELFTEST_ENTRY,
+					strlen(KSYM_SELFTEST_ENTRY)) != 0)
+#endif /* CONFIG_FTRACE_SELFTEST*/
+			kfree(entry->ksym_hbp->info.name);
+		kfree(entry->ksym_hbp);
+		kfree(entry);
+	}
+	mutex_unlock(&ksym_tracer_mutex);
+}
+
 /*
  * There can be several possible malformed requests and we attempt to capture
  * all of them. We enumerate some of the rules
@@ -163,7 +191,7 @@ static int parse_ksym_trace_str(char *in
 	/* Check for malformed request: (2), (1) and (5) */
 	if ((!input_string) ||
 		(strlen(input_string) != (KSYM_TRACER_OP_LEN + 1)) ||
-			(*addr == 0))
+			((*addr == 0) && strncmp(*ksymname, "*", strlen("*"))))
 		goto return_code;
 	ret = ksym_trace_get_access_type(input_string);
 
@@ -211,6 +239,7 @@ int process_new_ksym_entry(char *ksymnam
 	}
 	hlist_add_head_rcu(&(entry->ksym_hlist), &ksym_filter_head);
 	ksym_filter_entry_count++;
+	ksym_tracing_enabled = 1;
 
 	return 0;
 }
@@ -220,7 +249,7 @@ static ssize_t ksym_trace_filter_read(st
 {
 	struct trace_ksym *entry;
 	struct hlist_node *node;
-	char buf[KSYM_FILTER_ENTRY_LEN * KSYM_TRACER_MAX];
+	char buf[KSYM_FILTER_ENTRY_LEN * KSYM_TRACER_MAX] = "\0";
 	ssize_t ret, cnt = 0;
 
 	mutex_lock(&ksym_tracer_mutex);
@@ -251,9 +280,11 @@ static ssize_t ksym_trace_filter_write(s
 	unsigned long ksym_addr = 0;
 	int ret, op, changed = 0;
 
-	/* Ignore echo "" > ksym_trace_filter */
-	if (count == 0)
-		return 0;
+	/* Clear all breakpoint requests if echo "" > ksym_trace_filter */
+	if (count == 1) {
+		ksym_trace_reset(NULL);
+		return count;
+	}
 
 	input_string = kzalloc(count, GFP_KERNEL);
 	if (!input_string)
@@ -269,6 +300,12 @@ static ssize_t ksym_trace_filter_write(s
 		kfree(input_string);
 		return ret;
 	}
+	/* Clear all breakpoints if echo "*:---" > ksym_trace_filter */
+	if ((strncmp(ksymname, "*", strlen("*")) == 0) && (op == 0)) {
+		ksym_trace_reset(NULL);
+		kfree(input_string);
+		return count;
+	}
 
 	mutex_lock(&ksym_tracer_mutex);
 
@@ -325,34 +362,6 @@ static const struct file_operations ksym
 	.write		= ksym_trace_filter_write,
 };
 
-static void ksym_trace_reset(struct trace_array *tr)
-{
-	struct trace_ksym *entry;
-	struct hlist_node *node, *node1;
-
-	ksym_tracing_enabled = 0;
-
-	mutex_lock(&ksym_tracer_mutex);
-	hlist_for_each_entry_safe(entry, node, node1, &ksym_filter_head,
-								ksym_hlist) {
-		unregister_kernel_hw_breakpoint(entry->ksym_hbp);
-		ksym_filter_entry_count--;
-		hlist_del_rcu(&(entry->ksym_hlist));
-		synchronize_rcu();
-		/* Free the 'input_string' only if reset
-		 * after startup self-test
-		 */
-#ifdef CONFIG_FTRACE_SELFTEST
-		if (strncmp(entry->ksym_hbp->info.name, KSYM_SELFTEST_ENTRY,
-					strlen(KSYM_SELFTEST_ENTRY)) != 0)
-#endif /* CONFIG_FTRACE_SELFTEST*/
-			kfree(entry->ksym_hbp->info.name);
-		kfree(entry->ksym_hbp);
-		kfree(entry);
-	}
-	mutex_unlock(&ksym_tracer_mutex);
-}
-
 static int ksym_trace_init(struct trace_array *tr)
 {
 	int cpu, ret = 0;

[Patch 3/3] ksym_tracer: Documentation containing usage guide for ksym tracer

[K.Prasad]

[-- Attachment #1: documentation_patch_03 --]
[-- Type: text/plain, Size: 4511 bytes --]

This patch adds documentation for the ksym tracer plugin in ftrace. It
contains a minimal usage guide for the same.

Signed-off-by: K.Prasad <prasad@linux.vnet.ibm.com>
---
 Documentation/trace/ksym_tracer.txt |   90 ++++++++++++++++++++++++++++++++++++
 1 file changed, 90 insertions(+)

Index: linux-2.6-tip.hbkpt/Documentation/trace/ksym_tracer.txt
===================================================================
--- /dev/null
+++ linux-2.6-tip.hbkpt/Documentation/trace/ksym_tracer.txt
@@ -0,0 +1,90 @@
+			ksym_tracer - Kernel Symbol Tracer
+			----------------------------------
+			K.Prasad <prasad@linux.vnet.ibm.com>
+I. Introduction
+===============
+
+ksym_tracer uses the Hardware Breakpoint interface in the kernel to monitor
+kernel variables for memory access operations such as read and write.
+
+The number of kernel variables that can be monitored simultaneously is directly
+dependant upon the available free debug registers on the processor at the time
+of request.
+
+If the memory access operation on the variable added to the ksym tracer occurs,
+a trace containing details such as symbol name, function that accessed the
+variable, PID and cpu are logged onto the ring buffer.
+
+ksym tracer's use-cases can include the following:
+- Debug memory corruption issues: Trace write operations on a variable that is
+known to get corrupted on a system under debug
+- Profile variables: Understand how often a variable is being used by the
+system (such as read-mostly or write-mostly).
+
+II. Usage Guide
+===============
+
+The following is a list of steps to monitor a kernel variable using ksym
+tracer. It is illustrated by taking 'pid_max' as the kernel variable to be
+monitored.
+
+1) Mount debugfs to a directory on your system, say /sys/kernel/debug
+2) The memory access operations for which you want to monitor the variable can
+be specified as follows:
+ <ksym_name>:rwx
+The type of operation can be specified in three characters, and the valid
+combinations are dependant on the host processor. On x86 processors, the valid
+requests are '-w-' (write operations only) and 'rw-' (read or write
+operations). So to monitor 'pid_max' for write the corresponding string would be
+"pid_max:-w-".
+3) Do 'cat available_tracers' in <debugfs_mount>/tracing/ to check if
+'ksym_tracer' is listed as one of the available tracer. A 'ksym_trace_filter'
+file should be available to accept the kernel symbol inputs.
+4) echo  the kernel symbol along with the type operation string to
+ksym_trace_filter.
+e.g. echo pid_max:-w- > ksym_trace_filter
+Lookout for any error messages in the shell after the echo and in dmesg that
+indicates a failure of the request.
+5) Make sure that tracing is enabled by checking for 1 in 'tracing_enabled'
+file, if not 'echo 1 > tracing_enabled' to start tracing.
+
+Now a breakpoint over the kernel symbol (pid_max in this example) is active and
+is monitoring for any operations of the requested type. Upon such an operation,
+a trace containing the following information is logged and is available in the
+'trace' file.
+
+For instance, in case of the above example, the 'trace' file looks as shown
+below:
+# echo 32621 > /proc/sys/kernel/pid_max
+# cat trace
+# tracer: ksym_tracer
+#
+#       TASK-PID      CPU#      Symbol         Type    Function
+#          |           |          |              |         |
+bash            3027  2   pid_max               W  do_proc_dointvec_minmax_conv
+#
+
+In order to remove the trace, the kernel symbol should be echoed with all
+operators set to '-' like this '<ksym_name>:---'. For example,
+echo pid_max:--- > ksym_trace_filter.
+
+Alternatively if you choose to unregister all requests simultaneously you may
+do either of the following;
+echo > ksym_trace_filter
+echo *:--- > ksym_trace_filter
+
+The ksym tracer also provides a consolidated hit-counter that indicates the
+number of times a variable was subjected to the specified memory access
+operation. This is available in the file
+<debugfs_mount>/tracing/trace_stat/ksym_tracer.
+
+Limitations
+------------
+The known limitations of the ksym tracer are listed below. Efforts are
+through to support many of feature limitations.
+
+- ksym tracer can monitor only primary data types (such as int, char) and not
+arrays or instances or members of any structure.
+- Symbols that are defined in modules are not resolved by ksym tracer's symbol
+resolution mechanism.
+- Monitoring either read or write only is not supported on all processors.

Re: [Patch 1/3] ksym_tracer: Eliminate trace concatenation and machine stall issues post removal

[Frederic Weisbecker]

On Fri, Jun 19, 2009 at 10:54:53PM +0530, K.Prasad wrote:
> This patch fixes two issues reported with ksym tracer, seen after
> removal of a symbol from the tracer i) Concatenation of trace logs
> into a single line ii) Machine stall seen when logs are viewed
> using 'trace_pipe'.
> 
> Known issue: Upon removal, 'cat trace_pipe' (without any preceding
> command and any further output in the trace buffer) responds to
> SIGTERM quickly but only after an indeterminate amount of delay
> for SIGINT.
> 
> Signed-off-by: K.Prasad <prasad@linux.vnet.ibm.com>
> ---
>  kernel/trace/trace.h      |   12 +++++++++---
>  kernel/trace/trace_ksym.c |   11 ++++-------
>  2 files changed, 13 insertions(+), 10 deletions(-)
> 
> Index: linux-2.6-tip.hbkpt/kernel/trace/trace_ksym.c
> ===================================================================
> --- linux-2.6-tip.hbkpt.orig/kernel/trace/trace_ksym.c
> +++ linux-2.6-tip.hbkpt/kernel/trace/trace_ksym.c
> @@ -71,7 +71,7 @@ void ksym_hbp_handler(struct hw_breakpoi
>  {
>  	struct ring_buffer_event *event;
>  	struct trace_array *tr;
> -	struct trace_ksym *entry;
> +	struct trace_ksym_rb *entry;
>  	int pc;
>  
>  	if (!ksym_tracing_enabled)
> @@ -87,7 +87,7 @@ void ksym_hbp_handler(struct hw_breakpoi
>  
>  	entry = ring_buffer_event_data(event);
>  	strlcpy(entry->ksym_name, hbp->info.name, KSYM_SYMBOL_LEN);
> -	entry->ksym_hbp = hbp;
> +	memcpy(&(entry->ksym_hbp), hbp, sizeof(struct hw_breakpoint));


That looks wasteful. You only need the type from the arch_hw_breakpoint
and there you copy the whole generic breakpoint.

Frederic.



>  	entry->ip = instruction_pointer(regs);
>  	strlcpy(entry->p_name, current->comm, TASK_COMM_LEN);
>  #ifdef CONFIG_PROFILE_KSYM_TRACER
> @@ -380,7 +380,7 @@ static enum print_line_t ksym_trace_outp
>  {
>  	struct trace_entry *entry = iter->ent;
>  	struct trace_seq *s = &iter->seq;
> -	struct trace_ksym *field;
> +	struct trace_ksym_rb *field;
>  	char str[KSYM_SYMBOL_LEN];
>  	int ret;
>  
> @@ -394,17 +394,14 @@ static enum print_line_t ksym_trace_outp
>  	if (!ret)
>  		return TRACE_TYPE_PARTIAL_LINE;
>  
> -	switch (field->ksym_hbp->info.type) {
> +	switch (field->ksym_hbp.info.type) {
>  	case HW_BREAKPOINT_WRITE:
>  		ret = trace_seq_printf(s, " W  ");
>  		break;
>  	case HW_BREAKPOINT_RW:
>  		ret = trace_seq_printf(s, " RW ");
>  		break;
> -	default:
> -		return TRACE_TYPE_PARTIAL_LINE;
>  	}
> -
>  	if (!ret)
>  		return TRACE_TYPE_PARTIAL_LINE;
>  
> Index: linux-2.6-tip.hbkpt/kernel/trace/trace.h
> ===================================================================
> --- linux-2.6-tip.hbkpt.orig/kernel/trace/trace.h
> +++ linux-2.6-tip.hbkpt/kernel/trace/trace.h
> @@ -216,15 +216,21 @@ struct syscall_trace_exit {
>  extern int process_new_ksym_entry(char *ksymname, int op, unsigned long addr);
>  
>  struct trace_ksym {
> -	struct trace_entry	ent;
>  	struct hw_breakpoint	*ksym_hbp;
>  	unsigned long		ksym_addr;
> -	unsigned long		ip;
>  #ifdef CONFIG_PROFILE_KSYM_TRACER
>  	unsigned long		counter;
>  #endif
>  	struct hlist_node	ksym_hlist;
>  	char			ksym_name[KSYM_NAME_LEN];
> +};
> +
> +/* Ring buffer's copy of the breakpoint data */
> +struct trace_ksym_rb {
> +	struct trace_entry	ent;
> +	struct hw_breakpoint	ksym_hbp;
> +	unsigned long		ip;
> +	char			ksym_name[KSYM_NAME_LEN];
>  	char			p_name[TASK_COMM_LEN];
>  };
>  
> @@ -343,7 +349,7 @@ extern void __ftrace_bad_type(void);
>  			  TRACE_SYSCALL_ENTER);				\
>  		IF_ASSIGN(var, ent, struct syscall_trace_exit,		\
>  			  TRACE_SYSCALL_EXIT);				\
> -		IF_ASSIGN(var, ent, struct trace_ksym, TRACE_KSYM);	\
> +		IF_ASSIGN(var, ent, struct trace_ksym_rb, TRACE_KSYM);	\
>  		__ftrace_bad_type();					\
>  	} while (0)
>  
> 

Re: [Patch 2/3] ksym_tracer: Allow bulk removal using empty or wildcard string input

[Frederic Weisbecker]

On Fri, Jun 19, 2009 at 10:55:12PM +0530, K.Prasad wrote:
> Accept an empty string or wildcard input (of the form *:---) as input for
> ksym_trace_filter to perform bulk removal of all monitored entries.
>
> Signed-off-by: K.Prasad <prasad@linux.vnet.ibm.com>
> ---
>  kernel/trace/trace_ksym.c |   75 +++++++++++++++++++++++++---------------------
>  1 file changed, 42 insertions(+), 33 deletions(-)
> 
> Index: linux-2.6-tip.hbkpt/kernel/trace/trace_ksym.c
> ===================================================================
> --- linux-2.6-tip.hbkpt.orig/kernel/trace/trace_ksym.c
> +++ linux-2.6-tip.hbkpt/kernel/trace/trace_ksym.c
> @@ -138,6 +138,34 @@ static int ksym_trace_get_access_type(ch
>  	return access;
>  }
>  
> +static void ksym_trace_reset(struct trace_array *tr)
> +{
> +	struct trace_ksym *entry;
> +	struct hlist_node *node, *node1;
> +
> +	ksym_tracing_enabled = 0;
> +
> +	mutex_lock(&ksym_tracer_mutex);
> +	hlist_for_each_entry_safe(entry, node, node1, &ksym_filter_head,
> +								ksym_hlist) {
> +		unregister_kernel_hw_breakpoint(entry->ksym_hbp);
> +		ksym_filter_entry_count--;
> +		hlist_del_rcu(&(entry->ksym_hlist));
> +		synchronize_rcu();
> +		/* Free the 'input_string' only if reset
> +		 * after startup self-test
> +		 */
> +#ifdef CONFIG_FTRACE_SELFTEST
> +		if (strncmp(entry->ksym_hbp->info.name, KSYM_SELFTEST_ENTRY,
> +					strlen(KSYM_SELFTEST_ENTRY)) != 0)
> +#endif /* CONFIG_FTRACE_SELFTEST*/
> +			kfree(entry->ksym_hbp->info.name);
> +		kfree(entry->ksym_hbp);
> +		kfree(entry);
> +	}
> +	mutex_unlock(&ksym_tracer_mutex);
> +}
> +
>  /*
>   * There can be several possible malformed requests and we attempt to capture
>   * all of them. We enumerate some of the rules
> @@ -163,7 +191,7 @@ static int parse_ksym_trace_str(char *in
>  	/* Check for malformed request: (2), (1) and (5) */
>  	if ((!input_string) ||
>  		(strlen(input_string) != (KSYM_TRACER_OP_LEN + 1)) ||
> -			(*addr == 0))
> +			((*addr == 0) && strncmp(*ksymname, "*", strlen("*"))))



I guess you can just use strcmp() here, no? As in other use
of this same strncmp() in this patch.



>  		goto return_code;
>  	ret = ksym_trace_get_access_type(input_string);
>  
> @@ -211,6 +239,7 @@ int process_new_ksym_entry(char *ksymnam
>  	}
>  	hlist_add_head_rcu(&(entry->ksym_hlist), &ksym_filter_head);
>  	ksym_filter_entry_count++;
> +	ksym_tracing_enabled = 1;
>  
>  	return 0;
>  }
> @@ -220,7 +249,7 @@ static ssize_t ksym_trace_filter_read(st
>  {
>  	struct trace_ksym *entry;
>  	struct hlist_node *node;
> -	char buf[KSYM_FILTER_ENTRY_LEN * KSYM_TRACER_MAX];
> +	char buf[KSYM_FILTER_ENTRY_LEN * KSYM_TRACER_MAX] = "\0";
>  	ssize_t ret, cnt = 0;
>  
>  	mutex_lock(&ksym_tracer_mutex);
> @@ -251,9 +280,11 @@ static ssize_t ksym_trace_filter_write(s
>  	unsigned long ksym_addr = 0;
>  	int ret, op, changed = 0;
>  
> -	/* Ignore echo "" > ksym_trace_filter */
> -	if (count == 0)
> -		return 0;
> +	/* Clear all breakpoint requests if echo "" > ksym_trace_filter */
> +	if (count == 1) {
> +		ksym_trace_reset(NULL);
> +		return count;
> +	}


And then if I do:

echo -n 1 > ksym_trace_filter

That would also clean these breakpoints?

Frederic.



>  
>  	input_string = kzalloc(count, GFP_KERNEL);
>  	if (!input_string)
> @@ -269,6 +300,12 @@ static ssize_t ksym_trace_filter_write(s
>  		kfree(input_string);
>  		return ret;
>  	}
> +	/* Clear all breakpoints if echo "*:---" > ksym_trace_filter */
> +	if ((strncmp(ksymname, "*", strlen("*")) == 0) && (op == 0)) {
> +		ksym_trace_reset(NULL);
> +		kfree(input_string);
> +		return count;
> +	}
>  
>  	mutex_lock(&ksym_tracer_mutex);
>  
> @@ -325,34 +362,6 @@ static const struct file_operations ksym
>  	.write		= ksym_trace_filter_write,
>  };
>  
> -static void ksym_trace_reset(struct trace_array *tr)
> -{
> -	struct trace_ksym *entry;
> -	struct hlist_node *node, *node1;
> -
> -	ksym_tracing_enabled = 0;
> -
> -	mutex_lock(&ksym_tracer_mutex);
> -	hlist_for_each_entry_safe(entry, node, node1, &ksym_filter_head,
> -								ksym_hlist) {
> -		unregister_kernel_hw_breakpoint(entry->ksym_hbp);
> -		ksym_filter_entry_count--;
> -		hlist_del_rcu(&(entry->ksym_hlist));
> -		synchronize_rcu();
> -		/* Free the 'input_string' only if reset
> -		 * after startup self-test
> -		 */
> -#ifdef CONFIG_FTRACE_SELFTEST
> -		if (strncmp(entry->ksym_hbp->info.name, KSYM_SELFTEST_ENTRY,
> -					strlen(KSYM_SELFTEST_ENTRY)) != 0)
> -#endif /* CONFIG_FTRACE_SELFTEST*/
> -			kfree(entry->ksym_hbp->info.name);
> -		kfree(entry->ksym_hbp);
> -		kfree(entry);
> -	}
> -	mutex_unlock(&ksym_tracer_mutex);
> -}
> -
>  static int ksym_trace_init(struct trace_array *tr)
>  {
>  	int cpu, ret = 0;
> 

Re: [Patch 3/3] ksym_tracer: Documentation containing usage guide for ksym tracer

[Frederic Weisbecker]

On Fri, Jun 19, 2009 at 10:55:31PM +0530, K.Prasad wrote:
> This patch adds documentation for the ksym tracer plugin in ftrace. It
> contains a minimal usage guide for the same.
> 
> Signed-off-by: K.Prasad <prasad@linux.vnet.ibm.com>
> ---
>  Documentation/trace/ksym_tracer.txt |   90 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 90 insertions(+)
> 
> Index: linux-2.6-tip.hbkpt/Documentation/trace/ksym_tracer.txt
> ===================================================================
> --- /dev/null
> +++ linux-2.6-tip.hbkpt/Documentation/trace/ksym_tracer.txt
> @@ -0,0 +1,90 @@
> +			ksym_tracer - Kernel Symbol Tracer
> +			----------------------------------
> +			K.Prasad <prasad@linux.vnet.ibm.com>
> +I. Introduction
> +===============
> +
> +ksym_tracer uses the Hardware Breakpoint interface in the kernel to monitor
> +kernel variables for memory access operations such as read and write.
> +
> +The number of kernel variables that can be monitored simultaneously is directly
> +dependant upon the available free debug registers on the processor at the time
> +of request.
> +
> +If the memory access operation on the variable added to the ksym tracer occurs,
> +a trace containing details such as symbol name, function that accessed the
> +variable, PID and cpu are logged onto the ring buffer.
> +
> +ksym tracer's use-cases can include the following:
> +- Debug memory corruption issues: Trace write operations on a variable that is
> +known to get corrupted on a system under debug
> +- Profile variables: Understand how often a variable is being used by the
> +system (such as read-mostly or write-mostly).
> +
> +II. Usage Guide
> +===============
> +
> +The following is a list of steps to monitor a kernel variable using ksym
> +tracer. It is illustrated by taking 'pid_max' as the kernel variable to be
> +monitored.
> +
> +1) Mount debugfs to a directory on your system, say /sys/kernel/debug
> +2) The memory access operations for which you want to monitor the variable can
> +be specified as follows:
> + <ksym_name>:rwx
> +The type of operation can be specified in three characters, and the valid
> +combinations are dependant on the host processor. On x86 processors, the valid
> +requests are '-w-' (write operations only) and 'rw-' (read or write
> +operations). So to monitor 'pid_max' for write the corresponding string would be
> +"pid_max:-w-".
> +3) Do 'cat available_tracers' in <debugfs_mount>/tracing/ to check if
> +'ksym_tracer' is listed as one of the available tracer. A 'ksym_trace_filter'
> +file should be available to accept the kernel symbol inputs.
> +4) echo  the kernel symbol along with the type operation string to
> +ksym_trace_filter.
> +e.g. echo pid_max:-w- > ksym_trace_filter
> +Lookout for any error messages in the shell after the echo and in dmesg that
> +indicates a failure of the request.
> +5) Make sure that tracing is enabled by checking for 1 in 'tracing_enabled'
> +file, if not 'echo 1 > tracing_enabled' to start tracing.
> +
> +Now a breakpoint over the kernel symbol (pid_max in this example) is active and
> +is monitoring for any operations of the requested type. Upon such an operation,
> +a trace containing the following information is logged and is available in the
> +'trace' file.
> +
> +For instance, in case of the above example, the 'trace' file looks as shown
> +below:
> +# echo 32621 > /proc/sys/kernel/pid_max
> +# cat trace
> +# tracer: ksym_tracer
> +#
> +#       TASK-PID      CPU#      Symbol         Type    Function
> +#          |           |          |              |         |
> +bash            3027  2   pid_max               W  do_proc_dointvec_minmax_conv
> +#
> +
> +In order to remove the trace, the kernel symbol should be echoed with all
> +operators set to '-' like this '<ksym_name>:---'. For example,
> +echo pid_max:--- > ksym_trace_filter.
> +
> +Alternatively if you choose to unregister all requests simultaneously you may
> +do either of the following;
> +echo > ksym_trace_filter
> +echo *:--- > ksym_trace_filter
> +
> +The ksym tracer also provides a consolidated hit-counter that indicates the
> +number of times a variable was subjected to the specified memory access
> +operation. This is available in the file
> +<debugfs_mount>/tracing/trace_stat/ksym_tracer.
> +
> +Limitations
> +------------
> +The known limitations of the ksym tracer are listed below. Efforts are
> +through to support many of feature limitations.
> +
> +- ksym tracer can monitor only primary data types (such as int, char) and not
> +arrays or instances or members of any structure.
> +- Symbols that are defined in modules are not resolved by ksym tracer's symbol
> +resolution mechanism.
> +- Monitoring either read or write only is not supported on all processors.
> 


Nice documentation, thanks!

Re: [Patch 1/3] ksym_tracer: Eliminate trace concatenation and machine stall issues post removal

[K.Prasad]

On Sat, Jun 20, 2009 at 01:03:49AM +0200, Frederic Weisbecker wrote:
> On Fri, Jun 19, 2009 at 10:54:53PM +0530, K.Prasad wrote:
> > This patch fixes two issues reported with ksym tracer, seen after
> > removal of a symbol from the tracer i) Concatenation of trace logs
> > into a single line ii) Machine stall seen when logs are viewed
> > using 'trace_pipe'.
> > 
> > Known issue: Upon removal, 'cat trace_pipe' (without any preceding
> > command and any further output in the trace buffer) responds to
> > SIGTERM quickly but only after an indeterminate amount of delay
> > for SIGINT.
> > 
> > Signed-off-by: K.Prasad <prasad@linux.vnet.ibm.com>
> > ---
> >  kernel/trace/trace.h      |   12 +++++++++---
> >  kernel/trace/trace_ksym.c |   11 ++++-------
> >  2 files changed, 13 insertions(+), 10 deletions(-)
> > 
> > Index: linux-2.6-tip.hbkpt/kernel/trace/trace_ksym.c
> > ===================================================================
> > --- linux-2.6-tip.hbkpt.orig/kernel/trace/trace_ksym.c
> > +++ linux-2.6-tip.hbkpt/kernel/trace/trace_ksym.c
> > @@ -71,7 +71,7 @@ void ksym_hbp_handler(struct hw_breakpoi
> >  {
> >  	struct ring_buffer_event *event;
> >  	struct trace_array *tr;
> > -	struct trace_ksym *entry;
> > +	struct trace_ksym_rb *entry;
> >  	int pc;
> >  
> >  	if (!ksym_tracing_enabled)
> > @@ -87,7 +87,7 @@ void ksym_hbp_handler(struct hw_breakpoi
> >  
> >  	entry = ring_buffer_event_data(event);
> >  	strlcpy(entry->ksym_name, hbp->info.name, KSYM_SYMBOL_LEN);
> > -	entry->ksym_hbp = hbp;
> > +	memcpy(&(entry->ksym_hbp), hbp, sizeof(struct hw_breakpoint));
> 
> 
> That looks wasteful. You only need the type from the arch_hw_breakpoint
> and there you copy the whole generic breakpoint.
> 
> Frederic.
>

Well, the entire copy operation for ksym_name and p_name is indeed a
much bigger waste (KSYM_SYMBOL_LEN and TASK_COMM_LEN bytes respetively)
and the breakpoint structure size is tiny in comparison. 

The type information is an arch-specific field and it encapsulated
within 'struct hw_breakpoint'. In any case this solution needs
improvement more in terms of getting the tracer infrastructure to
identify stale entries (belonging to removed symbols) and prevent their
display. The existing trace logs contain huge redundant information due
to multiple copies of the same data and it would yield much better results
than such trivial optimisations.  The ksym_tracer, I believe, has just
unearthed the opportunity for the same as it is (is it?) the first such
ftrace plugin to partially remove entries.

Thanks,
K.Prasad

[Patch 2/3] ksym_tracer: Allow bulk removal using empty or wildcard string input - ver II

[K.Prasad]

Accept an empty string or wildcard input (of the form *:---) as input for
ksym_trace_filter to perform bulk removal of all monitored entries.

Signed-off-by: K.Prasad <prasad@linux.vnet.ibm.com>
---
 kernel/trace/trace_ksym.c |   77 +++++++++++++++++++++++++---------------------
 1 file changed, 43 insertions(+), 34 deletions(-)

Index: linux-2.6-tip.hbkpt/kernel/trace/trace_ksym.c
===================================================================
--- linux-2.6-tip.hbkpt.orig/kernel/trace/trace_ksym.c
+++ linux-2.6-tip.hbkpt/kernel/trace/trace_ksym.c
@@ -23,6 +23,7 @@
 #include <linux/debugfs.h>
 #include <linux/ftrace.h>
 #include <linux/module.h>
+#include <linux/ctype.h>
 #include <linux/fs.h>
 
 #include "trace_output.h"
@@ -138,6 +139,34 @@ static int ksym_trace_get_access_type(ch
 	return access;
 }
 
+static void ksym_trace_reset(struct trace_array *tr)
+{
+	struct trace_ksym *entry;
+	struct hlist_node *node, *node1;
+
+	ksym_tracing_enabled = 0;
+
+	mutex_lock(&ksym_tracer_mutex);
+	hlist_for_each_entry_safe(entry, node, node1, &ksym_filter_head,
+								ksym_hlist) {
+		unregister_kernel_hw_breakpoint(entry->ksym_hbp);
+		ksym_filter_entry_count--;
+		hlist_del_rcu(&(entry->ksym_hlist));
+		synchronize_rcu();
+		/* Free the 'input_string' only if reset
+		 * after startup self-test
+		 */
+#ifdef CONFIG_FTRACE_SELFTEST
+		if (strncmp(entry->ksym_hbp->info.name, KSYM_SELFTEST_ENTRY,
+					strlen(KSYM_SELFTEST_ENTRY)) != 0)
+#endif /* CONFIG_FTRACE_SELFTEST*/
+			kfree(entry->ksym_hbp->info.name);
+		kfree(entry->ksym_hbp);
+		kfree(entry);
+	}
+	mutex_unlock(&ksym_tracer_mutex);
+}
+
 /*
  * There can be several possible malformed requests and we attempt to capture
  * all of them. We enumerate some of the rules
@@ -163,7 +192,7 @@ static int parse_ksym_trace_str(char *in
 	/* Check for malformed request: (2), (1) and (5) */
 	if ((!input_string) ||
 		(strlen(input_string) != (KSYM_TRACER_OP_LEN + 1)) ||
-			(*addr == 0))
+			((*addr == 0) && strncmp(*ksymname, "*", strlen("*"))))
 		goto return_code;
 	ret = ksym_trace_get_access_type(input_string);
 
@@ -211,6 +240,7 @@ int process_new_ksym_entry(char *ksymnam
 	}
 	hlist_add_head_rcu(&(entry->ksym_hlist), &ksym_filter_head);
 	ksym_filter_entry_count++;
+	ksym_tracing_enabled = 1;
 
 	return 0;
 }
@@ -220,7 +250,7 @@ static ssize_t ksym_trace_filter_read(st
 {
 	struct trace_ksym *entry;
 	struct hlist_node *node;
-	char buf[KSYM_FILTER_ENTRY_LEN * KSYM_TRACER_MAX];
+	char buf[KSYM_FILTER_ENTRY_LEN * KSYM_TRACER_MAX] = "\0";
 	ssize_t ret, cnt = 0;
 
 	mutex_lock(&ksym_tracer_mutex);
@@ -251,10 +281,6 @@ static ssize_t ksym_trace_filter_write(s
 	unsigned long ksym_addr = 0;
 	int ret, op, changed = 0;
 
-	/* Ignore echo "" > ksym_trace_filter */
-	if (count == 0)
-		return 0;
-
 	input_string = kzalloc(count, GFP_KERNEL);
 	if (!input_string)
 		return -ENOMEM;
@@ -263,12 +289,23 @@ static ssize_t ksym_trace_filter_write(s
 		kfree(input_string);
 		return -EFAULT;
 	}
+	/* Clear all breakpoint requests if echo "" > ksym_trace_filter */
+	if ((count == 1) && !isalnum(input_string[0])) {
+		ksym_trace_reset(NULL);
+		return count;
+	}
 
 	ret = op = parse_ksym_trace_str(input_string, &ksymname, &ksym_addr);
 	if (ret < 0) {
 		kfree(input_string);
 		return ret;
 	}
+	/* Clear all breakpoints if echo "*:---" > ksym_trace_filter */
+	if ((strncmp(ksymname, "*", strlen("*")) == 0) && (op == 0)) {
+		ksym_trace_reset(NULL);
+		kfree(input_string);
+		return count;
+	}
 
 	mutex_lock(&ksym_tracer_mutex);
 
@@ -325,34 +362,6 @@ static const struct file_operations ksym
 	.write		= ksym_trace_filter_write,
 };
 
-static void ksym_trace_reset(struct trace_array *tr)
-{
-	struct trace_ksym *entry;
-	struct hlist_node *node, *node1;
-
-	ksym_tracing_enabled = 0;
-
-	mutex_lock(&ksym_tracer_mutex);
-	hlist_for_each_entry_safe(entry, node, node1, &ksym_filter_head,
-								ksym_hlist) {
-		unregister_kernel_hw_breakpoint(entry->ksym_hbp);
-		ksym_filter_entry_count--;
-		hlist_del_rcu(&(entry->ksym_hlist));
-		synchronize_rcu();
-		/* Free the 'input_string' only if reset
-		 * after startup self-test
-		 */
-#ifdef CONFIG_FTRACE_SELFTEST
-		if (strncmp(entry->ksym_hbp->info.name, KSYM_SELFTEST_ENTRY,
-					strlen(KSYM_SELFTEST_ENTRY)) != 0)
-#endif /* CONFIG_FTRACE_SELFTEST*/
-			kfree(entry->ksym_hbp->info.name);
-		kfree(entry->ksym_hbp);
-		kfree(entry);
-	}
-	mutex_unlock(&ksym_tracer_mutex);
-}
-
 static int ksym_trace_init(struct trace_array *tr)
 {
 	int cpu, ret = 0;

Re: [Patch 2/3] ksym_tracer: Allow bulk removal using empty or wildcard string input

[K.Prasad]

On Sat, Jun 20, 2009 at 01:16:41AM +0200, Frederic Weisbecker wrote:
> On Fri, Jun 19, 2009 at 10:55:12PM +0530, K.Prasad wrote:
> > Accept an empty string or wildcard input (of the form *:---) as input for
> > ksym_trace_filter to perform bulk removal of all monitored entries.
> >
> > Signed-off-by: K.Prasad <prasad@linux.vnet.ibm.com>
> > ---
> >  kernel/trace/trace_ksym.c |   75 +++++++++++++++++++++++++---------------------
> >  1 file changed, 42 insertions(+), 33 deletions(-)
> > 
<snipped>
> >  /*
> >   * There can be several possible malformed requests and we attempt to capture
> >   * all of them. We enumerate some of the rules
> > @@ -163,7 +191,7 @@ static int parse_ksym_trace_str(char *in
> >  	/* Check for malformed request: (2), (1) and (5) */
> >  	if ((!input_string) ||
> >  		(strlen(input_string) != (KSYM_TRACER_OP_LEN + 1)) ||
> > -			(*addr == 0))
> > +			((*addr == 0) && strncmp(*ksymname, "*", strlen("*"))))
> 
> 
> I guess you can just use strcmp() here, no? As in other use
> of this same strncmp() in this patch.
> 

I see both functions being used in the kernel at random (what happened
to all the buffer-overflow scare?)! Isn't an strncmp() preferred over
the plain strcmp() (atleast it cannot be the other way round)?

> > @@ -251,9 +280,11 @@ static ssize_t ksym_trace_filter_write(s
> >  	unsigned long ksym_addr = 0;
> >  	int ret, op, changed = 0;
> >  
> > -	/* Ignore echo "" > ksym_trace_filter */
> > -	if (count == 0)
> > -		return 0;
> > +	/* Clear all breakpoint requests if echo "" > ksym_trace_filter */
> > +	if (count == 1) {
> > +		ksym_trace_reset(NULL);
> > +		return count;
> > +	}
> 
> 
> And then if I do:
> 
> echo -n 1 > ksym_trace_filter
> 
> That would also clean these breakpoints?
> 
> Frederic.
>

No, I did not think about this scenario and I am changing the patch to
handle such a case. Also, the value of 'count' cannot be '0' as such
inputs are ignored (e.g. echo -n "" > ksym_trace_filter is simply
ignored) and so I'm removing the check for the same.

Please find a new version of the patch sent here:
http://lkml.org/lkml/2009/6/20/73.

Thanks,
K.Prasad

 

Re: [Patch 2/3] ksym_tracer: Allow bulk removal using empty or wildcard string input

[Frederic Weisbecker]

On Sat, Jun 20, 2009 at 07:07:52PM +0530, K.Prasad wrote:
> On Sat, Jun 20, 2009 at 01:16:41AM +0200, Frederic Weisbecker wrote:
> > On Fri, Jun 19, 2009 at 10:55:12PM +0530, K.Prasad wrote:
> > > Accept an empty string or wildcard input (of the form *:---) as input for
> > > ksym_trace_filter to perform bulk removal of all monitored entries.
> > >
> > > Signed-off-by: K.Prasad <prasad@linux.vnet.ibm.com>
> > > ---
> > >  kernel/trace/trace_ksym.c |   75 +++++++++++++++++++++++++---------------------
> > >  1 file changed, 42 insertions(+), 33 deletions(-)
> > > 
> <snipped>
> > >  /*
> > >   * There can be several possible malformed requests and we attempt to capture
> > >   * all of them. We enumerate some of the rules
> > > @@ -163,7 +191,7 @@ static int parse_ksym_trace_str(char *in
> > >  	/* Check for malformed request: (2), (1) and (5) */
> > >  	if ((!input_string) ||
> > >  		(strlen(input_string) != (KSYM_TRACER_OP_LEN + 1)) ||
> > > -			(*addr == 0))
> > > +			((*addr == 0) && strncmp(*ksymname, "*", strlen("*"))))
> > 
> > 
> > I guess you can just use strcmp() here, no? As in other use
> > of this same strncmp() in this patch.
> > 
> 
> I see both functions being used in the kernel at random (what happened
> to all the buffer-overflow scare?)! Isn't an strncmp() preferred over
> the plain strcmp() (atleast it cannot be the other way round)?


I guess it depends if the string has complicated origins :)
But a plain 1 for the len would be sufficient.
Or here you can just check **ksymname != '*'

Anyway, I'm arguing about neat things that don't matter that much.

Thanks.
 
> > > @@ -251,9 +280,11 @@ static ssize_t ksym_trace_filter_write(s
> > >  	unsigned long ksym_addr = 0;
> > >  	int ret, op, changed = 0;
> > >  
> > > -	/* Ignore echo "" > ksym_trace_filter */
> > > -	if (count == 0)
> > > -		return 0;
> > > +	/* Clear all breakpoint requests if echo "" > ksym_trace_filter */
> > > +	if (count == 1) {
> > > +		ksym_trace_reset(NULL);
> > > +		return count;
> > > +	}
> > 
> > 
> > And then if I do:
> > 
> > echo -n 1 > ksym_trace_filter
> > 
> > That would also clean these breakpoints?
> > 
> > Frederic.
> >
> 
> No, I did not think about this scenario and I am changing the patch to
> handle such a case. Also, the value of 'count' cannot be '0' as such
> inputs are ignored (e.g. echo -n "" > ksym_trace_filter is simply
> ignored) and so I'm removing the check for the same.
> 
> Please find a new version of the patch sent here:
> http://lkml.org/lkml/2009/6/20/73.
> 
> Thanks,
> K.Prasad
> 
>  

Re: [Patch 1/3] ksym_tracer: Eliminate trace concatenation and machine stall issues post removal

[Frederic Weisbecker]

On Sat, Jun 20, 2009 at 09:27:25AM +0530, K.Prasad wrote:
> On Sat, Jun 20, 2009 at 01:03:49AM +0200, Frederic Weisbecker wrote:
> > On Fri, Jun 19, 2009 at 10:54:53PM +0530, K.Prasad wrote:
> > > This patch fixes two issues reported with ksym tracer, seen after
> > > removal of a symbol from the tracer i) Concatenation of trace logs
> > > into a single line ii) Machine stall seen when logs are viewed
> > > using 'trace_pipe'.
> > > 
> > > Known issue: Upon removal, 'cat trace_pipe' (without any preceding
> > > command and any further output in the trace buffer) responds to
> > > SIGTERM quickly but only after an indeterminate amount of delay
> > > for SIGINT.
> > > 
> > > Signed-off-by: K.Prasad <prasad@linux.vnet.ibm.com>
> > > ---
> > >  kernel/trace/trace.h      |   12 +++++++++---
> > >  kernel/trace/trace_ksym.c |   11 ++++-------
> > >  2 files changed, 13 insertions(+), 10 deletions(-)
> > > 
> > > Index: linux-2.6-tip.hbkpt/kernel/trace/trace_ksym.c
> > > ===================================================================
> > > --- linux-2.6-tip.hbkpt.orig/kernel/trace/trace_ksym.c
> > > +++ linux-2.6-tip.hbkpt/kernel/trace/trace_ksym.c
> > > @@ -71,7 +71,7 @@ void ksym_hbp_handler(struct hw_breakpoi
> > >  {
> > >  	struct ring_buffer_event *event;
> > >  	struct trace_array *tr;
> > > -	struct trace_ksym *entry;
> > > +	struct trace_ksym_rb *entry;
> > >  	int pc;
> > >  
> > >  	if (!ksym_tracing_enabled)
> > > @@ -87,7 +87,7 @@ void ksym_hbp_handler(struct hw_breakpoi
> > >  
> > >  	entry = ring_buffer_event_data(event);
> > >  	strlcpy(entry->ksym_name, hbp->info.name, KSYM_SYMBOL_LEN);
> > > -	entry->ksym_hbp = hbp;
> > > +	memcpy(&(entry->ksym_hbp), hbp, sizeof(struct hw_breakpoint));
> > 
> > 
> > That looks wasteful. You only need the type from the arch_hw_breakpoint
> > and there you copy the whole generic breakpoint.
> > 
> > Frederic.
> >
> 
> Well, the entire copy operation for ksym_name and p_name is indeed a
> much bigger waste (KSYM_SYMBOL_LEN and TASK_COMM_LEN bytes respetively)
> and the breakpoint structure size is tiny in comparison. 
> 
> The type information is an arch-specific field and it encapsulated
> within 'struct hw_breakpoint'. In any case this solution needs
> improvement more in terms of getting the tracer infrastructure to
> identify stale entries (belonging to removed symbols) and prevent their
> display. The existing trace logs contain huge redundant information due
> to multiple copies of the same data and it would yield much better results
> than such trivial optimisations.  The ksym_tracer, I believe, has just
> unearthed the opportunity for the same as it is (is it?) the first such
> ftrace plugin to partially remove entries.


Well, we had such issue by the past and the problem can't
be easily solvable. Traces can possibily stay in the ring buffer for a while
until they become consumed by a reader.

It's safe and keep the things simple to just copy the required informations
in the ring buffer.

Also your type is indeed arch dependent, but copying struct hw_breakpoint
instead of the hw_breakpoint in the ring buffer doesn't solve the problem,
you'll still have some #ifdef ARCH once you'll need to support other archs.

Copying the whole struct breakpoint in the ring buffer doesn't make sense.
You're even copying the address of the triggered callback whereas all you
need is only the type field from the struct arch_hw_breakpoint ?


Btw, that makes me remind that I really think this hardware breakpoint API
should provide a real abstraction of the arch dependent hardware breakpoints.


> 
> Thanks,
> K.Prasad
> 

Re: [Patch 2/3] ksym_tracer: Allow bulk removal using empty or wildcard string input - ver II

[Frederic Weisbecker]

On Sat, Jun 20, 2009 at 07:05:07PM +0530, K.Prasad wrote:
> Accept an empty string or wildcard input (of the form *:---) as input for
> ksym_trace_filter to perform bulk removal of all monitored entries.
> 
> Signed-off-by: K.Prasad <prasad@linux.vnet.ibm.com>
> ---
>  kernel/trace/trace_ksym.c |   77 +++++++++++++++++++++++++---------------------
>  1 file changed, 43 insertions(+), 34 deletions(-)
> 
> Index: linux-2.6-tip.hbkpt/kernel/trace/trace_ksym.c
> ===================================================================
> --- linux-2.6-tip.hbkpt.orig/kernel/trace/trace_ksym.c
> +++ linux-2.6-tip.hbkpt/kernel/trace/trace_ksym.c
> @@ -23,6 +23,7 @@
>  #include <linux/debugfs.h>
>  #include <linux/ftrace.h>
>  #include <linux/module.h>
> +#include <linux/ctype.h>
>  #include <linux/fs.h>
>  
>  #include "trace_output.h"
> @@ -138,6 +139,34 @@ static int ksym_trace_get_access_type(ch
>  	return access;
>  }
>  
> +static void ksym_trace_reset(struct trace_array *tr)
> +{
> +	struct trace_ksym *entry;
> +	struct hlist_node *node, *node1;
> +
> +	ksym_tracing_enabled = 0;
> +
> +	mutex_lock(&ksym_tracer_mutex);
> +	hlist_for_each_entry_safe(entry, node, node1, &ksym_filter_head,
> +								ksym_hlist) {
> +		unregister_kernel_hw_breakpoint(entry->ksym_hbp);
> +		ksym_filter_entry_count--;
> +		hlist_del_rcu(&(entry->ksym_hlist));
> +		synchronize_rcu();
> +		/* Free the 'input_string' only if reset
> +		 * after startup self-test
> +		 */
> +#ifdef CONFIG_FTRACE_SELFTEST
> +		if (strncmp(entry->ksym_hbp->info.name, KSYM_SELFTEST_ENTRY,
> +					strlen(KSYM_SELFTEST_ENTRY)) != 0)
> +#endif /* CONFIG_FTRACE_SELFTEST*/
> +			kfree(entry->ksym_hbp->info.name);
> +		kfree(entry->ksym_hbp);
> +		kfree(entry);
> +	}
> +	mutex_unlock(&ksym_tracer_mutex);
> +}
> +
>  /*
>   * There can be several possible malformed requests and we attempt to capture
>   * all of them. We enumerate some of the rules
> @@ -163,7 +192,7 @@ static int parse_ksym_trace_str(char *in
>  	/* Check for malformed request: (2), (1) and (5) */
>  	if ((!input_string) ||
>  		(strlen(input_string) != (KSYM_TRACER_OP_LEN + 1)) ||
> -			(*addr == 0))
> +			((*addr == 0) && strncmp(*ksymname, "*", strlen("*"))))
>  		goto return_code;
>  	ret = ksym_trace_get_access_type(input_string);
>  
> @@ -211,6 +240,7 @@ int process_new_ksym_entry(char *ksymnam
>  	}
>  	hlist_add_head_rcu(&(entry->ksym_hlist), &ksym_filter_head);
>  	ksym_filter_entry_count++;
> +	ksym_tracing_enabled = 1;
>  
>  	return 0;
>  }
> @@ -220,7 +250,7 @@ static ssize_t ksym_trace_filter_read(st
>  {
>  	struct trace_ksym *entry;
>  	struct hlist_node *node;
> -	char buf[KSYM_FILTER_ENTRY_LEN * KSYM_TRACER_MAX];
> +	char buf[KSYM_FILTER_ENTRY_LEN * KSYM_TRACER_MAX] = "\0";
>  	ssize_t ret, cnt = 0;
>  
>  	mutex_lock(&ksym_tracer_mutex);
> @@ -251,10 +281,6 @@ static ssize_t ksym_trace_filter_write(s
>  	unsigned long ksym_addr = 0;
>  	int ret, op, changed = 0;
>  
> -	/* Ignore echo "" > ksym_trace_filter */
> -	if (count == 0)
> -		return 0;
> -
>  	input_string = kzalloc(count, GFP_KERNEL);
>  	if (!input_string)
>  		return -ENOMEM;
> @@ -263,12 +289,23 @@ static ssize_t ksym_trace_filter_write(s
>  		kfree(input_string);
>  		return -EFAULT;
>  	}
> +	/* Clear all breakpoint requests if echo "" > ksym_trace_filter */
> +	if ((count == 1) && !isalnum(input_string[0])) {
> +		ksym_trace_reset(NULL);
> +		return count;
> +	}
>  
>  	ret = op = parse_ksym_trace_str(input_string, &ksymname, &ksym_addr);
>  	if (ret < 0) {
>  		kfree(input_string);
>  		return ret;
>  	}
> +	/* Clear all breakpoints if echo "*:---" > ksym_trace_filter */
> +	if ((strncmp(ksymname, "*", strlen("*")) == 0) && (op == 0)) {


If you returned -EINVAL above, 


if (*ksymname == '*' && !op)

..would be shorter

Also that doesn't check if symname = "*something:---"



> +		ksym_trace_reset(NULL);
> +		kfree(input_string);
> +		return count;
> +	}
>  
>  	mutex_lock(&ksym_tracer_mutex);
>  
> @@ -325,34 +362,6 @@ static const struct file_operations ksym
>  	.write		= ksym_trace_filter_write,
>  };
>  
> -static void ksym_trace_reset(struct trace_array *tr)
> -{
> -	struct trace_ksym *entry;
> -	struct hlist_node *node, *node1;
> -
> -	ksym_tracing_enabled = 0;
> -
> -	mutex_lock(&ksym_tracer_mutex);
> -	hlist_for_each_entry_safe(entry, node, node1, &ksym_filter_head,
> -								ksym_hlist) {
> -		unregister_kernel_hw_breakpoint(entry->ksym_hbp);
> -		ksym_filter_entry_count--;
> -		hlist_del_rcu(&(entry->ksym_hlist));
> -		synchronize_rcu();
> -		/* Free the 'input_string' only if reset
> -		 * after startup self-test
> -		 */
> -#ifdef CONFIG_FTRACE_SELFTEST
> -		if (strncmp(entry->ksym_hbp->info.name, KSYM_SELFTEST_ENTRY,
> -					strlen(KSYM_SELFTEST_ENTRY)) != 0)
> -#endif /* CONFIG_FTRACE_SELFTEST*/
> -			kfree(entry->ksym_hbp->info.name);
> -		kfree(entry->ksym_hbp);
> -		kfree(entry);
> -	}
> -	mutex_unlock(&ksym_tracer_mutex);
> -}
> -
>  static int ksym_trace_init(struct trace_array *tr)
>  {
>  	int cpu, ret = 0;