Re: [PATCH v6 bpf-next 1/3] perf: enable branch record for software events

[Song Liu <songliubraving@fb.com>]

> 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