[bpf-next,v10,2/7] bpf: Enable private stack for eligible subprogs

Commit Message

Yonghong Song Nov. 7, 2024, 2:41 a.m. UTC

If private stack is requested by any subprog, set that subprog
prog->aux->priv_stack_requested to be true so later jit can allocate
private stack for that subprog properly.

Also set env->prog->aux->priv_stack_requested to be true if
subprog[0] requests private stack. This is a use case for a
single main prog (no subprogs) to use private stack.

Signed-off-by: Yonghong Song <[e-mail]>
---
 kernel/bpf/verifier.c | 7 +++++++
 1 file changed, 7 insertions(+)

Comments

Alexei Starovoitov Nov. 8, 2024, 7:11 p.m. UTC | #1

On Wed, Nov 6, 2024 at 6:42 PM Yonghong Song <[e-mail]> wrote:
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 2284b909b499..09bb9dc939d6 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -6278,6 +6278,10 @@ static int check_max_stack_depth(struct bpf_verifier_env *env)
>                                 return ret;
>                 }
>         }
> +

and patch 6 adds this line here:
+ env->prog->aux->priv_stack_requested = false;

> +       if (si[0].priv_stack_mode == PRIV_STACK_ADAPTIVE)
> +               env->prog->aux->priv_stack_requested = true;
> +

which makes the above hard to reason about.

I think the root of the problem is the dual meaning of
the priv_stack_requested flag.
On one side it's a way for sched-ext to ask bpf core to enable priv stack,
and on the other side it's a request from bpf core to bpf jit
to allocate it.

I think it's better to split these two conditions.
Extra bool is cheap.

How about 'bool priv_stack_requested' will be used by sched-ext only
and patch 6 largely stays as-is.

While patch 1 drops the introduction of priv_stack_requested flag.
Instead 'bool jits_use_priv_stack' is introduced in the patch 2
and used by JITs to allocate priv stack.

I know we use 'jit_requested' to tell JITs to jit it,
so we can bike shed on alternative ways to name these two flags.

>         return 0;
>  }
>
> @@ -20211,6 +20215,9 @@ static int jit_subprogs(struct bpf_verifier_env *env)
>
>                 func[i]->aux->name[0] = 'F';
>                 func[i]->aux->stack_depth = env->subprog_info[i].stack_depth;
> +               if (env->subprog_info[i].priv_stack_mode == PRIV_STACK_ADAPTIVE)
> +                       func[i]->aux->priv_stack_requested = true;
> +
>                 func[i]->jit_requested = 1;
>                 func[i]->blinding_requested = prog->blinding_requested;
>                 func[i]->aux->kfunc_tab = prog->aux->kfunc_tab;
> --
> 2.43.5
>

Yonghong Song Nov. 8, 2024, 9:41 p.m. UTC | #2

On 11/8/24 11:11 AM, Alexei Starovoitov wrote:
> On Wed, Nov 6, 2024 at 6:42 PM Yonghong Song <[e-mail]> wrote:
>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>> index 2284b909b499..09bb9dc939d6 100644
>> --- a/kernel/bpf/verifier.c
>> +++ b/kernel/bpf/verifier.c
>> @@ -6278,6 +6278,10 @@ static int check_max_stack_depth(struct bpf_verifier_env *env)
>>                                  return ret;
>>                  }
>>          }
>> +
> and patch 6 adds this line here:
> + env->prog->aux->priv_stack_requested = false;
>
>> +       if (si[0].priv_stack_mode == PRIV_STACK_ADAPTIVE)
>> +               env->prog->aux->priv_stack_requested = true;
>> +
> which makes the above hard to reason about.
>
> I think the root of the problem is the dual meaning of
> the priv_stack_requested flag.
> On one side it's a way for sched-ext to ask bpf core to enable priv stack,
> and on the other side it's a request from bpf core to bpf jit
> to allocate it.

You are right. I use the same priv_stack_requested to do two things.

>
> I think it's better to split these two conditions.
> Extra bool is cheap.
>
> How about 'bool priv_stack_requested' will be used by sched-ext only
> and patch 6 largely stays as-is.
>
> While patch 1 drops the introduction of priv_stack_requested flag.
> Instead 'bool jits_use_priv_stack' is introduced in the patch 2
> and used by JITs to allocate priv stack.
>
> I know we use 'jit_requested' to tell JITs to jit it,
> so we can bike shed on alternative ways to name these two flags.

Two bool's approach sound good to me. As you mentioned, two bool's
are not expensive and can make logic cleaner. Will do in the next
revision.

>
>>          return 0;
>>   }
>>
>> @@ -20211,6 +20215,9 @@ static int jit_subprogs(struct bpf_verifier_env *env)
>>
>>                  func[i]->aux->name[0] = 'F';
>>                  func[i]->aux->stack_depth = env->subprog_info[i].stack_depth;
>> +               if (env->subprog_info[i].priv_stack_mode == PRIV_STACK_ADAPTIVE)
>> +                       func[i]->aux->priv_stack_requested = true;
>> +
>>                  func[i]->jit_requested = 1;
>>                  func[i]->blinding_requested = prog->blinding_requested;
>>                  func[i]->aux->kfunc_tab = prog->aux->kfunc_tab;
>> --
>> 2.43.5
>>

Patch

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 2284b909b499..09bb9dc939d6 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -6278,6 +6278,10 @@  static int check_max_stack_depth(struct bpf_verifier_env *env)
 				return ret;
 		}
 	}
+
+	if (si[0].priv_stack_mode == PRIV_STACK_ADAPTIVE)
+		env->prog->aux->priv_stack_requested = true;
+
 	return 0;
 }
 
@@ -20211,6 +20215,9 @@  static int jit_subprogs(struct bpf_verifier_env *env)
 
 		func[i]->aux->name[0] = 'F';
 		func[i]->aux->stack_depth = env->subprog_info[i].stack_depth;
+		if (env->subprog_info[i].priv_stack_mode == PRIV_STACK_ADAPTIVE)
+			func[i]->aux->priv_stack_requested = true;
+
 		func[i]->jit_requested = 1;
 		func[i]->blinding_requested = prog->blinding_requested;
 		func[i]->aux->kfunc_tab = prog->aux->kfunc_tab;