Re: [PATCH bpf-next v3 09/14] bpf: Pull out a macro for interpreting atomic ALU operations

From: Yonghong Song <yhs@fb.com>
To: Brendan Jackman <jackmanb@google.com>
Cc: <bpf@vger.kernel.org>, Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	KP Singh <kpsingh@chromium.org>,
	Florent Revest <revest@chromium.org>,
	<linux-kernel@vger.kernel.org>, Jann Horn <jannh@google.com>
Subject: Re: [PATCH bpf-next v3 09/14] bpf: Pull out a macro for interpreting atomic ALU operations
Date: Fri, 4 Dec 2020 07:20:18 -0800	[thread overview]
Message-ID: <4a39dd08-f0ca-595f-1f59-a6acb44f3176@fb.com> (raw)
In-Reply-To: <X8oBaf4c+EAd8LQE@google.com>

On 12/4/20 1:29 AM, Brendan Jackman wrote:
> On Thu, Dec 03, 2020 at 10:30:18PM -0800, Yonghong Song wrote:
>>
>>
>> On 12/3/20 8:02 AM, Brendan Jackman wrote:
>>> Since the atomic operations that are added in subsequent commits are
>>> all isomorphic with BPF_ADD, pull out a macro to avoid the
>>> interpreter becoming dominated by lines of atomic-related code.
>>>
>>> Note that this sacrificies interpreter performance (combining
>>> STX_ATOMIC_W and STX_ATOMIC_DW into single switch case means that we
>>> need an extra conditional branch to differentiate them) in favour of
>>> compact and (relatively!) simple C code.
>>>
>>> Change-Id: I8cae5b66e75f34393de6063b91c05a8006fdd9e6
>>> Signed-off-by: Brendan Jackman <jackmanb@google.com>
>>
>> Ack with a minor suggestion below.
>>
>> Acked-by: Yonghong Song <yhs@fb.com>
>>
>>> ---
>>>    kernel/bpf/core.c | 79 +++++++++++++++++++++++------------------------
>>>    1 file changed, 38 insertions(+), 41 deletions(-)
>>>
>>> diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
>>> index 28f960bc2e30..498d3f067be7 100644
>>> --- a/kernel/bpf/core.c
>>> +++ b/kernel/bpf/core.c
>>> @@ -1618,55 +1618,52 @@ static u64 ___bpf_prog_run(u64 *regs, const struct bpf_insn *insn, u64 *stack)
>>>    	LDX_PROBE(DW, 8)
>>>    #undef LDX_PROBE
>>> -	STX_ATOMIC_W:
>>> -		switch (IMM) {
>>> -		case BPF_ADD:
>>> -			/* lock xadd *(u32 *)(dst_reg + off16) += src_reg */
>>> -			atomic_add((u32) SRC, (atomic_t *)(unsigned long)
>>> -				   (DST + insn->off));
>>> -			break;
>>> -		case BPF_ADD | BPF_FETCH:
>>> -			SRC = (u32) atomic_fetch_add(
>>> -				(u32) SRC,
>>> -				(atomic_t *)(unsigned long) (DST + insn->off));
>>> -			break;
>>> -		case BPF_XCHG:
>>> -			SRC = (u32) atomic_xchg(
>>> -				(atomic_t *)(unsigned long) (DST + insn->off),
>>> -				(u32) SRC);
>>> -			break;
>>> -		case BPF_CMPXCHG:
>>> -			BPF_R0 = (u32) atomic_cmpxchg(
>>> -				(atomic_t *)(unsigned long) (DST + insn->off),
>>> -				(u32) BPF_R0, (u32) SRC);
>>> +#define ATOMIC(BOP, KOP)						\
>>
>> ATOMIC a little bit generic. Maybe ATOMIC_FETCH_BOP?
> 
> Well it doesn't fetch in all cases and "BOP" is intended to
> differentiate from KOP i.e. BOP = BPF operation KOP = Kernel operation.
> 
> Could go with ATOMIC_ALU_OP?

ATOMIC_ALU_OP sounds good.

> 
>>> +		case BOP:						\
>>> +			if (BPF_SIZE(insn->code) == BPF_W)		\
>>> +				atomic_##KOP((u32) SRC, (atomic_t *)(unsigned long) \
>>> +					     (DST + insn->off));	\
>>> +			else						\
>>> +				atomic64_##KOP((u64) SRC, (atomic64_t *)(unsigned long) \
>>> +					       (DST + insn->off));	\
>>> +			break;						\
>>> +		case BOP | BPF_FETCH:					\
>>> +			if (BPF_SIZE(insn->code) == BPF_W)		\
>>> +				SRC = (u32) atomic_fetch_##KOP(		\
>>> +					(u32) SRC,			\
>>> +					(atomic_t *)(unsigned long) (DST + insn->off)); \
>>> +			else						\
>>> +				SRC = (u64) atomic64_fetch_##KOP(	\
>>> +					(u64) SRC,			\
>>> +					(atomic64_t *)(s64) (DST + insn->off)); \
>>>    			break;
>>> -		default:
>>> -			goto default_label;
>>> -		}
>>> -		CONT;
>>>    	STX_ATOMIC_DW:
>>> +	STX_ATOMIC_W:
>>>    		switch (IMM) {
>>> -		case BPF_ADD:
>>> -			/* lock xadd *(u64 *)(dst_reg + off16) += src_reg */
>>> -			atomic64_add((u64) SRC, (atomic64_t *)(unsigned long)
>>> -				     (DST + insn->off));
>>> -			break;
>>> -		case BPF_ADD | BPF_FETCH:
>>> -			SRC = (u64) atomic64_fetch_add(
>>> -				(u64) SRC,
>>> -				(atomic64_t *)(s64) (DST + insn->off));
>>> -			break;
>>> +		ATOMIC(BPF_ADD, add)
>>> +
>>>    		case BPF_XCHG:
>>> -			SRC = (u64) atomic64_xchg(
>>> -				(atomic64_t *)(u64) (DST + insn->off),
>>> -				(u64) SRC);
>>> +			if (BPF_SIZE(insn->code) == BPF_W)
>>> +				SRC = (u32) atomic_xchg(
>>> +					(atomic_t *)(unsigned long) (DST + insn->off),
>>> +					(u32) SRC);
>>> +			else
>>> +				SRC = (u64) atomic64_xchg(
>>> +					(atomic64_t *)(u64) (DST + insn->off),
>>> +					(u64) SRC);
>>>    			break;
>>>    		case BPF_CMPXCHG:
>>> -			BPF_R0 = (u64) atomic64_cmpxchg(
>>> -				(atomic64_t *)(u64) (DST + insn->off),
>>> -				(u64) BPF_R0, (u64) SRC);
>>> +			if (BPF_SIZE(insn->code) == BPF_W)
>>> +				BPF_R0 = (u32) atomic_cmpxchg(
>>> +					(atomic_t *)(unsigned long) (DST + insn->off),
>>> +					(u32) BPF_R0, (u32) SRC);
>>> +			else
>>> +				BPF_R0 = (u64) atomic64_cmpxchg(
>>> +					(atomic64_t *)(u64) (DST + insn->off),
>>> +					(u64) BPF_R0, (u64) SRC);
>>>    			break;
>>> +
>>>    		default:
>>>    			goto default_label;
>>>    		}
>>>