From: Yury Norov <ynorov@caviumnetworks.com>
To: Madhavan Srinivasan <maddy@linux.vnet.ibm.com>
Cc: <linux-kernel@vger.kernel.org>, <linuxppc-dev@lists.ozlabs.org>,
Arnaldo Carvalho de Melo <acme@kernel.org>,
Adrian Hunter <adrian.hunter@intel.com>,
Borislav Petkov <bp@suse.de>, David Ahern <dsahern@gmail.com>,
George Spelvin <linux@horizon.com>, Jiri Olsa <jolsa@redhat.com>,
Namhyung Kim <namhyung@kernel.org>,
Rasmus Villemoes <linux@rasmusvillemoes.dk>,
Wang Nan <wangnan0@huawei.com>, Yury Norov <yury.norov@gmail.com>,
Michael Ellerman <mpe@ellerman.id.au>
Subject: Re: [PATCH] tools/perf: fix the word selected in find_*_bit
Date: Wed, 15 Jun 2016 22:51:27 +0300 [thread overview]
Message-ID: <20160615195127.GA6039@yury-N73SV> (raw)
In-Reply-To: <1465990973-31483-1-git-send-email-maddy@linux.vnet.ibm.com>
Hi Madhavan,
On Wed, Jun 15, 2016 at 05:12:53PM +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 reading the wrong word. In find_first_bit(),
> one word at a time (based on BITS_PER_LONG) is loaded and
> checked for any bit set. In 32bit BE userspace,
> BITS_PER_LONG turns out to be 32, and for a mask value of
> "0x00000000000000ff", find_first_bit will return 32, instead of 0.
> Reason for this is that, value in the word0 is all zeros and value
> in word1 is 0xff. Ideally, second word in the mask should be loaded
> and searched. Patch swaps the word to look incase of 32bit BE.
I think this is not a problem of find_bit() at all. You have wrong
typecast as the source of problem (tools/perf/util/session.c"):
940 static void regs_dump__printf(u64 mask, u64 *regs)
941 {
942 unsigned rid, i = 0;
943
944 for_each_set_bit(rid, (unsigned long *) &mask, sizeof(mask) * 8) {
^^^^ Here ^^^^
945 u64 val = regs[i++];
946
947 printf(".... %-5s 0x%" PRIx64 "\n",
948 perf_reg_name(rid), val);
949 }
950 }
But for some reason you change correct find_bit()...
Though proper fix is like this for me:
static void regs_dump__printf(u64 mask, u64 *regs)
{
unsigned rid, i = 0;
unsigned long _mask[sizeof(mask)/sizeof(unsigned long)];
_mask[0] = mask & ULONG_MAX;
if (sizeof(mask) > sizeof(unsigned long))
_mask[1] = mask >> BITS_PER_LONG;
for_each_set_bit(rid, _mask, sizeof(mask) * BITS_PER_BYTE) {
u64 val = regs[i++];
printf(".... %-5s 0x%" PRIx64 "\n",
perf_reg_name(rid), val);
}
}
Maybe there already is some macro doing the conversion for you...
Yury.
> Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
> Cc: Adrian Hunter <adrian.hunter@intel.com>
> Cc: Borislav Petkov <bp@suse.de>
> Cc: David Ahern <dsahern@gmail.com>
> Cc: George Spelvin <linux@horizon.com>
> Cc: Jiri Olsa <jolsa@redhat.com>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> Cc: Wang Nan <wangnan0@huawei.com>
> Cc: Yury Norov <yury.norov@gmail.com>
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Signed-off-by: Madhavan Srinivasan <maddy@linux.vnet.ibm.com>
> ---
> tools/lib/find_bit.c | 17 +++++++++++++++++
> 1 file changed, 17 insertions(+)
>
> diff --git a/tools/lib/find_bit.c b/tools/lib/find_bit.c
> index 9122a9e80046..996b3e04324f 100644
> --- a/tools/lib/find_bit.c
> +++ b/tools/lib/find_bit.c
> @@ -37,7 +37,12 @@ static unsigned long _find_next_bit(const unsigned long *addr,
> if (!nbits || start >= nbits)
> return nbits;
>
> +#if (__BYTE_ORDER == __BIG_ENDIAN) && (BITS_PER_LONG != 64)
> + tmp = addr[(((nbits - 1)/BITS_PER_LONG) - (start / BITS_PER_LONG))]
> + ^ invert;
> +#else
> tmp = addr[start / BITS_PER_LONG] ^ invert;
> +#endif
>
> /* Handle 1st word. */
> tmp &= BITMAP_FIRST_WORD_MASK(start);
> @@ -48,7 +53,12 @@ static unsigned long _find_next_bit(const unsigned long *addr,
> if (start >= nbits)
> return nbits;
>
> +#if (__BYTE_ORDER == __BIG_ENDIAN) && (BITS_PER_LONG != 64)
> + tmp = addr[(((nbits - 1)/BITS_PER_LONG) - (start / BITS_PER_LONG))]
> + ^ invert;
> +#else
> tmp = addr[start / BITS_PER_LONG] ^ invert;
> +#endif
> }
>
> return min(start + __ffs(tmp), nbits);
> @@ -75,8 +85,15 @@ unsigned long find_first_bit(const unsigned long *addr, unsigned long size)
> unsigned long idx;
>
> for (idx = 0; idx * BITS_PER_LONG < size; idx++) {
> +#if (__BYTE_ORDER == __BIG_ENDIAN) && (BITS_PER_LONG != 64)
> + if (addr[(((size-1)/BITS_PER_LONG) - idx)])
> + return min(idx * BITS_PER_LONG +
> + __ffs(addr[(((size-1)/BITS_PER_LONG) - idx)]),
> + size);
> +#else
> if (addr[idx])
> return min(idx * BITS_PER_LONG + __ffs(addr[idx]), size);
> +#endif
> }
>
> return size;
> --
> 1.9.1
next prev parent reply other threads:[~2016-06-15 19:51 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-06-15 11:42 [PATCH] tools/perf: fix the word selected in find_*_bit Madhavan Srinivasan
2016-06-15 12:44 ` George Spelvin
2016-06-16 7:21 ` Madhavan Srinivasan
2016-06-15 19:51 ` Yury Norov [this message]
2016-06-15 21:11 ` Yury Norov
2016-06-15 21:29 ` Arnaldo Carvalho de Melo
2016-06-16 1:27 ` [PATCH] tools include: Fix wrong macro definitions for cpu_to_le* for big endian He Kuang
2016-06-16 1:32 ` He Kuang
2016-06-16 1:32 ` [PATCH 1/2] tools include: Sync byteorder/generic.h He Kuang
2016-06-16 6:39 ` Jiri Olsa
2016-06-16 1:32 ` [PATCH 2/2] tools include: Fix wrong macro definitions for cpu_to_le* for big endian He Kuang
2016-06-16 6:21 ` kbuild test robot
2016-06-16 6:39 ` Jiri Olsa
2016-06-16 1:35 ` [PATCH] tools/perf: fix the word selected in find_*_bit Hekuang
2016-06-16 13:11 ` Madhavan Srinivasan
2016-06-16 7:15 ` Madhavan Srinivasan
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20160615195127.GA6039@yury-N73SV \
--to=ynorov@caviumnetworks.com \
--cc=acme@kernel.org \
--cc=adrian.hunter@intel.com \
--cc=bp@suse.de \
--cc=dsahern@gmail.com \
--cc=jolsa@redhat.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux@horizon.com \
--cc=linux@rasmusvillemoes.dk \
--cc=linuxppc-dev@lists.ozlabs.org \
--cc=maddy@linux.vnet.ibm.com \
--cc=mpe@ellerman.id.au \
--cc=namhyung@kernel.org \
--cc=wangnan0@huawei.com \
--cc=yury.norov@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).