Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1424663 > unrolled thread
| Started by | Madhavan Srinivasan <maddy@linux.vnet.ibm.com> |
|---|---|
| First post | 2016-06-17 07:30 +0200 |
| Last post | 2016-06-17 13:30 +0200 |
| Articles | 4 — 2 participants |
Back to article view | Back to linux.kernel
[PATCH] tools/perf: Fix the mask in regs_dump__printf Madhavan Srinivasan <maddy@linux.vnet.ibm.com> - 2016-06-17 07:30 +0200
Re: [PATCH] tools/perf: Fix the mask in regs_dump__printf Jiri Olsa <jolsa@redhat.com> - 2016-06-17 08:40 +0200
Re: [PATCH] tools/perf: Fix the mask in regs_dump__printf Madhavan Srinivasan <maddy@linux.vnet.ibm.com> - 2016-06-17 11:20 +0200
Re: [PATCH] tools/perf: Fix the mask in regs_dump__printf Jiri Olsa <jolsa@redhat.com> - 2016-06-17 13:30 +0200
| From | Madhavan Srinivasan <maddy@linux.vnet.ibm.com> |
|---|---|
| Date | 2016-06-17 07:30 +0200 |
| Subject | [PATCH] tools/perf: Fix the mask in regs_dump__printf |
| Message-ID | <rKUvT-2ml-9@gated-at.bofh.it> |
When decoding the perf_regs mask in regs_dump__printf(),
we loop through the mask using find_first_bit and find_next_bit functions.
And mask is of type "u64". But "u64" is send as a "unsigned long *" to
lib functions along with sizeof().
While the exisitng code works fine in most of the case, when using a
32bit perf on a 64bit kernel (Big Endian), we end up reading the wrong word
in the u64 mask. Patch to fix the mask in regs_dump__printf().
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>
---
tools/perf/util/session.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
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",
--
1.9.1
[toc] | [next] | [standalone]
| From | Jiri Olsa <jolsa@redhat.com> |
|---|---|
| Date | 2016-06-17 08:40 +0200 |
| Message-ID | <rKVBD-30p-3@gated-at.bofh.it> |
| In reply to | #1424663 |
On Fri, Jun 17, 2016 at 10:52:38AM +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.
> And mask is of type "u64". But "u64" is send as a "unsigned long *" to
> lib functions along with sizeof().
>
> While the exisitng code works fine in most of the case, when using a
> 32bit perf on a 64bit kernel (Big Endian), we end up reading the wrong word
> in the u64 mask. Patch to fix the mask in regs_dump__printf().
>
> 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>
> ---
> tools/perf/util/session.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> 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;
> +
I think we should do this earlier when reading the mask,
not at the moment we just print it, like we do for other
types.. maybe we could do this as an extra bit for
perf_event__swap_ops[PERF_RECORD_SAMPLE] function
also there's print_sample_iregs in builtin-script.c that's
most likely affected as well
thanks,
jirka
> + for_each_set_bit(rid, _mask, sizeof(mask) * 8) {
> u64 val = regs[i++];
>
> printf(".... %-5s 0x%" PRIx64 "\n",
> --
> 1.9.1
>
[toc] | [prev] | [next] | [standalone]
| From | Madhavan Srinivasan <maddy@linux.vnet.ibm.com> |
|---|---|
| Date | 2016-06-17 11:20 +0200 |
| Message-ID | <rKY6t-4F5-5@gated-at.bofh.it> |
| In reply to | #1424718 |
On Friday 17 June 2016 12:07 PM, Jiri Olsa wrote:
> On Fri, Jun 17, 2016 at 10:52:38AM +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.
>> And mask is of type "u64". But "u64" is send as a "unsigned long *" to
>> lib functions along with sizeof().
>>
>> While the exisitng code works fine in most of the case, when using a
>> 32bit perf on a 64bit kernel (Big Endian), we end up reading the wrong word
>> in the u64 mask. Patch to fix the mask in regs_dump__printf().
>>
>> 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>
>> ---
>> tools/perf/util/session.c | 7 ++++++-
>> 1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> 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;
>> +
> I think we should do this earlier when reading the mask,
> not at the moment we just print it, like we do for other
> types.. maybe we could do this as an extra bit for
Ok. I tried something like this. Currently
mask is read in perf_evsel__parse_sample(),
so added for both sample type (PERF_SAMPLE_REGS_USER
and PERF_SAMPLE_REGS_INTR)
if (data->user_regs.abi) {
- u64 mask = evsel->attr.sample_regs_user;
+ u.val64 = evsel->attr.sample_regs_user;
- sz = hweight_long(mask) * sizeof(u64);
+ if (sizeof(u64) > sizeof(unsigned long)) {
+ u64 mask = u.val64;
+ u.val32[1] = mask >> 32;
+ u.val32[0] = mask & ULONG_MAX;
+ }
+
+ sz = hweight_long(u.val64) * sizeof(u64);
OVERFLOW_CHECK(array, sz, max_size);
- data->user_regs.mask = mask;
+ data->user_regs.mask = u.val64;
data->user_regs.regs = (u64 *)array;
Issue I see is when printing the mask value in a 32bit perf
on a 64bit kernel (big endian).
442044948492 0xdc0 [0x188]: PERF_RECORD_SAMPLE(IP, 0x1): 7299/7299:
0xc000000000059200 period: 12599 addr: 0
... intr regs: mask 0xffffffff000007ff ABI 32-bit
^^^ shld have been 0x7ffffffffff
I agree it is better to fix this when reading, but
we need to swap again when printing?
> perf_event__swap_ops[PERF_RECORD_SAMPLE] function
>
> also there's print_sample_iregs in builtin-script.c that's
> most likely affected as well
My bad. Should have seen this too.
Maddy
>
> thanks,
> jirka
>
>> + for_each_set_bit(rid, _mask, sizeof(mask) * 8) {
>> u64 val = regs[i++];
>>
>> printf(".... %-5s 0x%" PRIx64 "\n",
>> --
>> 1.9.1
>>
[toc] | [prev] | [next] | [standalone]
| From | Jiri Olsa <jolsa@redhat.com> |
|---|---|
| Date | 2016-06-17 13:30 +0200 |
| Message-ID | <rL08h-5Sp-5@gated-at.bofh.it> |
| In reply to | #1424839 |
On Fri, Jun 17, 2016 at 02:43:31PM +0530, Madhavan Srinivasan wrote:
SNIP
>
>
> if (data->user_regs.abi) {
> - u64 mask = evsel->attr.sample_regs_user;
> + u.val64 = evsel->attr.sample_regs_user;
>
> - sz = hweight_long(mask) * sizeof(u64);
> + if (sizeof(u64) > sizeof(unsigned long)) {
> + u64 mask = u.val64;
> + u.val32[1] = mask >> 32;
> + u.val32[0] = mask & ULONG_MAX;
> + }
> +
> + sz = hweight_long(u.val64) * sizeof(u64);
> OVERFLOW_CHECK(array, sz, max_size);
> - data->user_regs.mask = mask;
> + data->user_regs.mask = u.val64;
> data->user_regs.regs = (u64 *)array;
>
> Issue I see is when printing the mask value in a 32bit perf
> on a 64bit kernel (big endian).
>
> 442044948492 0xdc0 [0x188]: PERF_RECORD_SAMPLE(IP, 0x1): 7299/7299:
> 0xc000000000059200 period: 12599 addr: 0
> ... intr regs: mask 0xffffffff000007ff ABI 32-bit
> ^^^ shld have been 0x7ffffffffff
>
> I agree it is better to fix this when reading, but
> we need to swap again when printing?
hum, sample data should be swap at this point already..
see perf_session__process_event
jirka
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web