Skip to contentSkip to conversation

ftrace updates

PATCH Applied Steven Rostedt 2 participants 4 messages Aug 16, 2008

Subsystems (2)
Tracing ftrace
lore
Share & tools
lore

Steven Rostedt

author maintainer
#0 lore raw

cover ftrace updates

Due to the new mcount recording method, the assumptions about the records that hold the mcount locations have changed. Now we need to protect reading and writing of the records between module load and reading of the debugfs files. There is not much contention here, but they need to be protected none the less.

I simply use the ftrace_lock to accomplish this. The ftrace_lock should never be held under interrupt context so disabling interrupts should not be necessary. I've run this under lockdep and lockdep has not complained.

-- Steve

author maintainer
#1 lore raw

PATCH 1/2 ftrace: do not init module on ftrace disabled

If one of the self tests of ftrace has disabled the function tracer, do not run the code to convert the mcount calls in modules.

Signed-off-by Steven Rostedt <[email protected]>
---
 kernel/trace/ftrace.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
kernel/trace/ftrace.c +2 -2
Index: linux-tip.git/kernel/trace/ftrace.c
===================================================================
@@ -307,7 +307,7 @@ void ftrace_release(void *start, unsigne
 	unsigned long e = s + size;
 	int i;
 
-	if (!start)
+	if (ftrace_disabled || !start)
 		return;
 
 	/* No interrupt should call this */

@@ -1574,7 +1574,7 @@ static int ftrace_convert_nops(unsigned 
 
 void ftrace_init_module(unsigned long *start, unsigned long *end)
 {
-	if (start == end)
+	if (ftrace_disabled || start == end)
 		return;
 	ftrace_convert_nops(start, end);
 }
author maintainer
#2 lore raw

PATCH 2/2 ftrace: add necessary locking for ftrace records

In reply to Steven Rostedt

The new design of pre-recorded mcounts and updating the code outside of kstop_machine has changed the way the records themselves are protected.

This patch uses the ftrace_lock to protect the records. Note, the lock still does not need to be taken within calls that are only called via kstop_machine, since the that code can not run while the spin lock is held.

Also removed the hash_lock needed for the daemon when MCOUNT_RECORD is configured. Also did a slight cleanup of an unused variable.

Signed-off-by Steven Rostedt <[email protected]>
---
 kernel/trace/ftrace.c |   44 ++++++++++++++++++++++++++++++--------------
 1 file changed, 30 insertions(+), 14 deletions(-)
kernel/trace/ftrace.c +30 -14
Index: linux-tip.git/kernel/trace/ftrace.c
===================================================================
@@ -81,7 +81,7 @@ void clear_ftrace_function(void)
 
 static int __register_ftrace_function(struct ftrace_ops *ops)
 {
-	/* Should never be called by interrupts */
+	/* should not be called from interrupt context */
 	spin_lock(&ftrace_lock);
 
 	ops->next = ftrace_list;

@@ -115,6 +115,7 @@ static int __unregister_ftrace_function(
 	struct ftrace_ops **p;
 	int ret = 0;
 
+	/* should not be called from interrupt context */
 	spin_lock(&ftrace_lock);
 
 	/*

@@ -153,6 +154,21 @@ static int __unregister_ftrace_function(
 
 #ifdef CONFIG_DYNAMIC_FTRACE
 
+#ifndef CONFIG_FTRACE_MCOUNT_RECORD
+/*
+ * The hash lock is only needed when the recording of the mcount
+ * callers are dynamic. That is, by the caller themselves and
+ * not recorded via the compilation.
+ */
+static DEFINE_SPINLOCK(ftrace_hash_lock);
+#define ftrace_hash_lock(flags)	  spin_lock_irqsave(ftrace_hash_lock, flags)
+#define ftrace_hash_unlock(flags) spin_lock_irqsave(ftrace_hash_lock, flags)
+#else
+/* This is protected via the ftrace_lock with MCOUNT_RECORD. */
+#define ftrace_hash_lock(flags)   do { (void)flags; } while (0)
+#define ftrace_hash_unlock(flags) do { } while(0)
+#endif
+
 static struct task_struct *ftraced_task;
 
 enum {

@@ -171,7 +187,6 @@ static struct hlist_head ftrace_hash[FTR
 
 static DEFINE_PER_CPU(int, ftrace_shutdown_disable_cpu);
 
-static DEFINE_SPINLOCK(ftrace_shutdown_lock);
 static DEFINE_MUTEX(ftraced_lock);
 static DEFINE_MUTEX(ftrace_regex_lock);
 

@@ -310,7 +325,7 @@ void ftrace_release(void *start, unsigne
 	if (ftrace_disabled || !start)
 		return;
 
-	/* No interrupt should call this */
+	/* should not be called from interrupt context */
 	spin_lock(&ftrace_lock);
 
 	for (pg = ftrace_pages_start; pg; pg = pg->next) {

@@ -362,7 +377,6 @@ ftrace_record_ip(unsigned long ip)
 	unsigned long flags;
 	unsigned long key;
 	int resched;
-	int atomic;
 	int cpu;
 	static int once;
 

@@ -399,9 +413,7 @@ ftrace_record_ip(unsigned long ip)
 	if (ftrace_ip_in_hash(ip, key))
 		goto out;
 
-	atomic = irqs_disabled();
-
-	spin_lock_irqsave(&ftrace_shutdown_lock, flags);
+	ftrace_hash_lock(flags);
 
 	/* This ip may have hit the hash before the lock */
 	if (ftrace_ip_in_hash(ip, key))

@@ -418,7 +430,7 @@ ftrace_record_ip(unsigned long ip)
 	ftraced_trigger = 1;
 
  out_unlock:
-	spin_unlock_irqrestore(&ftrace_shutdown_lock, flags);
+	ftrace_hash_unlock(flags);
  out:
 	per_cpu(ftrace_shutdown_disable_cpu, cpu)--;
 

@@ -894,6 +906,8 @@ t_next(struct seq_file *m, void *v, loff
 
 	(*pos)++;
 
+	/* should not be called from interrupt context */
+	spin_lock(&ftrace_lock);
  retry:
 	if (iter->idx >= iter->pg->index) {
 		if (iter->pg->next) {

@@ -917,6 +931,7 @@ t_next(struct seq_file *m, void *v, loff
 			goto retry;
 		}
 	}
+	spin_unlock(&ftrace_lock);
 
 	iter->pos = *pos;
 

@@ -1030,8 +1045,8 @@ static void ftrace_filter_reset(int enab
 	unsigned long type = enable ? FTRACE_FL_FILTER : FTRACE_FL_NOTRACE;
 	unsigned i;
 
-	/* keep kstop machine from running */
-	preempt_disable();
+	/* should not be called from interrupt context */
+	spin_lock(&ftrace_lock);
 	if (enable)
 		ftrace_filtered = 0;
 	pg = ftrace_pages_start;

@@ -1044,7 +1059,7 @@ static void ftrace_filter_reset(int enab
 		}
 		pg = pg->next;
 	}
-	preempt_enable();
+	spin_unlock(&ftrace_lock);
 }
 
 static int

@@ -1156,8 +1171,8 @@ ftrace_match(unsigned char *buff, int le
 		}
 	}
 
-	/* keep kstop machine from running */
-	preempt_disable();
+	/* should not be called from interrupt context */
+	spin_lock(&ftrace_lock);
 	if (enable)
 		ftrace_filtered = 1;
 	pg = ftrace_pages_start;

@@ -1194,7 +1209,7 @@ ftrace_match(unsigned char *buff, int le
 		}
 		pg = pg->next;
 	}
-	preempt_enable();
+	spin_unlock(&ftrace_lock);
 }
 
 static ssize_t

@@ -1558,6 +1573,7 @@ static int ftrace_convert_nops(unsigned 
 	p = start;
 	while (p < end) {
 		addr = ftrace_call_adjust(*p++);
+		/* should not be called from interrupt context */
 		spin_lock(&ftrace_lock);
 		ftrace_record_ip(addr);
 		spin_unlock(&ftrace_lock);
Applied
#3 lore raw
In reply to Steven Rostedt
Quoted 10 lines · Steven Rostedt

Due to the new mcount recording method, the assumptions about the records that hold the mcount locations have changed. Now we need to protect reading and writing of the records between module load and reading of the debugfs files. There is not much contention here, but they need to be protected none the less.

I simply use the ftrace_lock to accomplish this. The ftrace_lock should never be held under interrupt context so disabling interrupts should not be necessary. I've run this under lockdep and lockdep has not complained.

applied to tip/tracing/ftrace - thanks Steve.

Ingo