Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1422930 > unrolled thread

[PATCH] tools/perf: fix the word selected in find_*_bit

Back to article view | Back to linux.kernel

Contents

  [PATCH] tools/perf: fix the word selected in find_*_bit Madhavan Srinivasan <maddy@linux.vnet.ibm.com> - 2016-06-15 13:50 +0200
    Re: [PATCH] tools/perf: fix the word selected in find_*_bit "George Spelvin" <linux@sciencehorizons.net> - 2016-06-15 14:50 +0200
      Re: [PATCH] tools/perf: fix the word selected in find_*_bit Madhavan Srinivasan <maddy@linux.vnet.ibm.com> - 2016-06-16 09:30 +0200
    Re: [PATCH] tools/perf: fix the word selected in find_*_bit Arnaldo Carvalho de Melo <acme@kernel.org> - 2016-06-15 23:40 +0200
      [PATCH] tools include: Fix wrong macro definitions for cpu_to_le* for big endian He Kuang <hekuang@huawei.com> - 2016-06-16 03:30 +0200
      [PATCH 1/2] tools include: Sync byteorder/generic.h He Kuang <hekuang@huawei.com> - 2016-06-16 03:40 +0200
        Re: [PATCH 1/2] tools include: Sync byteorder/generic.h Jiri Olsa <jolsa@redhat.com> - 2016-06-16 08:40 +0200
      Re: [PATCH] tools/perf: fix the word selected in find_*_bit Hekuang <hekuang@huawei.com> - 2016-06-16 03:40 +0200
      [PATCH 2/2] tools include: Fix wrong macro definitions for cpu_to_le* for big endian He Kuang <hekuang@huawei.com> - 2016-06-16 03:40 +0200
        Re: [PATCH 2/2] tools include: Fix wrong macro definitions for  cpu_to_le* for big endian Jiri Olsa <jolsa@redhat.com> - 2016-06-16 08:40 +0200
    Re: [PATCH] tools/perf: fix the word selected in find_*_bit Madhavan Srinivasan <maddy@linux.vnet.ibm.com> - 2016-06-16 09:20 +0200
    Re: [PATCH] tools/perf: fix the word selected in find_*_bit Madhavan Srinivasan <maddy@linux.vnet.ibm.com> - 2016-06-16 15:20 +0200

#1422930 — [PATCH] tools/perf: fix the word selected in find_*_bit

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.

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

[toc] | [next] | [standalone]

#1422983

Madhavan Srinivasan wrote:
> +#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

Than you for diagnosing this problem, but I don't think the fix
is correct.

1) It's not clear that all users of _find_next_bit and for_each_set_bit()
   want this change.
2) Is your code even correct?  I'd think you'd want addr[x ^ 1].  Are you
   sure you shpuld be reversing the whole array, and not just the halves of
   each 64-bit word?
3) You've now broken the case of 32-bit big-endian kernel.

I think the proper solution is uglier than this. :-(

[toc] | [prev] | [next] | [standalone]

#1423746

On Wednesday 15 June 2016 06:14 PM, George Spelvin wrote:
> Madhavan Srinivasan wrote:
>> +#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
> Than you for diagnosing this problem, but I don't think the fix
> is correct.
>
> 1) It's not clear that all users of _find_next_bit and for_each_set_bit()
>    want this change.
> 2) Is your code even correct?  I'd think you'd want addr[x ^ 1].  Are you
>    sure you shpuld be reversing the whole array, and not just the halves of
>    each 64-bit word?
> 3) You've now broken the case of 32-bit big-endian kernel.

Yes. But looks like we havent hit this case yet. Will post a fix.

Maddy

>
> I think the proper solution is uglier than this. :-(
>

[toc] | [prev] | [next] | [standalone]

#1423465

Em Thu, Jun 16, 2016 at 12:11:04AM +0300, Yury Norov escreveu:
> On Wed, Jun 15, 2016 at 10:51:27PM +0300, Yury Norov wrote:
> > Maybe there already is some macro doing the conversion for you...
> 
> yes it is, cpu_to_le64() is what you want

Beware that the cpu_to_le64() in tools/perf is bogus, we need to grab a
copy from the kernel sources.

- Arnaldo

[toc] | [prev] | [next] | [standalone]

#1423624 — [PATCH] tools include: Fix wrong macro definitions for cpu_to_le* for big endian

From: Wang Nan <wangnan0@huawei.com>

The cpu_to_le* macros in kernel.h are defined without considering
endianese. This patch includes "byteoder/generic.h" instead to fix the
bug, and removes redundant le64_to_cpu definition in intel-bts.c.

Signed-off-by: Wang Nan <wangnan0@huawei.com>
Signed-off-by: He Kuang <hekuang@huawei.com>
---
 tools/include/linux/kernel.h | 5 ++---
 tools/perf/util/intel-bts.c  | 5 -----
 2 files changed, 2 insertions(+), 8 deletions(-)

diff --git a/tools/include/linux/kernel.h b/tools/include/linux/kernel.h
index 76df535..6145e41 100644
--- a/tools/include/linux/kernel.h
+++ b/tools/include/linux/kernel.h
@@ -64,11 +64,10 @@
 #endif
 
 /*
- * Both need more care to handle endianness
+ * Need more care to handle endianness
  * (Don't use bitmap_copy_le() for now)
  */
-#define cpu_to_le64(x)	(x)
-#define cpu_to_le32(x)	(x)
+#include <linux/byteorder/generic.h>
 
 static inline int
 vscnprintf(char *buf, size_t size, const char *fmt, va_list args)
diff --git a/tools/perf/util/intel-bts.c b/tools/perf/util/intel-bts.c
index 9df9960..0e632c4 100644
--- a/tools/perf/util/intel-bts.c
+++ b/tools/perf/util/intel-bts.c
@@ -40,11 +40,6 @@
 #define INTEL_BTS_ERR_NOINSN  5
 #define INTEL_BTS_ERR_LOST    9
 
-#if __BYTE_ORDER == __BIG_ENDIAN
-#define le64_to_cpu bswap_64
-#else
-#define le64_to_cpu
-#endif
 
 struct intel_bts {
 	struct auxtrace			auxtrace;
-- 
1.8.3.4

[toc] | [prev] | [next] | [standalone]

#1423627 — [PATCH 1/2] tools include: Sync byteorder/generic.h

From: Wang Nan <wangnan0@huawei.com>

This patch copies "include/linux/byteorder/generic.h" to
"tools/include/linux/byteorder/generic.h" to enable other libraries to
use macros in it.

tools/perf/MANIFEST is also updated for 'make perf-*-src-pkg'.

Signed-off-by: Wang Nan <wangnan0@huawei.com>
Signed-off-by: He Kuang <hekuang@huawei.com>
---
 tools/include/linux/byteorder/generic.h | 48 +++++++++++++++++++++++++++++++++
 tools/perf/MANIFEST                     |  1 +
 2 files changed, 49 insertions(+)
 create mode 100644 tools/include/linux/byteorder/generic.h

diff --git a/tools/include/linux/byteorder/generic.h b/tools/include/linux/byteorder/generic.h
new file mode 100644
index 0000000..41b4507
--- /dev/null
+++ b/tools/include/linux/byteorder/generic.h
@@ -0,0 +1,48 @@
+#ifndef _TOOLS_LINUX_BYTEORDER_GENERIC_H
+#define _TOOLS_LINUX_BYTEORDER_GENERIC_H
+
+#include <endian.h>
+#include <byteswap.h>
+
+#define cpu_to_le64 __cpu_to_le64
+#define le64_to_cpu __le64_to_cpu
+#define cpu_to_le32 __cpu_to_le32
+#define le32_to_cpu __le32_to_cpu
+#define cpu_to_le16 __cpu_to_le16
+#define le16_to_cpu __le16_to_cpu
+#define cpu_to_be64 __cpu_to_be64
+#define be64_to_cpu __be64_to_cpu
+#define cpu_to_be32 __cpu_to_be32
+#define be32_to_cpu __be32_to_cpu
+#define cpu_to_be16 __cpu_to_be16
+#define be16_to_cpu __be16_to_cpu
+
+#if __BYTE_ORDER == __BIG_ENDIAN
+#define __cpu_to_le16 bswap_16
+#define __cpu_to_le32 bswap_32
+#define __cpu_to_le64 bswap_64
+#define __le16_to_cpu bswap_16
+#define __le32_to_cpu bswap_32
+#define __le64_to_cpu bswap_64
+#define __cpu_to_be16
+#define __cpu_to_be32
+#define __cpu_to_be64
+#define __be16_to_cpu
+#define __be32_to_cpu
+#define __be64_to_cpu
+#else
+#define __cpu_to_le16
+#define __cpu_to_le32
+#define __cpu_to_le64
+#define __le16_to_cpu
+#define __le32_to_cpu
+#define __le64_to_cpu
+#define __cpu_to_be16 bswap_16
+#define __cpu_to_be32 bswap_32
+#define __cpu_to_be64 bswap_64
+#define __be16_to_cpu bswap_16
+#define __be32_to_cpu bswap_32
+#define __be64_to_cpu bswap_64
+#endif
+
+#endif /* _TOOLS_LINUX_BYTEORDER_GENERIC_H */
diff --git a/tools/perf/MANIFEST b/tools/perf/MANIFEST
index 8c8c6b9..80ac3d4 100644
--- a/tools/perf/MANIFEST
+++ b/tools/perf/MANIFEST
@@ -46,6 +46,7 @@ tools/include/asm-generic/bitops/hweight.h
 tools/include/asm-generic/bitops.h
 tools/include/linux/atomic.h
 tools/include/linux/bitops.h
+tools/include/linux/byteorder/generic.h
 tools/include/linux/compiler.h
 tools/include/linux/filter.h
 tools/include/linux/hash.h
-- 
1.8.5.2

[toc] | [prev] | [next] | [standalone]

#1423708 — Re: [PATCH 1/2] tools include: Sync byteorder/generic.h

On Thu, Jun 16, 2016 at 01:32:08AM +0000, He Kuang wrote:
> From: Wang Nan <wangnan0@huawei.com>
> 
> This patch copies "include/linux/byteorder/generic.h" to
> "tools/include/linux/byteorder/generic.h" to enable other libraries to
> use macros in it.

it's not the file copied, as the changelog suggest,
just several macros, please fix the changelog

thanks,
jirka

> 
> tools/perf/MANIFEST is also updated for 'make perf-*-src-pkg'.
> 
> Signed-off-by: Wang Nan <wangnan0@huawei.com>
> Signed-off-by: He Kuang <hekuang@huawei.com>
> ---
>  tools/include/linux/byteorder/generic.h | 48 +++++++++++++++++++++++++++++++++
>  tools/perf/MANIFEST                     |  1 +
>  2 files changed, 49 insertions(+)
>  create mode 100644 tools/include/linux/byteorder/generic.h
> 
> diff --git a/tools/include/linux/byteorder/generic.h b/tools/include/linux/byteorder/generic.h
> new file mode 100644
> index 0000000..41b4507
> --- /dev/null
> +++ b/tools/include/linux/byteorder/generic.h
> @@ -0,0 +1,48 @@
> +#ifndef _TOOLS_LINUX_BYTEORDER_GENERIC_H
> +#define _TOOLS_LINUX_BYTEORDER_GENERIC_H
> +
> +#include <endian.h>
> +#include <byteswap.h>
> +
> +#define cpu_to_le64 __cpu_to_le64
> +#define le64_to_cpu __le64_to_cpu
> +#define cpu_to_le32 __cpu_to_le32
> +#define le32_to_cpu __le32_to_cpu
> +#define cpu_to_le16 __cpu_to_le16
> +#define le16_to_cpu __le16_to_cpu
> +#define cpu_to_be64 __cpu_to_be64
> +#define be64_to_cpu __be64_to_cpu
> +#define cpu_to_be32 __cpu_to_be32
> +#define be32_to_cpu __be32_to_cpu
> +#define cpu_to_be16 __cpu_to_be16
> +#define be16_to_cpu __be16_to_cpu
> +
> +#if __BYTE_ORDER == __BIG_ENDIAN
> +#define __cpu_to_le16 bswap_16
> +#define __cpu_to_le32 bswap_32
> +#define __cpu_to_le64 bswap_64
> +#define __le16_to_cpu bswap_16
> +#define __le32_to_cpu bswap_32
> +#define __le64_to_cpu bswap_64
> +#define __cpu_to_be16
> +#define __cpu_to_be32
> +#define __cpu_to_be64
> +#define __be16_to_cpu
> +#define __be32_to_cpu
> +#define __be64_to_cpu
> +#else
> +#define __cpu_to_le16
> +#define __cpu_to_le32
> +#define __cpu_to_le64
> +#define __le16_to_cpu
> +#define __le32_to_cpu
> +#define __le64_to_cpu
> +#define __cpu_to_be16 bswap_16
> +#define __cpu_to_be32 bswap_32
> +#define __cpu_to_be64 bswap_64
> +#define __be16_to_cpu bswap_16
> +#define __be32_to_cpu bswap_32
> +#define __be64_to_cpu bswap_64
> +#endif
> +
> +#endif /* _TOOLS_LINUX_BYTEORDER_GENERIC_H */
> diff --git a/tools/perf/MANIFEST b/tools/perf/MANIFEST
> index 8c8c6b9..80ac3d4 100644
> --- a/tools/perf/MANIFEST
> +++ b/tools/perf/MANIFEST
> @@ -46,6 +46,7 @@ tools/include/asm-generic/bitops/hweight.h
>  tools/include/asm-generic/bitops.h
>  tools/include/linux/atomic.h
>  tools/include/linux/bitops.h
> +tools/include/linux/byteorder/generic.h
>  tools/include/linux/compiler.h
>  tools/include/linux/filter.h
>  tools/include/linux/hash.h
> -- 
> 1.8.5.2
> 

[toc] | [prev] | [next] | [standalone]

#1423628

在 2016/6/16 5:29, Arnaldo Carvalho de Melo 写道:
> Em Thu, Jun 16, 2016 at 12:11:04AM +0300, Yury Norov escreveu:
>> On Wed, Jun 15, 2016 at 10:51:27PM +0300, Yury Norov wrote:
>>> Maybe there already is some macro doing the conversion for you...
>> yes it is, cpu_to_le64() is what you want
> Beware that the cpu_to_le64() in tools/perf is bogus, we need to grab a
> copy from the kernel sources.
>
> - Arnaldo
>
[PATCH 1/2] tools include: Sync byteorder/generic.h
[PATCH 2/2] tools include: Fix wrong macro definitions for cpu_to_le* 
for big endian

Here're two patches related to this issue, sorry for wrongly sent two more
reduntant mails.

Thank you.

[toc] | [prev] | [next] | [standalone]

#1423629 — [PATCH 2/2] tools include: Fix wrong macro definitions for cpu_to_le* for big endian

From: Wang Nan <wangnan0@huawei.com>

The cpu_to_le* macros in kernel.h are defined without considering
endianese. This patch includes "byteoder/generic.h" instead to fix the
bug, and removes redundant le64_to_cpu definition in intel-bts.c.

Signed-off-by: Wang Nan <wangnan0@huawei.com>
Signed-off-by: He Kuang <hekuang@huawei.com>
---
 tools/include/linux/kernel.h | 5 ++---
 tools/perf/util/intel-bts.c  | 5 -----
 2 files changed, 2 insertions(+), 8 deletions(-)

diff --git a/tools/include/linux/kernel.h b/tools/include/linux/kernel.h
index 76df535..6145e41 100644
--- a/tools/include/linux/kernel.h
+++ b/tools/include/linux/kernel.h
@@ -64,11 +64,10 @@
 #endif
 
 /*
- * Both need more care to handle endianness
+ * Need more care to handle endianness
  * (Don't use bitmap_copy_le() for now)
  */
-#define cpu_to_le64(x)	(x)
-#define cpu_to_le32(x)	(x)
+#include <linux/byteorder/generic.h>
 
 static inline int
 vscnprintf(char *buf, size_t size, const char *fmt, va_list args)
diff --git a/tools/perf/util/intel-bts.c b/tools/perf/util/intel-bts.c
index 9df9960..0e632c4 100644
--- a/tools/perf/util/intel-bts.c
+++ b/tools/perf/util/intel-bts.c
@@ -40,11 +40,6 @@
 #define INTEL_BTS_ERR_NOINSN  5
 #define INTEL_BTS_ERR_LOST    9
 
-#if __BYTE_ORDER == __BIG_ENDIAN
-#define le64_to_cpu bswap_64
-#else
-#define le64_to_cpu
-#endif
 
 struct intel_bts {
 	struct auxtrace			auxtrace;
-- 
1.8.5.2

[toc] | [prev] | [next] | [standalone]

#1423714 — Re: [PATCH 2/2] tools include: Fix wrong macro definitions for cpu_to_le* for big endian

On Thu, Jun 16, 2016 at 01:32:09AM +0000, He Kuang wrote:
> From: Wang Nan <wangnan0@huawei.com>
> 
> The cpu_to_le* macros in kernel.h are defined without considering
> endianese. This patch includes "byteoder/generic.h" instead to fix the
> bug, and removes redundant le64_to_cpu definition in intel-bts.c.
> 
> Signed-off-by: Wang Nan <wangnan0@huawei.com>
> Signed-off-by: He Kuang <hekuang@huawei.com>
> ---
>  tools/include/linux/kernel.h | 5 ++---
>  tools/perf/util/intel-bts.c  | 5 -----
>  2 files changed, 2 insertions(+), 8 deletions(-)
> 
> diff --git a/tools/include/linux/kernel.h b/tools/include/linux/kernel.h
> index 76df535..6145e41 100644
> --- a/tools/include/linux/kernel.h
> +++ b/tools/include/linux/kernel.h
> @@ -64,11 +64,10 @@
>  #endif
>  
>  /*
> - * Both need more care to handle endianness
> + * Need more care to handle endianness
>   * (Don't use bitmap_copy_le() for now)

what's the purpose of this comment now?

>   */
> -#define cpu_to_le64(x)	(x)
> -#define cpu_to_le32(x)	(x)
> +#include <linux/byteorder/generic.h>
>  
>  static inline int
>  vscnprintf(char *buf, size_t size, const char *fmt, va_list args)
> diff --git a/tools/perf/util/intel-bts.c b/tools/perf/util/intel-bts.c
> index 9df9960..0e632c4 100644
> --- a/tools/perf/util/intel-bts.c
> +++ b/tools/perf/util/intel-bts.c
> @@ -40,11 +40,6 @@
>  #define INTEL_BTS_ERR_NOINSN  5
>  #define INTEL_BTS_ERR_LOST    9
>  
> -#if __BYTE_ORDER == __BIG_ENDIAN
> -#define le64_to_cpu bswap_64
> -#else
> -#define le64_to_cpu
> -#endif

the purpose of this patchset is to unify these macros right?

there're more conversion defines in:
  util/intel-pt-decoder/intel-pt-pkt-decoder.c,

please remove them as well

thanks,
jirka

[toc] | [prev] | [next] | [standalone]

#1423737

On Thursday 16 June 2016 01:21 AM, Yury Norov wrote:
> 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...

Agreed, but reason for proposing fix in lib side is to avoid conversion
on each case (if any in future).

I will repost the fix as suggested.

Maddy
> 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

[toc] | [prev] | [next] | [standalone]

#1424041

On Thursday 16 June 2016 02:41 AM, Yury Norov wrote:
> On Wed, Jun 15, 2016 at 10:51:27PM +0300, Yury Norov wrote:
>> 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...
> yes it is, cpu_to_le64() is what you want

no wait, on second look, cpu_to_le64() is not right.
Because we will end up swapping within 32bit.
But what you suggested looks to be fine. I can repost
this with one minor tweak, right shift with 32 instead
of BITS_PER_LONG (since I see compiler errors in 64bit).

Maddy

>
>> 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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web