Messages in this thread | | | Subject | Re: [PATCH] perf timechart: dynamically determine event data offset | From | Namhyung Kim <> | Date | Wed, 27 Nov 2013 23:17:43 +0900 |
| |
Hi Arnaldo,
2013-11-27 (수), 10:44 -0300, Arnaldo Carvalho de Melo: > Em Wed, Nov 27, 2013 at 05:49:02PM +0900, Namhyung Kim escreveu: > > On Tue, 26 Nov 2013 18:54:37 +0400, Stanislav Fomichev wrote: > > > Since b000c8065a92 "tracing: Remove the extra 4 bytes of padding in events" > > > removed padding bytes, perf timechart got out of sync with the kernel's > > > trace_entry structure. > > > > We can't just align perf's trace_entry definition with the kernel because we > > > want timechart to continue working with old perf.data. Instead, we now > > > calculate event data offset dynamically using offset of first non-common > > > event field in the perf.data. > > > [SNIP] > > > @@ -304,13 +322,11 @@ struct trace_entry { > > > unsigned char flags; > > > unsigned char preempt_count; > > > int pid; > > > - int lock_depth; > > > }; > > > I had no chance to look into the timechart code in detail, but this is > > not good. The format of each trace event (so the struct trace_entry) > > was described in the format file under the event directory on debugfs. > > For cpu_frequency event, I get the following: > > > $ cat /sys/kernel/debug/tracing/events/power/cpu_frequency/format > > name: cpu_frequency > > ID: 315 > > format: > > field:unsigned short common_type; offset:0; size:2; signed:0; > > field:unsigned char common_flags; offset:2; size:1; signed:0; > > field:unsigned char common_preempt_count; offset:3; size:1; signed:0; > > field:int common_pid; offset:4; size:4; signed:1; > > > field:u32 state; offset:8; size:4; signed:0; > > field:u32 cpu_id; offset:12; size:4; signed:0; > > > print fmt: "state=%lu cpu_id=%lu", (unsigned long)REC->state, (unsigned long)REC->cpu_id > > > So it's not same as above struct trace_entry even with your change. > > It is... Albeit working just with patches is good and dandy and I even > was discussing with Jiri how patches should be clean and small, you're > not seeing the whole picture, see what is at the line just before > "unsigned char flags": > > struct trace_entry { > unsigned short type; > unsigned char flags; > unsigned char preempt_count; > int pid; > int lock_depth; > };
Argh, I missed that part, sorry. :)
> > Looking with pahole to see the offsets on a 64-bit arch: > > [acme@ssdandy linux]$ pahole -C trace_entry /tmp/build/perf/builtin-timechart.o > struct trace_entry { > short unsigned int type; /* 0 2 */ > unsigned char flags; /* 2 1 */ > unsigned char preempt_count; /* 3 1 */ > int pid; /* 4 4 */ > int lock_depth; /* 8 4 */ > > /* size: 12, cachelines: 1, members: 5 */ > /* last cacheline: 12 bytes */ > }; > [acme@ssdandy linux]$ > [acme@ssdandy linux]$ uname -a > Linux ssdandy 3.12.0-rc6+ #2 SMP Mon Oct 21 11:19:07 BRT 2013 x86_64 x86_64 x86_64 GNU/Linux > > And now on a 32-bit arch: > > acme@linux-goap:~> pahole -C trace_entry /tmp/build/perf/builtin-timechart.o > struct trace_entry { > short unsigned int type; /* 0 2 */ > unsigned char flags; /* 2 1 */ > unsigned char preempt_count; /* 3 1 */ > int pid; /* 4 4 */ > int lock_depth; /* 8 4 */ > > /* size: 12, cachelines: 1, members: 5 */ > /* last cacheline: 12 bytes */ > }; > acme@linux-goap:~> uname -a > Linux linux-goap 3.7.10-1.16-desktop #1 SMP PREEMPT Fri May 31 20:21:23 UTC 2013 (97c14ba) i686 i686 i386 GNU/Linux > acme@linux-goap:~> > > Same signature, 32-bit, 64-bit userland, so whoever wrote timechart, Arjan, I > think, made no mistakes at using the kernel exported interface, choosing the > most efficient way to extract the data, casting to a struct. > > Yeah, using the interface that searches for a field name to get the offset and > then add it to a pointer to then cast to the type will allow maximum flexibility, > but is not really efficient. > > Doing that for something that is not performance critical (probably) as > timechart probably is not a problem, but so is not a problem using the patch > that does the cast after finding the offset of the first non-common field.
I'm not sure how it affects the performance really. But in most cases, using the libtraceevent would be a better solution IMHO.
> > Having said that, I'll take the latest patch, using perf_evsel__intval et all, > if Namhyung sees no further problems with it.
Okay, after a quick glance I don't see anymore issue. You can add my Ack there.
> > BTW: in 'perf trace' I recently did some work to cache the offsets on a per evsel > private area, so that the string lookup is not done anymore in the raw_syscalls > tracepoint processing.
Good job. :)
Thanks, Namhyung
-- 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/
| |