diff mbox series

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

Message ID 20201203160245.1014867-10-jackmanb@google.com (mailing list archive)
State Superseded
Delegated to: BPF
Headers show
Series Atomics for eBPF | expand

Checks

Context Check Description
netdev/cover_letter success Link
netdev/fixes_present success Link
netdev/patch_count success Link
netdev/tree_selection success Clearly marked for bpf-next
netdev/subject_prefix success Link
netdev/source_inline success Was 0 now: 0
netdev/verify_signedoff success Link
netdev/module_param success Was 0 now: 0
netdev/build_32bit success Errors and warnings before: 8 this patch: 8
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/verify_fixes success Link
netdev/checkpatch fail CHECK: Lines should not end with a '(' CHECK: No space is necessary after a cast ERROR: Macros with multiple statements should be enclosed in a do - while loop ERROR: Remove Gerrit Change-Id's before submitting upstream ERROR: spaces required around that ':' (ctx:VxE) WARNING: labels should not be indented WARNING: line length of 81 exceeds 80 columns WARNING: line length of 85 exceeds 80 columns WARNING: line length of 86 exceeds 80 columns WARNING: line length of 89 exceeds 80 columns WARNING: macros should not use a trailing semicolon
netdev/build_allmodconfig_warn success Errors and warnings before: 8 this patch: 8
netdev/header_inline success Link
netdev/stable success Stable not CCed

Commit Message

Brendan Jackman Dec. 3, 2020, 4:02 p.m. UTC 
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>
---
 kernel/bpf/core.c | 79 +++++++++++++++++++++++------------------------
 1 file changed, 38 insertions(+), 41 deletions(-)

Comments

Yonghong Song Dec. 4, 2020, 6:30 a.m. UTC | #1 
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?

> +		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;
>   		}
>
Brendan Jackman Dec. 4, 2020, 9:29 a.m. UTC | #2 
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?

> > +		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;
> >   		}
> >
Yonghong Song Dec. 4, 2020, 3:20 p.m. UTC | #3 
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;
>>>    		}
>>>
diff mbox series

Patch

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)						\
+		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;
 		}