This is the mail archive of the
systemtap@sourceware.org
mailing list for the systemtap project.
Re: [PATCH] itrace spin lock fix
- From: Dave Nomura <dcnltc at us dot ibm dot com>
- To: David Smith <dsmith at redhat dot com>
- Cc: systemtap at sourceware dot org
- Date: Fri, 24 Oct 2008 17:32:23 -0700
- Subject: Re: [PATCH] itrace spin lock fix
- References: <48FFC1D4.7000801@us.ibm.com> <4901F5E4.7030200@redhat.com>
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
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;";