* [Qemu-devel] [PATCH v7 0/3] Simpletrace v2: Support multiple args, strings.
@ 2012-07-03 9:20 Harsh Prateek Bora
2012-07-03 9:20 ` [Qemu-devel] [PATCH v7 1/3] monitor: remove unused do_info_trace Harsh Prateek Bora
` (4 more replies)
0 siblings, 5 replies; 14+ messages in thread
From: Harsh Prateek Bora @ 2012-07-03 9:20 UTC (permalink / raw)
To: stefanha, qemu-devel, stefanha; +Cc: Harsh Prateek Bora
Existing simpletrace backend allows to trace at max 6 args and does not
support strings. This newer tracelog format gets rid of fixed size records
and therefore allows to trace variable number of args including strings.
Sample trace:
v9fs_version 0.000 tag=0xffff id=0x64 msize=0x2000 version=9P2000.L
v9fs_version_return 6.705 tag=0xffff id=0x64 msize=0x2000 version=9P2000.L
v9fs_attach 174.467 tag=0x1 id=0x68 fid=0x0 afid=0xffffffffffffffff
uname=nobody aname=
v9fs_attach_return 4720.454 tag=0x1 id=0x68 type=0xffffffffffffff80
version=0x4f2a4dd0 path=0x220ea6
v7:
- Introduced clear_buffer_range to rest buffer range after consuming record.
- Introduced cap on string length using MAX_TRACE_STRLEN (currently set to 512)
to avoid calculating wrong length when string length is > sizeof(uint32_t).
- Reverted idx refresh back to inside while-lipp in writeout_thread.
v6:
- Corrected flush condition and reverted un-necessary check in get_trace_record
- moved idx refresh out of while-loop in writeout_thread, see comments in code.
v5:
- Addressed Stefan's review comments on v4 series.
- Inroduced code to handle corrupted records due to racing tracers.
v4:
- removed unused safe_strlen interface (missed in v3).
v3:
- Addressed Stefan's review comments on v2 series
v2:
- moved elimination of st_print_trace to 1/3 patch
- use sizeof(TraceRecord) for #define ST_REC_HDR_LEN
v1:
- Simpletrace v2 for the new (pythonized) tracetool
Harsh Prateek Bora (3):
monitor: remove unused do_info_trace
Simpletrace v2: Support multiple arguments, strings.
Update simpletrace.py for new log format
monitor.c | 16 --
scripts/simpletrace.py | 116 +++++++++------
scripts/tracetool/backend/simple.py | 84 ++++++++---
trace/simple.c | 274 +++++++++++++++++++++--------------
trace/simple.h | 40 ++++-
5 files changed, 335 insertions(+), 195 deletions(-)
--
1.7.10.4
^ permalink raw reply [flat|nested] 14+ messages in thread
* [Qemu-devel] [PATCH v7 1/3] monitor: remove unused do_info_trace
2012-07-03 9:20 [Qemu-devel] [PATCH v7 0/3] Simpletrace v2: Support multiple args, strings Harsh Prateek Bora
@ 2012-07-03 9:20 ` Harsh Prateek Bora
2012-07-03 9:20 ` [Qemu-devel] [PATCH v7 2/3] Simpletrace v2: Support multiple arguments, strings Harsh Prateek Bora
` (3 subsequent siblings)
4 siblings, 0 replies; 14+ messages in thread
From: Harsh Prateek Bora @ 2012-07-03 9:20 UTC (permalink / raw)
To: stefanha, qemu-devel, stefanha; +Cc: Harsh Prateek Bora
Going forward with simpletrace v2 variable size trace records, we cannot
have a generic function to print trace event info and therefore this
interface becomes invalid.
As per Stefan Hajnoczi:
"This command is only available from the human monitor. It's not very
useful because it historically hasn't been able to pretty-print events
or show them in the right order (we use a ringbuffer but it prints
them out from index 0).
Therefore, I don't think we're under any obligation to keep this
command around. No one has complained about it's limitations - I
think this is a sign that no one has used it. I'd be okay with a
patch that removes it."
Ref: http://lists.gnu.org/archive/html/qemu-devel/2012-01/msg01268.html
Signed-off-by: Harsh Prateek Bora <harsh@linux.vnet.ibm.com>
---
monitor.c | 16 ----------------
trace/simple.c | 18 ------------------
trace/simple.h | 1 -
3 files changed, 35 deletions(-)
diff --git a/monitor.c b/monitor.c
index a3bc2c7..1fb00ab 100644
--- a/monitor.c
+++ b/monitor.c
@@ -795,13 +795,6 @@ static void do_info_cpu_stats(Monitor *mon)
}
#endif
-#if defined(CONFIG_TRACE_SIMPLE)
-static void do_info_trace(Monitor *mon)
-{
- st_print_trace((FILE *)mon, &monitor_fprintf);
-}
-#endif
-
static void do_trace_print_events(Monitor *mon)
{
trace_print_events((FILE *)mon, &monitor_fprintf);
@@ -2568,15 +2561,6 @@ static mon_cmd_t info_cmds[] = {
.help = "show roms",
.mhandler.info = do_info_roms,
},
-#if defined(CONFIG_TRACE_SIMPLE)
- {
- .name = "trace",
- .args_type = "",
- .params = "",
- .help = "show current contents of trace buffer",
- .mhandler.info = do_info_trace,
- },
-#endif
{
.name = "trace-events",
.args_type = "",
diff --git a/trace/simple.c b/trace/simple.c
index b4a3c6e..b64bcf4 100644
--- a/trace/simple.c
+++ b/trace/simple.c
@@ -291,24 +291,6 @@ void st_print_trace_file_status(FILE *stream, int (*stream_printf)(FILE *stream,
trace_file_name, trace_fp ? "on" : "off");
}
-void st_print_trace(FILE *stream, int (*stream_printf)(FILE *stream, const char *fmt, ...))
-{
- unsigned int i;
-
- for (i = 0; i < TRACE_BUF_LEN; i++) {
- TraceRecord record;
-
- if (!get_trace_record(i, &record)) {
- continue;
- }
- stream_printf(stream, "Event %" PRIu64 " : %" PRIx64 " %" PRIx64
- " %" PRIx64 " %" PRIx64 " %" PRIx64 " %" PRIx64 "\n",
- record.event, record.x1, record.x2,
- record.x3, record.x4, record.x5,
- record.x6);
- }
-}
-
void st_flush_trace_buffer(void)
{
flush_trace_file(true);
diff --git a/trace/simple.h b/trace/simple.h
index 466e75b..6b5358c 100644
--- a/trace/simple.h
+++ b/trace/simple.h
@@ -29,7 +29,6 @@ void trace3(TraceEventID event, uint64_t x1, uint64_t x2, uint64_t x3);
void trace4(TraceEventID event, uint64_t x1, uint64_t x2, uint64_t x3, uint64_t x4);
void trace5(TraceEventID event, uint64_t x1, uint64_t x2, uint64_t x3, uint64_t x4, uint64_t x5);
void trace6(TraceEventID event, uint64_t x1, uint64_t x2, uint64_t x3, uint64_t x4, uint64_t x5, uint64_t x6);
-void st_print_trace(FILE *stream, fprintf_function stream_printf);
void st_print_trace_file_status(FILE *stream, fprintf_function stream_printf);
void st_set_trace_file_enabled(bool enable);
bool st_set_trace_file(const char *file);
--
1.7.10.4
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [Qemu-devel] [PATCH v7 2/3] Simpletrace v2: Support multiple arguments, strings.
2012-07-03 9:20 [Qemu-devel] [PATCH v7 0/3] Simpletrace v2: Support multiple args, strings Harsh Prateek Bora
2012-07-03 9:20 ` [Qemu-devel] [PATCH v7 1/3] monitor: remove unused do_info_trace Harsh Prateek Bora
@ 2012-07-03 9:20 ` Harsh Prateek Bora
2012-07-17 15:21 ` Stefan Hajnoczi
2012-07-03 9:20 ` [Qemu-devel] [PATCH v7 3/3] Update simpletrace.py for new log format Harsh Prateek Bora
` (2 subsequent siblings)
4 siblings, 1 reply; 14+ messages in thread
From: Harsh Prateek Bora @ 2012-07-03 9:20 UTC (permalink / raw)
To: stefanha, qemu-devel, stefanha; +Cc: Harsh Prateek Bora
Existing simpletrace backend allows to trace at max 6 args and does not
support strings. This newer tracelog format gets rid of fixed size records
and therefore allows to trace variable number of args including strings.
Sample trace with strings:
v9fs_version 0.000 tag=0xffff id=0x64 msize=0x2000 version=9P2000.L
v9fs_version_return 6.705 tag=0xffff id=0x64 msize=0x2000 version=9P2000.L
Signed-off-by: Harsh Prateek Bora <harsh@linux.vnet.ibm.com>
---
scripts/tracetool/backend/simple.py | 84 +++++++++---
trace/simple.c | 256 ++++++++++++++++++++++-------------
trace/simple.h | 39 +++++-
3 files changed, 260 insertions(+), 119 deletions(-)
diff --git a/scripts/tracetool/backend/simple.py b/scripts/tracetool/backend/simple.py
index fbb5717..d3cf4da 100644
--- a/scripts/tracetool/backend/simple.py
+++ b/scripts/tracetool/backend/simple.py
@@ -15,9 +15,16 @@ __email__ = "stefanha@linux.vnet.ibm.com"
from tracetool import out
+def is_string(arg):
+ strtype = ('const char*', 'char*', 'const char *', 'char *')
+ if arg.lstrip().startswith(strtype):
+ return True
+ else:
+ return False
def c(events):
out('#include "trace.h"',
+ '#include "trace/simple.h"',
'',
'TraceEvent trace_list[] = {')
@@ -26,30 +33,69 @@ def c(events):
name = e.name,
)
- out('};')
-
-def h(events):
- out('#include "trace/simple.h"',
+ out('};',
'')
- for num, e in enumerate(events):
- if len(e.args):
- argstr = e.args.names()
- arg_prefix = ', (uint64_t)(uintptr_t)'
- cast_args = arg_prefix + arg_prefix.join(argstr)
- simple_args = (str(num) + cast_args)
- else:
- simple_args = str(num)
+ for num, event in enumerate(events):
+ sizes = []
+ for type_, name in event.args:
+ if is_string(type_):
+ sizes.append("4 + ((" + name + " ? strlen(" + name + ") : 0) % MAX_TRACE_STRLEN)")
+ else:
+ sizes.append("8")
+ sizestr = " + ".join(sizes)
+ if len(event.args) == 0:
+ sizestr = '0'
- out('static inline void trace_%(name)s(%(args)s)',
+ out('void trace_%(name)s(%(args)s)',
'{',
- ' trace%(argc)d(%(trace_args)s);',
- '}',
- name = e.name,
- args = e.args,
- argc = len(e.args),
- trace_args = simple_args,
+ ' TraceBufferRecord rec;',
+ '',
+ ' if (!trace_list[%(event_id)s].state) {',
+ ' return;',
+ ' }',
+ '',
+ ' if (trace_record_start(&rec, %(event_id)s, %(size_str)s)) {',
+ ' return; /* Trace Buffer Full, Event Dropped ! */',
+ ' }',
+ name = event.name,
+ args = event.args,
+ event_id = num,
+ size_str = sizestr,
)
+ if len(event.args) > 0:
+ for type_, name in event.args:
+ # string
+ if is_string(type_):
+ out(' trace_record_write_str(&rec, %(name)s);',
+ name = name,
+ )
+ # pointer var (not string)
+ elif type_.endswith('*'):
+ out(' trace_record_write_u64(&rec, (uint64_t)(uint64_t *)%(name)s);',
+ name = name,
+ )
+ # primitive data type
+ else:
+ out(' trace_record_write_u64(&rec, (uint64_t)%(name)s);',
+ name = name,
+ )
+
+ out(' trace_record_finish(&rec);',
+ '}',
+ '')
+
+
+def h(events):
+ out('#include "trace/simple.h"',
+ '')
+
+ for event in events:
+ out('void trace_%(name)s(%(args)s);',
+ name = event.name,
+ args = event.args,
+ )
+ out('')
out('#define NR_TRACE_EVENTS %d' % len(events))
out('extern TraceEvent trace_list[NR_TRACE_EVENTS];')
diff --git a/trace/simple.c b/trace/simple.c
index b64bcf4..3c14568 100644
--- a/trace/simple.c
+++ b/trace/simple.c
@@ -27,7 +27,7 @@
#define HEADER_MAGIC 0xf2b177cb0aa429b4ULL
/** Trace file version number, bump if format changes */
-#define HEADER_VERSION 0
+#define HEADER_VERSION 2
/** Records were dropped event ID */
#define DROPPED_EVENT_ID (~(uint64_t)0 - 1)
@@ -35,23 +35,6 @@
/** Trace record is valid */
#define TRACE_RECORD_VALID ((uint64_t)1 << 63)
-/** Trace buffer entry */
-typedef struct {
- uint64_t event;
- uint64_t timestamp_ns;
- uint64_t x1;
- uint64_t x2;
- uint64_t x3;
- uint64_t x4;
- uint64_t x5;
- uint64_t x6;
-} TraceRecord;
-
-enum {
- TRACE_BUF_LEN = 4096,
- TRACE_BUF_FLUSH_THRESHOLD = TRACE_BUF_LEN / 4,
-};
-
/*
* Trace records are written out by a dedicated thread. The thread waits for
* records to become available, writes them out, and then waits again.
@@ -62,11 +45,49 @@ static GCond *trace_empty_cond;
static bool trace_available;
static bool trace_writeout_enabled;
-static TraceRecord trace_buf[TRACE_BUF_LEN];
+enum {
+ TRACE_BUF_LEN = 4096 * 64,
+ TRACE_BUF_FLUSH_THRESHOLD = TRACE_BUF_LEN / 4,
+};
+
+uint8_t trace_buf[TRACE_BUF_LEN];
static unsigned int trace_idx;
+static unsigned int writeout_idx;
+static uint64_t dropped_events;
static FILE *trace_fp;
static char *trace_file_name = NULL;
+/* * Trace buffer entry */
+typedef struct {
+ uint64_t event; /* TraceEventID */
+ uint64_t timestamp_ns;
+ uint32_t length; /* in bytes */
+ uint32_t reserved; /* unused */
+ uint8_t arguments[];
+} TraceRecord;
+
+typedef struct {
+ uint64_t header_event_id; /* HEADER_EVENT_ID */
+ uint64_t header_magic; /* HEADER_MAGIC */
+ uint64_t header_version; /* HEADER_VERSION */
+} TraceRecordHeader;
+
+
+static void read_from_buffer(unsigned int idx, void *dataptr, size_t size);
+static unsigned int write_to_buffer(unsigned int idx, void *dataptr, size_t size);
+static void clear_buffer_range(unsigned int idx, size_t len);
+
+static void clear_buffer_range(unsigned int idx, size_t len)
+{
+ uint32_t num = 0;
+ while (num < len) {
+ if (idx >= TRACE_BUF_LEN) {
+ idx = idx % TRACE_BUF_LEN;
+ }
+ trace_buf[idx++] = 0;
+ num++;
+ }
+}
/**
* Read a trace record from the trace buffer
*
@@ -75,16 +96,31 @@ static char *trace_file_name = NULL;
*
* Returns false if the record is not valid.
*/
-static bool get_trace_record(unsigned int idx, TraceRecord *record)
+static bool get_trace_record(unsigned int idx, TraceRecord **recordptr)
{
- if (!(trace_buf[idx].event & TRACE_RECORD_VALID)) {
+ uint8_t rec_hdr[sizeof(TraceRecord)];
+ uint64_t event_flag = 0;
+ TraceRecord *record = (TraceRecord *) rec_hdr;
+ /* read the event flag to see if its a valid record */
+ read_from_buffer(idx, rec_hdr, sizeof(event_flag));
+
+ if (!(record->event & TRACE_RECORD_VALID)) {
return false;
}
__sync_synchronize(); /* read memory barrier before accessing record */
-
- *record = trace_buf[idx];
- record->event &= ~TRACE_RECORD_VALID;
+ /* read the record header to know record length */
+ read_from_buffer(idx, rec_hdr, sizeof(TraceRecord));
+ *recordptr = g_malloc(record->length);
+ /* make a copy of record to avoid being overwritten */
+ read_from_buffer(idx, (uint8_t *)*recordptr, record->length);
+ smp_rmb(); /* memory barrier before clearing valid flag */
+ (*recordptr)->event &= ~TRACE_RECORD_VALID;
+ /* clear the trace buffer range for consumed record otherwise any byte
+ * with its MSB set may be considered as a valid event id when the writer
+ * thread crosses this range of buffer again.
+ */
+ clear_buffer_range(idx, record->length);
return true;
}
@@ -120,29 +156,41 @@ static void wait_for_trace_records_available(void)
static gpointer writeout_thread(gpointer opaque)
{
- TraceRecord record;
- unsigned int writeout_idx = 0;
- unsigned int num_available, idx;
+ TraceRecord *recordptr, *dropped_ptr;
+ union {
+ TraceRecord rec;
+ uint8_t bytes[sizeof(TraceRecord) + sizeof(uint64_t)];
+ } dropped;
+ dropped_ptr = (TraceRecord *)dropped.bytes;
+ unsigned int idx = 0;
+ uint64_t dropped_count;
size_t unused __attribute__ ((unused));
for (;;) {
wait_for_trace_records_available();
- num_available = trace_idx - writeout_idx;
- if (num_available > TRACE_BUF_LEN) {
- record = (TraceRecord){
- .event = DROPPED_EVENT_ID,
- .x1 = num_available,
- };
- unused = fwrite(&record, sizeof(record), 1, trace_fp);
- writeout_idx += num_available;
+ if (dropped_events) {
+ dropped_ptr->event = DROPPED_EVENT_ID,
+ dropped_ptr->timestamp_ns = get_clock();
+ dropped_ptr->length = sizeof(TraceRecord) + sizeof(dropped_events),
+ dropped_ptr->reserved = 0;
+ while (1) {
+ dropped_count = dropped_events;
+ smp_rmb();
+ if (g_atomic_int_compare_and_exchange((gint *)&dropped_events,
+ dropped_count, 0)) {
+ break;
+ }
+ }
+ memcpy(dropped_ptr->arguments, &dropped_count, sizeof(uint64_t));
+ unused = fwrite(dropped_ptr, dropped_ptr->length, 1, trace_fp);
}
- idx = writeout_idx % TRACE_BUF_LEN;
- while (get_trace_record(idx, &record)) {
- trace_buf[idx].event = 0; /* clear valid bit */
- unused = fwrite(&record, sizeof(record), 1, trace_fp);
- idx = ++writeout_idx % TRACE_BUF_LEN;
+ while (get_trace_record(idx, &recordptr)) {
+ unused = fwrite(recordptr, recordptr->length, 1, trace_fp);
+ writeout_idx += recordptr->length;
+ g_free(recordptr);
+ idx = writeout_idx % TRACE_BUF_LEN;
}
fflush(trace_fp);
@@ -150,73 +198,94 @@ static gpointer writeout_thread(gpointer opaque)
return NULL;
}
-static void trace(TraceEventID event, uint64_t x1, uint64_t x2, uint64_t x3,
- uint64_t x4, uint64_t x5, uint64_t x6)
+void trace_record_write_u64(TraceBufferRecord *rec, uint64_t val)
{
- unsigned int idx;
- uint64_t timestamp;
-
- if (!trace_list[event].state) {
- return;
- }
-
- timestamp = get_clock();
-#if GLIB_CHECK_VERSION(2, 30, 0)
- idx = g_atomic_int_add((gint *)&trace_idx, 1) % TRACE_BUF_LEN;
-#else
- idx = g_atomic_int_exchange_and_add((gint *)&trace_idx, 1) % TRACE_BUF_LEN;
-#endif
- trace_buf[idx] = (TraceRecord){
- .event = event,
- .timestamp_ns = timestamp,
- .x1 = x1,
- .x2 = x2,
- .x3 = x3,
- .x4 = x4,
- .x5 = x5,
- .x6 = x6,
- };
- __sync_synchronize(); /* write barrier before marking as valid */
- trace_buf[idx].event |= TRACE_RECORD_VALID;
-
- if ((idx + 1) % TRACE_BUF_FLUSH_THRESHOLD == 0) {
- flush_trace_file(false);
- }
+ rec->rec_off = write_to_buffer(rec->rec_off, (uint8_t *)&val, sizeof(uint64_t));
}
-void trace0(TraceEventID event)
+void trace_record_write_str(TraceBufferRecord *rec, const char *s)
{
- trace(event, 0, 0, 0, 0, 0, 0);
+ /* Write string length first */
+ uint32_t slen = ((s == NULL ? 0 : strlen(s)) % MAX_TRACE_STRLEN);
+ rec->rec_off = write_to_buffer(rec->rec_off, (uint8_t *)&slen, sizeof(slen));
+ /* Write actual string now */
+ rec->rec_off = write_to_buffer(rec->rec_off, (uint8_t *)s, slen);
}
-void trace1(TraceEventID event, uint64_t x1)
+int trace_record_start(TraceBufferRecord *rec, TraceEventID event, size_t datasize)
{
- trace(event, x1, 0, 0, 0, 0, 0);
-}
+ unsigned int idx, rec_off, old_idx, new_idx;
+ uint32_t rec_len = sizeof(TraceRecord) + datasize;
+ uint64_t timestamp_ns = get_clock();
+
+ while (1) {
+ old_idx = trace_idx;
+ smp_rmb();
+ new_idx = old_idx + rec_len;
+
+ if (new_idx - writeout_idx > TRACE_BUF_LEN) {
+ /* Trace Buffer Full, Event dropped ! */
+ g_atomic_int_inc((gint *)&dropped_events);
+ return -ENOSPC;
+ }
-void trace2(TraceEventID event, uint64_t x1, uint64_t x2)
-{
- trace(event, x1, x2, 0, 0, 0, 0);
-}
+ if (g_atomic_int_compare_and_exchange((gint *)&trace_idx,
+ old_idx, new_idx)) {
+ break;
+ }
+ }
-void trace3(TraceEventID event, uint64_t x1, uint64_t x2, uint64_t x3)
-{
- trace(event, x1, x2, x3, 0, 0, 0);
+ idx = old_idx % TRACE_BUF_LEN;
+ /* To check later if threshold crossed */
+ rec->next_tbuf_idx = new_idx % TRACE_BUF_LEN;
+
+ rec_off = idx;
+ rec_off = write_to_buffer(rec_off, (uint8_t*)&event, sizeof(event));
+ rec_off = write_to_buffer(rec_off, (uint8_t*)×tamp_ns, sizeof(timestamp_ns));
+ rec_off = write_to_buffer(rec_off, (uint8_t*)&rec_len, sizeof(rec_len));
+
+ rec->tbuf_idx = idx;
+ rec->rec_off = (idx + sizeof(TraceRecord)) % TRACE_BUF_LEN;
+ return 0;
}
-void trace4(TraceEventID event, uint64_t x1, uint64_t x2, uint64_t x3, uint64_t x4)
+static void read_from_buffer(unsigned int idx, void *dataptr, size_t size)
{
- trace(event, x1, x2, x3, x4, 0, 0);
+ uint8_t *data_ptr = dataptr;
+ uint32_t x = 0;
+ while (x < size) {
+ if (idx >= TRACE_BUF_LEN) {
+ idx = idx % TRACE_BUF_LEN;
+ }
+ data_ptr[x++] = trace_buf[idx++];
+ }
}
-void trace5(TraceEventID event, uint64_t x1, uint64_t x2, uint64_t x3, uint64_t x4, uint64_t x5)
+static unsigned int write_to_buffer(unsigned int idx, void *dataptr, size_t size)
{
- trace(event, x1, x2, x3, x4, x5, 0);
+ uint8_t *data_ptr = dataptr;
+ uint32_t x = 0;
+ while (x < size) {
+ if (idx >= TRACE_BUF_LEN) {
+ idx = idx % TRACE_BUF_LEN;
+ }
+ trace_buf[idx++] = data_ptr[x++];
+ }
+ return idx; /* most callers wants to know where to write next */
}
-void trace6(TraceEventID event, uint64_t x1, uint64_t x2, uint64_t x3, uint64_t x4, uint64_t x5, uint64_t x6)
+void trace_record_finish(TraceBufferRecord *rec)
{
- trace(event, x1, x2, x3, x4, x5, x6);
+ uint8_t temp_rec[sizeof(TraceRecord)];
+ TraceRecord *record = (TraceRecord *) temp_rec;
+ read_from_buffer(rec->tbuf_idx, temp_rec, sizeof(TraceRecord));
+ __sync_synchronize(); /* write barrier before marking as valid */
+ record->event |= TRACE_RECORD_VALID;
+ write_to_buffer(rec->tbuf_idx, temp_rec, sizeof(TraceRecord));
+
+ if ((trace_idx - writeout_idx) > TRACE_BUF_FLUSH_THRESHOLD) {
+ flush_trace_file(false);
+ }
}
void st_set_trace_file_enabled(bool enable)
@@ -231,10 +300,11 @@ void st_set_trace_file_enabled(bool enable)
flush_trace_file(true);
if (enable) {
- static const TraceRecord header = {
- .event = HEADER_EVENT_ID,
- .timestamp_ns = HEADER_MAGIC,
- .x1 = HEADER_VERSION,
+ static const TraceRecordHeader header = {
+ .header_event_id = HEADER_EVENT_ID,
+ .header_magic = HEADER_MAGIC,
+ /* Older log readers will check for version at next location */
+ .header_version = HEADER_VERSION,
};
trace_fp = fopen(trace_file_name, "wb");
diff --git a/trace/simple.h b/trace/simple.h
index 6b5358c..b9818ff 100644
--- a/trace/simple.h
+++ b/trace/simple.h
@@ -22,16 +22,41 @@ typedef struct {
bool state;
} TraceEvent;
-void trace0(TraceEventID event);
-void trace1(TraceEventID event, uint64_t x1);
-void trace2(TraceEventID event, uint64_t x1, uint64_t x2);
-void trace3(TraceEventID event, uint64_t x1, uint64_t x2, uint64_t x3);
-void trace4(TraceEventID event, uint64_t x1, uint64_t x2, uint64_t x3, uint64_t x4);
-void trace5(TraceEventID event, uint64_t x1, uint64_t x2, uint64_t x3, uint64_t x4, uint64_t x5);
-void trace6(TraceEventID event, uint64_t x1, uint64_t x2, uint64_t x3, uint64_t x4, uint64_t x5, uint64_t x6);
void st_print_trace_file_status(FILE *stream, fprintf_function stream_printf);
void st_set_trace_file_enabled(bool enable);
bool st_set_trace_file(const char *file);
void st_flush_trace_buffer(void);
+typedef struct {
+ unsigned int tbuf_idx;
+ unsigned int next_tbuf_idx;
+ unsigned int rec_off;
+} TraceBufferRecord;
+
+/* Note for hackers: Make sure MAX_TRACE_LEN < sizeof(uint32_t) */
+#define MAX_TRACE_STRLEN 512
+/**
+ * Initialize a trace record and claim space for it in the buffer
+ *
+ * @arglen number of bytes required for arguments
+ */
+int trace_record_start(TraceBufferRecord *rec, TraceEventID id, size_t arglen);
+
+/**
+ * Append a 64-bit argument to a trace record
+ */
+void trace_record_write_u64(TraceBufferRecord *rec, uint64_t val);
+
+/**
+ * Append a string argument to a trace record
+ */
+void trace_record_write_str(TraceBufferRecord *rec, const char *s);
+
+/**
+ * Mark a trace record completed
+ *
+ * Don't append any more arguments to the trace record after calling this.
+ */
+void trace_record_finish(TraceBufferRecord *rec);
+
#endif /* TRACE_SIMPLE_H */
--
1.7.10.4
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [Qemu-devel] [PATCH v7 3/3] Update simpletrace.py for new log format
2012-07-03 9:20 [Qemu-devel] [PATCH v7 0/3] Simpletrace v2: Support multiple args, strings Harsh Prateek Bora
2012-07-03 9:20 ` [Qemu-devel] [PATCH v7 1/3] monitor: remove unused do_info_trace Harsh Prateek Bora
2012-07-03 9:20 ` [Qemu-devel] [PATCH v7 2/3] Simpletrace v2: Support multiple arguments, strings Harsh Prateek Bora
@ 2012-07-03 9:20 ` Harsh Prateek Bora
2012-07-17 14:07 ` [Qemu-devel] [PATCH v7 0/3] Simpletrace v2: Support multiple args, strings Stefan Hajnoczi
2012-07-17 15:23 ` Stefan Hajnoczi
4 siblings, 0 replies; 14+ messages in thread
From: Harsh Prateek Bora @ 2012-07-03 9:20 UTC (permalink / raw)
To: stefanha, qemu-devel, stefanha; +Cc: Harsh Prateek Bora
Support new tracelog format for multiple arguments and strings.
Signed-off-by: Harsh Prateek Bora <harsh@linux.vnet.ibm.com>
---
scripts/simpletrace.py | 116 +++++++++++++++++++++++++++++++-----------------
1 file changed, 75 insertions(+), 41 deletions(-)
diff --git a/scripts/simpletrace.py b/scripts/simpletrace.py
index f55e5e6..9b4419f 100755
--- a/scripts/simpletrace.py
+++ b/scripts/simpletrace.py
@@ -12,53 +12,69 @@
import struct
import re
import inspect
+from tracetool import _read_events, Event
+from tracetool.backend.simple import is_string
header_event_id = 0xffffffffffffffff
header_magic = 0xf2b177cb0aa429b4
-header_version = 0
dropped_event_id = 0xfffffffffffffffe
-trace_fmt = '=QQQQQQQQ'
-trace_len = struct.calcsize(trace_fmt)
-event_re = re.compile(r'(disable\s+)?([a-zA-Z0-9_]+)\(([^)]*)\).*')
+log_header_fmt = '=QQQ'
+rec_header_fmt = '=QQII'
-def parse_events(fobj):
- """Parse a trace-events file into {event_num: (name, arg1, ...)}."""
-
- def get_argnames(args):
- """Extract argument names from a parameter list."""
- return tuple(arg.split()[-1].lstrip('*') for arg in args.split(','))
-
- events = {dropped_event_id: ('dropped', 'count')}
- event_num = 0
- for line in fobj:
- m = event_re.match(line.strip())
- if m is None:
- continue
-
- disable, name, args = m.groups()
- events[event_num] = (name,) + get_argnames(args)
- event_num += 1
- return events
+def read_header(fobj, hfmt):
+ '''Read a trace record header'''
+ hlen = struct.calcsize(hfmt)
+ hdr = fobj.read(hlen)
+ if len(hdr) != hlen:
+ return None
+ return struct.unpack(hfmt, hdr)
-def read_record(fobj):
+def get_record(edict, rechdr, fobj):
"""Deserialize a trace record from a file into a tuple (event_num, timestamp, arg1, ..., arg6)."""
- s = fobj.read(trace_len)
- if len(s) != trace_len:
+ if rechdr is None:
return None
- return struct.unpack(trace_fmt, s)
+ rec = (rechdr[0], rechdr[1])
+ if rechdr[0] != dropped_event_id:
+ event_id = rechdr[0]
+ event = edict[event_id]
+ for type, name in event.args:
+ if is_string(type):
+ l = fobj.read(4)
+ (len,) = struct.unpack('=L', l)
+ s = fobj.read(len)
+ rec = rec + (s,)
+ else:
+ (value,) = struct.unpack('=Q', fobj.read(8))
+ rec = rec + (value,)
+ else:
+ (value,) = struct.unpack('=Q', fobj.read(8))
+ rec = rec + (value,)
+ return rec
+
+
+def read_record(edict, fobj):
+ """Deserialize a trace record from a file into a tuple (event_num, timestamp, arg1, ..., arg6)."""
+ rechdr = read_header(fobj, rec_header_fmt)
+ return get_record(edict, rechdr, fobj) # return tuple of record elements
-def read_trace_file(fobj):
+def read_trace_file(edict, fobj):
"""Deserialize trace records from a file, yielding record tuples (event_num, timestamp, arg1, ..., arg6)."""
- header = read_record(fobj)
+ header = read_header(fobj, log_header_fmt)
if header is None or \
header[0] != header_event_id or \
- header[1] != header_magic or \
- header[2] != header_version:
- raise ValueError('not a trace file or incompatible version')
+ header[1] != header_magic:
+ raise ValueError('Not a valid trace file!')
+ if header[2] != 0 and \
+ header[2] != 2:
+ raise ValueError('Unknown version of tracelog format!')
+
+ log_version = header[2]
+ if log_version == 0:
+ raise ValueError('Older log format, not supported with this Qemu release!')
while True:
- rec = read_record(fobj)
+ rec = read_record(edict, fobj)
if rec is None:
break
@@ -89,16 +105,29 @@ class Analyzer(object):
def process(events, log, analyzer):
"""Invoke an analyzer on each event in a log."""
if isinstance(events, str):
- events = parse_events(open(events, 'r'))
+ events = _read_events(open(events, 'r'))
if isinstance(log, str):
log = open(log, 'rb')
+ enabled_events = []
+ dropped_event = Event.build("Dropped_Event(uint64_t num_events_dropped)")
+ edict = {dropped_event_id: dropped_event}
+
+ for e in events:
+ if 'disable' not in e.properties:
+ enabled_events.append(e)
+ for num, event in enumerate(enabled_events):
+ edict[num] = event
+
def build_fn(analyzer, event):
- fn = getattr(analyzer, event[0], None)
+ if isinstance(event, str):
+ return analyzer.catchall
+
+ fn = getattr(analyzer, event.name, None)
if fn is None:
return analyzer.catchall
- event_argcount = len(event) - 1
+ event_argcount = len(event.args)
fn_argcount = len(inspect.getargspec(fn)[0]) - 1
if fn_argcount == event_argcount + 1:
# Include timestamp as first argument
@@ -109,9 +138,9 @@ def process(events, log, analyzer):
analyzer.begin()
fn_cache = {}
- for rec in read_trace_file(log):
+ for rec in read_trace_file(edict, log):
event_num = rec[0]
- event = events[event_num]
+ event = edict[event_num]
if event_num not in fn_cache:
fn_cache[event_num] = build_fn(analyzer, event)
fn_cache[event_num](event, rec)
@@ -128,7 +157,7 @@ def run(analyzer):
sys.stderr.write('usage: %s <trace-events> <trace-file>\n' % sys.argv[0])
sys.exit(1)
- events = parse_events(open(sys.argv[1], 'r'))
+ events = _read_events(open(sys.argv[1], 'r'))
process(events, sys.argv[2], analyzer)
if __name__ == '__main__':
@@ -137,15 +166,20 @@ if __name__ == '__main__':
self.last_timestamp = None
def catchall(self, event, rec):
+ i = 1
timestamp = rec[1]
if self.last_timestamp is None:
self.last_timestamp = timestamp
delta_ns = timestamp - self.last_timestamp
self.last_timestamp = timestamp
- fields = [event[0], '%0.3f' % (delta_ns / 1000.0)]
- for i in xrange(1, len(event)):
- fields.append('%s=0x%x' % (event[i], rec[i + 1]))
+ fields = [event.name, '%0.3f' % (delta_ns / 1000.0)]
+ for type, name in event.args:
+ if is_string(type):
+ fields.append('%s=%s' % (name, rec[i + 1]))
+ else:
+ fields.append('%s=0x%x' % (name, rec[i + 1]))
+ i += 1
print ' '.join(fields)
run(Formatter())
--
1.7.10.4
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH v7 0/3] Simpletrace v2: Support multiple args, strings.
2012-07-03 9:20 [Qemu-devel] [PATCH v7 0/3] Simpletrace v2: Support multiple args, strings Harsh Prateek Bora
` (2 preceding siblings ...)
2012-07-03 9:20 ` [Qemu-devel] [PATCH v7 3/3] Update simpletrace.py for new log format Harsh Prateek Bora
@ 2012-07-17 14:07 ` Stefan Hajnoczi
2012-07-17 14:14 ` Stefan Hajnoczi
2012-07-17 15:23 ` Stefan Hajnoczi
4 siblings, 1 reply; 14+ messages in thread
From: Stefan Hajnoczi @ 2012-07-17 14:07 UTC (permalink / raw)
To: Harsh Prateek Bora; +Cc: qemu-devel, stefanha
On Tue, Jul 3, 2012 at 10:20 AM, Harsh Prateek Bora
<harsh@linux.vnet.ibm.com> wrote:
> Existing simpletrace backend allows to trace at max 6 args and does not
> support strings. This newer tracelog format gets rid of fixed size records
> and therefore allows to trace variable number of args including strings.
>
> Sample trace:
> v9fs_version 0.000 tag=0xffff id=0x64 msize=0x2000 version=9P2000.L
> v9fs_version_return 6.705 tag=0xffff id=0x64 msize=0x2000 version=9P2000.L
> v9fs_attach 174.467 tag=0x1 id=0x68 fid=0x0 afid=0xffffffffffffffff
> uname=nobody aname=
> v9fs_attach_return 4720.454 tag=0x1 id=0x68 type=0xffffffffffffff80
> version=0x4f2a4dd0 path=0x220ea6
The writeout thread deadlocked itself when it called g_malloc() and
therefore became a tracing thread:
Thread 3 (Thread 0x7fad8b144700 (LWP 1387)):
#0 0x00007ffffa1e88de in ?? ()
#1 0x00007fad9421de86 in *__GI_clock_gettime
(clock_id=clock_id@entry=1, tp=tp@entry=0x7fad8b143be0) at
../sysdeps/unix/clock_gettime.c:116
#2 0x00007fad94813e46 in get_clock () at ./qemu-timer.h:124
#3 trace_record_start (rec=rec@entry=0x7fad8b143c10,
event=140383339428832, event@entry=0, datasize=datasize@entry=16) at
trace/simple.c:219
#4 0x00007fad947f7185 in trace_g_malloc (size=size@entry=32,
ptr=ptr@entry=0x7fad340008b0) at trace.c:770
#5 0x00007fad947b5754 in malloc_and_trace (n_bytes=32) at
/home/stefanha/qemu/vl.c:2240
#6 0x00007fad93d70de1 in g_malloc (n_bytes=n_bytes@entry=32) at
/tmp/buildd/glib2.0-2.32.3/./glib/gmem.c:159
#7 0x00007fad94813bce in get_trace_record (recordptr=<synthetic
pointer>, idx=56943) at trace/simple.c:114
#8 writeout_thread (opaque=<optimized out>) at trace/simple.c:189
#9 0x00007fad93d8ddf5 in g_thread_proxy (data=0x7fad96738800) at
/tmp/buildd/glib2.0-2.32.3/./glib/gthread.c:801
#10 0x00007fad90ed7b50 in start_thread (arg=<optimized out>) at
pthread_create.c:304
#11 0x00007fad90c226dd in clone () at
../sysdeps/unix/sysv/linux/x86_64/clone.S:112
#12 0x0000000000000000 in ?? ()
Stefan
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH v7 0/3] Simpletrace v2: Support multiple args, strings.
2012-07-17 14:07 ` [Qemu-devel] [PATCH v7 0/3] Simpletrace v2: Support multiple args, strings Stefan Hajnoczi
@ 2012-07-17 14:14 ` Stefan Hajnoczi
0 siblings, 0 replies; 14+ messages in thread
From: Stefan Hajnoczi @ 2012-07-17 14:14 UTC (permalink / raw)
To: Harsh Prateek Bora; +Cc: qemu-devel, stefanha
On Tue, Jul 17, 2012 at 3:07 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Tue, Jul 3, 2012 at 10:20 AM, Harsh Prateek Bora
> <harsh@linux.vnet.ibm.com> wrote:
>> Existing simpletrace backend allows to trace at max 6 args and does not
>> support strings. This newer tracelog format gets rid of fixed size records
>> and therefore allows to trace variable number of args including strings.
>>
>> Sample trace:
>> v9fs_version 0.000 tag=0xffff id=0x64 msize=0x2000 version=9P2000.L
>> v9fs_version_return 6.705 tag=0xffff id=0x64 msize=0x2000 version=9P2000.L
>> v9fs_attach 174.467 tag=0x1 id=0x68 fid=0x0 afid=0xffffffffffffffff
>> uname=nobody aname=
>> v9fs_attach_return 4720.454 tag=0x1 id=0x68 type=0xffffffffffffff80
>> version=0x4f2a4dd0 path=0x220ea6
>
> The writeout thread deadlocked itself when it called g_malloc() and
> therefore became a tracing thread:
>
> Thread 3 (Thread 0x7fad8b144700 (LWP 1387)):
> #0 0x00007ffffa1e88de in ?? ()
> #1 0x00007fad9421de86 in *__GI_clock_gettime
> (clock_id=clock_id@entry=1, tp=tp@entry=0x7fad8b143be0) at
> ../sysdeps/unix/clock_gettime.c:116
> #2 0x00007fad94813e46 in get_clock () at ./qemu-timer.h:124
> #3 trace_record_start (rec=rec@entry=0x7fad8b143c10,
> event=140383339428832, event@entry=0, datasize=datasize@entry=16) at
> trace/simple.c:219
> #4 0x00007fad947f7185 in trace_g_malloc (size=size@entry=32,
> ptr=ptr@entry=0x7fad340008b0) at trace.c:770
> #5 0x00007fad947b5754 in malloc_and_trace (n_bytes=32) at
> /home/stefanha/qemu/vl.c:2240
> #6 0x00007fad93d70de1 in g_malloc (n_bytes=n_bytes@entry=32) at
> /tmp/buildd/glib2.0-2.32.3/./glib/gmem.c:159
> #7 0x00007fad94813bce in get_trace_record (recordptr=<synthetic
> pointer>, idx=56943) at trace/simple.c:114
> #8 writeout_thread (opaque=<optimized out>) at trace/simple.c:189
> #9 0x00007fad93d8ddf5 in g_thread_proxy (data=0x7fad96738800) at
> /tmp/buildd/glib2.0-2.32.3/./glib/gthread.c:801
> #10 0x00007fad90ed7b50 in start_thread (arg=<optimized out>) at
> pthread_create.c:304
> #11 0x00007fad90c226dd in clone () at
> ../sysdeps/unix/sysv/linux/x86_64/clone.S:112
> #12 0x0000000000000000 in ?? ()
Here's a fix, in order to avoid reentrancy use raw malloc(3) and free(3):
diff --git a/trace/simple.c b/trace/simple.c
index 3c14568..5a4d607 100644
--- a/trace/simple.c
+++ b/trace/simple.c
@@ -111,7 +111,7 @@ static bool get_trace_record(unsigned int idx,
TraceRecord **recordptr)
__sync_synchronize(); /* read memory barrier before accessing record */
/* read the record header to know record length */
read_from_buffer(idx, rec_hdr, sizeof(TraceRecord));
- *recordptr = g_malloc(record->length);
+ *recordptr = malloc(record->length);
/* make a copy of record to avoid being overwritten */
read_from_buffer(idx, (uint8_t *)*recordptr, record->length);
smp_rmb(); /* memory barrier before clearing valid flag */
@@ -189,7 +189,7 @@ static gpointer writeout_thread(gpointer opaque)
while (get_trace_record(idx, &recordptr)) {
unused = fwrite(recordptr, recordptr->length, 1, trace_fp);
writeout_idx += recordptr->length;
- g_free(recordptr);
+ free(recordptr);
idx = writeout_idx % TRACE_BUF_LEN;
}
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH v7 2/3] Simpletrace v2: Support multiple arguments, strings.
2012-07-03 9:20 ` [Qemu-devel] [PATCH v7 2/3] Simpletrace v2: Support multiple arguments, strings Harsh Prateek Bora
@ 2012-07-17 15:21 ` Stefan Hajnoczi
2012-07-17 19:01 ` Harsh Bora
0 siblings, 1 reply; 14+ messages in thread
From: Stefan Hajnoczi @ 2012-07-17 15:21 UTC (permalink / raw)
To: Harsh Prateek Bora; +Cc: qemu-devel, stefanha
On Tue, Jul 3, 2012 at 10:20 AM, Harsh Prateek Bora
<harsh@linux.vnet.ibm.com> wrote:
> Existing simpletrace backend allows to trace at max 6 args and does not
> support strings. This newer tracelog format gets rid of fixed size records
> and therefore allows to trace variable number of args including strings.
>
> Sample trace with strings:
> v9fs_version 0.000 tag=0xffff id=0x64 msize=0x2000 version=9P2000.L
> v9fs_version_return 6.705 tag=0xffff id=0x64 msize=0x2000 version=9P2000.L
>
> Signed-off-by: Harsh Prateek Bora <harsh@linux.vnet.ibm.com>
> ---
> scripts/tracetool/backend/simple.py | 84 +++++++++---
> trace/simple.c | 256 ++++++++++++++++++++++-------------
> trace/simple.h | 39 +++++-
> 3 files changed, 260 insertions(+), 119 deletions(-)
>
> diff --git a/scripts/tracetool/backend/simple.py b/scripts/tracetool/backend/simple.py
> index fbb5717..d3cf4da 100644
> --- a/scripts/tracetool/backend/simple.py
> +++ b/scripts/tracetool/backend/simple.py
> @@ -15,9 +15,16 @@ __email__ = "stefanha@linux.vnet.ibm.com"
>
> from tracetool import out
>
> +def is_string(arg):
> + strtype = ('const char*', 'char*', 'const char *', 'char *')
> + if arg.lstrip().startswith(strtype):
> + return True
> + else:
> + return False
>
> def c(events):
> out('#include "trace.h"',
> + '#include "trace/simple.h"',
> '',
> 'TraceEvent trace_list[] = {')
>
> @@ -26,30 +33,69 @@ def c(events):
> name = e.name,
> )
>
> - out('};')
> -
> -def h(events):
> - out('#include "trace/simple.h"',
> + out('};',
> '')
>
> - for num, e in enumerate(events):
> - if len(e.args):
> - argstr = e.args.names()
> - arg_prefix = ', (uint64_t)(uintptr_t)'
> - cast_args = arg_prefix + arg_prefix.join(argstr)
> - simple_args = (str(num) + cast_args)
> - else:
> - simple_args = str(num)
> + for num, event in enumerate(events):
> + sizes = []
> + for type_, name in event.args:
> + if is_string(type_):
> + sizes.append("4 + ((" + name + " ? strlen(" + name + ") : 0) % MAX_TRACE_STRLEN)")
trace_record_write_str() and this code both use % to truncate the
string. If the string is 512 characters long you get an empty string.
That's weird and not normally how truncation works.
Perhaps it's better to change this Python code to emit something like:
size_t arg%(num)d_len = %(name)s ? MAX(strlen(%(name)s, MAX_TRACE_STRLEN)) : 0;
Then you can emit the following to actual store the string:
out(' trace_record_write_str(&rec, %(name)s, arg%(num)d_len);',
name=name, num=num)
The difference is that MAX() is used to truncate and we only do strlen() once.
> + else:
> + sizes.append("8")
> + sizestr = " + ".join(sizes)
> + if len(event.args) == 0:
> + sizestr = '0'
>
> - out('static inline void trace_%(name)s(%(args)s)',
> + out('void trace_%(name)s(%(args)s)',
> '{',
> - ' trace%(argc)d(%(trace_args)s);',
> - '}',
> - name = e.name,
> - args = e.args,
> - argc = len(e.args),
> - trace_args = simple_args,
> + ' TraceBufferRecord rec;',
> + '',
> + ' if (!trace_list[%(event_id)s].state) {',
> + ' return;',
> + ' }',
> + '',
> + ' if (trace_record_start(&rec, %(event_id)s, %(size_str)s)) {',
> + ' return; /* Trace Buffer Full, Event Dropped ! */',
> + ' }',
> + name = event.name,
> + args = event.args,
> + event_id = num,
> + size_str = sizestr,
> )
>
> + if len(event.args) > 0:
> + for type_, name in event.args:
> + # string
> + if is_string(type_):
> + out(' trace_record_write_str(&rec, %(name)s);',
> + name = name,
> + )
> + # pointer var (not string)
> + elif type_.endswith('*'):
> + out(' trace_record_write_u64(&rec, (uint64_t)(uint64_t *)%(name)s);',
> + name = name,
> + )
> + # primitive data type
> + else:
> + out(' trace_record_write_u64(&rec, (uint64_t)%(name)s);',
> + name = name,
> + )
> +
> + out(' trace_record_finish(&rec);',
> + '}',
> + '')
> +
> +
> +def h(events):
> + out('#include "trace/simple.h"',
> + '')
> +
> + for event in events:
> + out('void trace_%(name)s(%(args)s);',
> + name = event.name,
> + args = event.args,
> + )
> + out('')
> out('#define NR_TRACE_EVENTS %d' % len(events))
> out('extern TraceEvent trace_list[NR_TRACE_EVENTS];')
> diff --git a/trace/simple.c b/trace/simple.c
> index b64bcf4..3c14568 100644
> --- a/trace/simple.c
> +++ b/trace/simple.c
> @@ -27,7 +27,7 @@
> #define HEADER_MAGIC 0xf2b177cb0aa429b4ULL
>
> /** Trace file version number, bump if format changes */
> -#define HEADER_VERSION 0
> +#define HEADER_VERSION 2
>
> /** Records were dropped event ID */
> #define DROPPED_EVENT_ID (~(uint64_t)0 - 1)
> @@ -35,23 +35,6 @@
> /** Trace record is valid */
> #define TRACE_RECORD_VALID ((uint64_t)1 << 63)
>
> -/** Trace buffer entry */
> -typedef struct {
> - uint64_t event;
> - uint64_t timestamp_ns;
> - uint64_t x1;
> - uint64_t x2;
> - uint64_t x3;
> - uint64_t x4;
> - uint64_t x5;
> - uint64_t x6;
> -} TraceRecord;
> -
> -enum {
> - TRACE_BUF_LEN = 4096,
> - TRACE_BUF_FLUSH_THRESHOLD = TRACE_BUF_LEN / 4,
> -};
> -
> /*
> * Trace records are written out by a dedicated thread. The thread waits for
> * records to become available, writes them out, and then waits again.
> @@ -62,11 +45,49 @@ static GCond *trace_empty_cond;
> static bool trace_available;
> static bool trace_writeout_enabled;
>
> -static TraceRecord trace_buf[TRACE_BUF_LEN];
> +enum {
> + TRACE_BUF_LEN = 4096 * 64,
> + TRACE_BUF_FLUSH_THRESHOLD = TRACE_BUF_LEN / 4,
> +};
> +
> +uint8_t trace_buf[TRACE_BUF_LEN];
> static unsigned int trace_idx;
> +static unsigned int writeout_idx;
> +static uint64_t dropped_events;
> static FILE *trace_fp;
> static char *trace_file_name = NULL;
>
> +/* * Trace buffer entry */
> +typedef struct {
> + uint64_t event; /* TraceEventID */
> + uint64_t timestamp_ns;
> + uint32_t length; /* in bytes */
> + uint32_t reserved; /* unused */
> + uint8_t arguments[];
> +} TraceRecord;
> +
> +typedef struct {
> + uint64_t header_event_id; /* HEADER_EVENT_ID */
> + uint64_t header_magic; /* HEADER_MAGIC */
> + uint64_t header_version; /* HEADER_VERSION */
> +} TraceRecordHeader;
> +
> +
> +static void read_from_buffer(unsigned int idx, void *dataptr, size_t size);
> +static unsigned int write_to_buffer(unsigned int idx, void *dataptr, size_t size);
> +static void clear_buffer_range(unsigned int idx, size_t len);
clear_buffer_range() is defined on the next line, no need for a prototype.
> +static void clear_buffer_range(unsigned int idx, size_t len)
> +{
> + uint32_t num = 0;
> + while (num < len) {
> + if (idx >= TRACE_BUF_LEN) {
> + idx = idx % TRACE_BUF_LEN;
> + }
> + trace_buf[idx++] = 0;
> + num++;
> + }
> +}
> /**
> * Read a trace record from the trace buffer
> *
> @@ -75,16 +96,31 @@ static char *trace_file_name = NULL;
> *
> * Returns false if the record is not valid.
> */
> -static bool get_trace_record(unsigned int idx, TraceRecord *record)
> +static bool get_trace_record(unsigned int idx, TraceRecord **recordptr)
> {
> - if (!(trace_buf[idx].event & TRACE_RECORD_VALID)) {
> + uint8_t rec_hdr[sizeof(TraceRecord)];
> + uint64_t event_flag = 0;
> + TraceRecord *record = (TraceRecord *) rec_hdr;
Declaring rec_hdr as a uint8_t array is only because you're trying to
avoid a cast later? The easiest way to make this nice is to do what
memset(), memcpy() and friends do: just use a void *buf argument.
That way a caller can pass a TraceRecord* or any other pointer without
explicit casts, unions, or the uint8_t array trick you are using.
> + /* read the event flag to see if its a valid record */
> + read_from_buffer(idx, rec_hdr, sizeof(event_flag));
> +
> + if (!(record->event & TRACE_RECORD_VALID)) {
> return false;
> }
>
> __sync_synchronize(); /* read memory barrier before accessing record */
> -
> - *record = trace_buf[idx];
> - record->event &= ~TRACE_RECORD_VALID;
> + /* read the record header to know record length */
> + read_from_buffer(idx, rec_hdr, sizeof(TraceRecord));
> + *recordptr = g_malloc(record->length);
> + /* make a copy of record to avoid being overwritten */
> + read_from_buffer(idx, (uint8_t *)*recordptr, record->length);
> + smp_rmb(); /* memory barrier before clearing valid flag */
You are adding smp_*() instead of __sync_synchronize(). Please either
continue to use __sync_synchronize() or convert everything to smp_*()
barriers from qemu-barrier.h.
> + (*recordptr)->event &= ~TRACE_RECORD_VALID;
> + /* clear the trace buffer range for consumed record otherwise any byte
> + * with its MSB set may be considered as a valid event id when the writer
> + * thread crosses this range of buffer again.
> + */
> + clear_buffer_range(idx, record->length);
> return true;
> }
>
> @@ -120,29 +156,41 @@ static void wait_for_trace_records_available(void)
>
> static gpointer writeout_thread(gpointer opaque)
> {
> - TraceRecord record;
> - unsigned int writeout_idx = 0;
> - unsigned int num_available, idx;
> + TraceRecord *recordptr, *dropped_ptr;
> + union {
> + TraceRecord rec;
> + uint8_t bytes[sizeof(TraceRecord) + sizeof(uint64_t)];
> + } dropped;
> + dropped_ptr = (TraceRecord *)dropped.bytes;
dropped_ptr isn't needed, just use dropped.rec.
> + unsigned int idx = 0;
> + uint64_t dropped_count;
> size_t unused __attribute__ ((unused));
>
> for (;;) {
> wait_for_trace_records_available();
>
> - num_available = trace_idx - writeout_idx;
> - if (num_available > TRACE_BUF_LEN) {
> - record = (TraceRecord){
> - .event = DROPPED_EVENT_ID,
> - .x1 = num_available,
> - };
> - unused = fwrite(&record, sizeof(record), 1, trace_fp);
> - writeout_idx += num_available;
> + if (dropped_events) {
> + dropped_ptr->event = DROPPED_EVENT_ID,
> + dropped_ptr->timestamp_ns = get_clock();
> + dropped_ptr->length = sizeof(TraceRecord) + sizeof(dropped_events),
> + dropped_ptr->reserved = 0;
> + while (1) {
> + dropped_count = dropped_events;
> + smp_rmb();
Not needed, glib says g_atomic_int_compare_and_exchange() "acts as a
full compiler and hardware memory barrier".
http://developer.gnome.org/glib/2.32/glib-Atomic-Operations.html#g-atomic-int-compare-and-exchange
> + if (g_atomic_int_compare_and_exchange((gint *)&dropped_events,
> + dropped_count, 0)) {
> + break;
> + }
> + }
It's a shame we have to loop when a simple atomic exchange would have
done the job better but glib doesn't seem to support that.
Stefan
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH v7 0/3] Simpletrace v2: Support multiple args, strings.
2012-07-03 9:20 [Qemu-devel] [PATCH v7 0/3] Simpletrace v2: Support multiple args, strings Harsh Prateek Bora
` (3 preceding siblings ...)
2012-07-17 14:07 ` [Qemu-devel] [PATCH v7 0/3] Simpletrace v2: Support multiple args, strings Stefan Hajnoczi
@ 2012-07-17 15:23 ` Stefan Hajnoczi
2012-07-17 19:51 ` Harsh Bora
4 siblings, 1 reply; 14+ messages in thread
From: Stefan Hajnoczi @ 2012-07-17 15:23 UTC (permalink / raw)
To: Harsh Prateek Bora; +Cc: qemu-devel, stefanha
On Tue, Jul 3, 2012 at 10:20 AM, Harsh Prateek Bora
<harsh@linux.vnet.ibm.com> wrote:
> Existing simpletrace backend allows to trace at max 6 args and does not
> support strings. This newer tracelog format gets rid of fixed size records
> and therefore allows to trace variable number of args including strings.
>
> Sample trace:
> v9fs_version 0.000 tag=0xffff id=0x64 msize=0x2000 version=9P2000.L
> v9fs_version_return 6.705 tag=0xffff id=0x64 msize=0x2000 version=9P2000.L
> v9fs_attach 174.467 tag=0x1 id=0x68 fid=0x0 afid=0xffffffffffffffff
> uname=nobody aname=
> v9fs_attach_return 4720.454 tag=0x1 id=0x68 type=0xffffffffffffff80
> version=0x4f2a4dd0 path=0x220ea6
I have successfully tested it with the fix that I posted. Writing
simpletrace.Analyzer Python scripts still works - now with string
arguments too :).
Besides the last few comments on Patch 2, this looks okay now.
Stefan
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH v7 2/3] Simpletrace v2: Support multiple arguments, strings.
2012-07-17 15:21 ` Stefan Hajnoczi
@ 2012-07-17 19:01 ` Harsh Bora
2012-07-17 20:08 ` Harsh Bora
2012-07-18 8:53 ` Stefan Hajnoczi
0 siblings, 2 replies; 14+ messages in thread
From: Harsh Bora @ 2012-07-17 19:01 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: qemu-devel, stefanha
On 07/17/2012 08:51 PM, Stefan Hajnoczi wrote:
> On Tue, Jul 3, 2012 at 10:20 AM, Harsh Prateek Bora
> <harsh@linux.vnet.ibm.com> wrote:
>> Existing simpletrace backend allows to trace at max 6 args and does not
>> support strings. This newer tracelog format gets rid of fixed size records
>> and therefore allows to trace variable number of args including strings.
>>
>> Sample trace with strings:
>> v9fs_version 0.000 tag=0xffff id=0x64 msize=0x2000 version=9P2000.L
>> v9fs_version_return 6.705 tag=0xffff id=0x64 msize=0x2000 version=9P2000.L
>>
>> Signed-off-by: Harsh Prateek Bora <harsh@linux.vnet.ibm.com>
>> ---
>> scripts/tracetool/backend/simple.py | 84 +++++++++---
>> trace/simple.c | 256 ++++++++++++++++++++++-------------
>> trace/simple.h | 39 +++++-
>> 3 files changed, 260 insertions(+), 119 deletions(-)
>>
>> diff --git a/scripts/tracetool/backend/simple.py b/scripts/tracetool/backend/simple.py
>> index fbb5717..d3cf4da 100644
>> --- a/scripts/tracetool/backend/simple.py
>> +++ b/scripts/tracetool/backend/simple.py
>> @@ -15,9 +15,16 @@ __email__ = "stefanha@linux.vnet.ibm.com"
>>
>> from tracetool import out
>>
>> +def is_string(arg):
>> + strtype = ('const char*', 'char*', 'const char *', 'char *')
>> + if arg.lstrip().startswith(strtype):
>> + return True
>> + else:
>> + return False
>>
>> def c(events):
>> out('#include "trace.h"',
>> + '#include "trace/simple.h"',
>> '',
>> 'TraceEvent trace_list[] = {')
>>
>> @@ -26,30 +33,69 @@ def c(events):
>> name = e.name,
>> )
>>
>> - out('};')
>> -
>> -def h(events):
>> - out('#include "trace/simple.h"',
>> + out('};',
>> '')
>>
>> - for num, e in enumerate(events):
>> - if len(e.args):
>> - argstr = e.args.names()
>> - arg_prefix = ', (uint64_t)(uintptr_t)'
>> - cast_args = arg_prefix + arg_prefix.join(argstr)
>> - simple_args = (str(num) + cast_args)
>> - else:
>> - simple_args = str(num)
>> + for num, event in enumerate(events):
>> + sizes = []
>> + for type_, name in event.args:
>> + if is_string(type_):
>> + sizes.append("4 + ((" + name + " ? strlen(" + name + ") : 0) % MAX_TRACE_STRLEN)")
>
> trace_record_write_str() and this code both use % to truncate the
> string. If the string is 512 characters long you get an empty string.
> That's weird and not normally how truncation works.
>
> Perhaps it's better to change this Python code to emit something like:
> size_t arg%(num)d_len = %(name)s ? MAX(strlen(%(name)s, MAX_TRACE_STRLEN)) : 0;
>
I think we need to use MIN instead of MAX, right ?
Another question below ..
> Then you can emit the following to actual store the string:
>
> out(' trace_record_write_str(&rec, %(name)s, arg%(num)d_len);',
> name=name, num=num)
>
> The difference is that MAX() is used to truncate and we only do strlen() once.
>
>> + else:
>> + sizes.append("8")
>> + sizestr = " + ".join(sizes)
>> + if len(event.args) == 0:
>> + sizestr = '0'
>>
>> - out('static inline void trace_%(name)s(%(args)s)',
>> + out('void trace_%(name)s(%(args)s)',
>> '{',
>> - ' trace%(argc)d(%(trace_args)s);',
>> - '}',
>> - name = e.name,
>> - args = e.args,
>> - argc = len(e.args),
>> - trace_args = simple_args,
>> + ' TraceBufferRecord rec;',
>> + '',
>> + ' if (!trace_list[%(event_id)s].state) {',
>> + ' return;',
>> + ' }',
>> + '',
>> + ' if (trace_record_start(&rec, %(event_id)s, %(size_str)s)) {',
>> + ' return; /* Trace Buffer Full, Event Dropped ! */',
>> + ' }',
>> + name = event.name,
>> + args = event.args,
>> + event_id = num,
>> + size_str = sizestr,
>> )
>>
>> + if len(event.args) > 0:
>> + for type_, name in event.args:
>> + # string
>> + if is_string(type_):
>> + out(' trace_record_write_str(&rec, %(name)s);',
>> + name = name,
>> + )
>> + # pointer var (not string)
>> + elif type_.endswith('*'):
>> + out(' trace_record_write_u64(&rec, (uint64_t)(uint64_t *)%(name)s);',
>> + name = name,
>> + )
>> + # primitive data type
>> + else:
>> + out(' trace_record_write_u64(&rec, (uint64_t)%(name)s);',
>> + name = name,
>> + )
>> +
>> + out(' trace_record_finish(&rec);',
>> + '}',
>> + '')
>> +
>> +
>> +def h(events):
>> + out('#include "trace/simple.h"',
>> + '')
>> +
>> + for event in events:
>> + out('void trace_%(name)s(%(args)s);',
>> + name = event.name,
>> + args = event.args,
>> + )
>> + out('')
>> out('#define NR_TRACE_EVENTS %d' % len(events))
>> out('extern TraceEvent trace_list[NR_TRACE_EVENTS];')
>> diff --git a/trace/simple.c b/trace/simple.c
>> index b64bcf4..3c14568 100644
>> --- a/trace/simple.c
>> +++ b/trace/simple.c
>> @@ -27,7 +27,7 @@
>> #define HEADER_MAGIC 0xf2b177cb0aa429b4ULL
[..snip..]
>> /**
>> * Read a trace record from the trace buffer
>> *
>> @@ -75,16 +96,31 @@ static char *trace_file_name = NULL;
>> *
>> * Returns false if the record is not valid.
>> */
>> -static bool get_trace_record(unsigned int idx, TraceRecord *record)
>> +static bool get_trace_record(unsigned int idx, TraceRecord **recordptr)
>> {
>> - if (!(trace_buf[idx].event & TRACE_RECORD_VALID)) {
>> + uint8_t rec_hdr[sizeof(TraceRecord)];
>> + uint64_t event_flag = 0;
>> + TraceRecord *record = (TraceRecord *) rec_hdr;
>
> Declaring rec_hdr as a uint8_t array is only because you're trying to
> avoid a cast later? The easiest way to make this nice is to do what
> memset(), memcpy() and friends do: just use a void *buf argument.
> That way a caller can pass a TraceRecord* or any other pointer without
> explicit casts, unions, or the uint8_t array trick you are using.
Are you suggesting to use malloc() here?
void *rec_hdr = malloc(sizeof(TraceRecord));
I kept a static array to make sure structure padding doesnt take place.
I am not sure if using malloc here is recommended as we are reading
header and then writing this header byte-by-byte?
~ Harsh
>
>> + /* read the event flag to see if its a valid record */
>> + read_from_buffer(idx, rec_hdr, sizeof(event_flag));
>> +
>> + if (!(record->event & TRACE_RECORD_VALID)) {
>> return false;
>> }
>>
>> __sync_synchronize(); /* read memory barrier before accessing record */
>> -
>> - *record = trace_buf[idx];
>> - record->event &= ~TRACE_RECORD_VALID;
>> + /* read the record header to know record length */
>> + read_from_buffer(idx, rec_hdr, sizeof(TraceRecord));
>> + *recordptr = g_malloc(record->length);
>> + /* make a copy of record to avoid being overwritten */
>> + read_from_buffer(idx, (uint8_t *)*recordptr, record->length);
>> + smp_rmb(); /* memory barrier before clearing valid flag */
>
> You are adding smp_*() instead of __sync_synchronize(). Please either
> continue to use __sync_synchronize() or convert everything to smp_*()
> barriers from qemu-barrier.h.
>
>> + (*recordptr)->event &= ~TRACE_RECORD_VALID;
>> + /* clear the trace buffer range for consumed record otherwise any byte
>> + * with its MSB set may be considered as a valid event id when the writer
>> + * thread crosses this range of buffer again.
>> + */
>> + clear_buffer_range(idx, record->length);
>> return true;
>> }
>>
>> @@ -120,29 +156,41 @@ static void wait_for_trace_records_available(void)
>>
>> static gpointer writeout_thread(gpointer opaque)
>> {
>> - TraceRecord record;
>> - unsigned int writeout_idx = 0;
>> - unsigned int num_available, idx;
>> + TraceRecord *recordptr, *dropped_ptr;
>> + union {
>> + TraceRecord rec;
>> + uint8_t bytes[sizeof(TraceRecord) + sizeof(uint64_t)];
>> + } dropped;
>> + dropped_ptr = (TraceRecord *)dropped.bytes;
>
> dropped_ptr isn't needed, just use dropped.rec.
>
>> + unsigned int idx = 0;
>> + uint64_t dropped_count;
>> size_t unused __attribute__ ((unused));
>>
>> for (;;) {
>> wait_for_trace_records_available();
>>
>> - num_available = trace_idx - writeout_idx;
>> - if (num_available > TRACE_BUF_LEN) {
>> - record = (TraceRecord){
>> - .event = DROPPED_EVENT_ID,
>> - .x1 = num_available,
>> - };
>> - unused = fwrite(&record, sizeof(record), 1, trace_fp);
>> - writeout_idx += num_available;
>> + if (dropped_events) {
>> + dropped_ptr->event = DROPPED_EVENT_ID,
>> + dropped_ptr->timestamp_ns = get_clock();
>> + dropped_ptr->length = sizeof(TraceRecord) + sizeof(dropped_events),
>> + dropped_ptr->reserved = 0;
>> + while (1) {
>> + dropped_count = dropped_events;
>> + smp_rmb();
>
> Not needed, glib says g_atomic_int_compare_and_exchange() "acts as a
> full compiler and hardware memory barrier".
>
> http://developer.gnome.org/glib/2.32/glib-Atomic-Operations.html#g-atomic-int-compare-and-exchange
>
>> + if (g_atomic_int_compare_and_exchange((gint *)&dropped_events,
>> + dropped_count, 0)) {
>> + break;
>> + }
>> + }
>
> It's a shame we have to loop when a simple atomic exchange would have
> done the job better but glib doesn't seem to support that.
>
> Stefan
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH v7 0/3] Simpletrace v2: Support multiple args, strings.
2012-07-17 15:23 ` Stefan Hajnoczi
@ 2012-07-17 19:51 ` Harsh Bora
2012-07-18 8:51 ` Stefan Hajnoczi
0 siblings, 1 reply; 14+ messages in thread
From: Harsh Bora @ 2012-07-17 19:51 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: qemu-devel, stefanha
On 07/17/2012 08:53 PM, Stefan Hajnoczi wrote:
> On Tue, Jul 3, 2012 at 10:20 AM, Harsh Prateek Bora
> <harsh@linux.vnet.ibm.com> wrote:
>> Existing simpletrace backend allows to trace at max 6 args and does not
>> support strings. This newer tracelog format gets rid of fixed size records
>> and therefore allows to trace variable number of args including strings.
>>
>> Sample trace:
>> v9fs_version 0.000 tag=0xffff id=0x64 msize=0x2000 version=9P2000.L
>> v9fs_version_return 6.705 tag=0xffff id=0x64 msize=0x2000 version=9P2000.L
>> v9fs_attach 174.467 tag=0x1 id=0x68 fid=0x0 afid=0xffffffffffffffff
>> uname=nobody aname=
>> v9fs_attach_return 4720.454 tag=0x1 id=0x68 type=0xffffffffffffff80
>> version=0x4f2a4dd0 path=0x220ea6
>
> I have successfully tested it with the fix that I posted. Writing
> simpletrace.Analyzer Python scripts still works - now with string
> arguments too :).
>
> Besides the last few comments on Patch 2, this looks okay now.
Thanks, I have the updated patches ready except for the question asked
on prev reply. Shall I fold your fix in patch 2 with comments as
necessary. Let me know if I need to add your s-o-b after merging your
fix in patch #2? You can update the commit message as you feel
appropriate while merging to your tree though.
regards,
Harsh
>
> Stefan
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH v7 2/3] Simpletrace v2: Support multiple arguments, strings.
2012-07-17 19:01 ` Harsh Bora
@ 2012-07-17 20:08 ` Harsh Bora
2012-07-18 8:59 ` Stefan Hajnoczi
2012-07-18 8:53 ` Stefan Hajnoczi
1 sibling, 1 reply; 14+ messages in thread
From: Harsh Bora @ 2012-07-17 20:08 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: qemu-devel, stefanha
On 07/18/2012 12:31 AM, Harsh Bora wrote:
> On 07/17/2012 08:51 PM, Stefan Hajnoczi wrote:
>> On Tue, Jul 3, 2012 at 10:20 AM, Harsh Prateek Bora
>> <harsh@linux.vnet.ibm.com> wrote:
>>> Existing simpletrace backend allows to trace at max 6 args and does not
>>> support strings. This newer tracelog format gets rid of fixed size
>>> records
>>> and therefore allows to trace variable number of args including strings.
>>>
>>> Sample trace with strings:
>>> v9fs_version 0.000 tag=0xffff id=0x64 msize=0x2000 version=9P2000.L
>>> v9fs_version_return 6.705 tag=0xffff id=0x64 msize=0x2000
>>> version=9P2000.L
>>>
>>> Signed-off-by: Harsh Prateek Bora <harsh@linux.vnet.ibm.com>
>>> ---
>>> scripts/tracetool/backend/simple.py | 84 +++++++++---
>>> trace/simple.c | 256
>>> ++++++++++++++++++++++-------------
>>> trace/simple.h | 39 +++++-
>>> 3 files changed, 260 insertions(+), 119 deletions(-)
>>>
>>> diff --git a/scripts/tracetool/backend/simple.py
>>> b/scripts/tracetool/backend/simple.py
>>> index fbb5717..d3cf4da 100644
>>> --- a/scripts/tracetool/backend/simple.py
>>> +++ b/scripts/tracetool/backend/simple.py
>>> @@ -15,9 +15,16 @@ __email__ = "stefanha@linux.vnet.ibm.com"
>>>
>>> from tracetool import out
>>>
>>> +def is_string(arg):
>>> + strtype = ('const char*', 'char*', 'const char *', 'char *')
>>> + if arg.lstrip().startswith(strtype):
>>> + return True
>>> + else:
>>> + return False
>>>
>>> def c(events):
>>> out('#include "trace.h"',
>>> + '#include "trace/simple.h"',
>>> '',
>>> 'TraceEvent trace_list[] = {')
>>>
>>> @@ -26,30 +33,69 @@ def c(events):
>>> name = e.name,
>>> )
>>>
>>> - out('};')
>>> -
>>> -def h(events):
>>> - out('#include "trace/simple.h"',
>>> + out('};',
>>> '')
>>>
>>> - for num, e in enumerate(events):
>>> - if len(e.args):
>>> - argstr = e.args.names()
>>> - arg_prefix = ', (uint64_t)(uintptr_t)'
>>> - cast_args = arg_prefix + arg_prefix.join(argstr)
>>> - simple_args = (str(num) + cast_args)
>>> - else:
>>> - simple_args = str(num)
>>> + for num, event in enumerate(events):
>>> + sizes = []
>>> + for type_, name in event.args:
>>> + if is_string(type_):
>>> + sizes.append("4 + ((" + name + " ? strlen(" + name +
>>> ") : 0) % MAX_TRACE_STRLEN)")
>>
>> trace_record_write_str() and this code both use % to truncate the
>> string. If the string is 512 characters long you get an empty string.
>> That's weird and not normally how truncation works.
>>
>> Perhaps it's better to change this Python code to emit something like:
>> size_t arg%(num)d_len = %(name)s ? MAX(strlen(%(name)s,
>> MAX_TRACE_STRLEN)) : 0;
>>
>
> I think we need to use MIN instead of MAX, right ?
>
> Another question below ..
>
>> Then you can emit the following to actual store the string:
>>
>> out(' trace_record_write_str(&rec, %(name)s, arg%(num)d_len);',
>> name=name, num=num)
>>
>> The difference is that MAX() is used to truncate and we only do
>> strlen() once.
>>
>>> + else:
>>> + sizes.append("8")
>>> + sizestr = " + ".join(sizes)
>>> + if len(event.args) == 0:
>>> + sizestr = '0'
>>>
>>> - out('static inline void trace_%(name)s(%(args)s)',
>>> + out('void trace_%(name)s(%(args)s)',
>>> '{',
>>> - ' trace%(argc)d(%(trace_args)s);',
>>> - '}',
>>> - name = e.name,
>>> - args = e.args,
>>> - argc = len(e.args),
>>> - trace_args = simple_args,
>>> + ' TraceBufferRecord rec;',
>>> + '',
>>> + ' if (!trace_list[%(event_id)s].state) {',
>>> + ' return;',
>>> + ' }',
>>> + '',
>>> + ' if (trace_record_start(&rec, %(event_id)s,
>>> %(size_str)s)) {',
>>> + ' return; /* Trace Buffer Full, Event Dropped ! */',
>>> + ' }',
>>> + name = event.name,
>>> + args = event.args,
>>> + event_id = num,
>>> + size_str = sizestr,
>>> )
>>>
>>> + if len(event.args) > 0:
>>> + for type_, name in event.args:
>>> + # string
>>> + if is_string(type_):
>>> + out(' trace_record_write_str(&rec, %(name)s);',
>>> + name = name,
>>> + )
>>> + # pointer var (not string)
>>> + elif type_.endswith('*'):
>>> + out(' trace_record_write_u64(&rec,
>>> (uint64_t)(uint64_t *)%(name)s);',
>>> + name = name,
>>> + )
>>> + # primitive data type
>>> + else:
>>> + out(' trace_record_write_u64(&rec,
>>> (uint64_t)%(name)s);',
>>> + name = name,
>>> + )
>>> +
>>> + out(' trace_record_finish(&rec);',
>>> + '}',
>>> + '')
>>> +
>>> +
>>> +def h(events):
>>> + out('#include "trace/simple.h"',
>>> + '')
>>> +
>>> + for event in events:
>>> + out('void trace_%(name)s(%(args)s);',
>>> + name = event.name,
>>> + args = event.args,
>>> + )
>>> + out('')
>>> out('#define NR_TRACE_EVENTS %d' % len(events))
>>> out('extern TraceEvent trace_list[NR_TRACE_EVENTS];')
>>> diff --git a/trace/simple.c b/trace/simple.c
>>> index b64bcf4..3c14568 100644
>>> --- a/trace/simple.c
>>> +++ b/trace/simple.c
>>> @@ -27,7 +27,7 @@
>>> #define HEADER_MAGIC 0xf2b177cb0aa429b4ULL
>
> [..snip..]
>
>>> /**
>>> * Read a trace record from the trace buffer
>>> *
>>> @@ -75,16 +96,31 @@ static char *trace_file_name = NULL;
>>> *
>>> * Returns false if the record is not valid.
>>> */
>>> -static bool get_trace_record(unsigned int idx, TraceRecord *record)
>>> +static bool get_trace_record(unsigned int idx, TraceRecord **recordptr)
>>> {
>>> - if (!(trace_buf[idx].event & TRACE_RECORD_VALID)) {
>>> + uint8_t rec_hdr[sizeof(TraceRecord)];
>>> + uint64_t event_flag = 0;
>>> + TraceRecord *record = (TraceRecord *) rec_hdr;
>>
>> Declaring rec_hdr as a uint8_t array is only because you're trying to
>> avoid a cast later? The easiest way to make this nice is to do what
>> memset(), memcpy() and friends do: just use a void *buf argument.
>> That way a caller can pass a TraceRecord* or any other pointer without
>> explicit casts, unions, or the uint8_t array trick you are using.
>
> Are you suggesting to use malloc() here?
>
> void *rec_hdr = malloc(sizeof(TraceRecord));
>
> I kept a static array to make sure structure padding doesnt take place.
> I am not sure if using malloc here is recommended as we are reading
> header and then writing this header byte-by-byte?
Ah, I confused it with trace_record_finish where we write back the
previously read header, which is not the case here. However, I still
feel using an array is better here probably because:
1) We anyway cant use memcpy here to read from global buffer, we have to
use read_from_buffer to take care of buffer boundaries.
2) Isnt malloc() expensive for such a small allocation requirement?
regards,
Harsh
>
> ~ Harsh
>
>>
>>> + /* read the event flag to see if its a valid record */
>>> + read_from_buffer(idx, rec_hdr, sizeof(event_flag));
>>> +
>>> + if (!(record->event & TRACE_RECORD_VALID)) {
>>> return false;
>>> }
>>>
>>> __sync_synchronize(); /* read memory barrier before accessing
>>> record */
>>> -
>>> - *record = trace_buf[idx];
>>> - record->event &= ~TRACE_RECORD_VALID;
>>> + /* read the record header to know record length */
>>> + read_from_buffer(idx, rec_hdr, sizeof(TraceRecord));
>>> + *recordptr = g_malloc(record->length);
>>> + /* make a copy of record to avoid being overwritten */
>>> + read_from_buffer(idx, (uint8_t *)*recordptr, record->length);
>>> + smp_rmb(); /* memory barrier before clearing valid flag */
>>
>> You are adding smp_*() instead of __sync_synchronize(). Please either
>> continue to use __sync_synchronize() or convert everything to smp_*()
>> barriers from qemu-barrier.h.
>>
>>> + (*recordptr)->event &= ~TRACE_RECORD_VALID;
>>> + /* clear the trace buffer range for consumed record otherwise
>>> any byte
>>> + * with its MSB set may be considered as a valid event id when
>>> the writer
>>> + * thread crosses this range of buffer again.
>>> + */
>>> + clear_buffer_range(idx, record->length);
>>> return true;
>>> }
>>>
>>> @@ -120,29 +156,41 @@ static void wait_for_trace_records_available(void)
>>>
>>> static gpointer writeout_thread(gpointer opaque)
>>> {
>>> - TraceRecord record;
>>> - unsigned int writeout_idx = 0;
>>> - unsigned int num_available, idx;
>>> + TraceRecord *recordptr, *dropped_ptr;
>>> + union {
>>> + TraceRecord rec;
>>> + uint8_t bytes[sizeof(TraceRecord) + sizeof(uint64_t)];
>>> + } dropped;
>>> + dropped_ptr = (TraceRecord *)dropped.bytes;
>>
>> dropped_ptr isn't needed, just use dropped.rec.
>>
>>> + unsigned int idx = 0;
>>> + uint64_t dropped_count;
>>> size_t unused __attribute__ ((unused));
>>>
>>> for (;;) {
>>> wait_for_trace_records_available();
>>>
>>> - num_available = trace_idx - writeout_idx;
>>> - if (num_available > TRACE_BUF_LEN) {
>>> - record = (TraceRecord){
>>> - .event = DROPPED_EVENT_ID,
>>> - .x1 = num_available,
>>> - };
>>> - unused = fwrite(&record, sizeof(record), 1, trace_fp);
>>> - writeout_idx += num_available;
>>> + if (dropped_events) {
>>> + dropped_ptr->event = DROPPED_EVENT_ID,
>>> + dropped_ptr->timestamp_ns = get_clock();
>>> + dropped_ptr->length = sizeof(TraceRecord) +
>>> sizeof(dropped_events),
>>> + dropped_ptr->reserved = 0;
>>> + while (1) {
>>> + dropped_count = dropped_events;
>>> + smp_rmb();
>>
>> Not needed, glib says g_atomic_int_compare_and_exchange() "acts as a
>> full compiler and hardware memory barrier".
>>
>> http://developer.gnome.org/glib/2.32/glib-Atomic-Operations.html#g-atomic-int-compare-and-exchange
>>
>>
>>> + if (g_atomic_int_compare_and_exchange((gint
>>> *)&dropped_events,
>>> + dropped_count,
>>> 0)) {
>>> + break;
>>> + }
>>> + }
>>
>> It's a shame we have to loop when a simple atomic exchange would have
>> done the job better but glib doesn't seem to support that.
>>
>> Stefan
>>
>
>
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH v7 0/3] Simpletrace v2: Support multiple args, strings.
2012-07-17 19:51 ` Harsh Bora
@ 2012-07-18 8:51 ` Stefan Hajnoczi
0 siblings, 0 replies; 14+ messages in thread
From: Stefan Hajnoczi @ 2012-07-18 8:51 UTC (permalink / raw)
To: Harsh Bora; +Cc: qemu-devel, stefanha
On Tue, Jul 17, 2012 at 8:51 PM, Harsh Bora <harsh@linux.vnet.ibm.com> wrote:
> On 07/17/2012 08:53 PM, Stefan Hajnoczi wrote:
>>
>> On Tue, Jul 3, 2012 at 10:20 AM, Harsh Prateek Bora
>> <harsh@linux.vnet.ibm.com> wrote:
>>>
>>> Existing simpletrace backend allows to trace at max 6 args and does not
>>> support strings. This newer tracelog format gets rid of fixed size
>>> records
>>> and therefore allows to trace variable number of args including strings.
>>>
>>> Sample trace:
>>> v9fs_version 0.000 tag=0xffff id=0x64 msize=0x2000 version=9P2000.L
>>> v9fs_version_return 6.705 tag=0xffff id=0x64 msize=0x2000
>>> version=9P2000.L
>>> v9fs_attach 174.467 tag=0x1 id=0x68 fid=0x0 afid=0xffffffffffffffff
>>> uname=nobody aname=
>>> v9fs_attach_return 4720.454 tag=0x1 id=0x68 type=0xffffffffffffff80
>>> version=0x4f2a4dd0 path=0x220ea6
>>
>>
>> I have successfully tested it with the fix that I posted. Writing
>> simpletrace.Analyzer Python scripts still works - now with string
>> arguments too :).
>>
>> Besides the last few comments on Patch 2, this looks okay now.
>
>
> Thanks, I have the updated patches ready except for the question asked on
> prev reply. Shall I fold your fix in patch 2 with comments as necessary. Let
> me know if I need to add your s-o-b after merging your fix in patch #2? You
> can update the commit message as you feel appropriate while merging to your
> tree though.
It's a trivial patch, feel free to squash it without noting anything.
Stefan
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH v7 2/3] Simpletrace v2: Support multiple arguments, strings.
2012-07-17 19:01 ` Harsh Bora
2012-07-17 20:08 ` Harsh Bora
@ 2012-07-18 8:53 ` Stefan Hajnoczi
1 sibling, 0 replies; 14+ messages in thread
From: Stefan Hajnoczi @ 2012-07-18 8:53 UTC (permalink / raw)
To: Harsh Bora; +Cc: qemu-devel, stefanha
On Tue, Jul 17, 2012 at 8:01 PM, Harsh Bora <harsh@linux.vnet.ibm.com> wrote:
> On 07/17/2012 08:51 PM, Stefan Hajnoczi wrote:
>>
>> On Tue, Jul 3, 2012 at 10:20 AM, Harsh Prateek Bora
>> <harsh@linux.vnet.ibm.com> wrote:
>>>
>>> Existing simpletrace backend allows to trace at max 6 args and does not
>>> support strings. This newer tracelog format gets rid of fixed size
>>> records
>>> and therefore allows to trace variable number of args including strings.
>>>
>>> Sample trace with strings:
>>> v9fs_version 0.000 tag=0xffff id=0x64 msize=0x2000 version=9P2000.L
>>> v9fs_version_return 6.705 tag=0xffff id=0x64 msize=0x2000
>>> version=9P2000.L
>>>
>>> Signed-off-by: Harsh Prateek Bora <harsh@linux.vnet.ibm.com>
>>> ---
>>> scripts/tracetool/backend/simple.py | 84 +++++++++---
>>> trace/simple.c | 256
>>> ++++++++++++++++++++++-------------
>>> trace/simple.h | 39 +++++-
>>> 3 files changed, 260 insertions(+), 119 deletions(-)
>>>
>>> diff --git a/scripts/tracetool/backend/simple.py
>>> b/scripts/tracetool/backend/simple.py
>>> index fbb5717..d3cf4da 100644
>>> --- a/scripts/tracetool/backend/simple.py
>>> +++ b/scripts/tracetool/backend/simple.py
>>> @@ -15,9 +15,16 @@ __email__ = "stefanha@linux.vnet.ibm.com"
>>>
>>> from tracetool import out
>>>
>>> +def is_string(arg):
>>> + strtype = ('const char*', 'char*', 'const char *', 'char *')
>>> + if arg.lstrip().startswith(strtype):
>>> + return True
>>> + else:
>>> + return False
>>>
>>> def c(events):
>>> out('#include "trace.h"',
>>> + '#include "trace/simple.h"',
>>> '',
>>> 'TraceEvent trace_list[] = {')
>>>
>>> @@ -26,30 +33,69 @@ def c(events):
>>> name = e.name,
>>> )
>>>
>>> - out('};')
>>> -
>>> -def h(events):
>>> - out('#include "trace/simple.h"',
>>> + out('};',
>>> '')
>>>
>>> - for num, e in enumerate(events):
>>> - if len(e.args):
>>> - argstr = e.args.names()
>>> - arg_prefix = ', (uint64_t)(uintptr_t)'
>>> - cast_args = arg_prefix + arg_prefix.join(argstr)
>>> - simple_args = (str(num) + cast_args)
>>> - else:
>>> - simple_args = str(num)
>>> + for num, event in enumerate(events):
>>> + sizes = []
>>> + for type_, name in event.args:
>>> + if is_string(type_):
>>> + sizes.append("4 + ((" + name + " ? strlen(" + name + ")
>>> : 0) % MAX_TRACE_STRLEN)")
>>
>>
>> trace_record_write_str() and this code both use % to truncate the
>> string. If the string is 512 characters long you get an empty string.
>> That's weird and not normally how truncation works.
>>
>> Perhaps it's better to change this Python code to emit something like:
>> size_t arg%(num)d_len = %(name)s ? MAX(strlen(%(name)s, MAX_TRACE_STRLEN))
>> : 0;
>>
>
> I think we need to use MIN instead of MAX, right ?
Yes, my bad :).
Stefan
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH v7 2/3] Simpletrace v2: Support multiple arguments, strings.
2012-07-17 20:08 ` Harsh Bora
@ 2012-07-18 8:59 ` Stefan Hajnoczi
0 siblings, 0 replies; 14+ messages in thread
From: Stefan Hajnoczi @ 2012-07-18 8:59 UTC (permalink / raw)
To: Harsh Bora; +Cc: qemu-devel, stefanha
On Tue, Jul 17, 2012 at 9:08 PM, Harsh Bora <harsh@linux.vnet.ibm.com> wrote:
> On 07/18/2012 12:31 AM, Harsh Bora wrote:
>>
>> On 07/17/2012 08:51 PM, Stefan Hajnoczi wrote:
>>>
>>> On Tue, Jul 3, 2012 at 10:20 AM, Harsh Prateek Bora
>>> <harsh@linux.vnet.ibm.com> wrote:
>>>> @@ -75,16 +96,31 @@ static char *trace_file_name = NULL;
>>>> *
>>>> * Returns false if the record is not valid.
>>>> */
>>>> -static bool get_trace_record(unsigned int idx, TraceRecord *record)
>>>> +static bool get_trace_record(unsigned int idx, TraceRecord **recordptr)
>>>> {
>>>> - if (!(trace_buf[idx].event & TRACE_RECORD_VALID)) {
>>>> + uint8_t rec_hdr[sizeof(TraceRecord)];
>>>> + uint64_t event_flag = 0;
>>>> + TraceRecord *record = (TraceRecord *) rec_hdr;
>>>
>>>
>>> Declaring rec_hdr as a uint8_t array is only because you're trying to
>>> avoid a cast later? The easiest way to make this nice is to do what
>>> memset(), memcpy() and friends do: just use a void *buf argument.
>>> That way a caller can pass a TraceRecord* or any other pointer without
>>> explicit casts, unions, or the uint8_t array trick you are using.
>>
>>
>> Are you suggesting to use malloc() here?
>>
>> void *rec_hdr = malloc(sizeof(TraceRecord));
>>
>> I kept a static array to make sure structure padding doesnt take place.
>> I am not sure if using malloc here is recommended as we are reading
>> header and then writing this header byte-by-byte?
>
>
> Ah, I confused it with trace_record_finish where we write back the
> previously read header, which is not the case here. However, I still feel
> using an array is better here probably because:
> 1) We anyway cant use memcpy here to read from global buffer, we have to use
> read_from_buffer to take care of buffer boundaries.
> 2) Isnt malloc() expensive for such a small allocation requirement?
No malloc.
The code is basically playing games with types because the read/write
functions take a uint8_t* buffer argument but callers actually want to
use TraceRecord. You ended up declaring a uint8_t array and then
pointing a TraceRecord* into the array.
A cleaner way of doing this is to just use TraceRecord and make the
read/write functions take void* buffer arguments. My comment about
memset/memcpy was that these library functions do the same thing - it
allows callers to write clean and simple code.
You can drop the uint8_t arrays and TraceRecord* alias pointers. You
can also drop the union you have in one of the functions.
Just use a TraceRecord local variable and change the read/write helper
buffer argument to void*.
Stefan
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2012-07-18 8:59 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-07-03 9:20 [Qemu-devel] [PATCH v7 0/3] Simpletrace v2: Support multiple args, strings Harsh Prateek Bora
2012-07-03 9:20 ` [Qemu-devel] [PATCH v7 1/3] monitor: remove unused do_info_trace Harsh Prateek Bora
2012-07-03 9:20 ` [Qemu-devel] [PATCH v7 2/3] Simpletrace v2: Support multiple arguments, strings Harsh Prateek Bora
2012-07-17 15:21 ` Stefan Hajnoczi
2012-07-17 19:01 ` Harsh Bora
2012-07-17 20:08 ` Harsh Bora
2012-07-18 8:59 ` Stefan Hajnoczi
2012-07-18 8:53 ` Stefan Hajnoczi
2012-07-03 9:20 ` [Qemu-devel] [PATCH v7 3/3] Update simpletrace.py for new log format Harsh Prateek Bora
2012-07-17 14:07 ` [Qemu-devel] [PATCH v7 0/3] Simpletrace v2: Support multiple args, strings Stefan Hajnoczi
2012-07-17 14:14 ` Stefan Hajnoczi
2012-07-17 15:23 ` Stefan Hajnoczi
2012-07-17 19:51 ` Harsh Bora
2012-07-18 8:51 ` Stefan Hajnoczi
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.