[for-next][PATCH 16/17] uprobes: Allocate ->utask before handler_chain() for tracing handlers
From: Oleg Nesterov uprobe_trace_print() and uprobe_perf_print() need to pass the additional info to call_fetch() methods, currently there is no simple way to do this. current->utask looks like a natural place to hold this info, but we need to allocate it before handler_chain(). This is a bit unfortunate, perhaps we will find a better solution later, but this is simple and should work right now. Signed-off-by: Oleg Nesterov Acked-by: Masami Hiramatsu Acked-by: Oleg Nesterov Cc: Srikar Dronamraju Signed-off-by: Namhyung Kim --- kernel/events/uprobes.c | 4 1 file changed, 4 insertions(+) diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c index 24b7d6c..3cc8e0b 100644 --- a/kernel/events/uprobes.c +++ b/kernel/events/uprobes.c @@ -1828,6 +1828,10 @@ static void handle_swbp(struct pt_regs *regs) if (unlikely(!test_bit(UPROBE_COPY_INSN, >flags))) goto out; + /* Tracing handlers use ->utask to communicate with fetch methods */ + if (!get_utask()) + goto out; + handler_chain(uprobe, regs); if (can_skip_sstep(uprobe, regs)) goto out; -- 1.8.4.3 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[for-next][PATCH 16/17] uprobes: Allocate -utask before handler_chain() for tracing handlers
From: Oleg Nesterov o...@redhat.com uprobe_trace_print() and uprobe_perf_print() need to pass the additional info to call_fetch() methods, currently there is no simple way to do this. current-utask looks like a natural place to hold this info, but we need to allocate it before handler_chain(). This is a bit unfortunate, perhaps we will find a better solution later, but this is simple and should work right now. Signed-off-by: Oleg Nesterov o...@redhat.com Acked-by: Masami Hiramatsu masami.hiramatsu...@hitachi.com Acked-by: Oleg Nesterov o...@redhat.com Cc: Srikar Dronamraju sri...@linux.vnet.ibm.com Signed-off-by: Namhyung Kim namhy...@kernel.org --- kernel/events/uprobes.c | 4 1 file changed, 4 insertions(+) diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c index 24b7d6c..3cc8e0b 100644 --- a/kernel/events/uprobes.c +++ b/kernel/events/uprobes.c @@ -1828,6 +1828,10 @@ static void handle_swbp(struct pt_regs *regs) if (unlikely(!test_bit(UPROBE_COPY_INSN, uprobe-flags))) goto out; + /* Tracing handlers use -utask to communicate with fetch methods */ + if (!get_utask()) + goto out; + handler_chain(uprobe, regs); if (can_skip_sstep(uprobe, regs)) goto out; -- 1.8.4.3 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 16/17] uprobes: Allocate ->utask before handler_chain() for tracing handlers
From: Oleg Nesterov uprobe_trace_print() and uprobe_perf_print() need to pass the additional info to call_fetch() methods, currently there is no simple way to do this. current->utask looks like a natural place to hold this info, but we need to allocate it before handler_chain(). This is a bit unfortunate, perhaps we will find a better solution later, but this is simple and should work right now. Signed-off-by: Oleg Nesterov Acked-by: Masami Hiramatsu Cc: Srikar Dronamraju Signed-off-by: Namhyung Kim --- kernel/events/uprobes.c | 4 1 file changed, 4 insertions(+) diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c index 24b7d6ca871b..3cc8e0bb8acf 100644 --- a/kernel/events/uprobes.c +++ b/kernel/events/uprobes.c @@ -1828,6 +1828,10 @@ static void handle_swbp(struct pt_regs *regs) if (unlikely(!test_bit(UPROBE_COPY_INSN, >flags))) goto out; + /* Tracing handlers use ->utask to communicate with fetch methods */ + if (!get_utask()) + goto out; + handler_chain(uprobe, regs); if (can_skip_sstep(uprobe, regs)) goto out; -- 1.7.11.7 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 16/17] uprobes: Allocate -utask before handler_chain() for tracing handlers
From: Oleg Nesterov o...@redhat.com uprobe_trace_print() and uprobe_perf_print() need to pass the additional info to call_fetch() methods, currently there is no simple way to do this. current-utask looks like a natural place to hold this info, but we need to allocate it before handler_chain(). This is a bit unfortunate, perhaps we will find a better solution later, but this is simple and should work right now. Signed-off-by: Oleg Nesterov o...@redhat.com Acked-by: Masami Hiramatsu masami.hiramatsu...@hitachi.com Cc: Srikar Dronamraju sri...@linux.vnet.ibm.com Signed-off-by: Namhyung Kim namhy...@kernel.org --- kernel/events/uprobes.c | 4 1 file changed, 4 insertions(+) diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c index 24b7d6ca871b..3cc8e0bb8acf 100644 --- a/kernel/events/uprobes.c +++ b/kernel/events/uprobes.c @@ -1828,6 +1828,10 @@ static void handle_swbp(struct pt_regs *regs) if (unlikely(!test_bit(UPROBE_COPY_INSN, uprobe-flags))) goto out; + /* Tracing handlers use -utask to communicate with fetch methods */ + if (!get_utask()) + goto out; + handler_chain(uprobe, regs); if (can_skip_sstep(uprobe, regs)) goto out; -- 1.7.11.7 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/17] uprobes: Allocate ->utask before handler_chain() for tracing handlers
(2013/12/09 15:20), Namhyung Kim wrote: > From: Oleg Nesterov > > uprobe_trace_print() and uprobe_perf_print() need to pass the additional > info to call_fetch() methods, currently there is no simple way to do this. > > current->utask looks like a natural place to hold this info, but we need > to allocate it before handler_chain(). > > This is a bit unfortunate, perhaps we will find a better solution later, > but this is simnple and should work right now. > > Signed-off-by: Oleg Nesterov > Cc: Srikar Dronamraju > Cc: Masami Hiramatsu > Signed-off-by: Namhyung Kim Acked-by: Masami Hiramatsu Thank you! > --- > kernel/events/uprobes.c | 4 > 1 file changed, 4 insertions(+) > > diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c > index 24b7d6ca871b..3cc8e0bb8acf 100644 > --- a/kernel/events/uprobes.c > +++ b/kernel/events/uprobes.c > @@ -1828,6 +1828,10 @@ static void handle_swbp(struct pt_regs *regs) > if (unlikely(!test_bit(UPROBE_COPY_INSN, >flags))) > goto out; > > + /* Tracing handlers use ->utask to communicate with fetch methods */ > + if (!get_utask()) > + goto out; > + > handler_chain(uprobe, regs); > if (can_skip_sstep(uprobe, regs)) > goto out; > -- Masami HIRAMATSU IT Management Research Dept. Linux Technology Center Hitachi, Ltd., Yokohama Research Laboratory E-mail: masami.hiramatsu...@hitachi.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/17] uprobes: Allocate ->utask before handler_chain() for tracing handlers
(2013/12/13 4:46), Oleg Nesterov wrote: > On 12/12, Masami Hiramatsu wrote: >> >> (2013/12/12 3:11), Oleg Nesterov wrote: >>> On 12/11, Masami Hiramatsu wrote: But it could skip the handler_chain silently. It could confuse users why their probe doesn't hit as expected. >>> >>> No, we will restart the same (probed) instruction, handle_swbp() >>> will be called again, get_utask() will be called again. >> >> Hmm, in that case, how would you avoid infinite recursive loop?? > > Masami, I do not understand your concerns ;) see below. > >> Would you repeat it until get_utask() != NULL? > > Yes, the task will loop until kmalloc(GFP_KERNEL) succeeds, and I see > nothing wrong here. > > Just in case, let me remind that it won't loop in kernel mode, it can > take a signal, it can be killed. And it is not recursive, this is > like restart after page fault (which btw can fault again if the page > was unmapped again, and "in theory" this loop can be infinite too). Ah! I got it :) > And why this is bad? Once again, this is GFP_KERNEL allocation, if it > loops "indefinitely" there is something wrong. Even a single GFP_KERNEL > failure likely means the task is already killed by oom, so it will > simply exit when it returns to user-mode. Indeed. It should be killed. > And how this differs from, say, the "endless" should_alloc_retry() loop > in __alloc_pages_slowpath() ? And note that in this case we loop in > kernel mode. Of course this is not possible "in practice", but the same > is true for the "endless" loop you are worried about. Agreed, at least that is not uprobe's business :) > Hmm, in that case, should uprobes handlers never be called on ppc with this change? >>> >>> Why? With this change ppc will have ->utask != NULL even if it doesn't >>> need it at all. >> >> Ah, I see. This changes that. > > Yes, this is why the changelog says "a bit unfortunate", we allocate the > memory even there is no trace_uprobe consumer. So it would be nice to > cleanup this later somehow, but imho this is a low priority problem and > perhaps we will simply postulate that uprobe_consumer->handler() can > rely on task->utask != NULL and remove get_utask() from pre_ssout(). > The only necessary cleanup (in my opinion) is that we should add another > member into the union in uprobe_task for trace_uprobe.c, but again I > think we should do this later to avoid the (potentially conflicting) > changes in this series. > > Oleg. > Thank you, -- Masami HIRAMATSU IT Management Research Dept. Linux Technology Center Hitachi, Ltd., Yokohama Research Laboratory E-mail: masami.hiramatsu...@hitachi.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Re: [PATCH 16/17] uprobes: Allocate ->utask before handler_chain() for tracing handlers
On 12/12, Masami Hiramatsu wrote: > > (2013/12/12 3:11), Oleg Nesterov wrote: > > On 12/11, Masami Hiramatsu wrote: > >> > >> But it could skip the handler_chain silently. It could confuse users > >> why their probe doesn't hit as expected. > > > > No, we will restart the same (probed) instruction, handle_swbp() > > will be called again, get_utask() will be called again. > > Hmm, in that case, how would you avoid infinite recursive loop?? Masami, I do not understand your concerns ;) see below. > Would you repeat it until get_utask() != NULL? Yes, the task will loop until kmalloc(GFP_KERNEL) succeeds, and I see nothing wrong here. Just in case, let me remind that it won't loop in kernel mode, it can take a signal, it can be killed. And it is not recursive, this is like restart after page fault (which btw can fault again if the page was unmapped again, and "in theory" this loop can be infinite too). And why this is bad? Once again, this is GFP_KERNEL allocation, if it loops "indefinitely" there is something wrong. Even a single GFP_KERNEL failure likely means the task is already killed by oom, so it will simply exit when it returns to user-mode. And how this differs from, say, the "endless" should_alloc_retry() loop in __alloc_pages_slowpath() ? And note that in this case we loop in kernel mode. Of course this is not possible "in practice", but the same is true for the "endless" loop you are worried about. > >> Hmm, in that case, should uprobes handlers never be called on ppc with > >> this change? > > > > Why? With this change ppc will have ->utask != NULL even if it doesn't > > need it at all. > > Ah, I see. This changes that. Yes, this is why the changelog says "a bit unfortunate", we allocate the memory even there is no trace_uprobe consumer. So it would be nice to cleanup this later somehow, but imho this is a low priority problem and perhaps we will simply postulate that uprobe_consumer->handler() can rely on task->utask != NULL and remove get_utask() from pre_ssout(). The only necessary cleanup (in my opinion) is that we should add another member into the union in uprobe_task for trace_uprobe.c, but again I think we should do this later to avoid the (potentially conflicting) changes in this series. Oleg. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Re: [PATCH 16/17] uprobes: Allocate -utask before handler_chain() for tracing handlers
On 12/12, Masami Hiramatsu wrote: (2013/12/12 3:11), Oleg Nesterov wrote: On 12/11, Masami Hiramatsu wrote: But it could skip the handler_chain silently. It could confuse users why their probe doesn't hit as expected. No, we will restart the same (probed) instruction, handle_swbp() will be called again, get_utask() will be called again. Hmm, in that case, how would you avoid infinite recursive loop?? Masami, I do not understand your concerns ;) see below. Would you repeat it until get_utask() != NULL? Yes, the task will loop until kmalloc(GFP_KERNEL) succeeds, and I see nothing wrong here. Just in case, let me remind that it won't loop in kernel mode, it can take a signal, it can be killed. And it is not recursive, this is like restart after page fault (which btw can fault again if the page was unmapped again, and in theory this loop can be infinite too). And why this is bad? Once again, this is GFP_KERNEL allocation, if it loops indefinitely there is something wrong. Even a single GFP_KERNEL failure likely means the task is already killed by oom, so it will simply exit when it returns to user-mode. And how this differs from, say, the endless should_alloc_retry() loop in __alloc_pages_slowpath() ? And note that in this case we loop in kernel mode. Of course this is not possible in practice, but the same is true for the endless loop you are worried about. Hmm, in that case, should uprobes handlers never be called on ppc with this change? Why? With this change ppc will have -utask != NULL even if it doesn't need it at all. Ah, I see. This changes that. Yes, this is why the changelog says a bit unfortunate, we allocate the memory even there is no trace_uprobe consumer. So it would be nice to cleanup this later somehow, but imho this is a low priority problem and perhaps we will simply postulate that uprobe_consumer-handler() can rely on task-utask != NULL and remove get_utask() from pre_ssout(). The only necessary cleanup (in my opinion) is that we should add another member into the union in uprobe_task for trace_uprobe.c, but again I think we should do this later to avoid the (potentially conflicting) changes in this series. Oleg. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/17] uprobes: Allocate -utask before handler_chain() for tracing handlers
(2013/12/09 15:20), Namhyung Kim wrote: From: Oleg Nesterov o...@redhat.com uprobe_trace_print() and uprobe_perf_print() need to pass the additional info to call_fetch() methods, currently there is no simple way to do this. current-utask looks like a natural place to hold this info, but we need to allocate it before handler_chain(). This is a bit unfortunate, perhaps we will find a better solution later, but this is simnple and should work right now. Signed-off-by: Oleg Nesterov o...@redhat.com Cc: Srikar Dronamraju sri...@linux.vnet.ibm.com Cc: Masami Hiramatsu masami.hiramatsu...@hitachi.com Signed-off-by: Namhyung Kim namhy...@kernel.org Acked-by: Masami Hiramatsu masami.hiramatsu...@hitachi.com Thank you! --- kernel/events/uprobes.c | 4 1 file changed, 4 insertions(+) diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c index 24b7d6ca871b..3cc8e0bb8acf 100644 --- a/kernel/events/uprobes.c +++ b/kernel/events/uprobes.c @@ -1828,6 +1828,10 @@ static void handle_swbp(struct pt_regs *regs) if (unlikely(!test_bit(UPROBE_COPY_INSN, uprobe-flags))) goto out; + /* Tracing handlers use -utask to communicate with fetch methods */ + if (!get_utask()) + goto out; + handler_chain(uprobe, regs); if (can_skip_sstep(uprobe, regs)) goto out; -- Masami HIRAMATSU IT Management Research Dept. Linux Technology Center Hitachi, Ltd., Yokohama Research Laboratory E-mail: masami.hiramatsu...@hitachi.com -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/17] uprobes: Allocate -utask before handler_chain() for tracing handlers
(2013/12/13 4:46), Oleg Nesterov wrote: On 12/12, Masami Hiramatsu wrote: (2013/12/12 3:11), Oleg Nesterov wrote: On 12/11, Masami Hiramatsu wrote: But it could skip the handler_chain silently. It could confuse users why their probe doesn't hit as expected. No, we will restart the same (probed) instruction, handle_swbp() will be called again, get_utask() will be called again. Hmm, in that case, how would you avoid infinite recursive loop?? Masami, I do not understand your concerns ;) see below. Would you repeat it until get_utask() != NULL? Yes, the task will loop until kmalloc(GFP_KERNEL) succeeds, and I see nothing wrong here. Just in case, let me remind that it won't loop in kernel mode, it can take a signal, it can be killed. And it is not recursive, this is like restart after page fault (which btw can fault again if the page was unmapped again, and in theory this loop can be infinite too). Ah! I got it :) And why this is bad? Once again, this is GFP_KERNEL allocation, if it loops indefinitely there is something wrong. Even a single GFP_KERNEL failure likely means the task is already killed by oom, so it will simply exit when it returns to user-mode. Indeed. It should be killed. And how this differs from, say, the endless should_alloc_retry() loop in __alloc_pages_slowpath() ? And note that in this case we loop in kernel mode. Of course this is not possible in practice, but the same is true for the endless loop you are worried about. Agreed, at least that is not uprobe's business :) Hmm, in that case, should uprobes handlers never be called on ppc with this change? Why? With this change ppc will have -utask != NULL even if it doesn't need it at all. Ah, I see. This changes that. Yes, this is why the changelog says a bit unfortunate, we allocate the memory even there is no trace_uprobe consumer. So it would be nice to cleanup this later somehow, but imho this is a low priority problem and perhaps we will simply postulate that uprobe_consumer-handler() can rely on task-utask != NULL and remove get_utask() from pre_ssout(). The only necessary cleanup (in my opinion) is that we should add another member into the union in uprobe_task for trace_uprobe.c, but again I think we should do this later to avoid the (potentially conflicting) changes in this series. Oleg. Thank you, -- Masami HIRAMATSU IT Management Research Dept. Linux Technology Center Hitachi, Ltd., Yokohama Research Laboratory E-mail: masami.hiramatsu...@hitachi.com -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Re: [PATCH 16/17] uprobes: Allocate ->utask before handler_chain() for tracing handlers
(2013/12/12 3:11), Oleg Nesterov wrote: > On 12/11, Masami Hiramatsu wrote: >> >> (2013/12/11 0:57), Oleg Nesterov wrote: >>> On 12/10, Masami Hiramatsu wrote: and isn't it better to increment miss-hit counter of the uprobe? >>> >>> What do you mean? This is not miss-hit and ->utask == NULL is quite normal. >> >> But it could skip the handler_chain silently. It could confuse users >> why their probe doesn't hit as expected. > > No, we will restart the same (probed) instruction, handle_swbp() > will be called again, get_utask() will be called again. Hmm, in that case, how would you avoid infinite recursive loop?? Would you repeat it until get_utask() != NULL? > Not to mention that (in practice) if GFP_KERNEL fails the task is > already killed. > >>> For example, on ppc it can be always NULL because ppc likely emulates the >>> probed insn. >> >> Hmm, in that case, should uprobes handlers never be called on ppc with >> this change? > > Why? With this change ppc will have ->utask != NULL even if it doesn't > need it at all. Ah, I see. This changes that. Thank you, -- Masami HIRAMATSU IT Management Research Dept. Linux Technology Center Hitachi, Ltd., Yokohama Research Laboratory E-mail: masami.hiramatsu...@hitachi.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/17] uprobes: Allocate ->utask before handler_chain() for tracing handlers
On 12/11, Masami Hiramatsu wrote: > > (2013/12/11 0:57), Oleg Nesterov wrote: > > On 12/10, Masami Hiramatsu wrote: > >> > >> and isn't it better to increment > >> miss-hit counter of the uprobe? > > > > What do you mean? This is not miss-hit and ->utask == NULL is quite normal. > > But it could skip the handler_chain silently. It could confuse users > why their probe doesn't hit as expected. No, we will restart the same (probed) instruction, handle_swbp() will be called again, get_utask() will be called again. Not to mention that (in practice) if GFP_KERNEL fails the task is already killed. > > For example, on ppc it can be always NULL because ppc likely emulates the > > probed insn. > > Hmm, in that case, should uprobes handlers never be called on ppc with > this change? Why? With this change ppc will have ->utask != NULL even if it doesn't need it at all. Oleg. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/17] uprobes: Allocate -utask before handler_chain() for tracing handlers
On 12/11, Masami Hiramatsu wrote: (2013/12/11 0:57), Oleg Nesterov wrote: On 12/10, Masami Hiramatsu wrote: and isn't it better to increment miss-hit counter of the uprobe? What do you mean? This is not miss-hit and -utask == NULL is quite normal. But it could skip the handler_chain silently. It could confuse users why their probe doesn't hit as expected. No, we will restart the same (probed) instruction, handle_swbp() will be called again, get_utask() will be called again. Not to mention that (in practice) if GFP_KERNEL fails the task is already killed. For example, on ppc it can be always NULL because ppc likely emulates the probed insn. Hmm, in that case, should uprobes handlers never be called on ppc with this change? Why? With this change ppc will have -utask != NULL even if it doesn't need it at all. Oleg. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Re: [PATCH 16/17] uprobes: Allocate -utask before handler_chain() for tracing handlers
(2013/12/12 3:11), Oleg Nesterov wrote: On 12/11, Masami Hiramatsu wrote: (2013/12/11 0:57), Oleg Nesterov wrote: On 12/10, Masami Hiramatsu wrote: and isn't it better to increment miss-hit counter of the uprobe? What do you mean? This is not miss-hit and -utask == NULL is quite normal. But it could skip the handler_chain silently. It could confuse users why their probe doesn't hit as expected. No, we will restart the same (probed) instruction, handle_swbp() will be called again, get_utask() will be called again. Hmm, in that case, how would you avoid infinite recursive loop?? Would you repeat it until get_utask() != NULL? Not to mention that (in practice) if GFP_KERNEL fails the task is already killed. For example, on ppc it can be always NULL because ppc likely emulates the probed insn. Hmm, in that case, should uprobes handlers never be called on ppc with this change? Why? With this change ppc will have -utask != NULL even if it doesn't need it at all. Ah, I see. This changes that. Thank you, -- Masami HIRAMATSU IT Management Research Dept. Linux Technology Center Hitachi, Ltd., Yokohama Research Laboratory E-mail: masami.hiramatsu...@hitachi.com -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/17] uprobes: Allocate ->utask before handler_chain() for tracing handlers
(2013/12/11 0:57), Oleg Nesterov wrote: > On 12/10, Masami Hiramatsu wrote: >> >> (2013/12/09 15:20), Namhyung Kim wrote: >>> From: Oleg Nesterov >>> >>> uprobe_trace_print() and uprobe_perf_print() need to pass the additional >>> info to call_fetch() methods, currently there is no simple way to do this. >>> >>> current->utask looks like a natural place to hold this info, but we need >>> to allocate it before handler_chain(). >>> >>> This is a bit unfortunate, perhaps we will find a better solution later, >>> but this is simnple and should work right now. >> >> Hmm, when this will happen? > > Perhaps never. Perhaps it will stay forever and we remove get_utask() from > pre_ssout() (it is not needed after this patch). Ah, OK, get_utask() is almost same as kzalloc(). > However I still think we can cleanup this. And to remind, we need to clean > the usage of utask->vaddr in trace_uprobe.c anyway. We can either try to > find another place to pass the info, or we can create a helper(s) for the > tracing handlers to access (and populate if NULL) utask->handler_data. > Note that this (probably) also makes sense because we can unexport > "struct uprobe_task" (but this needs a couple of off-topic cleanups). > > We will see. Lets do the minimal change which can work right now, Namhyung > has enough more serious problems ;) > >> and isn't it better to increment >> miss-hit counter of the uprobe? > > What do you mean? This is not miss-hit and ->utask == NULL is quite normal. But it could skip the handler_chain silently. It could confuse users why their probe doesn't hit as expected. > For example, on ppc it can be always NULL because ppc likely emulates the > probed insn. Hmm, in that case, should uprobes handlers never be called on ppc with this change? > Or did you mean that if get_utask() fails we should report this somehow? I meant that if the uprobes hits some error and not work as expected, it should be reported somehow to users, and miss-hit counter will be a possible option. Thank you, -- Masami HIRAMATSU IT Management Research Dept. Linux Technology Center Hitachi, Ltd., Yokohama Research Laboratory E-mail: masami.hiramatsu...@hitachi.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/17] uprobes: Allocate ->utask before handler_chain() for tracing handlers
Hi Oleg, On Tue, 10 Dec 2013 16:57:44 +0100, Oleg Nesterov wrote: > On 12/10, Masami Hiramatsu wrote: >> >> (2013/12/09 15:20), Namhyung Kim wrote: >> > From: Oleg Nesterov >> > >> > uprobe_trace_print() and uprobe_perf_print() need to pass the additional >> > info to call_fetch() methods, currently there is no simple way to do this. >> > >> > current->utask looks like a natural place to hold this info, but we need >> > to allocate it before handler_chain(). >> > >> > This is a bit unfortunate, perhaps we will find a better solution later, >> > but this is simnple and should work right now. >> >> Hmm, when this will happen? > > Perhaps never. Perhaps it will stay forever and we remove get_utask() from > pre_ssout() (it is not needed after this patch). > > However I still think we can cleanup this. And to remind, we need to clean > the usage of utask->vaddr in trace_uprobe.c anyway. We can either try to > find another place to pass the info, or we can create a helper(s) for the > tracing handlers to access (and populate if NULL) utask->handler_data. > Note that this (probably) also makes sense because we can unexport > "struct uprobe_task" (but this needs a couple of off-topic cleanups). > > We will see. Lets do the minimal change which can work right now, Namhyung > has enough more serious problems ;) Very true. :) > >> and isn't it better to increment >> miss-hit counter of the uprobe? > > What do you mean? This is not miss-hit and ->utask == NULL is quite normal. > For example, on ppc it can be always NULL because ppc likely emulates the > probed insn. Yes, for x86, it's always NULL at first but then populated after doing single-step. What we try to do is just moving the allocation before calling handler since we need to carry some information through it. Thanks, Namhyung > > Or did you mean that if get_utask() fails we should report this somehow? > Well, GFP_KERNEL should "never" fail and even if it fails we will restart > the same insn and retry the allocation. > > Or did I miss your point completely ? > > Oleg. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/17] uprobes: Allocate ->utask before handler_chain() for tracing handlers
On 12/10, Masami Hiramatsu wrote: > > (2013/12/09 15:20), Namhyung Kim wrote: > > From: Oleg Nesterov > > > > uprobe_trace_print() and uprobe_perf_print() need to pass the additional > > info to call_fetch() methods, currently there is no simple way to do this. > > > > current->utask looks like a natural place to hold this info, but we need > > to allocate it before handler_chain(). > > > > This is a bit unfortunate, perhaps we will find a better solution later, > > but this is simnple and should work right now. > > Hmm, when this will happen? Perhaps never. Perhaps it will stay forever and we remove get_utask() from pre_ssout() (it is not needed after this patch). However I still think we can cleanup this. And to remind, we need to clean the usage of utask->vaddr in trace_uprobe.c anyway. We can either try to find another place to pass the info, or we can create a helper(s) for the tracing handlers to access (and populate if NULL) utask->handler_data. Note that this (probably) also makes sense because we can unexport "struct uprobe_task" (but this needs a couple of off-topic cleanups). We will see. Lets do the minimal change which can work right now, Namhyung has enough more serious problems ;) > and isn't it better to increment > miss-hit counter of the uprobe? What do you mean? This is not miss-hit and ->utask == NULL is quite normal. For example, on ppc it can be always NULL because ppc likely emulates the probed insn. Or did you mean that if get_utask() fails we should report this somehow? Well, GFP_KERNEL should "never" fail and even if it fails we will restart the same insn and retry the allocation. Or did I miss your point completely ? Oleg. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/17] uprobes: Allocate ->utask before handler_chain() for tracing handlers
(2013/12/09 15:20), Namhyung Kim wrote: > From: Oleg Nesterov > > uprobe_trace_print() and uprobe_perf_print() need to pass the additional > info to call_fetch() methods, currently there is no simple way to do this. > > current->utask looks like a natural place to hold this info, but we need > to allocate it before handler_chain(). > > This is a bit unfortunate, perhaps we will find a better solution later, > but this is simnple and should work right now. Hmm, when this will happen? and isn't it better to increment miss-hit counter of the uprobe? Thank you, > > Signed-off-by: Oleg Nesterov > Cc: Srikar Dronamraju > Cc: Masami Hiramatsu > Signed-off-by: Namhyung Kim > --- > kernel/events/uprobes.c | 4 > 1 file changed, 4 insertions(+) > > diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c > index 24b7d6ca871b..3cc8e0bb8acf 100644 > --- a/kernel/events/uprobes.c > +++ b/kernel/events/uprobes.c > @@ -1828,6 +1828,10 @@ static void handle_swbp(struct pt_regs *regs) > if (unlikely(!test_bit(UPROBE_COPY_INSN, >flags))) > goto out; > > + /* Tracing handlers use ->utask to communicate with fetch methods */ > + if (!get_utask()) > + goto out; > + > handler_chain(uprobe, regs); > if (can_skip_sstep(uprobe, regs)) > goto out; > -- Masami HIRAMATSU IT Management Research Dept. Linux Technology Center Hitachi, Ltd., Yokohama Research Laboratory E-mail: masami.hiramatsu...@hitachi.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/17] uprobes: Allocate -utask before handler_chain() for tracing handlers
Hi Oleg, On Tue, 10 Dec 2013 16:57:44 +0100, Oleg Nesterov wrote: On 12/10, Masami Hiramatsu wrote: (2013/12/09 15:20), Namhyung Kim wrote: From: Oleg Nesterov o...@redhat.com uprobe_trace_print() and uprobe_perf_print() need to pass the additional info to call_fetch() methods, currently there is no simple way to do this. current-utask looks like a natural place to hold this info, but we need to allocate it before handler_chain(). This is a bit unfortunate, perhaps we will find a better solution later, but this is simnple and should work right now. Hmm, when this will happen? Perhaps never. Perhaps it will stay forever and we remove get_utask() from pre_ssout() (it is not needed after this patch). However I still think we can cleanup this. And to remind, we need to clean the usage of utask-vaddr in trace_uprobe.c anyway. We can either try to find another place to pass the info, or we can create a helper(s) for the tracing handlers to access (and populate if NULL) utask-handler_data. Note that this (probably) also makes sense because we can unexport struct uprobe_task (but this needs a couple of off-topic cleanups). We will see. Lets do the minimal change which can work right now, Namhyung has enough more serious problems ;) Very true. :) and isn't it better to increment miss-hit counter of the uprobe? What do you mean? This is not miss-hit and -utask == NULL is quite normal. For example, on ppc it can be always NULL because ppc likely emulates the probed insn. Yes, for x86, it's always NULL at first but then populated after doing single-step. What we try to do is just moving the allocation before calling handler since we need to carry some information through it. Thanks, Namhyung Or did you mean that if get_utask() fails we should report this somehow? Well, GFP_KERNEL should never fail and even if it fails we will restart the same insn and retry the allocation. Or did I miss your point completely ? Oleg. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/17] uprobes: Allocate -utask before handler_chain() for tracing handlers
(2013/12/11 0:57), Oleg Nesterov wrote: On 12/10, Masami Hiramatsu wrote: (2013/12/09 15:20), Namhyung Kim wrote: From: Oleg Nesterov o...@redhat.com uprobe_trace_print() and uprobe_perf_print() need to pass the additional info to call_fetch() methods, currently there is no simple way to do this. current-utask looks like a natural place to hold this info, but we need to allocate it before handler_chain(). This is a bit unfortunate, perhaps we will find a better solution later, but this is simnple and should work right now. Hmm, when this will happen? Perhaps never. Perhaps it will stay forever and we remove get_utask() from pre_ssout() (it is not needed after this patch). Ah, OK, get_utask() is almost same as kzalloc(). However I still think we can cleanup this. And to remind, we need to clean the usage of utask-vaddr in trace_uprobe.c anyway. We can either try to find another place to pass the info, or we can create a helper(s) for the tracing handlers to access (and populate if NULL) utask-handler_data. Note that this (probably) also makes sense because we can unexport struct uprobe_task (but this needs a couple of off-topic cleanups). We will see. Lets do the minimal change which can work right now, Namhyung has enough more serious problems ;) and isn't it better to increment miss-hit counter of the uprobe? What do you mean? This is not miss-hit and -utask == NULL is quite normal. But it could skip the handler_chain silently. It could confuse users why their probe doesn't hit as expected. For example, on ppc it can be always NULL because ppc likely emulates the probed insn. Hmm, in that case, should uprobes handlers never be called on ppc with this change? Or did you mean that if get_utask() fails we should report this somehow? I meant that if the uprobes hits some error and not work as expected, it should be reported somehow to users, and miss-hit counter will be a possible option. Thank you, -- Masami HIRAMATSU IT Management Research Dept. Linux Technology Center Hitachi, Ltd., Yokohama Research Laboratory E-mail: masami.hiramatsu...@hitachi.com -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/17] uprobes: Allocate -utask before handler_chain() for tracing handlers
(2013/12/09 15:20), Namhyung Kim wrote: From: Oleg Nesterov o...@redhat.com uprobe_trace_print() and uprobe_perf_print() need to pass the additional info to call_fetch() methods, currently there is no simple way to do this. current-utask looks like a natural place to hold this info, but we need to allocate it before handler_chain(). This is a bit unfortunate, perhaps we will find a better solution later, but this is simnple and should work right now. Hmm, when this will happen? and isn't it better to increment miss-hit counter of the uprobe? Thank you, Signed-off-by: Oleg Nesterov o...@redhat.com Cc: Srikar Dronamraju sri...@linux.vnet.ibm.com Cc: Masami Hiramatsu masami.hiramatsu...@hitachi.com Signed-off-by: Namhyung Kim namhy...@kernel.org --- kernel/events/uprobes.c | 4 1 file changed, 4 insertions(+) diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c index 24b7d6ca871b..3cc8e0bb8acf 100644 --- a/kernel/events/uprobes.c +++ b/kernel/events/uprobes.c @@ -1828,6 +1828,10 @@ static void handle_swbp(struct pt_regs *regs) if (unlikely(!test_bit(UPROBE_COPY_INSN, uprobe-flags))) goto out; + /* Tracing handlers use -utask to communicate with fetch methods */ + if (!get_utask()) + goto out; + handler_chain(uprobe, regs); if (can_skip_sstep(uprobe, regs)) goto out; -- Masami HIRAMATSU IT Management Research Dept. Linux Technology Center Hitachi, Ltd., Yokohama Research Laboratory E-mail: masami.hiramatsu...@hitachi.com -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/17] uprobes: Allocate -utask before handler_chain() for tracing handlers
On 12/10, Masami Hiramatsu wrote: (2013/12/09 15:20), Namhyung Kim wrote: From: Oleg Nesterov o...@redhat.com uprobe_trace_print() and uprobe_perf_print() need to pass the additional info to call_fetch() methods, currently there is no simple way to do this. current-utask looks like a natural place to hold this info, but we need to allocate it before handler_chain(). This is a bit unfortunate, perhaps we will find a better solution later, but this is simnple and should work right now. Hmm, when this will happen? Perhaps never. Perhaps it will stay forever and we remove get_utask() from pre_ssout() (it is not needed after this patch). However I still think we can cleanup this. And to remind, we need to clean the usage of utask-vaddr in trace_uprobe.c anyway. We can either try to find another place to pass the info, or we can create a helper(s) for the tracing handlers to access (and populate if NULL) utask-handler_data. Note that this (probably) also makes sense because we can unexport struct uprobe_task (but this needs a couple of off-topic cleanups). We will see. Lets do the minimal change which can work right now, Namhyung has enough more serious problems ;) and isn't it better to increment miss-hit counter of the uprobe? What do you mean? This is not miss-hit and -utask == NULL is quite normal. For example, on ppc it can be always NULL because ppc likely emulates the probed insn. Or did you mean that if get_utask() fails we should report this somehow? Well, GFP_KERNEL should never fail and even if it fails we will restart the same insn and retry the allocation. Or did I miss your point completely ? Oleg. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 16/17] uprobes: Allocate ->utask before handler_chain() for tracing handlers
From: Oleg Nesterov uprobe_trace_print() and uprobe_perf_print() need to pass the additional info to call_fetch() methods, currently there is no simple way to do this. current->utask looks like a natural place to hold this info, but we need to allocate it before handler_chain(). This is a bit unfortunate, perhaps we will find a better solution later, but this is simnple and should work right now. Signed-off-by: Oleg Nesterov Cc: Srikar Dronamraju Cc: Masami Hiramatsu Signed-off-by: Namhyung Kim --- kernel/events/uprobes.c | 4 1 file changed, 4 insertions(+) diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c index 24b7d6ca871b..3cc8e0bb8acf 100644 --- a/kernel/events/uprobes.c +++ b/kernel/events/uprobes.c @@ -1828,6 +1828,10 @@ static void handle_swbp(struct pt_regs *regs) if (unlikely(!test_bit(UPROBE_COPY_INSN, >flags))) goto out; + /* Tracing handlers use ->utask to communicate with fetch methods */ + if (!get_utask()) + goto out; + handler_chain(uprobe, regs); if (can_skip_sstep(uprobe, regs)) goto out; -- 1.7.11.7 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 16/17] uprobes: Allocate -utask before handler_chain() for tracing handlers
From: Oleg Nesterov o...@redhat.com uprobe_trace_print() and uprobe_perf_print() need to pass the additional info to call_fetch() methods, currently there is no simple way to do this. current-utask looks like a natural place to hold this info, but we need to allocate it before handler_chain(). This is a bit unfortunate, perhaps we will find a better solution later, but this is simnple and should work right now. Signed-off-by: Oleg Nesterov o...@redhat.com Cc: Srikar Dronamraju sri...@linux.vnet.ibm.com Cc: Masami Hiramatsu masami.hiramatsu...@hitachi.com Signed-off-by: Namhyung Kim namhy...@kernel.org --- kernel/events/uprobes.c | 4 1 file changed, 4 insertions(+) diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c index 24b7d6ca871b..3cc8e0bb8acf 100644 --- a/kernel/events/uprobes.c +++ b/kernel/events/uprobes.c @@ -1828,6 +1828,10 @@ static void handle_swbp(struct pt_regs *regs) if (unlikely(!test_bit(UPROBE_COPY_INSN, uprobe-flags))) goto out; + /* Tracing handlers use -utask to communicate with fetch methods */ + if (!get_utask()) + goto out; + handler_chain(uprobe, regs); if (can_skip_sstep(uprobe, regs)) goto out; -- 1.7.11.7 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 16/17] uprobes: Allocate ->utask before handler_chain() for tracing handlers
From: Oleg Nesterov uprobe_trace_print() and uprobe_perf_print() need to pass the additional info to call_fetch() methods, currently there is no simple way to do this. current->utask looks like a natural place to hold this info, but we need to allocate it before handler_chain(). This is a bit unfortunate, perhaps we will find a better solution later, but this is simnple and should work right now. Signed-off-by: Oleg Nesterov Cc: Srikar Dronamraju Cc: Masami Hiramatsu Signed-off-by: Namhyung Kim --- kernel/events/uprobes.c | 4 1 file changed, 4 insertions(+) diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c index ad8e1bdca70e..a1aa4aed029c 100644 --- a/kernel/events/uprobes.c +++ b/kernel/events/uprobes.c @@ -1744,6 +1744,10 @@ static void handle_swbp(struct pt_regs *regs) if (unlikely(!test_bit(UPROBE_COPY_INSN, >flags))) goto out; + /* Tracing handlers use ->utask to communicate with fetch methods */ + if (!get_utask()) + goto out; + handler_chain(uprobe, regs); if (can_skip_sstep(uprobe, regs)) goto out; -- 1.7.11.7 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 16/17] uprobes: Allocate -utask before handler_chain() for tracing handlers
From: Oleg Nesterov o...@redhat.com uprobe_trace_print() and uprobe_perf_print() need to pass the additional info to call_fetch() methods, currently there is no simple way to do this. current-utask looks like a natural place to hold this info, but we need to allocate it before handler_chain(). This is a bit unfortunate, perhaps we will find a better solution later, but this is simnple and should work right now. Signed-off-by: Oleg Nesterov o...@redhat.com Cc: Srikar Dronamraju sri...@linux.vnet.ibm.com Cc: Masami Hiramatsu masami.hiramatsu...@hitachi.com Signed-off-by: Namhyung Kim namhy...@kernel.org --- kernel/events/uprobes.c | 4 1 file changed, 4 insertions(+) diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c index ad8e1bdca70e..a1aa4aed029c 100644 --- a/kernel/events/uprobes.c +++ b/kernel/events/uprobes.c @@ -1744,6 +1744,10 @@ static void handle_swbp(struct pt_regs *regs) if (unlikely(!test_bit(UPROBE_COPY_INSN, uprobe-flags))) goto out; + /* Tracing handlers use -utask to communicate with fetch methods */ + if (!get_utask()) + goto out; + handler_chain(uprobe, regs); if (can_skip_sstep(uprobe, regs)) goto out; -- 1.7.11.7 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
