diff mbox series

[bpf-next] bpf, mips: No need to use min() to get MAX_TAIL_CALL_CNT

Message ID 1661479927-6953-1-git-send-email-yangtiezhu@loongson.cn (mailing list archive)
State Superseded
Headers show
Series [bpf-next] bpf, mips: No need to use min() to get MAX_TAIL_CALL_CNT | expand

Commit Message

Tiezhu Yang Aug. 26, 2022, 2:12 a.m. UTC
MAX_TAIL_CALL_CNT is 33, so min(MAX_TAIL_CALL_CNT, 0xffff) is always
MAX_TAIL_CALL_CNT, it is better to use MAX_TAIL_CALL_CNT directly.

Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
---
 arch/mips/net/bpf_jit_comp32.c | 2 +-
 arch/mips/net/bpf_jit_comp64.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

Comments

Daniel Borkmann Aug. 26, 2022, 4:17 p.m. UTC | #1
On 8/26/22 4:12 AM, Tiezhu Yang wrote:
> MAX_TAIL_CALL_CNT is 33, so min(MAX_TAIL_CALL_CNT, 0xffff) is always
> MAX_TAIL_CALL_CNT, it is better to use MAX_TAIL_CALL_CNT directly.
> 
> Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
> ---
>   arch/mips/net/bpf_jit_comp32.c | 2 +-
>   arch/mips/net/bpf_jit_comp64.c | 2 +-
>   2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/mips/net/bpf_jit_comp32.c b/arch/mips/net/bpf_jit_comp32.c
> index 83c975d..8fee671 100644
> --- a/arch/mips/net/bpf_jit_comp32.c
> +++ b/arch/mips/net/bpf_jit_comp32.c
> @@ -1381,7 +1381,7 @@ void build_prologue(struct jit_context *ctx)
>   	 * 16-byte area in the parent's stack frame. On a tail call, the
>   	 * calling function jumps into the prologue after these instructions.
>   	 */
> -	emit(ctx, ori, MIPS_R_T9, MIPS_R_ZERO, min(MAX_TAIL_CALL_CNT, 0xffff));

I presume this is the max that can be encoded, right? Maybe just convert this
to a BUILD_BUG_ON(MAX_TAIL_CALL_CNT > 0xffff) with a comment on why the assertion
is there?

> +	emit(ctx, ori, MIPS_R_T9, MIPS_R_ZERO, MAX_TAIL_CALL_CNT);
>   	emit(ctx, sw, MIPS_R_T9, 0, MIPS_R_SP);
>   
>   	/*
> diff --git a/arch/mips/net/bpf_jit_comp64.c b/arch/mips/net/bpf_jit_comp64.c
> index 6475828..ac175af 100644
> --- a/arch/mips/net/bpf_jit_comp64.c
> +++ b/arch/mips/net/bpf_jit_comp64.c
> @@ -552,7 +552,7 @@ void build_prologue(struct jit_context *ctx)
>   	 * On a tail call, the calling function jumps into the prologue
>   	 * after this instruction.
>   	 */
> -	emit(ctx, ori, tc, MIPS_R_ZERO, min(MAX_TAIL_CALL_CNT, 0xffff));
> +	emit(ctx, ori, tc, MIPS_R_ZERO, MAX_TAIL_CALL_CNT);
>   
>   	/* === Entry-point for tail calls === */
>   
>
Johan Almbladh Aug. 26, 2022, 7:01 p.m. UTC | #2
On Fri, Aug 26, 2022 at 6:18 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 8/26/22 4:12 AM, Tiezhu Yang wrote:
> > MAX_TAIL_CALL_CNT is 33, so min(MAX_TAIL_CALL_CNT, 0xffff) is always
> > MAX_TAIL_CALL_CNT, it is better to use MAX_TAIL_CALL_CNT directly.
> >
> > Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
> > ---
> >   arch/mips/net/bpf_jit_comp32.c | 2 +-
> >   arch/mips/net/bpf_jit_comp64.c | 2 +-
> >   2 files changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/mips/net/bpf_jit_comp32.c b/arch/mips/net/bpf_jit_comp32.c
> > index 83c975d..8fee671 100644
> > --- a/arch/mips/net/bpf_jit_comp32.c
> > +++ b/arch/mips/net/bpf_jit_comp32.c
> > @@ -1381,7 +1381,7 @@ void build_prologue(struct jit_context *ctx)
> >        * 16-byte area in the parent's stack frame. On a tail call, the
> >        * calling function jumps into the prologue after these instructions.
> >        */
> > -     emit(ctx, ori, MIPS_R_T9, MIPS_R_ZERO, min(MAX_TAIL_CALL_CNT, 0xffff));
>
> I presume this is the max that can be encoded, right? Maybe just convert this
> to a BUILD_BUG_ON(MAX_TAIL_CALL_CNT > 0xffff) with a comment on why the assertion
> is there?

Correct. The min() is there for a reason. In the unlikely event that
the TCC limit is raised to more than 16 bits, it is clamped to the
maximum value allowed for the generated code (0xffff). One can argue
that it is better fail to compile instead of degrading gracefully, but
some kind check should be there IMO.

>
> > +     emit(ctx, ori, MIPS_R_T9, MIPS_R_ZERO, MAX_TAIL_CALL_CNT);
> >       emit(ctx, sw, MIPS_R_T9, 0, MIPS_R_SP);
> >
> >       /*
> > diff --git a/arch/mips/net/bpf_jit_comp64.c b/arch/mips/net/bpf_jit_comp64.c
> > index 6475828..ac175af 100644
> > --- a/arch/mips/net/bpf_jit_comp64.c
> > +++ b/arch/mips/net/bpf_jit_comp64.c
> > @@ -552,7 +552,7 @@ void build_prologue(struct jit_context *ctx)
> >        * On a tail call, the calling function jumps into the prologue
> >        * after this instruction.
> >        */
> > -     emit(ctx, ori, tc, MIPS_R_ZERO, min(MAX_TAIL_CALL_CNT, 0xffff));
> > +     emit(ctx, ori, tc, MIPS_R_ZERO, MAX_TAIL_CALL_CNT);
> >
> >       /* === Entry-point for tail calls === */
> >
> >
>
diff mbox series

Patch

diff --git a/arch/mips/net/bpf_jit_comp32.c b/arch/mips/net/bpf_jit_comp32.c
index 83c975d..8fee671 100644
--- a/arch/mips/net/bpf_jit_comp32.c
+++ b/arch/mips/net/bpf_jit_comp32.c
@@ -1381,7 +1381,7 @@  void build_prologue(struct jit_context *ctx)
 	 * 16-byte area in the parent's stack frame. On a tail call, the
 	 * calling function jumps into the prologue after these instructions.
 	 */
-	emit(ctx, ori, MIPS_R_T9, MIPS_R_ZERO, min(MAX_TAIL_CALL_CNT, 0xffff));
+	emit(ctx, ori, MIPS_R_T9, MIPS_R_ZERO, MAX_TAIL_CALL_CNT);
 	emit(ctx, sw, MIPS_R_T9, 0, MIPS_R_SP);
 
 	/*
diff --git a/arch/mips/net/bpf_jit_comp64.c b/arch/mips/net/bpf_jit_comp64.c
index 6475828..ac175af 100644
--- a/arch/mips/net/bpf_jit_comp64.c
+++ b/arch/mips/net/bpf_jit_comp64.c
@@ -552,7 +552,7 @@  void build_prologue(struct jit_context *ctx)
 	 * On a tail call, the calling function jumps into the prologue
 	 * after this instruction.
 	 */
-	emit(ctx, ori, tc, MIPS_R_ZERO, min(MAX_TAIL_CALL_CNT, 0xffff));
+	emit(ctx, ori, tc, MIPS_R_ZERO, MAX_TAIL_CALL_CNT);
 
 	/* === Entry-point for tail calls === */