Message-ID: <55494A29.6000309@plumgrid.com>
Date: Tue, 05 May 2015 15:54:33 -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 4/6] perf bpf: Convert arglist to bpf prologue
References: <1430820612-84443-1-git-send-email-hekuang@huawei.com> <1430820612-84443-5-git-send-email-hekuang@huawei.com>
In-Reply-To: <1430820612-84443-5-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: 1808
Lines: 57

On 5/5/15 3:10 AM, He Kuang wrote:
> When all arguments in bpf config section are collected in register and
> offset form, this patch will fetch them from bpf context register and
> place them as bpf input parameters.
>
> Bpf prologue is generated as the following steps:
> 1. alloc dst address in stack -> r1
> 2. set size -> r2
> 3. fetch base register and offset -> r3
> 4. call BPF_FUNC_probe_read
> 5. loop 1
> 6. save intermediate result and process next arg
> 7. restore intermediate result to arg2~5
>
> Signed-off-by: He Kuang <hekuang@huawei.com>
...
>
> +#define BPF_STRBUF_ADD(buf, statement)		\
> +	strbuf_add(buf,				\
> +		&statement,			\
> +		sizeof(struct bpf_insn))
> +
> +#define BPF_STRBUF_ADD_BUF(statement) BPF_STRBUF_ADD(buf, statement)
> +
> +int synthesize_probe_trace_arg_bpf_begin(struct strbuf *buf)
> +{
> +	/* save arg1 to ctx */
> +	BPF_STRBUF_ADD_BUF(BPF_MOV64_REG(BPF_REG_CTX, BPF_REG_ARG1));
> +	return 0;
> +}

the macro approach looks a bit ugly.
Why not to do it similar to net/core/filter.c style:
*insn++ = BPF_ALU32_IMM(BPF_RSH, dst_reg, 5);
Looks more readable to me.

> +	for (i = 0; i < num; i++) {
> +		/* restore r7~10 to arg2~5*/
> +		BPF_STRBUF_ADD_BUF(
> +			BPF_MOV64_REG(BPF_REG_ARG2 + i, BPF_REG_7 + i));
> +	}

comment and/or logic is wrong. r10 is read only stack. you cannot use
it as callee-saved.

> +	/* store intermediate results to r7~10 */
> +	BPF_STRBUF_ADD_BUF(BPF_MOV64_REG(BPF_REG_7 + index, BPF_REG_3));

should be r7-r9.
Also is there a check somewhere that accepts only 3 debuginfo-backed
args?

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