# \[PATCH bpf-next v2\] bpf: Simplify expression for identify bpf mem type - Subsystems: BPF JIT for NFP NICs, BPF \[CORE\], BPF \[GENERAL\] (Safe Dynamic Programs and Tools), NETRONOME ETHERNET DRIVERS, NETWORKING DRIVERS, THE REST - Maintainers: Jakub Kicinski, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Eduard Zingerman, Kumar Kartikeya Dwivedi, Andrew Lunn, "David S. Miller", Eric Dumazet, Paolo Abeni, Linus Torvalds - List: linux-api - 3 messages, 3 authors - Page: https://ratatoskr.run/linux-api/2021/03/6305909/t ## \[PATCH bpf-next v2\] bpf: Simplify expression for identify bpf mem type - From: Jianlin Lv \ - Date: 2021-03-18 06:37:48 - Message: https://ratatoskr.run/linux-api/2021/03/6305909.md ```text Added BPF_LD_ST_SIZE_MASK macro as mask of size modifier that help to reduce the evaluation of expressions in if statements, and remove BPF_SIZE_MASK in netronome driver. Signed-off-by: Jianlin Lv --- v2: Move the bpf_LD_ST_SIZE_MASK macro definition to include/linux/bpf.h --- drivers/net/ethernet/netronome/nfp/bpf/main.h | 8 +++----- include/linux/bpf.h | 1 + kernel/bpf/verifier.c | 12 ++++-------- 3 files changed, 8 insertions(+), 13 deletions(-) diff --git a/drivers/net/ethernet/netronome/nfp/bpf/main.h b/drivers/net/ethernet/netronome/nfp/bpf/main.h index d0e17eebddd9..e90981e69763 100644 --- a/drivers/net/ethernet/netronome/nfp/bpf/main.h +++ b/drivers/net/ethernet/netronome/nfp/bpf/main.h @@ -346,8 +346,6 @@ struct nfp_insn_meta { struct list_head l; }; -#define BPF_SIZE_MASK 0x18 - static inline u8 mbpf_class(const struct nfp_insn_meta *meta) { return BPF_CLASS(meta->insn.code); @@ -375,7 +373,7 @@ static inline bool is_mbpf_alu(const struct nfp_insn_meta *meta) static inline bool is_mbpf_load(const struct nfp_insn_meta *meta) { - return (meta->insn.code & ~BPF_SIZE_MASK) == (BPF_LDX | BPF_MEM); + return (meta->insn.code & ~BPF_LD_ST_SIZE_MASK) == (BPF_LDX | BPF_MEM); } static inline bool is_mbpf_jmp32(const struct nfp_insn_meta *meta) @@ -395,7 +393,7 @@ static inline bool is_mbpf_jmp(const struct nfp_insn_meta *meta) static inline bool is_mbpf_store(const struct nfp_insn_meta *meta) { - return (meta->insn.code & ~BPF_SIZE_MASK) == (BPF_STX | BPF_MEM); + return (meta->insn.code & ~BPF_LD_ST_SIZE_MASK) == (BPF_STX | BPF_MEM); } static inline bool is_mbpf_load_pkt(const struct nfp_insn_meta *meta) @@ -430,7 +428,7 @@ static inline bool is_mbpf_classic_store_pkt(const struct nfp_insn_meta *meta) static inline bool is_mbpf_atomic(const struct nfp_insn_meta *meta) { - return (meta->insn.code & ~BPF_SIZE_MASK) == (BPF_STX | BPF_ATOMIC); + return (meta->insn.code & ~BPF_LD_ST_SIZE_MASK) == (BPF_STX | BPF_ATOMIC); } static inline bool is_mbpf_mul(const struct nfp_insn_meta *meta) diff --git a/include/linux/bpf.h b/include/linux/bpf.h index a25730eaa148..e85924719c65 100644 --- a/include/linux/bpf.h +++ b/include/linux/bpf.h @@ -995,6 +995,7 @@ struct bpf_array { BPF_F_RDONLY_PROG | \ BPF_F_WRONLY | \ BPF_F_WRONLY_PROG) +#define BPF_LD_ST_SIZE_MASK 0x18 /* mask of size modifier */ #define BPF_MAP_CAN_READ BIT(0) #define BPF_MAP_CAN_WRITE BIT(1) diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index f9096b049cd6..29fdfdb8abfa 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -11384,15 +11384,11 @@ static int convert_ctx_accesses(struct bpf_verifier_env *env) for (i = 0; i < insn_cnt; i++, insn++) { bpf_convert_ctx_access_t convert_ctx_access; - if (insn->code == (BPF_LDX | BPF_MEM | BPF_B) || - insn->code == (BPF_LDX | BPF_MEM | BPF_H) || - insn->code == (BPF_LDX | BPF_MEM | BPF_W) || - insn->code == (BPF_LDX | BPF_MEM | BPF_DW)) + /* opcode: BPF_MEM | | BPF_LDX */ + if ((insn->code & ~BPF_LD_ST_SIZE_MASK) == (BPF_LDX | BPF_MEM)) type = BPF_READ; - else if (insn->code == (BPF_STX | BPF_MEM | BPF_B) || - insn->code == (BPF_STX | BPF_MEM | BPF_H) || - insn->code == (BPF_STX | BPF_MEM | BPF_W) || - insn->code == (BPF_STX | BPF_MEM | BPF_DW)) + /* opcode: BPF_MEM | | BPF_STX */ + else if ((insn->code & ~BPF_LD_ST_SIZE_MASK) == (BPF_STX | BPF_MEM)) type = BPF_WRITE; else continue; -- 2.25.1 ``` ## Re: \[PATCH bpf-next v2\] bpf: Simplify expression for identify bpf mem type - From: Daniel Borkmann \ - Date: 2021-03-18 15:58:49 - Message: https://ratatoskr.run/linux-api/2021/03/6306529.md ```text On 3/18/21 7:36 AM, Jianlin Lv wrote: > Added BPF_LD_ST_SIZE_MASK macro as mask of size modifier that help to > reduce the evaluation of expressions in if statements, > and remove BPF_SIZE_MASK in netronome driver. > > Signed-off-by: Jianlin Lv > --- > v2: Move the bpf_LD_ST_SIZE_MASK macro definition to include/linux/bpf.h > --- > drivers/net/ethernet/netronome/nfp/bpf/main.h | 8 +++----- > include/linux/bpf.h | 1 + > kernel/bpf/verifier.c | 12 ++++-------- > 3 files changed, 8 insertions(+), 13 deletions(-) > > diff --git a/drivers/net/ethernet/netronome/nfp/bpf/main.h b/drivers/net/ethernet/netronome/nfp/bpf/main.h > index d0e17eebddd9..e90981e69763 100644 > --- a/drivers/net/ethernet/netronome/nfp/bpf/main.h > +++ b/drivers/net/ethernet/netronome/nfp/bpf/main.h > @@ -346,8 +346,6 @@ struct nfp_insn_meta { > struct list_head l; > }; > > -#define BPF_SIZE_MASK 0x18 > - > static inline u8 mbpf_class(const struct nfp_insn_meta *meta) > { > return BPF_CLASS(meta->insn.code); > @@ -375,7 +373,7 @@ static inline bool is_mbpf_alu(const struct nfp_insn_meta *meta) > > static inline bool is_mbpf_load(const struct nfp_insn_meta *meta) > { > - return (meta->insn.code & ~BPF_SIZE_MASK) == (BPF_LDX | BPF_MEM); > + return (meta->insn.code & ~BPF_LD_ST_SIZE_MASK) == (BPF_LDX | BPF_MEM); > } > > static inline bool is_mbpf_jmp32(const struct nfp_insn_meta *meta) > @@ -395,7 +393,7 @@ static inline bool is_mbpf_jmp(const struct nfp_insn_meta *meta) > > static inline bool is_mbpf_store(const struct nfp_insn_meta *meta) > { > - return (meta->insn.code & ~BPF_SIZE_MASK) == (BPF_STX | BPF_MEM); > + return (meta->insn.code & ~BPF_LD_ST_SIZE_MASK) == (BPF_STX | BPF_MEM); > } > > static inline bool is_mbpf_load_pkt(const struct nfp_insn_meta *meta) > @@ -430,7 +428,7 @@ static inline bool is_mbpf_classic_store_pkt(const struct nfp_insn_meta *meta) > > static inline bool is_mbpf_atomic(const struct nfp_insn_meta *meta) > { > - return (meta->insn.code & ~BPF_SIZE_MASK) == (BPF_STX | BPF_ATOMIC); > + return (meta->insn.code & ~BPF_LD_ST_SIZE_MASK) == (BPF_STX | BPF_ATOMIC); > } > > static inline bool is_mbpf_mul(const struct nfp_insn_meta *meta) > diff --git a/include/linux/bpf.h b/include/linux/bpf.h > index a25730eaa148..e85924719c65 100644 > --- a/include/linux/bpf.h > +++ b/include/linux/bpf.h > @@ -995,6 +995,7 @@ struct bpf_array { > BPF_F_RDONLY_PROG | \ > BPF_F_WRONLY | \ > BPF_F_WRONLY_PROG) > +#define BPF_LD_ST_SIZE_MASK 0x18 /* mask of size modifier */ > > #define BPF_MAP_CAN_READ BIT(0) > #define BPF_MAP_CAN_WRITE BIT(1) > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index f9096b049cd6..29fdfdb8abfa 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -11384,15 +11384,11 @@ static int convert_ctx_accesses(struct bpf_verifier_env *env) > for (i = 0; i < insn_cnt; i++, insn++) { > bpf_convert_ctx_access_t convert_ctx_access; > > - if (insn->code == (BPF_LDX | BPF_MEM | BPF_B) || > - insn->code == (BPF_LDX | BPF_MEM | BPF_H) || > - insn->code == (BPF_LDX | BPF_MEM | BPF_W) || > - insn->code == (BPF_LDX | BPF_MEM | BPF_DW)) > + /* opcode: BPF_MEM | | BPF_LDX */ > + if ((insn->code & ~BPF_LD_ST_SIZE_MASK) == (BPF_LDX | BPF_MEM)) > type = BPF_READ; > - else if (insn->code == (BPF_STX | BPF_MEM | BPF_B) || > - insn->code == (BPF_STX | BPF_MEM | BPF_H) || > - insn->code == (BPF_STX | BPF_MEM | BPF_W) || > - insn->code == (BPF_STX | BPF_MEM | BPF_DW)) > + /* opcode: BPF_MEM | | BPF_STX */ > + else if ((insn->code & ~BPF_LD_ST_SIZE_MASK) == (BPF_STX | BPF_MEM)) > type = BPF_WRITE; > else > continue; > To me this cleanup makes the code harder to read, in particular on verfier side, I don't think it's worth it, especially given it's not in (highly) performance critical code. Thanks, Daniel ``` ## Re: \[PATCH bpf-next v2\] bpf: Simplify expression for identify bpf mem type - From: Jianlin Lv \ - Date: 2021-03-19 01:59:27 - Message: https://ratatoskr.run/linux-api/2021/03/6307068.md ```text On Thu, Mar 18, 2021 at 11:58 PM Daniel Borkmann [ref] wrote: > > On 3/18/21 7:36 AM, Jianlin Lv wrote: > > Added BPF_LD_ST_SIZE_MASK macro as mask of size modifier that help to > > reduce the evaluation of expressions in if statements, > > and remove BPF_SIZE_MASK in netronome driver. > > > > Signed-off-by: Jianlin Lv > > --- > > v2: Move the bpf_LD_ST_SIZE_MASK macro definition to include/linux/bpf.h > > --- > > drivers/net/ethernet/netronome/nfp/bpf/main.h | 8 +++----- > > include/linux/bpf.h | 1 + > > kernel/bpf/verifier.c | 12 ++++-------- > > 3 files changed, 8 insertions(+), 13 deletions(-) > > > > diff --git a/drivers/net/ethernet/netronome/nfp/bpf/main.h b/drivers/net/ethernet/netronome/nfp/bpf/main.h > > index d0e17eebddd9..e90981e69763 100644 > > --- a/drivers/net/ethernet/netronome/nfp/bpf/main.h > > +++ b/drivers/net/ethernet/netronome/nfp/bpf/main.h > > @@ -346,8 +346,6 @@ struct nfp_insn_meta { > > struct list_head l; > > }; > > > > -#define BPF_SIZE_MASK 0x18 > > - > > static inline u8 mbpf_class(const struct nfp_insn_meta *meta) > > { > > return BPF_CLASS(meta->insn.code); > > @@ -375,7 +373,7 @@ static inline bool is_mbpf_alu(const struct nfp_insn_meta *meta) > > > > static inline bool is_mbpf_load(const struct nfp_insn_meta *meta) > > { > > - return (meta->insn.code & ~BPF_SIZE_MASK) == (BPF_LDX | BPF_MEM); > > + return (meta->insn.code & ~BPF_LD_ST_SIZE_MASK) == (BPF_LDX | BPF_MEM); > > } > > > > static inline bool is_mbpf_jmp32(const struct nfp_insn_meta *meta) > > @@ -395,7 +393,7 @@ static inline bool is_mbpf_jmp(const struct nfp_insn_meta *meta) > > > > static inline bool is_mbpf_store(const struct nfp_insn_meta *meta) > > { > > - return (meta->insn.code & ~BPF_SIZE_MASK) == (BPF_STX | BPF_MEM); > > + return (meta->insn.code & ~BPF_LD_ST_SIZE_MASK) == (BPF_STX | BPF_MEM); > > } > > > > static inline bool is_mbpf_load_pkt(const struct nfp_insn_meta *meta) > > @@ -430,7 +428,7 @@ static inline bool is_mbpf_classic_store_pkt(const struct nfp_insn_meta *meta) > > > > static inline bool is_mbpf_atomic(const struct nfp_insn_meta *meta) > > { > > - return (meta->insn.code & ~BPF_SIZE_MASK) == (BPF_STX | BPF_ATOMIC); > > + return (meta->insn.code & ~BPF_LD_ST_SIZE_MASK) == (BPF_STX | BPF_ATOMIC); > > } > > > > static inline bool is_mbpf_mul(const struct nfp_insn_meta *meta) > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h > > index a25730eaa148..e85924719c65 100644 > > --- a/include/linux/bpf.h > > +++ b/include/linux/bpf.h > > @@ -995,6 +995,7 @@ struct bpf_array { > > BPF_F_RDONLY_PROG | \ > > BPF_F_WRONLY | \ > > BPF_F_WRONLY_PROG) > > +#define BPF_LD_ST_SIZE_MASK 0x18 /* mask of size modifier */ > > > > #define BPF_MAP_CAN_READ BIT(0) > > #define BPF_MAP_CAN_WRITE BIT(1) > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > > index f9096b049cd6..29fdfdb8abfa 100644 > > --- a/kernel/bpf/verifier.c > > +++ b/kernel/bpf/verifier.c > > @@ -11384,15 +11384,11 @@ static int convert_ctx_accesses(struct bpf_verifier_env *env) > > for (i = 0; i < insn_cnt; i++, insn++) { > > bpf_convert_ctx_access_t convert_ctx_access; > > > > - if (insn->code == (BPF_LDX | BPF_MEM | BPF_B) || > > - insn->code == (BPF_LDX | BPF_MEM | BPF_H) || > > - insn->code == (BPF_LDX | BPF_MEM | BPF_W) || > > - insn->code == (BPF_LDX | BPF_MEM | BPF_DW)) > > + /* opcode: BPF_MEM | | BPF_LDX */ > > + if ((insn->code & ~BPF_LD_ST_SIZE_MASK) == (BPF_LDX | BPF_MEM)) > > type = BPF_READ; > > - else if (insn->code == (BPF_STX | BPF_MEM | BPF_B) || > > - insn->code == (BPF_STX | BPF_MEM | BPF_H) || > > - insn->code == (BPF_STX | BPF_MEM | BPF_W) || > > - insn->code == (BPF_STX | BPF_MEM | BPF_DW)) > > + /* opcode: BPF_MEM | | BPF_STX */ > > + else if ((insn->code & ~BPF_LD_ST_SIZE_MASK) == (BPF_STX | BPF_MEM)) > > type = BPF_WRITE; > > else > > continue; > > > > To me this cleanup makes the code harder to read, in particular on verfier side, > I don't think it's worth it, especially given it's not in (highly) performance > critical code. > > Thanks, > Daniel I have some different opinions. I think the addition of the mask just helps developers understand that the currently processed instruction covers all possible values of size; In addition, from my experience in reading the verfier.c file, to fully understand this part of the code, it needs to understand the meaning of each instruction. It is really hard to say that this is an easy task, at least for me. Haha ^_^ Regards, Jianlin ```