> On Aug 5, 2020, at 3:50 PM, Alexei Starovoitov <alexei.starovoitov@xxxxxxxxx> wrote: > > On Wed, Aug 05, 2020 at 06:56:26PM +0000, Song Liu wrote: >> >> >>> On Aug 5, 2020, at 10:16 AM, Alexei Starovoitov <alexei.starovoitov@xxxxxxxxx> wrote: >>> >>> On Wed, Aug 05, 2020 at 04:47:30AM +0000, Song Liu wrote: >>>> >>>> Being able to trigger BPF program on a different CPU could enable many >>>> use cases and optimizations. The use case I am looking at is to access >>>> perf_event and percpu maps on the target CPU. For example: >>>> 0. trigger the program >>>> 1. read perf_event on cpu x; >>>> 2. (optional) check which process is running on cpu x; >>>> 3. add perf_event value to percpu map(s) on cpu x. >>> >>> If the whole thing is about doing the above then I don't understand why new >>> prog type is needed. >> >> I was under the (probably wrong) impression that adding prog type is not >> that big a deal. > > Not a big deal when it's necessary. > >>> Can prog_test_run support existing BPF_PROG_TYPE_KPROBE? >> >> I haven't looked into all the details, but I bet this is possible. >> >>> "enable many use cases" sounds vague. I don't think folks reading >>> the patches can guess those "use cases". >>> "Testing existing kprobe bpf progs" would sound more convincing to me. >>> If the test_run framework can be extended to trigger kprobe with correct pt_regs. >>> As part of it test_run would trigger on a given cpu with $ip pointing >>> to some test fuction in test_run.c. For local test_run the stack trace >>> would include bpf syscall chain. For IPI the stack trace would include >>> the corresponding kernel pieces where top is our special test function. >>> Sort of like pseudo kprobe where there is no actual kprobe logic, >>> since kprobe prog doesn't care about mechanism. It needs correct >>> pt_regs only as input context. >>> The kprobe prog output (return value) has special meaning though, >>> so may be kprobe prog type is not a good fit. >>> Something like fentry/fexit may be better, since verifier check_return_code() >>> enforces 'return 0'. So their return value is effectively "void". >>> Then prog_test_run would need to gain an ability to trigger >>> fentry/fexit prog on a given cpu. >> >> Maybe we add a new attach type for BPF_PROG_TYPE_TRACING, which is in >> parallel with BPF_TRACE_FENTRY and BPF_TRACE_EXIT? Say BPF_TRACE_USER? >> (Just realized I like this name :-D, it matches USDT...). Then we can >> enable test_run for most (if not all) tracing programs, including >> fentry/fexit. > > Why new hook? Why prog_test_run cmd cannot be made to work > BPF_PROG_TYPE_TRACING when it's loaded as BPF_TRACE_FENTRY and attach_btf_id > points to special test function? > The test_run cmd will trigger execution of that special function. I am not sure I am following 100%. IIUC, the special test function is a kernel function, and we attach fentry program to it. When multiple fentry programs attach to the function, these programs will need proper filter logic. Alternatively, if test_run just prepare the ctx and call BPF_PROG_RUN(), like in bpf_test_run(), we don't need the special test function. So I do think the new attach type requires new hook. It is just like BPF_TRACE_FENTRY without valid attach_btf_id. Of course, we can reserve a test function and use it for attach_btf_id. If test_run just calls BPF_PROG_RUN(), we will probably never touch the test function. IMO, we are choosing from two options. 1. FENTRY on special function. User will specify attach_btf_id on the special function. 2. new attach type (BPF_TRACE_USER), that do not require attach_btf_id; and there is no need for a special function. I personally think #2 is cleaner API. But I have no objection if #1 is better in other means. Thanks, Song
