Messages in this thread | | | Date | Wed, 19 May 2010 09:39:10 +0200 | From | Frederic Weisbecker <> | Subject | Re: [RFC PATCH] perf: align raw sample data on 64-bit boundaries |
| |
On Tue, May 18, 2010 at 05:12:27PM +0200, Robert Richter wrote: > On 19.04.10 14:19:57, Stephane Eranian wrote: > > > + perf_sample_data_init(&data, 0); > > > + if (event->attr.sample_type & PERF_SAMPLE_RAW) { > > > + for (i = 1; i < size; i++) > > > + rdmsrl(msr++, *buf++); > > > + raw.size = sizeof(u64) * size; > > > + raw.data = buffer; > > > + data.raw = &raw; > > > + } > > > + > > > > Need to add the padding: raw.size = sizeof(u64) * size + sizeof(u32); > > When thinking about this I was wondering if it wouldn't make sense to > better fix the alignment and move the data buffer to a 64 bit > boundary. So take a look at the enclosed RFC patch. Though it breaks > the ABI it would solve some problems. I think more than it introduces. > Hopefully I found all effected code locations using it. > > -Robert > > -- > > From 2427dda67b072f27ecff678f8829b9e2fc537988 Mon Sep 17 00:00:00 2001 > From: Robert Richter <robert.richter@amd.com> > Date: Fri, 7 May 2010 15:32:45 +0200 > Subject: [PATCH] perf: align raw sample data on 64-bit boundaries > > In the current implementation 64 bit raw sample data values are not > aligned due to the 32 bit size header. The size header is located > before the data buffer on a 64 bit boundary. This leads to unaligned > memory access to the data buffer for arrays or structures containing > 64 bit values. To avoid this, the patch adds a 32 bit reserved value > to the raw sample data header. The data buffer starts then at a 64 bit > boundary. > > This changes the ABI and requires changes in the userland tools. For > tools/perf this is at a single location in event.c only. This could > also introduce some overhead on smaller architectures, but currently > this is only used on x86 or for transferring raw tracepoint > data.
No this is used on any architectures that event have a minimal support for perf events.
I use tracepoint raw samples in sparc 64 for example (which has much more than the minimal support).
> Though an ABI change should be avoided, it is worth to align raw > sample data on 64-bit boundaries as the change fixes unaligned memory > access, eases the implementation for raw sample data and reduces the > risk of data corruption due to different pad structures inserted by > the compiler. > > Signed-off-by: Robert Richter <robert.richter@amd.com> > ---
I don't think we should do this. Ok it's true we've screwed up something there but breaking the ABI is going to make the things even worst I think.
I would feel better with a new PERF_SAMPLE_RAW_ALIGNED sample_type and schedule the deprecation of PERF_SAMPLE_RAW for later but keep it for some releases.
What do you think?
-- 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/
| |