Message-ID: <554944C1.3060705@plumgrid.com>
Date: Tue, 05 May 2015 15:31:29 -0700
From: Alexei Starovoitov <ast@plumgrid.com>
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:31.0) Gecko/20100101 Thunderbird/31.6.0
MIME-Version: 1.0
To: He Kuang <hekuang@huawei.com>, davem@davemloft.net, acme@kernel.org,
        mingo@redhat.com, a.p.zijlstra@chello.nl,
        masami.hiramatsu.pt@hitachi.com, jolsa@kernel.org
CC: wangnan0@huawei.com, linux-kernel@vger.kernel.org, pi3orama@163.com
Subject: Re: [RFC PATCH 0/6] perf bpf: Probing with local variable
References: <1430820612-84443-1-git-send-email-hekuang@huawei.com>
In-Reply-To: <1430820612-84443-1-git-send-email-hekuang@huawei.com>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2736
Lines: 60

On 5/5/15 3:10 AM, He Kuang wrote:
> This patch set is based on https://lkml.org/lkml/2015/4/30/264
>
> By using bpf 'config' section like this:
>
>    char _config2[] SEC("config") = "generic_perform_write=generic_perform_write+122 file->f_mapping->a_ops bytes offset";
>    SEC("generic_perform_write")
>    int NODE_generic_perform_write (struct pt_regs *ctx, void *a_ops, void *bytes, void* offset) {
>            char fmt[] = "NODE_generic_perform_write, a_ops=%p, bytes=%p, offset=%p\n";
>            bpf_trace_printk(fmt, sizeof(fmt), a_ops, bytes, offset);
>            return 1;
>    }
>
> In this example, 'bytes' and 'offset' are local variables, a_ops is in
> the structure field of file parameter, and we probe in the body of the
> generic_perform_write() function.
>
> Perf can fetch and convert all the arguments and then we translate them
> into bpf bytecode as a prologue before calling bpf body functions. In
> the prologue, we fetch arguments from bpf context register and place
> them according to bpf calling conventions so the body function can
> access them as formal parameters.

great idea! Like it a lot.
Looking at current implementation I think the limit is <=3 arguments,
which I think is perfectly fine for now. Just worth mentioning in
the doc.

Two high level comments:
- can you collapse SEC("config") with SEC("func_name") ?
It seems that "func_name" is only used as reference inside "config".
I understand that you're proposing one "config" section where multiple
descriptions are strcat together, but why? Something like:
SEC("kprobe/generic_perform_write+122(file->f_mapping->a_ops, bytes, 
offset)")
int func(...) { ... }
should be enough and more concise.

- please support it without debug info when kprobe is placed at the top
of the function. Like:
SEC("kprobe/generic_perform_write(void*, void*, long long)")
even without debug info it will copy ctx->di into r2, ctx->si into r3
and ctx->dx into r4.
Not everyone has access to debug info when writing programs.
Your prologue generator removes headache of figuring out cpu calling
convention and can even work for 32-bit architectures.
Also I think it belongs in tools/lib/bpf/ together with section walking
and loading bits. As we discussed in the other thread it makes to create
a proper library out of samples/bpf/libbpf*, bpf_load* and use it
everywhere. Once it's there I'd like to rewrite samples/bpf/*_kern.c
using this simplified access to function arguments.

Other comments per patch...


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