Message-ID: <CAG3TDc3YR69eh3YNz14BsMM126Wv6kF_kNdz1pZiW-+Raop07w@mail.gmail.com>
Date:   Mon, 13 Nov 2017 00:06:17 -0800
From:   peng yu <yupeng0921@...il.com>
To:     Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc:     linux-kernel@...r.kernel.org, ast@...nel.org, daniel@...earbox.net,
        rostedt@...dmis.org, mingo@...hat.com,
        "David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org
Subject: Re: [ftrace-bpf 1/5] add BPF_PROG_TYPE_FTRACE to bpf

> 1. anything bpf related has to go via net-next tree.
I found there is a net-next git repo:
https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git
I will use this repo for the further bpf-ftrace patch set.

> 2.
> this obviously breaks ABI. New types can only be added to the end.
Sure, I will add the new type at the end.

> 3.
> this won't even compile, since ftrace_regs is only added in the patch 4.
It could compile, as the ftrace_regs related code is inside the
"#ifdef FTRACE_BPF_FILTER" macro, if this macro is not defined, no
ftrace_regs related code would be compiled.


> Since bpf program will see ftrace_regs as an input it becomes
> abi, so has to be defined in uapi/linux/bpf_ftrace.h or similar.
> We need to think through how to make it generic across archs
> instead of defining ftrace_regs for each arch.
I'm not sure whether I'm fully understand your meaning. Like kprobe,
the ftrace-bpf need to get a function's parameters and check them. So
it won't be abi stable, and it should depend on architecture
implement. I can create a header file like uapi/linux/bpf_ftrace.h,
but I noticed that kprobe doesn't have such a header file, if I'm
wrong, please let me know. And about make it generic across archs, I
know kprobe use pt_regs as parameter, the pt_regs is defined on each
arch, so I can't see how bpf-ftrace can get a generic interface across
archs if it need to check function's parameters. If I misunderstand
anything, please let me know.

> 4.
> the patch 2/3 takes an approach of passing FD integer value in text form
> to the kernel. That approach was discussed years ago and rejected.
> It has to use binary interface like perf_event + ioctl.
> See RFC patches where we're extending perf_event_open syscall to
> support binary access to kprobe/uprobe.
> imo binary interface to ftrace is pre-requisite to ftrace+bpf work.
> We've had too many issues with text based kprobe api to repeat
> the same mistake here.
I notice the kprobe-bpf prog is set through the PERF_EVENT_IOC_SET_BPF
ioctl, I may try to see whether I can reuse this interface, or if it
is not suitable, I will try to define a new binary interface.

> 5.
> patch 4 hacks save_mcount_regs asm to pass ctx pointer in %rcx
> whereas it's only used in ftrace_graph_caller which doesn't seem right.
> It points out to another issue that such ftrace+bpf integration
> is only done for ftrace_graph_caller without extensibility in mind.
> If we do ftrace+bpf I'd rather see generic framework that applies
> to all of ftrace instead of single feature of it.
It is a hard problem. The ftrace framework has lots of tracers,
function tracer and function graph tracer use the 'gcc -pg' directly,
other tracers use tracepoint, I should spend more time to find a
suitable solution.


> 6.
> copyright line copy-pasted incorrectly.
I will fix it.

Summary:
The question 1,2 and 6 are easy to fix. About question 4, I need to do
more research, it shouldn't be very hard. About question 3 and 5, both

2017-11-12 17:02 GMT-08:00 Alexei Starovoitov <alexei.starovoitov@...il.com>:
> On Sun, Nov 12, 2017 at 07:28:24AM +0000, yupeng0921@...il.com wrote:
>> Add a new type BPF_PROG_TYPE_FTRACE to bpf, let bpf can be attached to
>> ftrace. Ftrace pass the function parameters to bpf prog, bpf prog
>> return 1 or 0 to indicate whether ftrace can trace this function. The
>> major propose is provide an accurate way to trigger function graph
>> trace. Changes in code:
>> 1. add FTRACE_BPF_FILTER in kernel/trace/Kconfig. Let ftrace pass
>> function parameter to bpf need to modify architecture dependent code,
>> so this feature will only be enabled only when it is enabled in
>> Kconfig and the architecture support this feature. If an architecture
>> support this feature, it should define a macro whose name is
>> FTRACE_BPF_FILTER, e.g.:
>> So other code in kernel can check whether the macro FTRACE_BPF_FILTER
>> is defined to know whether this feature is really enabled.
>> 2. add BPF_PROG_TYPE_FTRACE in bpf_prog_type
>> 3. check kernel version when load BPF_PROG_TYPE_FTRACE bpf prog
>> 4. define ftrace_prog_func_proto, the prog input is a struct
>> ftrace_regs type pointer, it is similar as pt_regs in kprobe, it
>> is an architecture dependent code, if an architecture doens't define
>> FTRACE_BPF_FILTER, use a fake ftrace_prog_func_proto.
>> 5. add BPF_PROG_TYPE in bpf_types.h
>>
>> Signed-off-by: yupeng0921@...il.com
>
> In general I like the bigger concept of adding bpf filtering to ftrace,
> but there are a lot of fundamental issues with this patch set.
>
> 1. anything bpf related has to go via net-next tree.
>
>> --- a/include/uapi/linux/bpf.h
>> +++ b/include/uapi/linux/bpf.h
>> @@ -118,6 +118,7 @@ enum bpf_prog_type {
>>       BPF_PROG_TYPE_UNSPEC,
>>       BPF_PROG_TYPE_SOCKET_FILTER,
>>       BPF_PROG_TYPE_KPROBE,
>> +     BPF_PROG_TYPE_FTRACE,
>>       BPF_PROG_TYPE_SCHED_CLS,
>
> 2.
> this obviously breaks ABI. New types can only be added to the end.
>
>> +static bool ftrace_prog_is_valid_access(int off, int size,
>> +                                     enum bpf_access_type type,
>> +                                     struct bpf_insn_access_aux *info)
>> +{
>> +     if (off < 0 || off >= sizeof(struct ftrace_regs))
>> +             return false;
>
> 3.
> this won't even compile, since ftrace_regs is only added in the patch 4.
>
> Since bpf program will see ftrace_regs as an input it becomes
> abi, so has to be defined in uapi/linux/bpf_ftrace.h or similar.
> We need to think through how to make it generic across archs
> instead of defining ftrace_regs for each arch.
>
> 4.
> the patch 2/3 takes an approach of passing FD integer value in text form
> to the kernel. That approach was discussed years ago and rejected.
> It has to use binary interface like perf_event + ioctl.
> See RFC patches where we're extending perf_event_open syscall to
> support binary access to kprobe/uprobe.
> imo binary interface to ftrace is pre-requisite to ftrace+bpf work.
> We've had too many issues with text based kprobe api to repeat
> the same mistake here.
>
> 5.
> patch 4 hacks save_mcount_regs asm to pass ctx pointer in %rcx
> whereas it's only used in ftrace_graph_caller which doesn't seem right.
> It points out to another issue that such ftrace+bpf integration
> is only done for ftrace_graph_caller without extensibility in mind.
> If we do ftrace+bpf I'd rather see generic framework that applies
> to all of ftrace instead of single feature of it.
>
> 6.
> copyright line copy-pasted incorrectly.
>

Powered by blists - more mailing lists