Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Song Liu <songliubraving@fb.com>
To:     bpf <bpf@vger.kernel.org>,
        open list <linux-kernel@vger.kernel.org>,
        "Peter Ziljstra" <peterz@infradead.org>
CC:     Arnaldo Carvalho de Melo <acme@kernel.org>,
        Ingo Molnar <mingo@redhat.com>,
        Kajol Jain <kjain@linux.ibm.com>,
        Kernel Team <Kernel-team@fb.com>
Subject: Re: [PATCH v6 bpf-next 0/3] bpf: introduce bpf_get_branch_snapshot
Thread-Topic: [PATCH v6 bpf-next 0/3] bpf: introduce bpf_get_branch_snapshot
Thread-Index: AQHXpCbirPiXWtbxT0SKMqbyy1st0quZBdMAgAM8BQA=
Date:   Thu, 9 Sep 2021 21:53:09 +0000
Message-ID: <30C533EB-171C-4A7F-BDC6-12C191219EE9@fb.com>
References: <20210907202802.3675104-1-songliubraving@fb.com>
 <F638720E-F0A7-468F-8862-907CFB19E4A2@fb.com>
In-Reply-To: <F638720E-F0A7-468F-8862-907CFB19E4A2@fb.com>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?b0RU8zmZRgsj6kuBZDt6Bu12qChr6JPel1/6h5JLXxHQ/8Rv8W6d+JEB4sdi?=
 =?us-ascii?Q?z5CWd7nKiOk8RXbOgShRnMOQ8EnoAgRYtVnt5iI8UAjljZd7S1bJYzgsIc5q?=
 =?us-ascii?Q?2JNDoDLl2lq/5PkGxjPGH9nBlB6x8Tdp/Mup3acEJT/4A9HchbvVKNs2LxMB?=
 =?us-ascii?Q?zD/JIhZRt2/GotD3Y70SrnYRJlwbZXPlUHnQanNwjkDHbz9tpCOc0xwGRj5J?=
 =?us-ascii?Q?7AyW6ZjlWcHNU4VEXvyDfypmRM8RwSM6EU4Nsu+doxE2vHqVRxt+b08At6Y4?=
 =?us-ascii?Q?V2+EN8n4qU7BaxMtMpAEtPJ0xF6a+8/kKVc7g8kTyXzvoctWZcif+kIH6XFU?=
 =?us-ascii?Q?puEdN6T5vYZM4n691cUlepMPM7L2T6aoAhMjrSB4r95nolK1RnfctvP4J8AL?=
 =?us-ascii?Q?I9gJULdPTG2z+gFc669I8W+8nSzydRFmpu2+RZOH0aN8RP0/TQypJ38xpu5P?=
 =?us-ascii?Q?+x6k3o8At+mZaUTV5C9O+ohg9QwARfpRd7utXNo6t3YPrwY6iZo8GHC3loeU?=
 =?us-ascii?Q?NDDcGYgqYQWfHOclexOMT42vsZ+IOD7Rxajb6LwZ8TSip9O4x7BLMG6hFzow?=
 =?us-ascii?Q?dz2ZAlEvzfgI0n52G2kgweMDmjBIANGGCsUIiRSf58g+pWZmSly4ZfkuK2NP?=
 =?us-ascii?Q?Catw9Th2lfaX0rmZHGNhxjLNc0IzJhnY8+gJaD5mgTqFrtjypn89JPCpqaxZ?=
 =?us-ascii?Q?G411+yqwkRvfFPrC0cei5M7z7AUe1hpKevp6NVQsi+LQVVGa1pHc684gDuQd?=
 =?us-ascii?Q?ZVhFCZhq+ntMwfXJqUFF4LN7Mleu31mXnzPfTI61/4HY3f93PeOgECIBqrZB?=
 =?us-ascii?Q?n1Kid6AqetrS2o50+8/NCnrZ2aeUw4z4EyLL1pA1pJo/qpE7IKkc56Im1dnj?=
 =?us-ascii?Q?V/wKJf/dnMHQCFjtYOELhmN/UjSRU2hcv1du7Ug81HuDC4zTx5Ce4dusJoOf?=
 =?us-ascii?Q?6oarjLGKHnrjI4+R1QOohAsppr2+bd8BmRSeuyIGBQWW1VfK2L2XuvEVWbzO?=
 =?us-ascii?Q?rwV8pQFNCfCfAjOIyK4uPrtDrIL1jnAqpALKXiRTdAfxENwABLyAabBC/PfU?=
 =?us-ascii?Q?1RWjiTLUmMfvqIAXhlEVU/L/wHSCjkZY4rqLny2ov9g3Ql9TchUbDZd+f7cv?=
 =?us-ascii?Q?7XMRR3XrmfvYSP5jar39Z3v0mVuyHhdyywTwIkhwiU0EouW71hi12PxeCPaZ?=
 =?us-ascii?Q?zGnfthlX8ShmgYYkQ/qr3y+yG+IRhvQ5JPb95pTiFJDJGAiuvumVIXwRgM02?=
 =?us-ascii?Q?v34oA+F/cykOLzOAkWzXrY5pdV/YlE+z4PIrA7H3tgDy+aB8LsNkFzXMflpQ?=
 =?us-ascii?Q?ilQTs37wk0H1hzZC5bP/2+iKxayg8UCYnoPetMo5SWYpXbB25cOGP9NAv32p?=
 =?us-ascii?Q?sBPAWWQ=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <39038576B508774DA02F3DF36EFCDD4E@namprd15.prod.outlook.com>
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: SA1PR15MB5109.namprd15.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: c9311ee6-52a8-4772-9aac-08d973dc368e
X-MS-Exchange-CrossTenant-originalarrivaltime: 09 Sep 2021 21:53:09.3825
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 8ae927fe-1255-47a7-a2af-5f3a069daaa2
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: W/1LQu1i+ctAZKjUMOre3OfSJp+KnKBvD6g9s3YXQPeu0Mti3pXrX9wTAT+0JmhDCFsRAcSMW7B+Yx33GHwAlQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA1PR15MB5112
X-OriginatorOrg: fb.com
X-Proofpoint-ORIG-GUID: Zpzx_nIWe2tucre571PF1jnc1JALh0Zj
X-Proofpoint-GUID: Zpzx_nIWe2tucre571PF1jnc1JALh0Zj
X-Proofpoint-UnRewURL: 0 URL was un-rewritten
MIME-Version: 1.0
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.391,18.0.790
 definitions=2021-09-09_08:2021-09-09,2021-09-09 signatures=0
X-Proofpoint-Spam-Details: rule=fb_default_notspam policy=fb_default score=0 suspectscore=0
 clxscore=1015 mlxscore=0 malwarescore=0 impostorscore=0 phishscore=0
 bulkscore=0 spamscore=0 adultscore=0 mlxlogscore=999 lowpriorityscore=0
 priorityscore=1501 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2109030001 definitions=main-2109090134
X-FB-Internal: deliver
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Peter, 

Do you have further comments/concerns on v6? If not, could you please 
reply with your Reviewed-by or Acked-by?

Thanks,
Song

> On Sep 7, 2021, at 1:29 PM, Song Liu <songliubraving@fb.com> wrote:
> 
> 
> 
>> On Sep 7, 2021, at 1:27 PM, Song Liu <songliubraving@fb.com> wrote:
> 
> Forgot to add changes:
> 
> Changes v5 => v6
> 1. Add local_irq_save/restore to intel_pmu_snapshot_branch_stack. 
>   (Peter)
> 2. Remove buf and size check in bpf_get_branch_snapshot, move flags 
>   check to later fo the function. (Peter, Andrii)
> 3. Revise comments for bpf_get_branch_snapshot in bpf.h (Andrii)
> 
>> 
>> Changes v4 => v5
>> 1. Modify perf_snapshot_branch_stack_t to save some memcpy. (Andrii)
>> 2. Minor fixes in selftests. (Andrii)
>> 
>> Changes v3 => v4:
>> 1. Do not reshuffle intel_pmu_disable_all(). Use some inline to save LBR
>>  entries. (Peter)
>> 2. Move static_call(perf_snapshot_branch_stack) to the helper. (Alexei)
>> 3. Add argument flags to bpf_get_branch_snapshot. (Andrii)
>> 4. Make MAX_BRANCH_SNAPSHOT an enum (Andrii). And rename it as
>>  PERF_MAX_BRANCH_SNAPSHOT
>> 5. Make bpf_get_branch_snapshot similar to bpf_read_branch_records.
>>  (Andrii)
>> 6. Move the test target function to bpf_testmod. Updated kallsyms_find_next
>>  to work properly with modules. (Andrii)
>> 
>> Changes v2 => v3:
>> 1. Fix the use of static_call. (Peter)
>> 2. Limit the use to perfmon version >= 2. (Peter)
>> 3. Modify intel_pmu_snapshot_branch_stack() to use intel_pmu_disable_all
>>  and intel_pmu_enable_all().
>> 
>> Changes v1 => v2:
>> 1. Rename the helper as bpf_get_branch_snapshot;
>> 2. Fix/simplify the use of static_call;
>> 3. Instead of percpu variables, let intel_pmu_snapshot_branch_stack output
>>  branch records to an output argument of type perf_branch_snapshot.
>> 
>> Branch stack can be very useful in understanding software events. For
>> example, when a long function, e.g. sys_perf_event_open, returns an errno,
>> it is not obvious why the function failed. Branch stack could provide very
>> helpful information in this type of scenarios.
>> 
>> This set adds support to read branch stack with a new BPF helper
>> bpf_get_branch_trace(). Currently, this is only supported in Intel systems.
>> It is also possible to support the same feaure for PowerPC.
>> 
>> The hardware that records the branch stace is not stopped automatically on
>> software events. Therefore, it is necessary to stop it in software soon.
>> Otherwise, the hardware buffers/registers will be flushed. One of the key
>> design consideration in this set is to minimize the number of branch record
>> entries between the event triggers and the hardware recorder is stopped.
>> Based on this goal, current design is different from the discussions in
>> original RFC [1]:
>> 1) Static call is used when supported, to save function pointer
>>   dereference;
>> 2) intel_pmu_lbr_disable_all is used instead of perf_pmu_disable(),
>>   because the latter uses about 10 entries before stopping LBR.
>> 
>> With current code, on Intel CPU, LBR is stopped after 10 branch entries
>> after fexit triggers:
>> 
>> ID: 0 from intel_pmu_lbr_disable_all+58 to intel_pmu_lbr_disable_all+93
>> ID: 1 from intel_pmu_lbr_disable_all+54 to intel_pmu_lbr_disable_all+58
>> ID: 2 from intel_pmu_snapshot_branch_stack+102 to intel_pmu_lbr_disable_all+0
>> ID: 3 from bpf_get_branch_snapshot+18 to intel_pmu_snapshot_branch_stack+0
>> ID: 4 from bpf_get_branch_snapshot+18 to bpf_get_branch_snapshot+0
>> ID: 5 from __brk_limit+474918983 to bpf_get_branch_snapshot+0
>> ID: 6 from __bpf_prog_enter+34 to __brk_limit+474918971
>> ID: 7 from migrate_disable+60 to __bpf_prog_enter+9
>> ID: 8 from __bpf_prog_enter+4 to migrate_disable+0
>> ID: 9 from bpf_testmod_loop_test+20 to __bpf_prog_enter+0
>> ID: 10 from bpf_testmod_loop_test+20 to bpf_testmod_loop_test+13
>> ID: 11 from bpf_testmod_loop_test+20 to bpf_testmod_loop_test+13
>> ID: 12 from bpf_testmod_loop_test+20 to bpf_testmod_loop_test+13
>> ID: 13 from bpf_testmod_loop_test+20 to bpf_testmod_loop_test+13
>> ...
>> 
>> [1] https://lore.kernel.org/bpf/20210818012937.2522409-1-songliubraving@fb.com/
>> 
>> Song Liu (3):
>> perf: enable branch record for software events
>> bpf: introduce helper bpf_get_branch_snapshot
>> selftests/bpf: add test for bpf_get_branch_snapshot
>> 
>> arch/x86/events/intel/core.c                  |  29 ++++-
>> arch/x86/events/intel/ds.c                    |   8 --
>> arch/x86/events/perf_event.h                  |  10 +-
>> include/linux/perf_event.h                    |  23 ++++
>> include/uapi/linux/bpf.h                      |  22 ++++
>> kernel/bpf/trampoline.c                       |   3 +-
>> kernel/events/core.c                          |   2 +
>> kernel/trace/bpf_trace.c                      |  30 ++++++
>> tools/include/uapi/linux/bpf.h                |  22 ++++
>> .../selftests/bpf/bpf_testmod/bpf_testmod.c   |  19 +++-
>> .../selftests/bpf/prog_tests/core_reloc.c     |  14 +--
>> .../bpf/prog_tests/get_branch_snapshot.c      | 100 ++++++++++++++++++
>> .../selftests/bpf/prog_tests/module_attach.c  |  39 -------
>> .../selftests/bpf/progs/get_branch_snapshot.c |  40 +++++++
>> tools/testing/selftests/bpf/test_progs.c      |  39 +++++++
>> tools/testing/selftests/bpf/test_progs.h      |   2 +
>> tools/testing/selftests/bpf/trace_helpers.c   |  37 +++++++
>> tools/testing/selftests/bpf/trace_helpers.h   |   5 +
>> 18 files changed, 378 insertions(+), 66 deletions(-)
>> create mode 100644 tools/testing/selftests/bpf/prog_tests/get_branch_snapshot.c
>> create mode 100644 tools/testing/selftests/bpf/progs/get_branch_snapshot.c
>> 
>> --
>> 2.30.2
>