On Fri, Mar 13, 2020 at 8:45 PM Daniel T. Lee <danieltimlee@xxxxxxxxx> wrote:
>
> The bpf_program__attach of libbpf(using bpf_link) is much more intuitive
> than the previous method using ioctl.
>
> bpf_program__attach_perf_event manages the enable of perf_event and
> attach of BPF programs to it, so there's no neeed to do this
> directly with ioctl.
>
> In addition, bpf_link provides consistency in the use of API because it
> allows disable (detach, destroy) for multiple events to be treated as
> one bpf_link__destroy. Also, bpf_link__destroy manages the close() of
> perf_event fd.
>
> This commit refactors samples that attach the bpf program to perf_event
> by using libbbpf instead of ioctl. Also the bpf_load in the samples were
> removed and migrated to use libbbpf API.
>
> Signed-off-by: Daniel T. Lee <danieltimlee@xxxxxxxxx>
> ---
> Changes in v2:
> - check memory allocation is successful
> - clean up allocated memory on error
>
> Changes in v3:
> - Improve pointer error check (IS_ERR())
> - change to calloc for easier destroy of bpf_link
> - remove perf_event fd list since bpf_link handles fd
> - use newer bpf_object__{open/load} API instead of bpf_prog_load
> - perf_event for _SC_NPROCESSORS_ONLN instead of _SC_NPROCESSORS_CONF
> - find program with name explicitly instead of bpf_program__next
> - unconditional bpf_link__destroy() on cleanup
>
> Changes in v4:
> - bpf_link *, bpf_object * set NULL on init & err for easier destroy
> - close bpf object with bpf_object__close()
>
> samples/bpf/Makefile | 4 +-
> samples/bpf/sampleip_user.c | 98 +++++++++++++++++++----------
> samples/bpf/trace_event_user.c | 112 ++++++++++++++++++++++-----------
> 3 files changed, 143 insertions(+), 71 deletions(-)
>
Few more int_exit() problems, sorry I didn't catch it first few times,
I'm not very familiar with all these bpf samples.
[...]
> all_cpu_err:
> - for (i--; i >= 0; i--) {
> - ioctl(pmu_fd[i], PERF_EVENT_IOC_DISABLE);
> - close(pmu_fd[i]);
> - }
> - free(pmu_fd);
> + for (i--; i >= 0; i--)
> + bpf_link__destroy(links[i]);
> +err:
> + free(links);
> if (error)
> int_exit(0);
if (error) you should exit with error, no?
> }
>
> static void test_perf_event_task(struct perf_event_attr *attr)
> {
[...]
> err:
> - ioctl(pmu_fd, PERF_EVENT_IOC_DISABLE);
> - close(pmu_fd);
> + bpf_link__destroy(link);
> if (error)
> int_exit(0);
same comment about exiting with error
> }
> @@ -282,7 +297,9 @@ static void test_bpf_perf_event(void)
[...]
> @@ -305,6 +343,10 @@ int main(int argc, char **argv)
> return 0;
> }
> test_bpf_perf_event();
> + error = 0;
> +
> +cleanup:
> + bpf_object__close(obj);
> int_exit(0);
here and in previous sample int_exit for whatever purpose sends KILL
signal and exits with 0, that seems weird. Any idea why it was done
that way?
> - return 0;
> + return error;
so with that int_ext() implementation you will never get to this error
> }
> --
> 2.25.1
>
