Messages in this thread | | | Date | Mon, 26 Apr 2021 14:44:21 +0800 | From | Leo Yan <> | Subject | Re: [PATCH v1 2/3] perf session: Add swap operation for event TIME_CONV |
| |
Hi Adrian,
On Mon, Apr 26, 2021 at 08:40:00AM +0300, Adrian Hunter wrote:
[...]
> > +static void perf_event__time_conv_swap(union perf_event *event, > > + bool sample_id_all __maybe_unused) > > +{ > > + size_t time_zero_size; > > + > > + event->time_conv.time_shift = bswap_64(event->time_conv.time_shift); > > + event->time_conv.time_mult = bswap_64(event->time_conv.time_mult); > > + event->time_conv.time_zero = bswap_64(event->time_conv.time_zero); > > + > > + time_zero_size = (void *)&event->time_conv.time_cycles - (void *)event; > > + if (event->header.size > time_zero_size) { > > I wonder if we could have a helper for this e.g. (untested) > > #define event_contains(obj, mem) (obj.header.size > offsetof(typeof(obj), mem)) > > if (event_contains(event->time_conv, time_cycles)) {
Yeah, this is a better implementation. Will refine patch for this.
> > + event->time_conv.time_cycles = bswap_64(event->time_conv.time_cycles); > > + event->time_conv.time_mask = bswap_64(event->time_conv.time_mask); > > + event->time_conv.cap_user_time_zero = > > + bswap_32(event->time_conv.cap_user_time_zero); > > + event->time_conv.cap_user_time_short = > > + bswap_32(event->time_conv.cap_user_time_short); > > 'struct perf_record_time_conv' contains bool, the sizeof which, AFAIK, is not defined. > Is it really 4 bytes on your implementation? It is only 1 byte with gcc on x86.
On Arm64, bool is also 1 byte with GCC.
When working on this patch, I checked the size for structure perf_record_time_conv, it gave out the structure size is 56; I wrongly thought the bool size is 4 bytes and there have no padding. In fact, bool size is 1 byte and GCC pads 6 bytes at the end of structure.
> Either way, you should change 'struct perf_record_time_conv' so it uses a type of known size. > Since you are the only one using it, it should match your implementation.
Will change to "unsigned int" value. Thank you for reviewing and suggestions.
Leo
> > + } > > +} > > + > > typedef void (*perf_event__swap_op)(union perf_event *event, > > bool sample_id_all); > > > > @@ -985,7 +1005,7 @@ static perf_event__swap_op perf_event__swap_ops[] = { > > [PERF_RECORD_STAT] = perf_event__stat_swap, > > [PERF_RECORD_STAT_ROUND] = perf_event__stat_round_swap, > > [PERF_RECORD_EVENT_UPDATE] = perf_event__event_update_swap, > > - [PERF_RECORD_TIME_CONV] = perf_event__all64_swap, > > + [PERF_RECORD_TIME_CONV] = perf_event__time_conv_swap, > > [PERF_RECORD_HEADER_MAX] = NULL, > > }; > > > > >
| |