From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=50131 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1OXBrK-0008Ep-1A for qemu-devel@nongnu.org; Fri, 09 Jul 2010 07:35:11 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1OXBrH-0005op-Tv for qemu-devel@nongnu.org; Fri, 09 Jul 2010 07:35:09 -0400 Received: from e23smtp05.au.ibm.com ([202.81.31.147]:35846) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1OXBrH-0005oQ-2H for qemu-devel@nongnu.org; Fri, 09 Jul 2010 07:35:07 -0400 Received: from d23relay04.au.ibm.com (d23relay04.au.ibm.com [202.81.31.246]) by e23smtp05.au.ibm.com (8.14.4/8.13.1) with ESMTP id o69BUpxn006005 for ; Fri, 9 Jul 2010 21:30:51 +1000 Received: from d23av03.au.ibm.com (d23av03.au.ibm.com [9.190.234.97]) by d23relay04.au.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id o69BZ4iA1216678 for ; Fri, 9 Jul 2010 21:35:04 +1000 Received: from d23av03.au.ibm.com (loopback [127.0.0.1]) by d23av03.au.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id o69BZ4qk020719 for ; Fri, 9 Jul 2010 21:35:04 +1000 Message-ID: <4C370965.1050801@linux.vnet.ibm.com> Date: Fri, 09 Jul 2010 17:05:01 +0530 From: Prerna Saxena MIME-Version: 1.0 References: <20100630211145.41631855@zephyr> <20100701091839.GB2344@stefan-thinkpad.transitives.com> <20100706133417.1a344397@zephyr> <20100706101001.GA6651@stefan-thinkpad.transitives.com> <20100708105858.7b456295@zephyr> <20100708092031.GC2174@stefan-thinkpad.transitives.com> <4C35B494.2050800@linux.vnet.ibm.com> <20100708133419.GA22424@stefan-thinkpad.transitives.com> In-Reply-To: <20100708133419.GA22424@stefan-thinkpad.transitives.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: [Qemu-devel] Re: [RFC v3][PATCH][Tracing] Fix build errors for target i386-linux-user List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: Maneesh Soni , qemu-devel , Ananth Narayan On 07/08/2010 07:04 PM, Stefan Hajnoczi wrote: > On Thu, Jul 08, 2010 at 04:50:52PM +0530, Prerna Saxena wrote: >> On 07/08/2010 02:50 PM, Stefan Hajnoczi wrote: >>> On Thu, Jul 08, 2010 at 10:58:58AM +0530, Prerna Saxena wrote: >>>> [PATCH] Separate monitor command handler interfaces and tracing internals. >>>> >>>> >>>> Signed-off-by: Prerna Saxena >>>> --- >>>> monitor.c | 23 +++++++++++++++++++++++ >>>> simpletrace.c | 51 +++++++++++++++++++++++++++++---------------------- >>>> tracetool | 7 +++++++ >>>> 3 files changed, 59 insertions(+), 22 deletions(-) >>>> >>>> diff --git a/monitor.c b/monitor.c >>>> index 433a3ec..1f89938 100644 >>>> --- a/monitor.c >>>> +++ b/monitor.c >>>> @@ -540,6 +540,29 @@ static void do_change_trace_event_state(Monitor *mon, const QDict *qdict) >>>> bool new_state = qdict_get_bool(qdict, "option"); >>>> change_trace_event_state(tp_name, new_state); >>>> } >>>> + >>>> +void do_info_trace(Monitor *mon) >>>> +{ >>>> + unsigned int i; >>>> + char rec[MAX_TRACE_STR_LEN]; >>>> + unsigned int trace_idx = get_trace_idx(); >>>> + >>>> + for (i = 0; i< trace_idx ; i++) { >>>> + if (format_trace_string(i, rec)) { >>>> + monitor_printf(mon, rec); >>>> + } >>>> + } >>>> +} >>>> + >>>> +void do_info_all_trace_events(Monitor *mon) >>>> +{ >>>> + unsigned int i; >>>> + >>>> + for (i = 0; i< NR_TRACE_EVENTS; i++) { >>>> + monitor_printf(mon, "%s [Event ID %u] : state %u\n", >>>> + trace_list[i].tp_name, i, trace_list[i].state); >>>> + } >>>> +} >>>> #endif >>>> >>>> static void user_monitor_complete(void *opaque, QObject *ret_data) >>>> diff --git a/simpletrace.c b/simpletrace.c >>>> index 57c41fc..c7b1e7e 100644 >>>> --- a/simpletrace.c >>>> +++ b/simpletrace.c >>>> @@ -1,8 +1,8 @@ >>>> #include >>>> #include >>>> -#include "monitor.h" >>>> #include "trace.h" >>>> >>>> +/* Remember to update TRACE_REC_SIZE when changing TraceRecord structure */ >>> >>> I can't see TRACE_REC_SIZE anywhere else in this patch. >> >> Oops. This comment must go. The connotation was for >> MAX_TRACE_STR_LEN to be large enough to hold the formatted string, >> but I'm not sure if there is a way to test that. >> Done in v4. >>> >>>> typedef struct { >>>> unsigned long event; >>>> unsigned long x1; >>>> @@ -69,27 +69,6 @@ void trace5(TraceEventID event, unsigned long x1, unsigned long x2, unsigned lon >>>> trace(event, x1, x2, x3, x4, x5); >>>> } >>>> >>>> -void do_info_trace(Monitor *mon) >>>> -{ >>>> - unsigned int i; >>>> - >>>> - for (i = 0; i< trace_idx ; i++) { >>>> - monitor_printf(mon, "Event %lu : %lx %lx %lx %lx %lx\n", >>>> - trace_buf[i].event, trace_buf[i].x1, trace_buf[i].x2, >>>> - trace_buf[i].x3, trace_buf[i].x4, trace_buf[i].x5); >>>> - } >>>> -} >>>> - >>>> -void do_info_all_trace_events(Monitor *mon) >>>> -{ >>>> - unsigned int i; >>>> - >>>> - for (i = 0; i< NR_TRACE_EVENTS; i++) { >>>> - monitor_printf(mon, "%s [Event ID %u] : state %u\n", >>>> - trace_list[i].tp_name, i, trace_list[i].state); >>>> - } >>>> -} >>>> - >>>> static TraceEvent* find_trace_event_by_name(const char *tname) >>>> { >>>> unsigned int i; >>>> @@ -115,3 +94,31 @@ void change_trace_event_state(const char *tname, bool tstate) >>>> tp->state = tstate; >>>> } >>>> } >>>> + >>>> +/** >>>> + * Return the current trace index. >>>> + * >>>> + */ >>>> +unsigned int get_trace_idx(void) >>>> +{ >>>> + return trace_idx; >>>> +} >>> >>> format_trace_string() returns NULL if the index is beyond the last valid >>> trace record. monitor.c doesn't need to know how many trace records >>> there are ahead of time, it can just keep printing until it gets NULL. >>> I don't feel strongly about this but wanted to mention it. >> >> format_trace_string() returns NULL when the index passed exceeds the >> size of trace buffer. This function is meant for printing current >> contents of trace buffer, which may be less than the entire buffer >> size. > > Sorry, you're right the patch will return NULL if the index exceeds the > size of the trace buffer. > > The idea I was suggesting requires it to return NULL when the index>= > trace_idx. > I've tried to keep this as generic as possible. get_trace_idx() can be put to use to query state of trace buffer in different scenarios. >>> >>>> + >>>> +/** >>>> + * returns formatted TraceRecord at a given index in the trace buffer. >>>> + * FORMAT : "Event %lu : %lx %lx %lx %lx %lx\n" >>>> + * >>>> + * @idx : index in the buffer for which trace record is returned. >>>> + * @trace_str : output string passed. >>>> + */ >>>> +char* format_trace_string(unsigned int idx, char trace_str[]) >>>> +{ >>>> + TraceRecord rec; >>>> + if (idx>= TRACE_BUF_LEN || sizeof(trace_str)>= MAX_TRACE_STR_LEN) { >>> >>> sizeof(trace_str) == sizeof(char *), not the size of the caller's array in bytes. >> >> Hmm, I'll need to scrap off this check. >> Done. >>> >>> The fixed size limit can be eliminated using asprintf(3), which >>> allocates a string of the right size while doing the string formatting. >>> The caller of format_trace_string() is then responsible for freeing the >>> string when they are done with it. >>> >> >> I am somehow reluctant to allocate memory here and free it somewhere >> else. Calls for memory leaks quite easily in case it gets missed. >> I'd rather use stack-allocated arrays that clean up after the call >> to the handler is done. > > Okay. > >> >>>> + return NULL; >>>> + } >>>> + rec = trace_buf[idx]; >>> >>> Is it necessary to copy the trace record here? >> >> In my understanding, this would run in the context of monitor >> handlers, which are executed in a separate thread other than the >> main qemu execution loop. Since sprintf() is a longer operation, >> considering the trace_idx might get incremented in the meantime -- I >> had thought copying the TraceRecord would be achieved more quickly >> with lesser probability of index slipping away. Might be an >> over-optimization -- pls correct me if I'm wrong :-) > > I haven't read the monitor code but I'd expect it to be executed in the > iothread like all other asynchronous IO handlers on file descriptors. A > quick dig through monitor.c and qemu-char.c suggests that that is uses > qemu_set_fd_handler() and will therefore be called with the qemu mutex > held. > Ok, removed this in v4. >> >>> >>>> + sprintf(&trace_str[0], "Event %lu : %lx %lx %lx %lx %lx\n", >>>> + rec.event, rec.x1, rec.x2, rec.x3, rec.x4, rec.x5); >>>> + return&trace_str[0]; >>>> +} >>>> diff --git a/tracetool b/tracetool >>>> index c77280d..b7a0499 100755 >>>> --- a/tracetool >>>> +++ b/tracetool >>>> @@ -125,6 +125,11 @@ typedef struct { >>>> bool state; >>>> } TraceEvent; >>>> >>>> +/* Max size of trace string to be displayed via the monitor. >>>> + * Format : "Event %lu : %lx %lx %lx %lx %lx\n" >>>> + */ >>>> +#define MAX_TRACE_STR_LEN 100 >>>> + >>>> void trace1(TraceEventID event, unsigned long x1); >>>> void trace2(TraceEventID event, unsigned long x1, unsigned long x2); >>>> void trace3(TraceEventID event, unsigned long x1, unsigned long x2, unsigned long x3); >>>> @@ -133,6 +138,8 @@ void trace5(TraceEventID event, unsigned long x1, unsigned long x2, unsigned lon >>>> void do_info_trace(Monitor *mon); >>>> void do_info_all_trace_events(Monitor *mon); >>>> void change_trace_event_state(const char *tname, bool tstate); >>>> +unsigned int get_trace_idx(void); >>>> +char* format_trace_string(unsigned int idx, char *trace_str); >>> >>> I think we need to choose a prefix like simpletrace_*() or something >>> more concise (but not "strace_" because it's too confusing). Other >>> subsystems tend to do this: pci_*(), ram_*(), etc. >>> >> >> Agree, it is useful. However, simpletrace_ is too big for a prefix. >> Maybe st_ works, though it is slightly on the ambiguous side ? > > Cool, st_ works for me. > Great. I'll post a separate patch to address the name changes. >> >>> Perhaps let's do it as a separate patch to fix up all of the simple >>> trace backend. >>> >> Regards, -- Prerna Saxena Linux Technology Centre, IBM Systems and Technology Lab, Bangalore, India