* [PATCH 0/1] uprobes/tracing: uprobe_perf_open() forgets to handle the error from uprobe_apply()
@ 2014-04-23 16:58 Oleg Nesterov
2014-04-23 16:58 ` [PATCH 1/1] " Oleg Nesterov
0 siblings, 1 reply; 9+ messages in thread
From: Oleg Nesterov @ 2014-04-23 16:58 UTC (permalink / raw)
To: Ingo Molnar, Steven Rostedt
Cc: Ananth N Mavinakayanahalli, Anton Arapov, David Long,
Masami Hiramatsu, Srikar Dronamraju, linux-kernel
Hello,
Steven, I am going to ask Ingo to pull this fix along with other
pending uprobes changes, but please let me know if you want to take
this patch.
Oleg.
kernel/trace/trace_uprobe.c | 46 +++++++++++++++++++++++-------------------
1 files changed, 25 insertions(+), 21 deletions(-)
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/1] uprobes/tracing: uprobe_perf_open() forgets to handle the error from uprobe_apply()
2014-04-23 16:58 [PATCH 0/1] uprobes/tracing: uprobe_perf_open() forgets to handle the error from uprobe_apply() Oleg Nesterov
@ 2014-04-23 16:58 ` Oleg Nesterov
2014-04-24 3:17 ` Steven Rostedt
2014-04-24 9:03 ` [PATCH 1/1] uprobes/tracing: uprobe_perf_open() forgets to handle the error from uprobe_apply() Srikar Dronamraju
0 siblings, 2 replies; 9+ messages in thread
From: Oleg Nesterov @ 2014-04-23 16:58 UTC (permalink / raw)
To: Ingo Molnar, Steven Rostedt
Cc: Ananth N Mavinakayanahalli, Anton Arapov, David Long,
Masami Hiramatsu, Srikar Dronamraju, linux-kernel
uprobe_perf_open()->uprobe_apply() can fail, but this error is wrongly
ignored. Change uprobe_perf_open() to do uprobe_perf_close() and return
the error code in this case.
Change uprobe_perf_close() to propogate the error from uprobe_apply()
as well, although it should not fail.
The patch looks more complicated because it moves uprobe_perf_close()
up to make it visible to uprobe_perf_open().
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
kernel/trace/trace_uprobe.c | 46 +++++++++++++++++++++++-------------------
1 files changed, 25 insertions(+), 21 deletions(-)
diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
index 930e514..9aad3e2 100644
--- a/kernel/trace/trace_uprobe.c
+++ b/kernel/trace/trace_uprobe.c
@@ -1003,56 +1003,60 @@ uprobe_filter_event(struct trace_uprobe *tu, struct perf_event *event)
return __uprobe_perf_filter(&tu->filter, event->hw.tp_target->mm);
}
-static int uprobe_perf_open(struct trace_uprobe *tu, struct perf_event *event)
+static int uprobe_perf_close(struct trace_uprobe *tu, struct perf_event *event)
{
bool done;
write_lock(&tu->filter.rwlock);
if (event->hw.tp_target) {
- /*
- * event->parent != NULL means copy_process(), we can avoid
- * uprobe_apply(). current->mm must be probed and we can rely
- * on dup_mmap() which preserves the already installed bp's.
- *
- * attr.enable_on_exec means that exec/mmap will install the
- * breakpoints we need.
- */
+ list_del(&event->hw.tp_list);
done = tu->filter.nr_systemwide ||
- event->parent || event->attr.enable_on_exec ||
+ (event->hw.tp_target->flags & PF_EXITING) ||
uprobe_filter_event(tu, event);
- list_add(&event->hw.tp_list, &tu->filter.perf_events);
} else {
+ tu->filter.nr_systemwide--;
done = tu->filter.nr_systemwide;
- tu->filter.nr_systemwide++;
}
write_unlock(&tu->filter.rwlock);
if (!done)
- uprobe_apply(tu->inode, tu->offset, &tu->consumer, true);
+ return uprobe_apply(tu->inode, tu->offset, &tu->consumer, false);
return 0;
}
-static int uprobe_perf_close(struct trace_uprobe *tu, struct perf_event *event)
+static int uprobe_perf_open(struct trace_uprobe *tu, struct perf_event *event)
{
bool done;
+ int err;
write_lock(&tu->filter.rwlock);
if (event->hw.tp_target) {
- list_del(&event->hw.tp_list);
+ /*
+ * event->parent != NULL means copy_process(), we can avoid
+ * uprobe_apply(). current->mm must be probed and we can rely
+ * on dup_mmap() which preserves the already installed bp's.
+ *
+ * attr.enable_on_exec means that exec/mmap will install the
+ * breakpoints we need.
+ */
done = tu->filter.nr_systemwide ||
- (event->hw.tp_target->flags & PF_EXITING) ||
+ event->parent || event->attr.enable_on_exec ||
uprobe_filter_event(tu, event);
+ list_add(&event->hw.tp_list, &tu->filter.perf_events);
} else {
- tu->filter.nr_systemwide--;
done = tu->filter.nr_systemwide;
+ tu->filter.nr_systemwide++;
}
write_unlock(&tu->filter.rwlock);
- if (!done)
- uprobe_apply(tu->inode, tu->offset, &tu->consumer, false);
-
- return 0;
+ err = 0;
+ if (!done) {
+ err = uprobe_apply(tu->inode, tu->offset, &tu->consumer, true);
+ if (err)
+ uprobe_perf_close(tu, event);
+ }
+ return err;
}
static bool uprobe_perf_filter(struct uprobe_consumer *uc,
--
1.5.5.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/1] uprobes/tracing: uprobe_perf_open() forgets to handle the error from uprobe_apply()
2014-04-23 16:58 ` [PATCH 1/1] " Oleg Nesterov
@ 2014-04-24 3:17 ` Steven Rostedt
2014-04-24 11:54 ` [PATCH v2 0/2] uprobes/tracing: uprobe_perf_open()->uprobe_apply() fix Oleg Nesterov
2014-04-24 9:03 ` [PATCH 1/1] uprobes/tracing: uprobe_perf_open() forgets to handle the error from uprobe_apply() Srikar Dronamraju
1 sibling, 1 reply; 9+ messages in thread
From: Steven Rostedt @ 2014-04-24 3:17 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Ingo Molnar, Ananth N Mavinakayanahalli, Anton Arapov,
David Long, Masami Hiramatsu, Srikar Dronamraju, linux-kernel
On Wed, 23 Apr 2014 18:58:30 +0200
Oleg Nesterov <oleg@redhat.com> wrote:
> uprobe_perf_open()->uprobe_apply() can fail, but this error is wrongly
> ignored. Change uprobe_perf_open() to do uprobe_perf_close() and return
> the error code in this case.
>
> Change uprobe_perf_close() to propogate the error from uprobe_apply()
> as well, although it should not fail.
>
> The patch looks more complicated because it moves uprobe_perf_close()
> up to make it visible to uprobe_perf_open().
>
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
> kernel/trace/trace_uprobe.c | 46 +++++++++++++++++++++++-------------------
> 1 files changed, 25 insertions(+), 21 deletions(-)
>
> diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
> index 930e514..9aad3e2 100644
> --- a/kernel/trace/trace_uprobe.c
> +++ b/kernel/trace/trace_uprobe.c
> @@ -1003,56 +1003,60 @@ uprobe_filter_event(struct trace_uprobe *tu, struct perf_event *event)
> return __uprobe_perf_filter(&tu->filter, event->hw.tp_target->mm);
> }
>
> -static int uprobe_perf_open(struct trace_uprobe *tu, struct perf_event *event)
> +static int uprobe_perf_close(struct trace_uprobe *tu, struct perf_event *event)
Egad, this confused the heck out of me. I didn't notice the swap in
functions and was wondering what you were doing. I didn't realize this
is what you meant by moving the uprobe_perf_close() up. I was thinking
you moved the call up or something, not the function itself physically
in the file.
/me tries to continue dazed and confused.
> {
> bool done;
>
> write_lock(&tu->filter.rwlock);
> if (event->hw.tp_target) {
> - /*
> - * event->parent != NULL means copy_process(), we can avoid
> - * uprobe_apply(). current->mm must be probed and we can rely
> - * on dup_mmap() which preserves the already installed bp's.
> - *
> - * attr.enable_on_exec means that exec/mmap will install the
> - * breakpoints we need.
> - */
> + list_del(&event->hw.tp_list);
> done = tu->filter.nr_systemwide ||
> - event->parent || event->attr.enable_on_exec ||
> + (event->hw.tp_target->flags & PF_EXITING) ||
> uprobe_filter_event(tu, event);
> - list_add(&event->hw.tp_list, &tu->filter.perf_events);
> } else {
> + tu->filter.nr_systemwide--;
> done = tu->filter.nr_systemwide;
> - tu->filter.nr_systemwide++;
> }
> write_unlock(&tu->filter.rwlock);
>
> if (!done)
> - uprobe_apply(tu->inode, tu->offset, &tu->consumer, true);
> + return uprobe_apply(tu->inode, tu->offset, &tu->consumer, false);
>
> return 0;
> }
>
> -static int uprobe_perf_close(struct trace_uprobe *tu, struct perf_event *event)
> +static int uprobe_perf_open(struct trace_uprobe *tu, struct perf_event *event)
> {
> bool done;
> + int err;
>
> write_lock(&tu->filter.rwlock);
> if (event->hw.tp_target) {
> - list_del(&event->hw.tp_list);
> + /*
> + * event->parent != NULL means copy_process(), we can avoid
> + * uprobe_apply(). current->mm must be probed and we can rely
> + * on dup_mmap() which preserves the already installed bp's.
> + *
> + * attr.enable_on_exec means that exec/mmap will install the
> + * breakpoints we need.
> + */
> done = tu->filter.nr_systemwide ||
> - (event->hw.tp_target->flags & PF_EXITING) ||
> + event->parent || event->attr.enable_on_exec ||
> uprobe_filter_event(tu, event);
> + list_add(&event->hw.tp_list, &tu->filter.perf_events);
> } else {
> - tu->filter.nr_systemwide--;
> done = tu->filter.nr_systemwide;
> + tu->filter.nr_systemwide++;
> }
> write_unlock(&tu->filter.rwlock);
>
> - if (!done)
> - uprobe_apply(tu->inode, tu->offset, &tu->consumer, false);
> -
> - return 0;
> + err = 0;
> + if (!done) {
> + err = uprobe_apply(tu->inode, tu->offset, &tu->consumer, true);
> + if (err)
> + uprobe_perf_close(tu, event);
> + }
> + return err;
You can add by Acked-by, but next time, please make this into two
patches. One to do the move, the other to do the change.
Thanks!
-- Steve
> }
>
> static bool uprobe_perf_filter(struct uprobe_consumer *uc,
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/1] uprobes/tracing: uprobe_perf_open() forgets to handle the error from uprobe_apply()
2014-04-23 16:58 ` [PATCH 1/1] " Oleg Nesterov
2014-04-24 3:17 ` Steven Rostedt
@ 2014-04-24 9:03 ` Srikar Dronamraju
2014-04-24 12:27 ` Oleg Nesterov
1 sibling, 1 reply; 9+ messages in thread
From: Srikar Dronamraju @ 2014-04-24 9:03 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Ingo Molnar, Steven Rostedt, Ananth N Mavinakayanahalli,
Anton Arapov, David Long, Masami Hiramatsu, linux-kernel
Acked-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
(with 2 nits that you can ignore)
> done = tu->filter.nr_systemwide ||
> - event->parent || event->attr.enable_on_exec ||
> + (event->hw.tp_target->flags & PF_EXITING) ||
> uprobe_filter_event(tu, event);
> - list_add(&event->hw.tp_list, &tu->filter.perf_events);
> } else {
> + tu->filter.nr_systemwide--;
> done = tu->filter.nr_systemwide;
> - tu->filter.nr_systemwide++;
> }
Nit: I think 2 lines can be made into
done = --tu->filter.nr_systemwide;
<snipped>
> done = tu->filter.nr_systemwide ||
> - (event->hw.tp_target->flags & PF_EXITING) ||
> + event->parent || event->attr.enable_on_exec ||
> uprobe_filter_event(tu, event);
> + list_add(&event->hw.tp_list, &tu->filter.perf_events);
> } else {
> - tu->filter.nr_systemwide--;
> done = tu->filter.nr_systemwide;
> + tu->filter.nr_systemwide++;
> }
Nit: Similarly lines can be made into
done = tu->filter.nr_systemwide++;
--
Thanks and Regards
Srikar Dronamraju
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2 0/2] uprobes/tracing: uprobe_perf_open()->uprobe_apply() fix
2014-04-24 3:17 ` Steven Rostedt
@ 2014-04-24 11:54 ` Oleg Nesterov
2014-04-24 11:55 ` [PATCH v2 1/2] uprobes/tracing: Make uprobe_perf_close() visible to uprobe_perf_open() Oleg Nesterov
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Oleg Nesterov @ 2014-04-24 11:54 UTC (permalink / raw)
To: Steven Rostedt
Cc: Ingo Molnar, Ananth N Mavinakayanahalli, Anton Arapov,
David Long, Masami Hiramatsu, Srikar Dronamraju, linux-kernel
On 04/23, Steven Rostedt wrote:
>
> Egad, this confused the heck out of me. I didn't notice the swap in
> functions and was wondering what you were doing. I didn't realize this
> is what you meant by moving the uprobe_perf_close() up. I was thinking
> you moved the call up or something, not the function itself physically
> in the file.
...
>
> You can add by Acked-by, but next time, please make this into two
> patches. One to do the move, the other to do the change.
OK...
I tried to lessen the number of patches I have ;) But I agree, this simple
fix looks too complicated without preparation which only moves the code.
Let me split it then. Result is the same do I preserved the acks, this is
what I am going to add to my tree.
Thanks!
Oleg.
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2 1/2] uprobes/tracing: Make uprobe_perf_close() visible to uprobe_perf_open()
2014-04-24 11:54 ` [PATCH v2 0/2] uprobes/tracing: uprobe_perf_open()->uprobe_apply() fix Oleg Nesterov
@ 2014-04-24 11:55 ` Oleg Nesterov
2014-04-24 11:55 ` [PATCH v2 2/2] uprobes/tracing: Fix uprobe_perf_open() on uprobe_apply() failure Oleg Nesterov
2014-04-24 12:37 ` [PATCH v2 0/2] uprobes/tracing: uprobe_perf_open()->uprobe_apply() fix Steven Rostedt
2 siblings, 0 replies; 9+ messages in thread
From: Oleg Nesterov @ 2014-04-24 11:55 UTC (permalink / raw)
To: Steven Rostedt
Cc: Ingo Molnar, Ananth N Mavinakayanahalli, Anton Arapov,
David Long, Masami Hiramatsu, Srikar Dronamraju, linux-kernel
Preparation. Move uprobe_perf_close() up before uprobe_perf_open() to
avoid the forward declaration in the next patch and make it readable.
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Acked-by: Steven Rostedt <rostedt@goodmis.org>
Acked-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
---
kernel/trace/trace_uprobe.c | 36 ++++++++++++++++++------------------
1 files changed, 18 insertions(+), 18 deletions(-)
diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
index 930e514..1ed0030 100644
--- a/kernel/trace/trace_uprobe.c
+++ b/kernel/trace/trace_uprobe.c
@@ -1003,54 +1003,54 @@ uprobe_filter_event(struct trace_uprobe *tu, struct perf_event *event)
return __uprobe_perf_filter(&tu->filter, event->hw.tp_target->mm);
}
-static int uprobe_perf_open(struct trace_uprobe *tu, struct perf_event *event)
+static int uprobe_perf_close(struct trace_uprobe *tu, struct perf_event *event)
{
bool done;
write_lock(&tu->filter.rwlock);
if (event->hw.tp_target) {
- /*
- * event->parent != NULL means copy_process(), we can avoid
- * uprobe_apply(). current->mm must be probed and we can rely
- * on dup_mmap() which preserves the already installed bp's.
- *
- * attr.enable_on_exec means that exec/mmap will install the
- * breakpoints we need.
- */
+ list_del(&event->hw.tp_list);
done = tu->filter.nr_systemwide ||
- event->parent || event->attr.enable_on_exec ||
+ (event->hw.tp_target->flags & PF_EXITING) ||
uprobe_filter_event(tu, event);
- list_add(&event->hw.tp_list, &tu->filter.perf_events);
} else {
+ tu->filter.nr_systemwide--;
done = tu->filter.nr_systemwide;
- tu->filter.nr_systemwide++;
}
write_unlock(&tu->filter.rwlock);
if (!done)
- uprobe_apply(tu->inode, tu->offset, &tu->consumer, true);
+ uprobe_apply(tu->inode, tu->offset, &tu->consumer, false);
return 0;
}
-static int uprobe_perf_close(struct trace_uprobe *tu, struct perf_event *event)
+static int uprobe_perf_open(struct trace_uprobe *tu, struct perf_event *event)
{
bool done;
write_lock(&tu->filter.rwlock);
if (event->hw.tp_target) {
- list_del(&event->hw.tp_list);
+ /*
+ * event->parent != NULL means copy_process(), we can avoid
+ * uprobe_apply(). current->mm must be probed and we can rely
+ * on dup_mmap() which preserves the already installed bp's.
+ *
+ * attr.enable_on_exec means that exec/mmap will install the
+ * breakpoints we need.
+ */
done = tu->filter.nr_systemwide ||
- (event->hw.tp_target->flags & PF_EXITING) ||
+ event->parent || event->attr.enable_on_exec ||
uprobe_filter_event(tu, event);
+ list_add(&event->hw.tp_list, &tu->filter.perf_events);
} else {
- tu->filter.nr_systemwide--;
done = tu->filter.nr_systemwide;
+ tu->filter.nr_systemwide++;
}
write_unlock(&tu->filter.rwlock);
if (!done)
- uprobe_apply(tu->inode, tu->offset, &tu->consumer, false);
+ uprobe_apply(tu->inode, tu->offset, &tu->consumer, true);
return 0;
}
--
1.5.5.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v2 2/2] uprobes/tracing: Fix uprobe_perf_open() on uprobe_apply() failure
2014-04-24 11:54 ` [PATCH v2 0/2] uprobes/tracing: uprobe_perf_open()->uprobe_apply() fix Oleg Nesterov
2014-04-24 11:55 ` [PATCH v2 1/2] uprobes/tracing: Make uprobe_perf_close() visible to uprobe_perf_open() Oleg Nesterov
@ 2014-04-24 11:55 ` Oleg Nesterov
2014-04-24 12:37 ` [PATCH v2 0/2] uprobes/tracing: uprobe_perf_open()->uprobe_apply() fix Steven Rostedt
2 siblings, 0 replies; 9+ messages in thread
From: Oleg Nesterov @ 2014-04-24 11:55 UTC (permalink / raw)
To: Steven Rostedt
Cc: Ingo Molnar, Ananth N Mavinakayanahalli, Anton Arapov,
David Long, Masami Hiramatsu, Srikar Dronamraju, linux-kernel
uprobe_perf_open()->uprobe_apply() can fail, but this error is wrongly
ignored. Change uprobe_perf_open() to do uprobe_perf_close() and return
the error code in this case.
Change uprobe_perf_close() to propogate the error from uprobe_apply()
as well, although it should not fail.
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Acked-by: Steven Rostedt <rostedt@goodmis.org>
Acked-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
---
kernel/trace/trace_uprobe.c | 14 +++++++++-----
1 files changed, 9 insertions(+), 5 deletions(-)
diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
index 1ed0030..9aad3e2 100644
--- a/kernel/trace/trace_uprobe.c
+++ b/kernel/trace/trace_uprobe.c
@@ -1020,7 +1020,7 @@ static int uprobe_perf_close(struct trace_uprobe *tu, struct perf_event *event)
write_unlock(&tu->filter.rwlock);
if (!done)
- uprobe_apply(tu->inode, tu->offset, &tu->consumer, false);
+ return uprobe_apply(tu->inode, tu->offset, &tu->consumer, false);
return 0;
}
@@ -1028,6 +1028,7 @@ static int uprobe_perf_close(struct trace_uprobe *tu, struct perf_event *event)
static int uprobe_perf_open(struct trace_uprobe *tu, struct perf_event *event)
{
bool done;
+ int err;
write_lock(&tu->filter.rwlock);
if (event->hw.tp_target) {
@@ -1049,10 +1050,13 @@ static int uprobe_perf_open(struct trace_uprobe *tu, struct perf_event *event)
}
write_unlock(&tu->filter.rwlock);
- if (!done)
- uprobe_apply(tu->inode, tu->offset, &tu->consumer, true);
-
- return 0;
+ err = 0;
+ if (!done) {
+ err = uprobe_apply(tu->inode, tu->offset, &tu->consumer, true);
+ if (err)
+ uprobe_perf_close(tu, event);
+ }
+ return err;
}
static bool uprobe_perf_filter(struct uprobe_consumer *uc,
--
1.5.5.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/1] uprobes/tracing: uprobe_perf_open() forgets to handle the error from uprobe_apply()
2014-04-24 9:03 ` [PATCH 1/1] uprobes/tracing: uprobe_perf_open() forgets to handle the error from uprobe_apply() Srikar Dronamraju
@ 2014-04-24 12:27 ` Oleg Nesterov
0 siblings, 0 replies; 9+ messages in thread
From: Oleg Nesterov @ 2014-04-24 12:27 UTC (permalink / raw)
To: Srikar Dronamraju
Cc: Ingo Molnar, Steven Rostedt, Ananth N Mavinakayanahalli,
Anton Arapov, David Long, Masami Hiramatsu, linux-kernel
On 04/24, Srikar Dronamraju wrote:
>
> Acked-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
Thanks, included!
> > + tu->filter.nr_systemwide--;
> > done = tu->filter.nr_systemwide;
> > - tu->filter.nr_systemwide++;
> > }
>
> Nit: I think 2 lines can be made into
> done = --tu->filter.nr_systemwide;
...
> > - tu->filter.nr_systemwide--;
> > done = tu->filter.nr_systemwide;
> > + tu->filter.nr_systemwide++;
> > }
>
> Nit: Similarly lines can be made into
> done = tu->filter.nr_systemwide++;
Yes, perhaps, but this has nothing to do with this patch, it does not
change this code.
And can't resist... you know, initially I wrote this code this way, but
then I decided to make it more straightforward to avoid the potential nits
from reviewers ;)
Oleg.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 0/2] uprobes/tracing: uprobe_perf_open()->uprobe_apply() fix
2014-04-24 11:54 ` [PATCH v2 0/2] uprobes/tracing: uprobe_perf_open()->uprobe_apply() fix Oleg Nesterov
2014-04-24 11:55 ` [PATCH v2 1/2] uprobes/tracing: Make uprobe_perf_close() visible to uprobe_perf_open() Oleg Nesterov
2014-04-24 11:55 ` [PATCH v2 2/2] uprobes/tracing: Fix uprobe_perf_open() on uprobe_apply() failure Oleg Nesterov
@ 2014-04-24 12:37 ` Steven Rostedt
2 siblings, 0 replies; 9+ messages in thread
From: Steven Rostedt @ 2014-04-24 12:37 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Ingo Molnar, Ananth N Mavinakayanahalli, Anton Arapov,
David Long, Masami Hiramatsu, Srikar Dronamraju, linux-kernel
On Thu, 24 Apr 2014 13:54:58 +0200
Oleg Nesterov <oleg@redhat.com> wrote:
> On 04/23, Steven Rostedt wrote:
> >
> > Egad, this confused the heck out of me. I didn't notice the swap in
> > functions and was wondering what you were doing. I didn't realize this
> > is what you meant by moving the uprobe_perf_close() up. I was thinking
> > you moved the call up or something, not the function itself physically
> > in the file.
> ...
> >
> > You can add by Acked-by, but next time, please make this into two
> > patches. One to do the move, the other to do the change.
>
> OK...
>
> I tried to lessen the number of patches I have ;) But I agree, this simple
> fix looks too complicated without preparation which only moves the code.
Yeah, sometimes a simple fix just seems wrong to break into two. But if
it helps in code review, it's definitely worth it.
>
> Let me split it then. Result is the same do I preserved the acks, this is
> what I am going to add to my tree.
They look good (and much easier to review).
-- Steve
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2014-04-24 12:37 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-04-23 16:58 [PATCH 0/1] uprobes/tracing: uprobe_perf_open() forgets to handle the error from uprobe_apply() Oleg Nesterov
2014-04-23 16:58 ` [PATCH 1/1] " Oleg Nesterov
2014-04-24 3:17 ` Steven Rostedt
2014-04-24 11:54 ` [PATCH v2 0/2] uprobes/tracing: uprobe_perf_open()->uprobe_apply() fix Oleg Nesterov
2014-04-24 11:55 ` [PATCH v2 1/2] uprobes/tracing: Make uprobe_perf_close() visible to uprobe_perf_open() Oleg Nesterov
2014-04-24 11:55 ` [PATCH v2 2/2] uprobes/tracing: Fix uprobe_perf_open() on uprobe_apply() failure Oleg Nesterov
2014-04-24 12:37 ` [PATCH v2 0/2] uprobes/tracing: uprobe_perf_open()->uprobe_apply() fix Steven Rostedt
2014-04-24 9:03 ` [PATCH 1/1] uprobes/tracing: uprobe_perf_open() forgets to handle the error from uprobe_apply() Srikar Dronamraju
2014-04-24 12:27 ` Oleg Nesterov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).