From: Song Liu <songliubraving@fb.com>
To: Jiri Olsa <jolsa@redhat.com>
Cc: lkml <linux-kernel@vger.kernel.org>,
Kernel Team <Kernel-team@fb.com>,
"peterz@infradead.org" <peterz@infradead.org>,
"mingo@redhat.com" <mingo@redhat.com>,
"acme@kernel.org" <acme@kernel.org>,
"mark.rutland@arm.com" <mark.rutland@arm.com>,
"alexander.shishkin@linux.intel.com"
<alexander.shishkin@linux.intel.com>,
"namhyung@kernel.org" <namhyung@kernel.org>
Subject: Re: [PATCH v2 2/2] perf-stat: enable counting events for BPF programs
Date: Tue, 8 Dec 2020 01:36:57 +0000 [thread overview]
Message-ID: <C94864E9-CE05-4AEA-A986-731BFC0C95FF@fb.com> (raw)
In-Reply-To: <20201207220703.GA4116109@krava>
> On Dec 7, 2020, at 2:07 PM, Jiri Olsa <jolsa@redhat.com> wrote:
>
> On Thu, Dec 03, 2020 at 10:13:10PM -0800, Song Liu wrote:
>
> SNIP
>
>> +#include "bpf_skel/bpf_prog_profiler.skel.h"
>> +
>> +static inline void *u64_to_ptr(__u64 ptr)
>> +{
>> + return (void *)(unsigned long)ptr;
>> +}
>> +
>> +static void set_max_rlimit(void)
>> +{
>> + struct rlimit rinf = { RLIM_INFINITY, RLIM_INFINITY };
>> +
>> + setrlimit(RLIMIT_MEMLOCK, &rinf);
>> +}
>> +
>> +static inline struct bpf_counter *bpf_counter_alloc(void)
>
> why is this inlined?
We don't need the inline here. I will remove it in the next version.
>
> SNIP
>
>> +static int bpf_program_profiler_load_one(struct evsel *evsel, u32 prog_id)
>> +{
>> + struct bpf_prog_profiler_bpf *skel;
>> + struct bpf_counter *counter;
>> + struct bpf_program *prog;
>> + char *prog_name;
>> + int prog_fd;
>> + int err;
>> +
>> + prog_fd = bpf_prog_get_fd_by_id(prog_id);
>> + if (prog_fd < 0) {
>> + pr_debug("Failed to open fd for bpf prog %u\n", prog_id);
>> + return -1;
>> + }
>> + counter = bpf_counter_alloc();
>> + if (!counter)
>> + return -1;
>> +
>> + skel = bpf_prog_profiler_bpf__open();
>> + if (!skel) {
>> + pr_debug("Failed to load bpf skeleton\n");
>
> I'm still getting
>
> [root@dell-r440-01 perf]# ./perf stat -b 38
> libbpf: elf: skipping unrecognized data section(9) .eh_frame
> libbpf: elf: skipping relo section(15) .rel.eh_frame for section(9) .eh_frame
> libbpf: XXX is not found in vmlinux BTF
> libbpf: failed to load object 'bpf_prog_profiler_bpf'
> libbpf: failed to load BPF skeleton 'bpf_prog_profiler_bpf': -2
> ...
>
> with id 38 being:
>
> 38: tracepoint name sys_enter tag 03418b72a610af75 gpl
> loaded_at 2020-12-07T22:54:05+0100 uid 0
> xlated 272B jited 153B memlock 4096B map_ids 1
>
> how is this supposed to work when there's XXX in the
> program's section? libbpf is trying to find XXX in
> kernel BTF and fails of course
I think this is because this program doesn't have BTF. The actual failed
function was bpf_program__set_attach_target(). So the error message above
should be "Failed to _open_ bpf skeleton". I will fix the error messages.
>
>
>> + free(counter);
>> + return -1;
>> + }
>> + skel->rodata->num_cpu = evsel__nr_cpus(evsel);
>> +
>> + bpf_map__resize(skel->maps.events, evsel__nr_cpus(evsel));
>> + bpf_map__resize(skel->maps.fentry_readings, 1);
>> + bpf_map__resize(skel->maps.accum_readings, 1);
>> +
>
> SNIP
>
>> +static int bpf_program_profiler__read(struct evsel *evsel)
>> +{
>> + int num_cpu = evsel__nr_cpus(evsel);
>> + struct bpf_perf_event_value values[num_cpu];
>> + struct bpf_counter *counter;
>> + int reading_map_fd;
>> + __u32 key = 0;
>> + int err, cpu;
>> +
>> + if (list_empty(&evsel->bpf_counter_list))
>> + return -EAGAIN;
>> +
>> + for (cpu = 0; cpu < num_cpu; cpu++) {
>> + perf_counts(evsel->counts, cpu, 0)->val = 0;
>> + perf_counts(evsel->counts, cpu, 0)->ena = 0;
>> + perf_counts(evsel->counts, cpu, 0)->run = 0;
>> + }
>> + list_for_each_entry(counter, &evsel->bpf_counter_list, list) {
>> + struct bpf_prog_profiler_bpf *skel = counter->skel;
>> +
>> + reading_map_fd = bpf_map__fd(skel->maps.accum_readings);
>> +
>> + err = bpf_map_lookup_elem(reading_map_fd, &key, values);
>> + if (err) {
>> + fprintf(stderr, "failed to read value\n");
>> + return err;
>> + }
>> +
>> + for (cpu = 0; cpu < num_cpu; cpu++) {
>> + perf_counts(evsel->counts, cpu, 0)->val += values[cpu].counter;
>> + perf_counts(evsel->counts, cpu, 0)->ena += values[cpu].enabled;
>> + perf_counts(evsel->counts, cpu, 0)->run += values[cpu].running;
>> + }
>
> so we sum everything up for all provided bpf IDs,
> should we count/display them separately?
I think that's the default behavior with --pid x,y,z or --cpu a,b,c.
Do we need to separate them?
>
> SNIP
>
>> +SEC("fentry/XXX")
>> +int BPF_PROG(fentry_XXX)
>> +{
>> + __u32 key = bpf_get_smp_processor_id();
>> + struct bpf_perf_event_value reading;
>> + struct bpf_perf_event_value *ptr;
>> + __u32 zero = 0;
>> + long err;
>> +
>> + /* look up before reading, to reduce error */
>> + ptr = bpf_map_lookup_elem(&fentry_readings, &zero);
>> + if (!ptr)
>> + return 0;
>> +
>> + err = bpf_perf_event_read_value(&events, key, &reading,
>> + sizeof(reading));
>
> can't we read directly to ptr in here?
Yes, we can! Thanks for catching this.
>
> SNIP
>
>> /* THREAD and SYSTEM/CPU are mutually exclusive */
>> if (target->per_thread && (target->system_wide || target->cpu_list)) {
>> target->per_thread = false;
>> @@ -109,6 +137,10 @@ static const char *target__error_str[] = {
>> "PID/TID switch overriding SYSTEM",
>> "UID switch overriding SYSTEM",
>> "SYSTEM/CPU switch overriding PER-THREAD",
>> + "BPF switch overriding CPU",
>> + "BPF switch overriding PID/TID",
>> + "BPF switch overriding UID",
>> + "BPF switch overriding THREAD",
>> "Invalid User: %s",
>> "Problems obtaining information for user %s",
>> };
>> @@ -134,7 +166,7 @@ int target__strerror(struct target *target, int errnum,
>>
>> switch (errnum) {
>> case TARGET_ERRNO__PID_OVERRIDE_CPU ...
>> - TARGET_ERRNO__SYSTEM_OVERRIDE_THREAD:
>
> hum, this should stay, no?
We need this to show the warning like:
~/perf stat -e cycles,instructions -b 245561 -C 0
BPF switch overriding CPU
...
Thanks,
Song
next prev parent reply other threads:[~2020-12-08 1:38 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-12-04 6:13 [PATCH v2 0/2] Introduce perf-stat -b for BPF programs Song Liu
2020-12-04 6:13 ` [PATCH v2 1/2] perf: support build BPF skeletons with perf Song Liu
2020-12-07 20:25 ` Jiri Olsa
2020-12-08 0:59 ` Song Liu
2020-12-04 6:13 ` [PATCH v2 2/2] perf-stat: enable counting events for BPF programs Song Liu
2020-12-07 22:07 ` Jiri Olsa
2020-12-08 1:36 ` Song Liu [this message]
2020-12-08 10:24 ` Jiri Olsa
2020-12-08 18:16 ` Song Liu
2020-12-09 15:55 ` Jiri Olsa
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=C94864E9-CE05-4AEA-A986-731BFC0C95FF@fb.com \
--to=songliubraving@fb.com \
--cc=Kernel-team@fb.com \
--cc=acme@kernel.org \
--cc=alexander.shishkin@linux.intel.com \
--cc=jolsa@redhat.com \
--cc=linux-kernel@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=mingo@redhat.com \
--cc=namhyung@kernel.org \
--cc=peterz@infradead.org \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).