tracing/kprobes: move free_trace_probe into unregister_trace_probe

PATCH In review zhangwei(Jovi) 4 participants 6 messages Jun 25, 2013 Tracing

lore

Share & tools

lore

zhangwei(Jovi)

author

2013-06-25 08:17 UTC #0 lore raw

PATCH 1/2 tracing/kprobes: move free_trace_probe into unregister_trace_probe

There have no good reason to call free_trace_probe every time when unregister_trace_probe return 0.

Move free_trace_probe into unregister_trace_probe, make code simpler.

Signed-off-by zhangwei(Jovi) <[email protected]>

Cc Masami Hiramatsu <[email protected]>

Cc Oleg Nesterov <[email protected]>

---
 kernel/trace/trace_kprobe.c |    5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

kernel/trace/trace_kprobe.c +1 -4

index 9f46e98..f193c38 100644
@@ -401,6 +401,7 @@ static int unregister_trace_probe(struct trace_probe *tp)
 	__unregister_trace_probe(tp);
 	list_del(&tp->list);
 	unregister_probe_event(tp);
+	free_trace_probe(tp);
 
 	return 0;
 }

@@ -419,7 +420,6 @@ static int register_trace_probe(struct trace_probe *tp)
 		ret = unregister_trace_probe(old_tp);
 		if (ret < 0)
 			goto end;
-		free_trace_probe(old_tp);
 	}
 
 	/* Register new event */

@@ -550,8 +550,6 @@ static int create_trace_probe(int argc, char **argv)
 		}
 		/* delete an event */
 		ret = unregister_trace_probe(tp);
-		if (ret == 0)
-			free_trace_probe(tp);
 		mutex_unlock(&probe_lock);
 		return ret;
 	}

@@ -680,7 +678,6 @@ static int release_all_trace_probes(void)
 	while (!list_empty(&probe_list)) {
 		tp = list_entry(probe_list.next, struct trace_probe, list);
 		unregister_trace_probe(tp);
-		free_trace_probe(tp);
 	}
 
 end:

Signature · 1 line

1.7.9.7

Masami Hiramatsu

2013-06-25 10:10 UTC #1 lore raw

Quoted 5 lines · zhangwei(Jovi)

There have no good reason to call free_trace_probe every time when unregister_trace_probe return 0.
Move free_trace_probe into unregister_trace_probe, make code simpler.

Sorry, nack. For the symmetrical coding reason, I don't like involving "free" and "alloc" into "unregister"/"register" functions. I think those should be just another actions.

Thank you,

Quoted 46 lines

Signed-off-by zhangwei(Jovi) <[email protected]>

Cc Masami Hiramatsu <[email protected]>

Cc Oleg Nesterov <[email protected]>

---
 kernel/trace/trace_kprobe.c |    5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

kernel/trace/trace_kprobe.c +1 -4

index 9f46e98..f193c38 100644
@@ -401,6 +401,7 @@ static int unregister_trace_probe(struct trace_probe *tp)
 	__unregister_trace_probe(tp);
 	list_del(&tp->list);
 	unregister_probe_event(tp);
+	free_trace_probe(tp);
 
 	return 0;
 }

@@ -419,7 +420,6 @@ static int register_trace_probe(struct trace_probe *tp)
 		ret = unregister_trace_probe(old_tp);
 		if (ret < 0)
 			goto end;
-		free_trace_probe(old_tp);
 	}
 
 	/* Register new event */

@@ -550,8 +550,6 @@ static int create_trace_probe(int argc, char **argv)
 		}
 		/* delete an event */
 		ret = unregister_trace_probe(tp);
-		if (ret == 0)
-			free_trace_probe(tp);
 		mutex_unlock(&probe_lock);
 		return ret;
 	}

@@ -680,7 +678,6 @@ static int release_all_trace_probes(void)
 	while (!list_empty(&probe_list)) {
 		tp = list_entry(probe_list.next, struct trace_probe, list);
 		unregister_trace_probe(tp);
-		free_trace_probe(tp);
 	}
 
 end:

Signature · 4 lines

Masami HIRAMATSU
IT Management Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: [email protected]

zhangwei(Jovi)

author

2013-06-25 10:39 UTC #2 lore raw

Quoted 4 lines · Masami Hiramatsu

Quoted 3 lines · zhangwei(Jovi)
There have no good reason to call free_trace_probe every time when unregister_trace_probe return 0.

Quoted 8 lines

Quoted 2 lines
Move free_trace_probe into unregister_trace_probe, make code simpler.
Sorry, nack. For the symmetrical coding reason, I don't like involving "free" and "alloc" into "unregister"/"register" functions. I think those should be just another actions.
Thank you,

That's fine, I just saw there have a little inconsistent between trace_kprobe.c and trace_uprobe.c.

Please ignore this patch if you don't like. :)
Thanks.

Quoted 49 lines

Quoted 47 lines

Signed-off-by zhangwei(Jovi) <[email protected]>

Cc Masami Hiramatsu <[email protected]>

Cc Oleg Nesterov <[email protected]>

---
 kernel/trace/trace_kprobe.c |    5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

kernel/trace/trace_kprobe.c +1 -4

index 9f46e98..f193c38 100644
@@ -401,6 +401,7 @@ static int unregister_trace_probe(struct trace_probe *tp)
 	__unregister_trace_probe(tp);
 	list_del(&tp->list);
 	unregister_probe_event(tp);
+	free_trace_probe(tp);
 
 	return 0;
 }

@@ -419,7 +420,6 @@ static int register_trace_probe(struct trace_probe *tp)
 		ret = unregister_trace_probe(old_tp);
 		if (ret < 0)
 			goto end;
-		free_trace_probe(old_tp);
 	}
 
 	/* Register new event */

@@ -550,8 +550,6 @@ static int create_trace_probe(int argc, char **argv)
 		}
 		/* delete an event */
 		ret = unregister_trace_probe(tp);
-		if (ret == 0)
-			free_trace_probe(tp);
 		mutex_unlock(&probe_lock);
 		return ret;
 	}

@@ -680,7 +678,6 @@ static int release_all_trace_probes(void)
 	while (!list_empty(&probe_list)) {
 		tp = list_entry(probe_list.next, struct trace_probe, list);
 		unregister_trace_probe(tp);
-		free_trace_probe(tp);
 	}
 
 end:

Steven Rostedt

maintainer

2013-06-25 13:34 UTC #3 lore raw

Quoted 8 lines · zhangwei(Jovi)

Quoted 7 lines · Masami Hiramatsu
Quoted 5 lines · zhangwei(Jovi)
There have no good reason to call free_trace_probe every time when unregister_trace_probe return 0.
Move free_trace_probe into unregister_trace_probe, make code simpler.

Quoted 9 lines

Quoted 5 lines
Sorry, nack. For the symmetrical coding reason, I don't like involving "free" and "alloc" into "unregister"/"register" functions. I think those should be just another actions.
Thank you,
That's fine, I just saw there have a little inconsistent between trace_kprobe.c and trace_uprobe.c.

Is there a place that trace_kprobe.c frees the tp structure in unregister?

-- Steve

Jovi Zhang

2013-06-25 14:48 UTC #4 lore raw

Quoted 13 lines · Steven Rostedt

Quoted 12 lines · zhangwei(Jovi)
Quoted 11 lines · Masami Hiramatsu
Quoted 5 lines · zhangwei(Jovi)
There have no good reason to call free_trace_probe every time when unregister_trace_probe return 0.
Move free_trace_probe into unregister_trace_probe, make code simpler.
Sorry, nack. For the symmetrical coding reason, I don't like involving "free" and "alloc" into "unregister"/"register" functions. I think those should be just another actions.

Quoted 9 lines

Quoted 5 lines
Quoted 1 line
Thank you,
That's fine, I just saw there have a little inconsistent between trace_kprobe.c and trace_uprobe.c.
Is there a place that trace_kprobe.c frees the tp structure in unregister?

I won't argue put the free operation into unregister function in trace_kprobe.c, as I said, one minor problem is the code pattern between trace_kprobe.c and trace_uprobe.c is so similar on unregister, but with little inconsistent, maybe we can unify those code in trace_probe.c someday.

Anyway, this is not big functionality issue, free to leave in there if authors don't argue.

jovi

Steven Rostedt

maintainer

2013-06-25 13:31 UTC #5 lore raw

In reply to Masami Hiramatsu

Quoted 10 lines · Masami Hiramatsu

Quoted 5 lines · zhangwei(Jovi)
There have no good reason to call free_trace_probe every time when unregister_trace_probe return 0.
Move free_trace_probe into unregister_trace_probe, make code simpler.
Sorry, nack. For the symmetrical coding reason, I don't like involving "free" and "alloc" into "unregister"/"register" functions. I think those should be just another actions.

I totally agree with Masami here. Please do not add "side effects" to the unregister function.

Thanks,

-- Steve