Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S966456Ab3HIJ5G (ORCPT ); Fri, 9 Aug 2013 05:57:06 -0400 Received: from mail7.hitachi.co.jp ([133.145.228.42]:53182 "EHLO mail7.hitachi.co.jp" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S965803Ab3HIJ5E (ORCPT ); Fri, 9 Aug 2013 05:57:04 -0400 Message-ID: <5204BCE6.2070102@hitachi.com> Date: Fri, 09 Aug 2013 18:56:54 +0900 From: Masami Hiramatsu Organization: Hitachi, Ltd., Japan User-Agent: Mozilla/5.0 (Windows NT 5.2; rv:13.0) Gecko/20120614 Thunderbird/13.0.1 MIME-Version: 1.0 To: Namhyung Kim Cc: Steven Rostedt , Namhyung Kim , Hyeoncheol Lee , LKML , Srikar Dronamraju , Oleg Nesterov , "zhangwei(Jovi)" , Arnaldo Carvalho de Melo Subject: Re: [PATCH 10/13] tracing/uprobes: Fetch args before reserving a ring buffer References: <1376037909-17797-1-git-send-email-namhyung@kernel.org> <1376037909-17797-11-git-send-email-namhyung@kernel.org> In-Reply-To: <1376037909-17797-11-git-send-email-namhyung@kernel.org> Content-Type: text/plain; charset=ISO-2022-JP Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5166 Lines: 161 (2013/08/09 17:45), Namhyung Kim wrote: > From: Namhyung Kim > > Fetching from user space should be done in a non-atomic context. So > use a temporary buffer and copy its content to the ring buffer > atomically. > > While at it, use __get_data_size() and store_trace_args() to reduce > code duplication. I just concern using kmalloc() in the event handler. For fetching user memory which can be swapped out, that is true. But most of the cases, we can presume that it exists on the physical memory. I'd like to ask the opinions of Srikar and Oleg. BTW, you'd better add to patch description why previously this is not needed, and your series needs it too. :) Thank you, > Cc: Masami Hiramatsu > Cc: Srikar Dronamraju > Cc: Oleg Nesterov > Cc: zhangwei(Jovi) > Cc: Arnaldo Carvalho de Melo > Signed-off-by: Namhyung Kim > --- > kernel/trace/trace_uprobe.c | 69 ++++++++++++++++++++++++++++++++++----------- > 1 file changed, 53 insertions(+), 16 deletions(-) > > diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c > index f991cac2b9ba..2888b95b063f 100644 > --- a/kernel/trace/trace_uprobe.c > +++ b/kernel/trace/trace_uprobe.c > @@ -516,15 +516,31 @@ static void uprobe_trace_print(struct trace_uprobe *tu, > struct uprobe_trace_entry_head *entry; > struct ring_buffer_event *event; > struct ring_buffer *buffer; > - void *data; > - int size, i; > + void *data, *tmp; > + int size, dsize, esize; > struct ftrace_event_call *call = &tu->p.call; > > - size = SIZEOF_TRACE_ENTRY(is_ret_probe(tu)); > + dsize = __get_data_size(&tu->p, regs); > + esize = SIZEOF_TRACE_ENTRY(is_ret_probe(tu)); > + > + /* > + * A temporary buffer is used for storing fetched data before reserving > + * the ring buffer because fetching from user space should be done in a > + * non-atomic context. > + */ > + tmp = kmalloc(tu->p.size + dsize, GFP_KERNEL); > + if (tmp == NULL) > + return; > + > + store_trace_args(esize, &tu->p, regs, tmp, dsize); > + > + size = esize + tu->p.size + dsize; > event = trace_current_buffer_lock_reserve(&buffer, call->event.type, > - size + tu->p.size, 0, 0); > - if (!event) > + size, 0, 0); > + if (!event) { > + kfree(tmp); > return; > + } > > entry = ring_buffer_event_data(event); > if (is_ret_probe(tu)) { > @@ -536,13 +552,12 @@ static void uprobe_trace_print(struct trace_uprobe *tu, > data = DATAOF_TRACE_ENTRY(entry, false); > } > > - for (i = 0; i < tu->p.nr_args; i++) { > - call_fetch(&tu->p.args[i].fetch, regs, > - data + tu->p.args[i].offset); > - } > + memcpy(data, tmp, tu->p.size + dsize); > > if (!filter_current_check_discard(buffer, call, entry, event)) > trace_buffer_unlock_commit(buffer, event, 0, 0); > + > + kfree(tmp); > } > > /* uprobe handler */ > @@ -756,11 +771,30 @@ static void uprobe_perf_print(struct trace_uprobe *tu, > struct ftrace_event_call *call = &tu->p.call; > struct uprobe_trace_entry_head *entry; > struct hlist_head *head; > - void *data; > - int size, rctx, i; > + void *data, *tmp; > + int size, dsize, esize; > + int rctx; > + > + dsize = __get_data_size(&tu->p, regs); > + esize = SIZEOF_TRACE_ENTRY(is_ret_probe(tu)); > + > + /* > + * A temporary buffer is used for storing fetched data before reserving > + * the ring buffer because fetching from user space should be done in a > + * non-atomic context. > + */ > + tmp = kmalloc(tu->p.size + dsize, GFP_KERNEL); > + if (tmp == NULL) > + return; > > - size = SIZEOF_TRACE_ENTRY(is_ret_probe(tu)); > - size = ALIGN(size + tu->p.size + sizeof(u32), sizeof(u64)) - sizeof(u32); > + store_trace_args(esize, &tu->p, regs, tmp, dsize); > + > + size = esize + tu->p.size + dsize; > + size = ALIGN(size + sizeof(u32), sizeof(u64)) - sizeof(u32); > + if (WARN_ONCE(size > PERF_MAX_TRACE_SIZE, "profile buffer not large enough")) { > + kfree(tmp); > + return; > + } > > preempt_disable(); > head = this_cpu_ptr(call->perf_events); > @@ -780,15 +814,18 @@ static void uprobe_perf_print(struct trace_uprobe *tu, > data = DATAOF_TRACE_ENTRY(entry, false); > } > > - for (i = 0; i < tu->p.nr_args; i++) { > - struct probe_arg *parg = &tu->p.args[i]; > + memcpy(data, tmp, tu->p.size + dsize); > + > + if (size - esize > tu->p.size + dsize) { > + int len = tu->p.size + dsize; > > - call_fetch(&parg->fetch, regs, data + parg->offset); > + memset(data + len, 0, size - esize - len); > } > > perf_trace_buf_submit(entry, size, rctx, 0, 1, regs, head, NULL); > out: > preempt_enable(); > + kfree(tmp); > } > > /* uprobe profile handler */ > -- Masami HIRAMATSU IT Management Research Dept. Linux Technology Center Hitachi, Ltd., Yokohama Research Laboratory E-mail: masami.hiramatsu.pt@hitachi.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/