Message-ID: <5673AFE0.1000006@huawei.com>
Date: Fri, 18 Dec 2015 15:04:00 +0800
From: "Wangnan (F)" <wangnan0@huawei.com>
User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Thunderbird/31.6.0
MIME-Version: 1.0
To: Alexei Starovoitov <alexei.starovoitov@gmail.com>
CC: <ast@kernel.org>, <agartrell@fb.com>, <acme@redhat.com>,
        <bblanco@plumgrid.com>, <daniel@iogearbox.net>,
        <daniel.wagner@bmw-carit.de>, <davem@davemloft.net>,
        <mingo@kernel.org>, <jolsa@kernel.org>, <xiakaixu@huawei.com>,
        <holzheu@linux.vnet.ibm.com>, <yang.shi@linaro.org>,
        <linux-kernel@vger.kernel.org>, <pi3orama@163.com>
Subject: Re: [PATCH 08/10] bpf samples: Add utils.[ch] for using BPF
References: <1450329794-161948-1-git-send-email-wangnan0@huawei.com> <1450329794-161948-9-git-send-email-wangnan0@huawei.com> <20151217231138.GA34078@ast-mbp.thefacebook.com> <5673659F.9090004@huawei.com> <20151218061929.GA59584@ast-mbp.thefacebook.com>
In-Reply-To: <20151218061929.GA59584@ast-mbp.thefacebook.com>
Content-Type: text/plain; charset="utf-8"; format=flowed
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4611
Lines: 125


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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/