Re: [RFC PATCH bpf-next 2/7] bpf: Add struct argument info in btf_func_model

[Yonghong Song <yhs@fb.com>]

On 8/9/22 5:25 PM, Andrii Nakryiko wrote:
> On Tue, Aug 9, 2022 at 10:38 AM Yonghong Song <yhs@fb.com> wrote:
>>
>>
>>
>> On 8/8/22 5:02 PM, Andrii Nakryiko wrote:
>>> On Tue, Jul 26, 2022 at 10:11 AM Yonghong Song <yhs@fb.com> wrote:
>>>>
>>>> Add struct argument information in btf_func_model and such information
>>>> will be used in arch specific function arch_prepare_bpf_trampoline()
>>>> to prepare argument access properly in trampoline.
>>>>
>>>> Signed-off-by: Yonghong Song <yhs@fb.com>
>>>> ---
>>>>    include/linux/bpf.h | 9 +++++++++
>>>>    1 file changed, 9 insertions(+)
>>>>
>>>> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
>>>> index 20c26aed7896..173b42cf3940 100644
>>>> --- a/include/linux/bpf.h
>>>> +++ b/include/linux/bpf.h
>>>> @@ -726,10 +726,19 @@ enum bpf_cgroup_storage_type {
>>>>     */
>>>>    #define MAX_BPF_FUNC_REG_ARGS 5
>>>>
>>>> +/* The maximum number of struct arguments a single function may have. */
>>>> +#define MAX_BPF_FUNC_STRUCT_ARGS 2
>>>> +
>>>>    struct btf_func_model {
>>>>           u8 ret_size;
>>>>           u8 nr_args;
>>>>           u8 arg_size[MAX_BPF_FUNC_ARGS];
>>>> +       /* The struct_arg_idx should be in increasing order like (0, 2, ...).
>>>> +        * The struct_arg_bsize encodes the struct field byte size
>>>> +        * for the corresponding struct argument index.
>>>> +        */
>>>> +       u8 struct_arg_idx[MAX_BPF_FUNC_STRUCT_ARGS];
>>>> +       u8 struct_arg_bsize[MAX_BPF_FUNC_STRUCT_ARGS];
>>>
>>> Few questions here. It might be a bad idea, but I thought I'd bring it
>>> up anyway.
>>>
>>> So, is there any benefit into having these separate struct_arg_idx and
>>> struct_arg_bsize fields and then processing arg_size completely
>>> separate from struct_arg_idx/struct_arg_bsize in patch #4? Reading
>>> patch #4 it felt like it would be much easier to keep track of things
>>> if we had a single loop going over all the arguments, and then if some
>>> argument is a struct -- do some extra step to copy up to 16 bytes onto
>>> stack and store the pointer there (and skip up to one extra argument).
>>> And if it's not a struct arg -- do what we do right now.
>>>
>>> What if instead we keep btf_func_mode definition as is, but for struct
>>> argument we add extra flag to arg_size[struct_arg_idx] value to mark
>>> that it is a struct argument. This limits arg_size to 128 bytes, but I
>>> think it's more than enough for both struct and non-struct cases,
>>> right? Distill function would make sure that nr_args matches number of
>>> logical arguments and not number of registers.
>>>
>>> Would that work? Would that make anything harder to implement in
>>> arch-specific code? I don't see what, but I haven't grokked all the
>>> details of patch #4, so I'm sorry if I missed something obvious. The
>>> way I see it, it will make overall logic for saving/restoring
>>> registers more uniform, roughly:
>>>
>>> for (int arg_idx = 0; arg_idx < model->arg_size; arg_idx++) {
>>>     if (arg & BTF_FMODEL_STRUCT_ARG) {
>>>       /* handle struct, calc extra registers "consumed" from
>>> arg_size[arg_idx] ~BTF_FMODEL_STRUCT_ARG */
>>>     } else {
>>>       /* just a normal register */
>>>     }
>>> }
>>
>> Your suggestion sounds good to me. Yes, we already have
>> arg_size array. We can add a
>>          bool is_struct_arg[MAX_BPF_FUNC_ARGS];
>> to indicate whether the argument is a struct or not.
>> In this case, we can avoid duplication between
>> arg_size and struct_arg_bsize.
>>
> 
> I was imagining that we'll just use the existing arg_size and define
> that the upper bit is a struct/non-struct bit. But if that's too
> confusing and cryptic, I wonder if it's better to have
> 
> u8 arg_flags[MAX_BPF_FUNC_ARGS];

I prefer a separate array, which seems cleanly.

> 
> instead and define the BPF_FNARG_STRUCT flag.
> 
> For what you did in your other patch set (u8/u16 handling for func
> result), we can then define ret_flags and have a flag whether the
> argument is integer and whether it's signed in such flags (instead of
> bit fields).

Currently, we only have use case for whether it is a struct. But
agree we can make it extensible with an enum to indicate what kind
of info for the argument.

> 
> This way we have a unified and more extendable "size+flags" approach
> both for input arguments and return result.
> 
> WDYT?
> 
>>>
>>>
>>> If we do stick to current approach, though, let's please
>>> s/struct_arg_bsize/struct_arg_size/. Isn't arg_size also and already
>>> in bytes? It will keep naming and meaning consistent across struct and
>>> non-struct args.
>>>
>>> BTW, by not having btf_func_model encode register indices in
>>> struct_arg_idx we keep btf_func_model pretty architecture-agnostic,
>>> right? It will be per each architecture specific implementation to
>>> perform mapping this *model* into actual registers used?
>>>
>>>
>>>
>>>
>>>>    };
>>>>
>>>>    /* Restore arguments before returning from trampoline to let original function
>>>> --
>>>> 2.30.2
>>>>