Re: [PATCH] itrace spin lock fix

[Dave Nomura <dcnltc@us.ibm.com>]

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

I merged your patch in with mine and fixed the ChangeLog entry.

David Smith wrote:
> Dave Nomura wrote:
>   
>> Back in August Frank reported a problem running the itrace test on an
>> x86-64 machine.  This was caused by an uninitialized spin_lock and
>> possibly by some utrace callouts that didn't need in the spin_lock
>> protected regions.  This patch fixes that problem.
>>     
>
> Your patch's ChangeLog is wrong.  You should put the entry in
> src/runtime/ChangeLog, not src/ChangeLog.
>
> I ran this patch on a RHEL5 x86_64 (2.6.18-92.1.13.el5debug) machine.
> Systemtap couldn't compile modules without the following small patch:
>
> diff --git a/tapsets.cxx b/tapsets.cxx
>
> index bed2796..057a554 100644
>
> --- a/tapsets.cxx
>
> +++ b/tapsets.cxx
>
> @@ -5778,6 +5778,7 @@ itrace_derived_probe_group::emit_module_decls
> (systemtap_\
> session& s)
>
>
>
>    s.op->newline();
>
>    s.op->newline() << "/* ---- itrace probes ---- */";
>
> +  s.op->newline() << "#include \"task_finder.c\"";
>
>    s.op->newline() << "struct stap_itrace_probe {";
>
>    s.op->indent(1);
>
>    s.op->newline() << "struct stap_task_finder_target tgt;";
>
> With the above patch, systemtap could compile modules again.  The
> itrace.exp test script ran (with the early exit deleted), but failed
> because the itrace probes never got hit.
>
>   


-- 
Dave Nomura
LTC Linux Power Toolchain


[-- Attachment #2: itrace_spin_lock.patch --]
[-- Type: text/plain, Size: 3562 bytes --]

diff -paurN old/runtime/ChangeLog new/runtime/ChangeLog
--- old/runtime/ChangeLog	2008-10-22 16:19:10.000000000 -0700
+++ new/runtime/ChangeLog	2008-10-24 10:05:58.000000000 -0700
@@ -1,3 +1,7 @@
+2008-10-24  Dave Nomura  <dcnltc@us.ibm.com>
+
+	* itrace.c: remove utrace calls from spin_lock regions
+
 2008-10-17  Wenji Huang  <wenji.huang@oracle.com>
 
 	* task_finder_vma.c (__stp_tf_vma_get_free_entry): Initialize entry.
diff -paurN old/runtime/itrace.c new/runtime/itrace.c
--- old/runtime/itrace.c	2008-10-22 16:19:10.000000000 -0700
+++ new/runtime/itrace.c	2008-10-24 10:04:18.000000000 -0700
@@ -137,6 +137,7 @@ static struct itrace_info *create_itrace
 	struct stap_itrace_probe *itrace_probe)
 {
 	struct itrace_info *ui;
+	struct utrace_attached_engine *engine;
 
 	if (debug)
 		printk(KERN_INFO "create_itrace_info: tid=%d\n", tsk->pid);
@@ -151,23 +152,23 @@ static struct itrace_info *create_itrace
 #endif
 	INIT_LIST_HEAD(&ui->link);
 
-	/* push ui onto usr_itrace_info */
-	spin_lock(&itrace_lock);
-	list_add(&ui->link, &usr_itrace_info);
-
 	/* attach a single stepping engine */
-	ui->engine = utrace_attach(ui->tsk, UTRACE_ATTACH_CREATE, &utrace_ops, ui);
-	if (IS_ERR(ui->engine)) {
+	engine = utrace_attach(ui->tsk, UTRACE_ATTACH_CREATE, &utrace_ops, ui);
+	if (IS_ERR(engine)) {
 		printk(KERN_ERR "utrace_attach returns %ld\n",
-			PTR_ERR(ui->engine));
-		ui = NULL;
-	} else {
-		utrace_set_flags(tsk, ui->engine, ui->engine->flags |
-			ui->step_flag |
-			UTRACE_EVENT(CLONE) | UTRACE_EVENT_SIGNAL_ALL |
-			UTRACE_EVENT(DEATH));
+			PTR_ERR(engine));
+		return NULL;
 	}
+
+	/* push ui onto usr_itrace_info */
+	spin_lock(&itrace_lock);
+	list_add(&ui->link, &usr_itrace_info);
 	spin_unlock(&itrace_lock);
+
+	utrace_set_flags(tsk, engine, engine->flags | step_flag |
+		UTRACE_EVENT(CLONE) | UTRACE_EVENT_SIGNAL_ALL |
+		UTRACE_EVENT(DEATH));
+
 	return ui;
 }
 
@@ -192,6 +193,8 @@ int usr_itrace_init(int single_step, pid
 	struct itrace_info *ui;
 	struct task_struct *tsk;
 
+	spin_lock_init(&itrace_lock);
+
 	rcu_read_lock();
 	tsk = find_task_by_pid(tid);
 	if (!tsk) {
@@ -210,11 +213,6 @@ int usr_itrace_init(int single_step, pid
 	put_task_struct(tsk);
 	rcu_read_unlock();
 
-	spin_lock_init(&itrace_lock);
-
-	/* set initial state */
-	spin_lock(&itrace_lock);
-	spin_unlock(&itrace_lock);
 	printk(KERN_INFO "usr_itrace_init: completed for tid = %d\n", tid);
 
 	return 0;
@@ -223,6 +221,8 @@ int usr_itrace_init(int single_step, pid
 void static remove_usr_itrace_info(struct itrace_info *ui)
 {
 	struct itrace_info *tmp;
+	struct task_struct *tsk;
+	struct utrace_attached_engine *engine;
 
 	if (!ui)
 		return;
@@ -231,11 +231,14 @@ void static remove_usr_itrace_info(struc
 		printk(KERN_INFO "remove_usr_itrace_info: tid=%d\n", ui->tid);
 
 	spin_lock(&itrace_lock);
-	if (ui->tsk && ui->engine) {
-		(void) utrace_detach(ui->tsk, ui->engine);
-	}
+	tsk = ui->tsk;
+	engine = ui->engine;
 	list_del(&ui->link);
 	spin_unlock(&itrace_lock);
+
+	if (tsk && engine) {
+		(void) utrace_detach(tsk, engine);
+	}
 	kfree(ui);
 }
 
diff -paurN old/tapsets.cxx new/tapsets.cxx
--- old/tapsets.cxx	2008-10-24 10:23:22.000000000 -0700
+++ new/tapsets.cxx	2008-10-24 10:27:04.000000000 -0700
@@ -5780,6 +5780,7 @@ itrace_derived_probe_group::emit_module_
 
   s.op->newline();
   s.op->newline() << "/* ---- itrace probes ---- */";
+  s.op->newline() << "#include \"task_finder.c\"";
   s.op->newline() << "struct stap_itrace_probe {";
   s.op->indent(1);
   s.op->newline() << "struct stap_task_finder_target tgt;";