| Message ID | 1466412241-27502-1-git-send-email-maddy@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
On Mon, Jun 20, 2016 at 02:14:01PM +0530, Madhavan Srinivasan wrote: > When decoding the perf_regs mask in regs_dump__printf(), > we loop through the mask using find_first_bit and find_next_bit functions. > "mask" is of type "u64", but sent as a "unsigned long *" to > lib functions along with sizeof(). While the exisitng code works fine in > most of the case, the logic is broken when using a 32bit perf on a > 64bit kernel (Big Endian). We end up reading the wrong word of the u64 > first in the lib functions. hum, I still don't see why this happens.. why do we read the wrong word in this case? thanks, jirka > first in the lib functions. Proposed fix is to swap the words of the > u64 to handle this case. This is not endianess swap. > SNIP
On Mon, Jun 20, 2016 at 02:14:01PM +0530, Madhavan Srinivasan wrote: SNIP > diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c > index e3ce2f34d3ad..76d5006ebcc3 100644 > --- a/tools/perf/builtin-script.c > +++ b/tools/perf/builtin-script.c > @@ -412,11 +412,16 @@ static void print_sample_iregs(struct perf_sample *sample, > struct regs_dump *regs = &sample->intr_regs; > uint64_t mask = attr->sample_regs_intr; > unsigned i = 0, r; > + unsigned long _mask[sizeof(mask)/sizeof(unsigned long)]; > > if (!regs) > return; > > - for_each_set_bit(r, (unsigned long *) &mask, sizeof(mask) * 8) { > + _mask[0] = mask & ULONG_MAX; > + if (sizeof(mask) > sizeof(unsigned long)) > + _mask[1] = mask >> 32; > + > + for_each_set_bit(r, _mask, sizeof(mask) * 8) { > u64 val = regs->regs[i++]; > printf("%5s:0x%"PRIx64" ", perf_reg_name(r), val); > } > diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c > index 5214974e841a..2eaa42a4832a 100644 > --- a/tools/perf/util/session.c > +++ b/tools/perf/util/session.c > @@ -940,8 +940,13 @@ static void branch_stack__printf(struct perf_sample *sample) > static void regs_dump__printf(u64 mask, u64 *regs) > { > unsigned rid, i = 0; > + unsigned long _mask[sizeof(mask)/sizeof(unsigned long)]; > > - for_each_set_bit(rid, (unsigned long *) &mask, sizeof(mask) * 8) { > + _mask[0] = mask & ULONG_MAX; > + if (sizeof(mask) > sizeof(unsigned long)) > + _mask[1] = mask >> 32; > + > + for_each_set_bit(rid, _mask, sizeof(mask) * 8) { > u64 val = regs[i++]; could you please move the common code into the function? thanks, jirka
On 2016/6/20 17:18, Jiri Olsa wrote: > On Mon, Jun 20, 2016 at 02:14:01PM +0530, Madhavan Srinivasan wrote: >> When decoding the perf_regs mask in regs_dump__printf(), >> we loop through the mask using find_first_bit and find_next_bit functions. >> "mask" is of type "u64", but sent as a "unsigned long *" to >> lib functions along with sizeof(). While the exisitng code works fine in >> most of the case, the logic is broken when using a 32bit perf on a >> 64bit kernel (Big Endian). We end up reading the wrong word of the u64 >> first in the lib functions. > hum, I still don't see why this happens.. why do we read the > wrong word in this case? If you read a u64 using (u32 *)(&val)[0] and (u32 *)(&val)[1] you can get wrong value. This is what _find_next_bit() is doing. In a big endian environment where 'unsigned long' is 32 bits long, "(u32 *)(&val)[0]" gets upper 32 bits, but without this patch perf assumes it gets lower 32 bits. The root cause is wrongly convert u64 value to bitmap. Thank you.
On Mon, Jun 20, 2016 at 05:27:25PM +0800, Wangnan (F) wrote: > > > On 2016/6/20 17:18, Jiri Olsa wrote: > > On Mon, Jun 20, 2016 at 02:14:01PM +0530, Madhavan Srinivasan wrote: > > > When decoding the perf_regs mask in regs_dump__printf(), > > > we loop through the mask using find_first_bit and find_next_bit functions. > > > "mask" is of type "u64", but sent as a "unsigned long *" to > > > lib functions along with sizeof(). While the exisitng code works fine in > > > most of the case, the logic is broken when using a 32bit perf on a > > > 64bit kernel (Big Endian). We end up reading the wrong word of the u64 > > > first in the lib functions. > > hum, I still don't see why this happens.. why do we read the > > wrong word in this case? > > If you read a u64 using (u32 *)(&val)[0] and (u32 *)(&val)[1] > you can get wrong value. This is what _find_next_bit() is doing. > > In a big endian environment where 'unsigned long' is 32 bits > long, "(u32 *)(&val)[0]" gets upper 32 bits, but without this patch > perf assumes it gets lower 32 bits. The root cause is wrongly convert > u64 value to bitmap. i see, could you please put this into comment in the code? also we could have common function for that, to keep it on one place only, like bitmap_from_u64 or so thanks, jirka
On Monday 20 June 2016 03:10 PM, Jiri Olsa wrote: > On Mon, Jun 20, 2016 at 05:27:25PM +0800, Wangnan (F) wrote: >> >> On 2016/6/20 17:18, Jiri Olsa wrote: >>> On Mon, Jun 20, 2016 at 02:14:01PM +0530, Madhavan Srinivasan wrote: >>>> When decoding the perf_regs mask in regs_dump__printf(), >>>> we loop through the mask using find_first_bit and find_next_bit functions. >>>> "mask" is of type "u64", but sent as a "unsigned long *" to >>>> lib functions along with sizeof(). While the exisitng code works fine in >>>> most of the case, the logic is broken when using a 32bit perf on a >>>> 64bit kernel (Big Endian). We end up reading the wrong word of the u64 >>>> first in the lib functions. >>> hum, I still don't see why this happens.. why do we read the >>> wrong word in this case? >> If you read a u64 using (u32 *)(&val)[0] and (u32 *)(&val)[1] >> you can get wrong value. This is what _find_next_bit() is doing. Also in find_first_bit(). >> >> In a big endian environment where 'unsigned long' is 32 bits >> long, "(u32 *)(&val)[0]" gets upper 32 bits, but without this patch >> perf assumes it gets lower 32 bits. The root cause is wrongly convert >> u64 value to bitmap. > i see, could you please put this into comment in the code? > > also we could have common function for that, to keep it on > one place only, like bitmap_from_u64 or so Sure will do. > > thanks, > jirka >
diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c index e3ce2f34d3ad..76d5006ebcc3 100644 --- a/tools/perf/builtin-script.c +++ b/tools/perf/builtin-script.c @@ -412,11 +412,16 @@ static void print_sample_iregs(struct perf_sample *sample, struct regs_dump *regs = &sample->intr_regs; uint64_t mask = attr->sample_regs_intr; unsigned i = 0, r; + unsigned long _mask[sizeof(mask)/sizeof(unsigned long)]; if (!regs) return; - for_each_set_bit(r, (unsigned long *) &mask, sizeof(mask) * 8) { + _mask[0] = mask & ULONG_MAX; + if (sizeof(mask) > sizeof(unsigned long)) + _mask[1] = mask >> 32; + + for_each_set_bit(r, _mask, sizeof(mask) * 8) { u64 val = regs->regs[i++]; printf("%5s:0x%"PRIx64" ", perf_reg_name(r), val); } diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c index 5214974e841a..2eaa42a4832a 100644 --- a/tools/perf/util/session.c +++ b/tools/perf/util/session.c @@ -940,8 +940,13 @@ static void branch_stack__printf(struct perf_sample *sample) static void regs_dump__printf(u64 mask, u64 *regs) { unsigned rid, i = 0; + unsigned long _mask[sizeof(mask)/sizeof(unsigned long)]; - for_each_set_bit(rid, (unsigned long *) &mask, sizeof(mask) * 8) { + _mask[0] = mask & ULONG_MAX; + if (sizeof(mask) > sizeof(unsigned long)) + _mask[1] = mask >> 32; + + for_each_set_bit(rid, _mask, sizeof(mask) * 8) { u64 val = regs[i++]; printf(".... %-5s 0x%" PRIx64 "\n",
When decoding the perf_regs mask in regs_dump__printf(), we loop through the mask using find_first_bit and find_next_bit functions. "mask" is of type "u64", but sent as a "unsigned long *" to lib functions along with sizeof(). While the exisitng code works fine in most of the case, the logic is broken when using a 32bit perf on a 64bit kernel (Big Endian). We end up reading the wrong word of the u64 first in the lib functions. Proposed fix is to swap the words of the u64 to handle this case. This is not endianess swap. Suggested-by: Yury Norov <ynorov@caviumnetworks.com> Cc: Yury Norov <ynorov@caviumnetworks.com> Cc: Peter Zijlstra <peterz@infradead.org> Cc: Ingo Molnar <mingo@redhat.com> Cc: Arnaldo Carvalho de Melo <acme@kernel.org> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com> Cc: Jiri Olsa <jolsa@kernel.org> Cc: Adrian Hunter <adrian.hunter@intel.com> Cc: Kan Liang <kan.liang@intel.com> Cc: Wang Nan <wangnan0@huawei.com> Cc: Michael Ellerman <mpe@ellerman.id.au> Signed-off-by: Madhavan Srinivasan <maddy@linux.vnet.ibm.com> --- Changelog v1: 1)updated commit message and patch subject 2)Add the fix to print_sample_iregs() in builtin-script.c tools/perf/builtin-script.c | 7 ++++++- tools/perf/util/session.c | 7 ++++++- 2 files changed, 12 insertions(+), 2 deletions(-)