From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from na01-bl2-obe.outbound.protection.outlook.com (mail-bl2on0082.outbound.protection.outlook.com [65.55.169.82]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-SHA384 (256/256 bits)) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id 3rVHbT6hJ0zDqZf for ; Thu, 16 Jun 2016 06:06:45 +1000 (AEST) Date: Wed, 15 Jun 2016 22:51:27 +0300 From: Yury Norov To: Madhavan Srinivasan CC: , , "Arnaldo Carvalho de Melo" , Adrian Hunter , Borislav Petkov , David Ahern , George Spelvin , Jiri Olsa , Namhyung Kim , Rasmus Villemoes , Wang Nan , Yury Norov , Michael Ellerman Subject: Re: [PATCH] tools/perf: fix the word selected in find_*_bit Message-ID: <20160615195127.GA6039@yury-N73SV> References: <1465990973-31483-1-git-send-email-maddy@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" In-Reply-To: <1465990973-31483-1-git-send-email-maddy@linux.vnet.ibm.com> List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , 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 > Cc: Adrian Hunter > Cc: Borislav Petkov > Cc: David Ahern > Cc: George Spelvin > Cc: Jiri Olsa > Cc: Namhyung Kim > Cc: Rasmus Villemoes > Cc: Wang Nan > Cc: Yury Norov > Cc: Michael Ellerman > Signed-off-by: Madhavan Srinivasan > --- > 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