linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH bpf-next v2] bpf: Simplify expression for identify bpf mem type
@ 2021-03-18  6:36 Jianlin Lv
  2021-03-18 15:58 ` Daniel Borkmann
  0 siblings, 1 reply; 3+ messages in thread
From: Jianlin Lv @ 2021-03-18  6:36 UTC (permalink / raw)
  To: bpf
  Cc: kuba, simon.horman, davem, ast, alexei.starovoitov, daniel,
	andrii, kafai, songliubraving, yhs, john.fastabend, kpsingh,
	linux-kernel, netdev, oss-drivers, linux-api, Jianlin.Lv,
	iecedge

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;
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 3+ messages in thread
* Re: [PATCH bpf-next v2] bpf: Simplify expression for identify bpf mem type
  2021-03-18  6:36 [PATCH bpf-next v2] bpf: Simplify expression for identify bpf mem type Jianlin Lv
@ 2021-03-18 15:58 ` Daniel Borkmann
  2021-03-19  1:58   ` Jianlin Lv
  0 siblings, 1 reply; 3+ messages in thread
From: Daniel Borkmann @ 2021-03-18 15:58 UTC (permalink / raw)
  To: Jianlin Lv, bpf
  Cc: kuba, simon.horman, davem, ast, alexei.starovoitov, andrii,
	kafai, songliubraving, yhs, john.fastabend, kpsingh,
	linux-kernel, netdev, oss-drivers, linux-api, iecedge

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

^ permalink raw reply	[flat|nested] 3+ messages in thread
* Re: [PATCH bpf-next v2] bpf: Simplify expression for identify bpf mem type
  2021-03-18 15:58 ` Daniel Borkmann
@ 2021-03-19  1:58   ` Jianlin Lv
  0 siblings, 0 replies; 3+ messages in thread
From: Jianlin Lv @ 2021-03-19  1:58 UTC (permalink / raw)
  To: Daniel Borkmann
  Cc: Jianlin Lv, bpf, kuba, simon.horman, davem, ast,
	alexei.starovoitov, andrii, kafai, songliubraving, yhs,
	john.fastabend, kpsingh, linux-kernel, netdev, oss-drivers,
	linux-api

On Thu, Mar 18, 2021 at 11:58 PM Daniel Borkmann <daniel@iogearbox.net> 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 <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

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

^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-03-19  1:59 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-18  6:36 [PATCH bpf-next v2] bpf: Simplify expression for identify bpf mem type Jianlin Lv
2021-03-18 15:58 ` Daniel Borkmann
2021-03-19  1:58   ` Jianlin Lv

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).