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:     Peter Zijlstra <peterz@infradead.org>
CC:     "bpf@vger.kernel.org" <bpf@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "acme@kernel.org" <acme@kernel.org>,
        "mingo@redhat.com" <mingo@redhat.com>,
        "kjain@linux.ibm.com" <kjain@linux.ibm.com>,
        Kernel Team <Kernel-team@fb.com>,
        John Fastabend <john.fastabend@gmail.com>,
        "Andrii Nakryiko" <andrii@kernel.org>
Subject: Re: [PATCH v6 bpf-next 1/3] perf: enable branch record for software
 events
Thread-Topic: [PATCH v6 bpf-next 1/3] perf: enable branch record for software
 events
Thread-Index: AQHXpCby8uzfjDX52kWXwjwsop4wLqudGFYAgAA1/ICAAExtgIAAA5iAgAACr4CAAALQgA==
Date:   Fri, 10 Sep 2021 19:00:08 +0000
Message-ID: <6830FC62-995A-4282-BD30-76E2506ED993@fb.com>
References: <20210907202802.3675104-1-songliubraving@fb.com>
 <20210907202802.3675104-2-songliubraving@fb.com>
 <YTs2MpaI7iofckJI@hirez.programming.kicks-ass.net>
 <YTtjeyfJXXiDielu@hirez.programming.kicks-ass.net>
 <96445733-055E-41E3-986B-5E1DC04ADEFA@fb.com>
 <20210910184027.GQ4323@worktop.programming.kicks-ass.net>
 <20210910185003.GC5106@worktop.programming.kicks-ass.net>
In-Reply-To: <20210910185003.GC5106@worktop.programming.kicks-ass.net>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?LR6hr1U5gHJciPhCVJ7GKXRP6T1xJLVW5aGyMLnLpFCKZ9QGhcM9oMKIshth?=
 =?us-ascii?Q?/TDIM1IJNHgo5W88GczzHczPpIH2fxTQsYOhTTSG5OJtcAI/Jx5CPfwsjp1f?=
 =?us-ascii?Q?YNtSS1EznEG3rQd1XYbVOqdzvSM7cfNRhWZ41Q0VBvkUHsi3bfLC0/7f10jA?=
 =?us-ascii?Q?giamKIrZA9lgWNFKryhjCdjdkmPBVFM/14jVJl+fTnFa6EE1DSHZ0KtQNxaX?=
 =?us-ascii?Q?uqYS9K+YHve2Hd1F4IStlNSU0KyMuHAGKzmY0myWY7S0PFETLu0tyHKYeHQK?=
 =?us-ascii?Q?OKr2sYfcOz1ys1kVQee6onouLxC6jR9ppBxqGGcehd5RcDB7g82WI2sgijkc?=
 =?us-ascii?Q?5xbyVh+CWhP8CbcgFsqXSfEbgfG2aiFT2bZWXNk1ukieoh+2uRU8YDN/6HNG?=
 =?us-ascii?Q?j+afVYlfyXZM10b+lMQNjcRIGA7/zQH0c3yl07IW+oxYlr8a5xL8B0AoNEqH?=
 =?us-ascii?Q?wb8lNJW/NygEVMAV2/qUMH91kj/HenHKoowVjAVxzCktrRxwL6KRsOObYGYz?=
 =?us-ascii?Q?71gGa+YE1UHJvTtYSpgJPAEqvX1vyiGc9QF9QT7OSairEub4EhP4vVJp9Bxp?=
 =?us-ascii?Q?r0hmAzPdlFYpYeHtfKeQN14lHrGzMXbS92aW2nzcGsnVYvq6H8OlRX0kixBI?=
 =?us-ascii?Q?Vpeh5b8PEw6iuq+CWL01fCIqJX3J1uO4qpREay/3m39luRRTL0LB1O5c7cGF?=
 =?us-ascii?Q?koFkS1Jl/cU9eW2i5krVV8fhnWGhPiVDD9bgqEKLHVXZik1oFIME1ogPJavC?=
 =?us-ascii?Q?hNj4SyluehkuD8YVrSJ+yV3RnDR9shwRtqI8EO7z4JrDZbn3q2yHQWqFM0IT?=
 =?us-ascii?Q?kCbzWyH5I40pT1N2FiLAjidfb1/p9tjCFNJXmH93DIteBR28f96DrgsghiCt?=
 =?us-ascii?Q?msLHJv5lrhLqoz49ytmrdWGzJb2lYr9/Pp7vR1rBD0qVY+Zbkpi8BSumE16p?=
 =?us-ascii?Q?7vg881L5NqxGlzO+WCCoSbFZnJ+BfeQ5tTwZfQwyRkzJyMxw1GcBydBKtY84?=
 =?us-ascii?Q?eb3cqIr2uRunrxetP9VPp5calRmX7gcTWpKvYHjKbujkNu+PobZM9GYnY7V7?=
 =?us-ascii?Q?mNQlB1jipZn36AY0fStVoRpvUodu9lJbRC9ql7BwkA9u8IalPOGgJbExmdOq?=
 =?us-ascii?Q?2QkDmHMaO6VY1lXOAQScM/PnO84mBX2t04tsvtjhWIygzbsOIKxDpEh/lI2X?=
 =?us-ascii?Q?InKtAxqvXOXkrli6L5azRUnPFcLUflPo+Jd6pRduyJKGYIm9lXBB/0qP9uiS?=
 =?us-ascii?Q?ftiVnb70ZLeF4l9Xk5937k/X1d1N5q24yPM7NyjmWfFucf2dVHNLuskF+BQc?=
 =?us-ascii?Q?3f8Tusvq4lWY9ls4sk+WWsnAziPlQqLtQ3K28OJv/2esO27v58NaK5iifzLI?=
 =?us-ascii?Q?QyzgFqk=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <C7A405EE9611ED4393BA497A00359561@namprd15.prod.outlook.com>
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: SA1PR15MB5109.namprd15.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 83e7217b-5fd3-47ae-1dd4-08d9748d3552
X-MS-Exchange-CrossTenant-originalarrivaltime: 10 Sep 2021 19:00:08.2509
 (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: tlGNuRht8XBbzjHS2Yrgf2y49BHXPcoVUG0/ai1xSYpx8hVKwaO/bt8dC3hGgWape+eaKxOTVNFgQqxvIMeU3Q==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA1PR15MB5094
X-OriginatorOrg: fb.com
X-Proofpoint-GUID: XjPg5QZMxurXopPWZJXehCp97jPDRB4q
X-Proofpoint-ORIG-GUID: XjPg5QZMxurXopPWZJXehCp97jPDRB4q
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.391,18.0.790
 definitions=2021-09-10_07:2021-09-09,2021-09-10 signatures=0
X-Proofpoint-Spam-Details: rule=fb_default_notspam policy=fb_default score=0 phishscore=0
 priorityscore=1501 lowpriorityscore=0 suspectscore=0 spamscore=0
 mlxlogscore=736 malwarescore=0 impostorscore=0 adultscore=0 clxscore=1015
 bulkscore=0 mlxscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2109030001 definitions=main-2109100110
X-FB-Internal: deliver
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


> On Sep 10, 2021, at 11:50 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> 
> On Fri, Sep 10, 2021 at 08:40:27PM +0200, Peter Zijlstra wrote:
>> On Fri, Sep 10, 2021 at 06:27:36PM +0000, Song Liu wrote:
>> 
>>> This works great and saves 3 entries! We have the following now:
>> 
>> Yay!
>> 
>>> ID: 0 from bpf_get_branch_snapshot+18 to intel_pmu_snapshot_branch_stack+0
>> 
>> is unavoidable, we need to end up in intel_pmu_snapshot_branch_stack()
>> eventually.
>> 
>>> ID: 1 from __brk_limit+477143934 to bpf_get_branch_snapshot+0
>> 
>> could be elided by having the JIT emit the call to
>> intel_pmu_snapshot_branch_stack directly, instead of laundering it
>> through that helper I suppose.
>> 
>>> ID: 2 from __brk_limit+477192263 to __brk_limit+477143880  # trampoline 
>>> ID: 3 from __bpf_prog_enter+34 to __brk_limit+477192251
>> 
>> -ENOCLUE
>> 
>>> ID: 4 from migrate_disable+60 to __bpf_prog_enter+9
>>> ID: 5 from __bpf_prog_enter+4 to migrate_disable+0
>> 
>> I suppose we can reduce that to a single branch if we inline
>> migrate_disable() here, that thing unfortunately needs one branch
>> itself.
> 
> Oooh, since we put local_irq_save/restore() in
> intel_pmu_snapshot_branch_stack(), we no longer need to be after
> migrate_disable(). You could go back to placing it earlier!

Hmm.. not really. We call migrate_disable() before entering the BPF program. 
And the helper calls snapshot_branch_stack() inside the BPF program. To move
it to before migrate_disable(), we will have to add a "whether to snapshot
branch stack" check before entering the BPF program. This check, while is
cheap, is added to all BPF programs on this hook, even when the program does 
not use snapshot at all. So we would rather keep all logic inside the helper, 
and not touch the common path. 

Thanks,
Song