Message-ID: <20171117234843.7vuxcndyakuxh7gu@ast-mbp>
Date:   Fri, 17 Nov 2017 15:48:45 -0800
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     peng yu <yupeng0921@...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

On Mon, Nov 13, 2017 at 12:06:17AM -0800, peng yu wrote:
> > 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.

all of ftrace are called at function entry and calling convention
is fixed per architecture, so we can make a generic and stable
struct bpf_ftrace_args {
  __u64 arg1, arg2, .. arg5;
};
save_mcount_regs doesn't care what order the regs are stored
so the same stack space can be used to keep bpf_ftrace_args
and used in restore_mcount_regs.
I'd also make it depend on DYNAMIC_FTRACE_WITH_REGS to avoid
dealing with obscure corner cases.

> 
> > 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.

since all of ftrace goes through the same function entry point
it should be possible to have one generic bpf filter interface
suitable for all tracers that ftrace supports.

Powered by blists - more mailing lists