From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-10.8 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 115E9C433EF for ; Fri, 10 Sep 2021 12:09:31 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id E3E05611C7 for ; Fri, 10 Sep 2021 12:09:30 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233036AbhIJMKk (ORCPT ); Fri, 10 Sep 2021 08:10:40 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:53922 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232873AbhIJMKg (ORCPT ); Fri, 10 Sep 2021 08:10:36 -0400 Received: from ozlabs.org (ozlabs.org [IPv6:2401:3900:2:1::2]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 2CB9EC061574 for ; Fri, 10 Sep 2021 05:09:24 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ellerman.id.au; s=201909; t=1631275760; bh=pb1Nmeuk6fmE1u4MF/nb7SoeJbvxphmpweu8NYMXWG0=; h=From:To:Cc:Subject:In-Reply-To:References:Date:From; b=FpyqFjWNe6GQOwYAFkVnCFD/CJtU/FeJJ8bjdiHqSBdJTXLDo+8lnn3jQ//cQe71v AjZCT4iPZe74NSnCisluphzHZGybROjY+5GpCT56fWjmaz0B1kLbUzwRO7w62FNLcg lpcp2Nen+RMRL+Ka6HnqusUvBwiwVAfBN6Kh/r2WlnbfJbB7F28xMIdfTZMqrP9App ZBsB8Re2jgMejPC+wX56Yo/bQnNf8GD4kKIroTyqbkK3/zoxTTsIyJZ1vtsSnmUQ38 RPvS87awgC+KG1wloKAeXiD8488kCUDqVeqdR15ykQUtNX+oCQGobsfrZoSrtN01RP uArZnch0plSxw== Received: from authenticated.ozlabs.org (localhost [127.0.0.1]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange ECDHE (P-256) server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by mail.ozlabs.org (Postfix) with ESMTPSA id 4H5ZRf6jq5z9sW5; Fri, 10 Sep 2021 22:09:18 +1000 (AEST) From: Michael Ellerman To: Peter Zijlstra , Stephane Eranian Cc: linux-kernel@vger.kernel.org, acme@redhat.com, jolsa@redhat.com, kim.phillips@amd.com, namhyung@kernel.org, irogers@google.com, atrajeev@linux.vnet.ibm.com, maddy@linux.ibm.com Subject: Re: [PATCH v1 01/13] perf/core: add union to struct perf_branch_entry In-Reply-To: <20210909190342.GE4323@worktop.programming.kicks-ass.net> References: <20210909075700.4025355-1-eranian@google.com> <20210909075700.4025355-2-eranian@google.com> <20210909190342.GE4323@worktop.programming.kicks-ass.net> Date: Fri, 10 Sep 2021 22:09:12 +1000 Message-ID: <878s04my3b.fsf@mpe.ellerman.id.au> MIME-Version: 1.0 Content-Type: text/plain Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Peter Zijlstra writes: > On Thu, Sep 09, 2021 at 12:56:48AM -0700, Stephane Eranian wrote: >> diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h >> index f92880a15645..eb11f383f4be 100644 >> --- a/include/uapi/linux/perf_event.h >> +++ b/include/uapi/linux/perf_event.h >> @@ -1329,13 +1329,18 @@ union perf_mem_data_src { >> struct perf_branch_entry { >> __u64 from; >> __u64 to; >> - __u64 mispred:1, /* target mispredicted */ >> - predicted:1,/* target predicted */ >> - in_tx:1, /* in transaction */ >> - abort:1, /* transaction abort */ >> - cycles:16, /* cycle count to last branch */ >> - type:4, /* branch type */ >> - reserved:40; >> + union { >> + __u64 val; /* to make it easier to clear all fields */ >> + struct { >> + __u64 mispred:1, /* target mispredicted */ >> + predicted:1,/* target predicted */ >> + in_tx:1, /* in transaction */ >> + abort:1, /* transaction abort */ >> + cycles:16, /* cycle count to last branch */ >> + type:4, /* branch type */ >> + reserved:40; >> + }; >> + }; >> }; > > > Hurpmh... all other bitfields have ENDIAN_BITFIELD things except this > one. Power folks, could you please have a look? The bit number of each field changes between big and little endian, but as long as kernel and userspace are the same endian, and both only access values via the bitfields then it works. It's preferable to have the ENDIAN_BITFIELD thing, so that the bit numbers are stable, but we can't change it now without breaking ABI :/ Adding the union risks having code that accesses val and expects to see mispred in bit 0 for example, which it's not on big endian. If it's just for clearing easily we could do that with a static inline that sets all the bitfields. With my compiler here (GCC 10) it's smart enough to just turn it into a single unsigned long store of 0. eg: static inline void clear_perf_branch_entry_flags(struct perf_branch_entry *e) { e->mispred = 0; e->predicted = 0; e->in_tx = 0; e->abort = 0; e->cycles = 0; e->type = 0; e->reserved = 0; } It does look like we have a bug in perf tool though, if I take a perf.data from a big endian system to a little endian one I don't see any of the branch flags decoded. eg: BE: 2413132652524 0x1db8 [0x2d0]: PERF_RECORD_SAMPLE(IP, 0x1): 5279/5279: 0xc00000000045c028 period: 923003 addr: 0 ... branch stack: nr:28 ..... 0: c00000000045c028 -> c00000000dce7604 0 cycles P 0 LE: 2413132652524 0x1db8 [0x2d0]: PERF_RECORD_SAMPLE(IP, 0x1): 5279/5279: 0xc00000000045c028 period: 923003 addr: 0 ... branch stack: nr:28 ..... 0: c00000000045c028 -> c00000000dce7604 0 cycles 0 ^ missing P I guess we're missing a byte swap somewhere. cheers