From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752287AbcFUPkO (ORCPT ); Tue, 21 Jun 2016 11:40:14 -0400 Received: from mail-by2on0058.outbound.protection.outlook.com ([207.46.100.58]:35012 "EHLO na01-by2-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751830AbcFUPkK (ORCPT ); Tue, 21 Jun 2016 11:40:10 -0400 Authentication-Results: spf=none (sender IP is ) smtp.mailfrom=Yuri.Norov@caviumnetworks.com; Date: Tue, 21 Jun 2016 18:35:31 +0300 From: Yury Norov To: Madhavan Srinivasan CC: , , Peter Zijlstra , Ingo Molnar , Arnaldo Carvalho de Melo , Alexander Shishkin , Jiri Olsa , Adrian Hunter , Kan Liang , Wang Nan , Michael Ellerman Subject: Re: [PATCH v3] tools/perf: Fix the mask in regs_dump__printf and print_sample_iregs Message-ID: <20160621153531.GA32361@yury-N73SV> References: <1466521000-11329-1-git-send-email-maddy@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Disposition: inline In-Reply-To: <1466521000-11329-1-git-send-email-maddy@linux.vnet.ibm.com> User-Agent: Mutt/1.5.24 (2015-08-30) X-Originating-IP: [95.143.213.121] X-ClientProxiedBy: HE1PR02CA0081.eurprd02.prod.outlook.com (10.163.170.49) To BN4PR07MB2242.namprd07.prod.outlook.com (10.164.63.148) X-MS-Office365-Filtering-Correlation-Id: 7dfa81dc-202a-43ee-d905-08d399ea4c53 X-Microsoft-Exchange-Diagnostics: 1;BN4PR07MB2242;2:xs8n4Xohv4va6TkWH0t0Eg7OL40ir8mK+RgAIJ3rd0wxsLvbrNwfC4sEvwcm6jWzRrBWAJ0ks8wRo/VFGR05lVesO5jxV/eRoHCA1FCpG+0ZyGBUBI0ZGUJ2j1RFu2jq1U9qxrMb3aVN8pDUW+nkEsohU7gFcOKz4KGWE0K57BVLMXMqThctrXtqGIDHc1Bo;3:vQmS9ZKZA1fGKduL+qt21D0qBXe831VYEe0c9nb/p9LdADisby8wvcsaf39Rhmtug7bzkP1Nz0phQB3KHYFmJ8wiEMQ1T8uB++1sg+AsIsegHLcPmZZFd25mlu/SqGiE X-Microsoft-Antispam: UriScan:;BCL:0;PCL:0;RULEID:;SRVR:BN4PR07MB2242; X-Microsoft-Exchange-Diagnostics: 1;BN4PR07MB2242;25:ljmQW5ouG8f3Czo/JWjmPiaC+vh8l6Fj9DkmSFJj9GSfxgDBDqRejAszkaXTuKzkCoRtDxTN1sursp12lCz7IGDhm3G68iUQRhULEOLf6GQ0p0Hbc5q6eIR/ThybuJtozLGoHAni4NN4+/hjYTKcjlZvkdcfW4aQmlD2ApIgUxMM42m/h1gKyuJjlATRH+QpEZe2kEICV8vVZ35gM8Z/pc03hckIsCSqNTOeqaId01Tv7wlBbpf+6Z6KEnn52/E1YPwvCG1bQqZoBFzTlYChExnDEQK6pRTqQsUEyyNAPx04XMDkYwSv9CFQM84ax2zl4moB4kqz5+x8HmWS0kxVwEEr51iLhcHQzQEi3GissZJuR/6SVOwG80R22g55IPU+yj7l7wEI9XmC0UVHbAx7xXQ+7Q4rqUKihqYLROfTpMj92vN45RMLSeK64YaeYk5RM9VAokrEN0U119CRolWQM0FpN7kNMMFUdsWiH6o+PwY/+2SWTJ6KnWgjApmmQMAonCpvTR5QfY1/J16fCaj50EN/qNCdxVui3I7xEZEQzt23Kmhp4XCSbDwcXgCXkMecUu2iLqDkGCpFCIo3LA/5l41x9gMY2uDH1YBL9WAyJ7a84lz4P6MyuFHO+e6WEbZYHI0p2/VarYfgStzBVKx5XkWF2A4hBTF0hxPy93zU1i2gieHMmW6rmgkCDIXx4uaIJuJwblU4l1Xee2xaPXINRsDjm2IbXreyRSDEaHiHfUa9+ei4Qt/XGbeQqYz1SCN5 X-Microsoft-Exchange-Diagnostics: 1;BN4PR07MB2242;20:MempnVjTNK+6P4O3GY6jX/cVJDKmriLQvGyv0qlY0l4lZhtMIHxuzIeXUbvMDwIyN3PQ1cPATJdForAo5b6A/DyZwx2Gf44XfQd6m7kApECcUDmetZDx/OvuuZwRWa3c/cXYAV9uS1ts4Q13nkwk3zhIr3+nQzTJJcD4hX2eGhGiqrg/YrHdHslYhM7WOGqgU5GVwW0b7liXa/wgwBoWMpNkf1Kv3ijk1e5E4UIB4OewCd9t0KM08UOyAxAT/XGPc6W5x6p/nzyeE9CSK+wyszp/un6LHXwegEbeiCvRKTxhcCoHiwLAFnoXoLRTtMToSm2LItIS0xtKMHuUSkzL5WPQ8w4q8IHM5UibvBSk4kVNMInHm2MfupopEvkCKDfeU2rcfKcxAjJ5+VkTyN1ubYYekTRxy8gtZCQeAL+0R33FHJFvwvkOgvSlVKtzabeCE4idehMWqxQYYiyir4Njh4kgZwaQl9ML8VPFOvAfLveghA54WtL/7xCI9bWsxq1wrO7FGv69GBuWJAVDjiGi2eDkoqqw70lCLoaJjKO1RvuPycCqAUwl3dISXeA0ORZdPKh0S1RbqMwCUPqnoobEW0zbQPCaWLmqmnAakYhV+U8= X-Microsoft-Antispam-PRVS: X-Exchange-Antispam-Report-Test: UriScan:(50582790962513)(104084551191319)(228905959029699); X-Exchange-Antispam-Report-CFA-Test: BCL:0;PCL:0;RULEID:(601004)(2401047)(5005006)(8121501046)(10201501046)(3002001);SRVR:BN4PR07MB2242;BCL:0;PCL:0;RULEID:;SRVR:BN4PR07MB2242; X-Microsoft-Exchange-Diagnostics: 1;BN4PR07MB2242;4:lP+rqclGQpYJ2AgOSe110zOsudNtRHI0VFI2nsGGpKG3Xt9vkJzx0JamsvCqgZGH0KK2touKjJLD0TKghYMWVYvorh9JIhqmng014T4xL6TC9BwfaAUqstTGShm2G4DjIm+sHPko1yqQpEcpT5oVX7TPRpCqunqy9LXHgiXfAcPDyCXY7oCJgd45ZRE0XWpdXzVLrHbh+LRNESIEfbLECIkrjU+Sllbu4XVBDWKAwCHMsCtxW1UFxdI5u/4IK9Z+wrIQELnHwX3i2Kywd41CYR06MNa9ASW1qh6T72bBbH5wwzklqsdXQW1tctZK9ny4IYgdJjsEYDzMcw+njFffRL+epXTTWZyBJRZ997DrRrx3Je88zA55rUGQX5VIqE+tsypbisWK8LwfUTPlYC2vuTQ1GDSlf+TPzkUkiScydfoR4vrVVP0QJ511hbTI3r8nk3kN5kHJH+75IEpzXrQdNjLP4wRdLwDvBTVdQJZk4es= X-Forefront-PRVS: 098076C36C X-Forefront-Antispam-Report: SFV:NSPM;SFS:(10009020)(4630300001)(979002)(6069001)(6009001)(7916002)(24454002)(54534003)(199003)(189002)(46406003)(19580395003)(42186005)(19580405001)(76506005)(77096005)(9686002)(7736002)(1076002)(83506001)(97756001)(6116002)(3846002)(586003)(110136002)(105586002)(81156014)(92566002)(2906002)(2950100001)(50986999)(7846002)(33716001)(66066001)(50466002)(76176999)(47776003)(68736007)(54356999)(23726003)(8676002)(33656002)(4326007)(101416001)(106356001)(189998001)(97736004)(81166006)(4001350100001)(969003)(989001)(999001)(1009001)(1019001);DIR:OUT;SFP:1101;SCL:1;SRVR:BN4PR07MB2242;H:localhost;FPR:;SPF:None;PTR:InfoNoRecords;A:1;MX:1;LANG:en; X-Microsoft-Exchange-Diagnostics: =?us-ascii?Q?1;BN4PR07MB2242;23:W49QHtDMpE5hwFdGgKFuv6Lkpg9fpiAiySmFahG6r?= =?us-ascii?Q?TWLETHm3B5QsCo/Z4eDrUjIxConLpziS5MzMbeakPTuKZgD/gyMqbd/XrHeU?= =?us-ascii?Q?s78kJQScQiDXAJFNGueCDSXwUf9gHsKD1xVxTB0z2g+yBW+Tq6xOqjT5/Ij6?= =?us-ascii?Q?0CLn2IxrVN2zX2ZVfbceExx/XMo0KKP/+/jX4zHXKdHgHxe+PxhBh0x4esvi?= =?us-ascii?Q?2cDMFaXgqGzBe0VDwjUB5ydBp52zMHGMcvTG+P0gynftWmZxC4YPj3gYAYte?= =?us-ascii?Q?vnEgShDxt/aT4Mg5D3uIkchLLdL/0oEEGzCRnFG8KCB4OkNxlbELa8C0Lf6q?= =?us-ascii?Q?Imilb6r/deBhYZiQ0ZIMFUMvJ+CPU1Syofh8yKYWHo6KmDAHmu/6jBXIfbeb?= =?us-ascii?Q?uDb/CKWqrfGnmZLCH0sR4sWbtI8ACD3kLfxDdhABuVRPD1/b4AAD+r4xZQzk?= =?us-ascii?Q?N2FSjN8qJdfkRk+jsQWmn8aiuCAVpibCUt2j6Y3Fi7gZnF1MDPexTn6d3CSy?= =?us-ascii?Q?IPdvjntn0VD2k9Cxup8vjS60yMAQPCW0QpR8QfdOc/7868bSKCAg9c9WR0zu?= =?us-ascii?Q?gPqsbocYslzO498fwkkA6ppL2G1qsspyg//theo1ICAutsTkyU4N0NmhVXE/?= =?us-ascii?Q?AiHDXRDrUKyBlksEvFcl9064ur4k1HYDVgwpjqL8EfWy7jJkKd6XtKTdx6h3?= =?us-ascii?Q?4aCJR6TYe07WZ61uIilhbEdkX4fM99EA3OC8t6ScRwE5JHR3X+KKfPp5aXsS?= =?us-ascii?Q?tlchOUEozTyt5FaXAvvSkkzLcN1u97bnMTlzwkPDVRL+sG20ZGoHpWCzsj1V?= =?us-ascii?Q?0MYdPv8LLm42gJjzEF8VNbLzskyUhytIXzIA2W9EnegBir5pHplbTeVmKwd2?= =?us-ascii?Q?SeAAddzu/3FTOzA0+uJQkQm5bTh+fY7EiEuGhmA110wcSSRAwutIlxw8od6r?= =?us-ascii?Q?nT4hFh1BM3haLgWUyYTEWUHXl0PYhhBvhr9+V+NMprvYAsyIYx5lUKNIFmQe?= =?us-ascii?Q?hIVVqIU8Y8jazduom+S3Rg7gAUSiLvS+zgnwlByPYW94TheHqYAb2dobsSLs?= =?us-ascii?Q?KrTZ+1rv5xRdfZo4vDzt3KEMRRt9Y79XS+gagJr9LIT+3K48bC5Hs5xJlvi+?= =?us-ascii?Q?7pjhEsCoS0r2mRtHJ2UnlAalwoQza0iaWBYs3jeDBIX5VG/gft4FQwaQtlmp?= =?us-ascii?Q?+ly2tWhY7lsGMdUzRUHYiHfuaouoNwDjhcWRaEq+2kXGDM4lj7mIQGbqLCpf?= =?us-ascii?Q?lN0LE7//5LtCeB3c5G8aFOEufcCRJyIWDFkvrmW?= X-Microsoft-Exchange-Diagnostics: 1;BN4PR07MB2242;6:1fEOaCmIxmImZ86praH4Enl7+EPqmIGQrOCoXfOH+I3/k4ptM/px/0tqouUeJyZvXrlv1Uae+UpAEFhm4NFe2p7wuX04B5kGDMX5FJKQnUml+K915UXR9Hw/ReS3rsiTRXapx+1+/mk2L+KKhQ8NnRkyiC1ZwsPFf5mRlYu8tcZYGQsT6W7QjDPg67IRIsnnOqsbhoXc8sWpKOCVS6LT9xxVq7k+wpcHGhH5vWWMVjdjYIN0s3sh9t6rQshQBeAuw0dCLj9PHj9JTXYhwcwh9NwpissaQR0orDEaBuJg0tY=;5:yioqozDEWZLMguTu3RGTdp8I67Ll9EfyVn9fG1izLhz+cjkHxQRv1MT+kK/7X+ApQpD5rTpNQh8+pknjYm2f80Mji2/+vjlbEwp+OO6y1hnwdGng98W0Lwh/GLax61yyTA6TH608nJqjqd76M7KGyg==;24:PbSMYW43OQH3A4SsR2NprPsR7w7lGWEBgN2wTLT1/jMUxwqO6+fS2OqICZuzSre1aJTEUxVu+6eGbwbiqx4V+pUfkt8aHlE0/Q6faqzILtk=;7:jEgBqM/E9D9Tix5Nwi4XqwhmLYWSBpr7+UHE8rMAQ0ccvpJuhYT71Hk/yKjnrvohCaZp63yNHx6HXnl0SBzwC6FU+jORSaGrrLlS/pqZt0uo6B2GKi0XU8kie6aa3A5oK6MIzDg9ODoZYLHqNx+SNgPTGqMJDq5ts0QfTXNB+Z1O6td2eXChVHECnG+lc39OAWrtsOG3iV6GtB/Oc8eQh+n0k+JMTPNp8oHEN4dadnqs0EfBM1oTq3qWjsevMHhP SpamDiagnosticOutput: 1:99 SpamDiagnosticMetadata: NSPM X-OriginatorOrg: caviumnetworks.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 21 Jun 2016 15:39:56.7028 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN4PR07MB2242 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Jun 21, 2016 at 08:26:40PM +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). > When reading u64 using (u32 *)(&val)[0], perf (lib/find_*_bit()) assumes it gets > lower 32bits of u64 which is wrong. Proposed fix is to swap the words > of the u64 to handle this case. This is _not_ endianess swap. > > Suggested-by: Yury Norov > Cc: Yury Norov > Cc: Peter Zijlstra > Cc: Ingo Molnar > Cc: Arnaldo Carvalho de Melo > Cc: Alexander Shishkin > Cc: Jiri Olsa > Cc: Adrian Hunter > Cc: Kan Liang > Cc: Wang Nan > Cc: Michael Ellerman > Signed-off-by: Madhavan Srinivasan > --- > Changelog v2: > 1)Moved the swap code to a common function > 2)Added more comments in the code > > Changelog v1: > 1)updated commit message and patch subject > 2)Add the fix to print_sample_iregs() in builtin-script.c > > tools/include/linux/bitmap.h | 9 +++++++++ What about include/linux/bitmap.h? I think we'd place it there first. > tools/perf/builtin-script.c | 16 +++++++++++++++- > tools/perf/util/session.c | 16 +++++++++++++++- > 3 files changed, 39 insertions(+), 2 deletions(-) > > diff --git a/tools/include/linux/bitmap.h b/tools/include/linux/bitmap.h > index 28f5493da491..79998b26eb04 100644 > --- a/tools/include/linux/bitmap.h > +++ b/tools/include/linux/bitmap.h > @@ -2,6 +2,7 @@ > #define _PERF_BITOPS_H > > #include > +#include > #include > > #define DECLARE_BITMAP(name,bits) \ > @@ -22,6 +23,14 @@ void __bitmap_or(unsigned long *dst, const unsigned long *bitmap1, > #define small_const_nbits(nbits) \ > (__builtin_constant_p(nbits) && (nbits) <= BITS_PER_LONG) > > +static inline void bitmap_from_u64(unsigned long *_mask, u64 mask) Inline is not required. Some people don't not like it. Underscored parameter in function declaration is not the best idea as well. Try: static void bitmap_from_u64(unsigned long *bitmap, u64 mask) > +{ > + _mask[0] = mask & ULONG_MAX; > + > + if (sizeof(mask) > sizeof(unsigned long)) > + _mask[1] = mask >> 32; > +} > + > static inline void bitmap_zero(unsigned long *dst, int nbits) > { > if (small_const_nbits(nbits)) > diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c > index e3ce2f34d3ad..73928310fd91 100644 > --- a/tools/perf/builtin-script.c > +++ b/tools/perf/builtin-script.c > @@ -412,11 +412,25 @@ 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 we start with it, I think we'd hide declaration machinery as well: #define DECLARE_L64_BITMAP(__name) unsigned long __name[sizeof(u64)/sizeof(unsigned long)] or #define L64_BITMAP_SIZE (sizeof(u64)/sizeof(unsigned long)) Or both :) Whatever you prefer. > > if (!regs) > return; > > - for_each_set_bit(r, (unsigned long *) &mask, sizeof(mask) * 8) { > + /* > + * Since u64 is passed as 'unsigned long *', check > + * to see whether we need to swap words within u64. > + * Reason being, in 32 bit big endian userspace on a > + * 64bit kernel, 'unsigned long' is 32 bits. > + * When reading u64 using (u32 *)(&val)[0] and (u32 *)(&val)[1], > + * we will get wrong value for the mask. This is what > + * find_first_bit() and find_next_bit() is doing. > + * Issue here is "(u32 *)(&val)[0]" gets upper 32 bits of u64, > + * but perf assumes it gets lower 32bits of u64. Hence the check > + * and swap. > + */ > + bitmap_from_u64(_mask, mask); > + 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..1337b1c73f82 100644 > --- a/tools/perf/util/session.c > +++ b/tools/perf/util/session.c > @@ -940,8 +940,22 @@ 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) { > + /* > + * Since u64 is passed as 'unsigned long *', check > + * to see whether we need to swap words within u64. > + * Reason being, in 32 bit big endian userspace on a > + * 64bit kernel, 'unsigned long' is 32 bits. > + * When reading u64 using (u32 *)(&val)[0] and (u32 *)(&val)[1], > + * we will get wrong value for the mask. This is what > + * find_first_bit() and find_next_bit() is doing. > + * Issue here is "(u32 *)(&val)[0]" gets upper 32 bits of u64, > + * but perf assumes it gets lower 32bits of u64. Hence the check > + * and swap. > + */ Identical comments... I'd prefer to see it in commit message, or better in function description. For me it's pretty straightforward in understanding what happens here in-place without comments. > + bitmap_from_u64(_mask, mask); > + for_each_set_bit(rid, _mask, sizeof(mask) * 8) { > u64 val = regs[i++]; > > printf(".... %-5s 0x%" PRIx64 "\n", > -- > 1.9.1