LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Song Liu <songliubraving@fb.com>
To: Peter Zijlstra <peterz@infradead.org>
Cc: "open list:BPF (Safe dynamic programs and tools)" 
	<bpf@vger.kernel.org>, LKML <linux-kernel@vger.kernel.org>,
	Arnaldo Carvalho de Melo <acme@kernel.org>,
	Ingo Molnar <mingo@redhat.com>, Kernel Team <Kernel-team@fb.com>
Subject: Re: [PATCH bpf-next 1/3] perf: enable branch record for software events
Date: Wed, 25 Aug 2021 15:22:51 +0000	[thread overview]
Message-ID: <19CA9F65-E45B-4AE5-9742-3D89ECF0CEF4@fb.com> (raw)
In-Reply-To: <YSYy87ta1GpXCCzk@hirez.programming.kicks-ass.net>



> On Aug 25, 2021, at 5:09 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> 
> On Mon, Aug 23, 2021 at 11:01:55PM -0700, Song Liu wrote:
> 
>> arch/x86/events/intel/core.c |  5 ++++-
>> arch/x86/events/intel/lbr.c  | 12 ++++++++++++
>> arch/x86/events/perf_event.h |  2 ++
>> include/linux/perf_event.h   | 33 +++++++++++++++++++++++++++++++++
>> kernel/events/core.c         | 28 ++++++++++++++++++++++++++++
>> 5 files changed, 79 insertions(+), 1 deletion(-)
> 
> No PowerPC support :/

I don't have PowerPC system for testing at the moment. I guess we can decide
the overall framework now, and ask PowerPC folks' help on PowerPC support
later? 

> 
>> +void intel_pmu_snapshot_branch_stack(void)
>> +{
>> +	struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
>> +
>> +	intel_pmu_lbr_disable_all();
>> +	intel_pmu_lbr_read();
>> +	memcpy(this_cpu_ptr(&perf_branch_snapshot_entries), cpuc->lbr_entries,
>> +	       sizeof(struct perf_branch_entry) * x86_pmu.lbr_nr);
>> +	*this_cpu_ptr(&perf_branch_snapshot_size) = x86_pmu.lbr_nr;
>> +	intel_pmu_lbr_enable_all(false);
>> +}
> 
> Still has the layering violation and issues vs PMI.

Yes, this is the biggest change after I test with this more. I tested with 
perf_[disable|enable]_pmu(), and function pointer in "struct pmu". However,
all these logic consumes LBR entries. In one of the version, 22 out of the
32 LBR entries are branches after the fexit event. Most of them are from
perf_disable_pmu(). And each function pointer consumes 1 or 2 entries. 
This would be worse for systems with fewer LBR entries. 

On the other hand, I think current version was not too bad. It may corrupt
some samples when there is collision between this and PMI. But it should not
cause serious issues. Did I miss anything more serious? 

> 
>> +#ifdef CONFIG_HAVE_STATIC_CALL
>> +DECLARE_STATIC_CALL(perf_snapshot_branch_stack,
>> +		    perf_default_snapshot_branch_stack);
>> +#else
>> +extern void (*perf_snapshot_branch_stack)(void);
>> +#endif
> 
> That's weird, static call should work unconditionally, and fall back to
> a regular function pointer exactly like you do here. Search for:
> "Generic Implementation" in include/linux/static_call.h

Thanks for the pointer. Let me look into it. 
> 
>> diff --git a/kernel/events/core.c b/kernel/events/core.c
>> index 011cc5069b7ba..b42cc20451709 100644
>> --- a/kernel/events/core.c
>> +++ b/kernel/events/core.c
> 
>> +#ifdef CONFIG_HAVE_STATIC_CALL
>> +DEFINE_STATIC_CALL(perf_snapshot_branch_stack,
>> +		   perf_default_snapshot_branch_stack);
>> +#else
>> +void (*perf_snapshot_branch_stack)(void) = perf_default_snapshot_branch_stack;
>> +#endif
> 
> Idem.
> 
> Something like:
> 
> DEFINE_STATIC_CALL_NULL(perf_snapshot_branch_stack, void (*)(void));
> 
> with usage like: static_call_cond(perf_snapshot_branch_stack)();
> 
> Should unconditionally work.
> 
>> +int perf_read_branch_snapshot(void *buf, size_t len)
>> +{
>> +	int cnt;
>> +
>> +	memcpy(buf, *this_cpu_ptr(&perf_branch_snapshot_entries),
>> +	       min_t(u32, (u32)len,
>> +		     sizeof(struct perf_branch_entry) * MAX_BRANCH_SNAPSHOT));
>> +	cnt =  *this_cpu_ptr(&perf_branch_snapshot_size);
>> +
>> +	return (cnt > 0) ? cnt : -EOPNOTSUPP;
>> +}
> 
> Doesn't seem used at all..

At the moment, we only use this from BPF side (see 2/3). We sure can use it
from perf side, but that would require discussions on the user interface. 
How about we have that discussion later? 

Thanks,
Song

  reply	other threads:[~2021-08-25 15:23 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-24  6:01 [PATCH bpf-next 0/3] bpf: introduce bpf_get_branch_trace Song Liu
2021-08-24  6:01 ` [PATCH bpf-next 1/3] perf: enable branch record for software events Song Liu
2021-08-25 12:09   ` Peter Zijlstra
2021-08-25 15:22     ` Song Liu [this message]
2021-08-26  7:56       ` kajoljain
2021-08-26 16:41         ` Song Liu
2021-08-24  6:01 ` [PATCH bpf-next 2/3] bpf: introduce helper bpf_get_branch_trace Song Liu
2021-08-25  1:14   ` kernel test robot
2021-08-24  6:01 ` [PATCH bpf-next 3/3] selftests/bpf: add test for bpf_get_branch_trace Song Liu
2021-08-31  1:30 [selftests/bpf] 8dff2c1958: BUG:using_smp_processor_id()in_preemptible kernel test robot
2021-08-31  4:45 ` Song Liu

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=19CA9F65-E45B-4AE5-9742-3D89ECF0CEF4@fb.com \
    --to=songliubraving@fb.com \
    --cc=Kernel-team@fb.com \
    --cc=acme@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mingo@redhat.com \
    --cc=peterz@infradead.org \
    --subject='Re: [PATCH bpf-next 1/3] perf: enable branch record for software events' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).