Message-ID: <5673659F.9090004@huawei.com>
Date: Fri, 18 Dec 2015 09:47:11 +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>
In-Reply-To: <20151217231138.GA34078@ast-mbp.thefacebook.com>
Content-Type: text/plain; charset="utf-8"; format=flowed
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4610
Lines: 140


On 2015/12/18 7:11, Alexei Starovoitov wrote:
> On Thu, Dec 17, 2015 at 05:23:12AM +0000, Wang Nan wrote:
>> We are going to uses libbpf to replace old libbpf.[ch] and
>> bpf_load.[ch]. This is the first patch of this work. In this patch,
>> several macros and helpers in libbpf.[ch] and bpf_load.[ch] are
>> merged into utils.[ch]. utils.[ch] utilizes libbpf in tools/lib to
>> deal with BPF related things. They would be compiled after Makefile
>> changes.
>>
>> Signed-off-by: Wang Nan <wangnan0@huawei.com>
> ...
>> +#define IS_ERR_VALUE(x) unlikely((x) >= (unsigned long)-MAX_ERRNO)
>> +
>> +static inline void * __must_check ERR_PTR(long error_)
>> +{
>> +	return (void *) error_;
>> +}
>> +
>> +static inline long __must_check PTR_ERR(__force const void *ptr)
>> +{
>> +	return (long) ptr;
>> +}
>> +
>> +static inline bool __must_check IS_ERR(__force const void *ptr)
>> +{
>> +	return IS_ERR_VALUE((unsigned long)ptr);
>> +}
> why copy paste this? I don't see the code that uses that.

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.

Now I'm thinking provide LIBBPF_{IS_ERR,PTR_ERR}(),  in libbpf itself.

>> +	bpf_object__for_each_program(prog, obj) {
>> +		const char *event = bpf_program__title(prog, false);
>> +		int fd, err;
>> +
>> +		LIBBPF_PTR_ASSERT(event, goto errout);
>> +		__LIBBPF_ASSERT(fd = bpf_program__nth_fd(prog, 0),
>> +				>= 0,
>> +				goto errout);
>> +
>> +		if (strncmp(event, "kprobe/", 7) == 0)
>> +			err = create_kprobes(fd, event + 7, true);
>> +		else if (strncmp(event, "kretprobe/", 10) == 0)
>> +			err = create_kprobes(fd, event + 10, false);
> I have a feeling that all bpf+socket, tcbpf1_kernc and trace_output_*.c
> are broken, since I don't see a code that attaches programs to sockets
> and to perf_event.
> How did you test it?

I tested all samples (except tcbpf1_kern, because it is loaded by tc but tc
has not switched to libbpf) in my environment. They are okay for me. There's
no socket attaching code in this patchset because they are in sockex?_user.c
like this:

         obj = load_bpf_file(filename);
         if (!obj)
                 return 1;
         ...
         prog_fd = get_prog_fd(obj, 0);
         ...
         sock = open_raw_sock("lo");

         assert(setsockopt(sock, SOL_SOCKET, SO_ATTACH_BPF, &prog_fd,
                           sizeof(prog_fd)) == 0);

And I don't touch the setsockopt in all patches.

>> diff --git a/samples/bpf/utils.h b/samples/bpf/utils.h
>> new file mode 100644
>> index 0000000..5962a68
>> --- /dev/null
>> +++ b/samples/bpf/utils.h
>> @@ -0,0 +1,217 @@
>> +#ifndef __SAMPELS_UTILS_H
>> +#define __SAMPELS_UTILS_H
>> +
>> +#include <bpf/libbpf.h>
>> +#include <bpf/bpf.h>
>> +
>> +/* ALU ops on registers, bpf_add|sub|...: dst_reg += src_reg */
>> +
>> +#define BPF_ALU64_REG(OP, DST, SRC)				\
>> +	((struct bpf_insn) {					\
>> +		.code  = BPF_ALU64 | BPF_OP(OP) | BPF_X,	\
>> +		.dst_reg = DST,					\
>> +		.src_reg = SRC,					\
>> +		.off   = 0,					\
>> +		.imm   = 0 })
> this probably belongs in tools/lib/bpf/bpf.h instead of samples.

Orignally they are macros defined in linux/filter.h. We have 3
filter.h in kernel tree:

include/linux/filter.h
include/uapi/linux/filter.h
tools/include/linux/filter.h

These macros belong to include/linux/filter.h, not part of uapi,
so we have to do things like what we have done for
tools/include/linux/filter.h.

What about moving them into include/uapi/linux/filter.h ? Then
normal user programs like those in samples/bpf can access
them easier.

> The whole set depends on changes in perf/core tree, but
> in net-next we have extra commit 30b50aa612018, so I don't see an easy way
> to route this patch without creating across-tree merge conflicts during
> merge window.
> I'd suggest to apply all required work to tools/lib/bpf/ into perf/core
> and leave samples/bpf/ after merge window.
Good suggestion.

I'll resend them after the PowerPC building breakage fixing is
collected.

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/