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

[Yonghong Song <yonghong.song@linux.dev>]

On 11/8/24 11:11 AM, Alexei Starovoitov wrote:
> On Wed, Nov 6, 2024 at 6:42 PM Yonghong Song <yonghong.song@linux.dev> 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
>>