Messages in this thread | | | Subject | Re: [PATCH bpf-next v2] bpf: Simplify expression for identify bpf mem type | From | Daniel Borkmann <> | Date | Thu, 18 Mar 2021 16:58:02 +0100 |
| |
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 <Jianlin.Lv@arm.com> > --- > 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 | <size> | 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 | <size> | 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
| |