Re: [PATCH 08/10] bpf samples: Add utils.[ch] for using BPF

[Wangnan (F)]

On 2015/12/18 14:19, Alexei Starovoitov wrote:
> On Fri, Dec 18, 2015 at 09:47:11AM +0800, Wangnan (F) wrote:
> > This is a limitation in tools/lib/bpf/libbpf.h, which has a #include
> > <linux/err.h>
> > in its header.
> >
> > libbpf.h requires this include because its API uses ERR_PTR() to encode
> > error code.
> > For example, when calling bpf_object__open(), caller should use IS_ERR() to
> > check its
> > return value instead of compare with NULL, and use PTR_ERR() to retrive
> > error number.
> >
> > However, linux/err.h is not a part of uapi. To make libbpf work, one has to
> > create its
> > own err.h.
> Why tools/include/linux/err.h is not suitable for everyone?
>
> > Now I'm thinking provide LIBBPF_{IS_ERR,PTR_ERR}(),  in libbpf itself.
> seems odd. we already have user space err.h in tools/include.

Currently samples/bpf doesn't have an -I$(srctree)/tools/include.

I tried to add it into CFLAGS of samples/bpf. It causes other problems,
This is what I get:

In file included from 
/home/w00229757/kernel-hydrogen/samples/bpf/sock_example.c:27:0:
/usr/include/linux/ip.h:101:2: error: unknown type name â__sum16â
  __sum16 check;
  ^
make[3]: *** [samples/bpf/sock_example.o] Error 1
make[2]: *** [samples/bpf/] Error 2
make[1]: *** [sub-make] Error 2
make: *** [__sub-make] Error 2

And after fixing __sum16 in linux/types.h:

  HOSTCC  samples/bpf/tracex4_user.o
  HOSTLD  samples/bpf/tracex4
  HOSTCC  samples/bpf/tracex5_user.o
/kernel/samples/bpf/tracex5_user.c: In function 
âinstall_accept_all_seccompâ:
/kernel/samples/bpf/tracex5_user.c:15:21: error: array type has 
incomplete element type
  struct sock_filter filter[] = {
                     ^
/kernel/samples/bpf/tracex5_user.c:16:3: warning: implicit declaration 
of function âBPF_STMTâ [-Wimplicit-function-declaration]
   BPF_STMT(BPF_RET+BPF_K, SECCOMP_RET_ALLOW),
   ^
/kernel/samples/bpf/tracex5_user.c:18:9: error: variable âprogâ has 
initializer but incomplete type
  struct sock_fprog prog = {
         ^

Finally we need to add sock_filter, sock_fprog, BPF_STMT into 
tools/include/linux/filter.h.

It is okay, but different from what I really want to do. I'll discuss 
this later.
> > And I don't touch the setsockopt in all patches.
> ok, but where is the bit that does attach to perf_event to make trace_output work?

I didn't change this test_bpf_perf_event() function (only the function 
name).
It creates a bpf-output perf event. This event is inserted into a
BPF_MAP_TYPE_PERF_EVENT_ARRAY by bpf_map_update_elem().

static void test_bpf_perf_event(int map_fd)
{
        struct perf_event_attr attr = {
                .sample_type = PERF_SAMPLE_RAW,
                .type = PERF_TYPE_SOFTWARE,
                .config = PERF_COUNT_SW_BPF_OUTPUT,
        };
        int key = 0;

        pmu_fd = perf_event_open(&attr, -1/*pid*/, 0/*cpu*/, 
-1/*group_fd*/, 0);

        assert(pmu_fd >= 0);
        assert(bpf_map_update_elem(map_fd, &key, &pmu_fd, BPF_ANY) == 0);
        ioctl(pmu_fd, PERF_EVENT_IOC_ENABLE, 0);
}

And you read from this pmu_fd, get results. The logical is unchanged.

> > Orignally they are macros defined in linux/filter.h.
> no. they were never part of offical filter.h. Only in my earlier versions
> of bpf patches, but we decided to drop them before they got into net-next.
>
> > What about moving them into include/uapi/linux/filter.h ? Then
> > normal user programs like those in samples/bpf can access
> > them easier.
> we don't want to add these macros to uapi.
> Why not to add it to
> tools/include/linux/filter.h
> instead?

What I want to do in this patchset is not only removing original libbpf.c
and bpf_load.c. In fact I want libbpf in tools/lib/bpf becomes a public
available library for other userspace tools (tc for example). Switching
samples/bpf into libbpf is the first step of this goal. From doing this
I found and fixed some limitation, like those missed BPF map operations.
Making libbpf.h and bpf.h available for normal userspace programs is also
important.

Having the above goal, I think you can understand why improving 
tools/include
is not a good idea. You don't want to force a normal userspace program setup
a similar header environment for using libbpf. It is relatively a small
library. So it would be good if bpf.h and libbpf.h only depend on what can
be found in uapi.

Thank you.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/