| Message ID | 20220726171140.710070-1-yhs@fb.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | BPF |
| Headers | show |
| Series | bpf: Support struct value argument for trampoline base progs | expand |
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 */ } } 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 >
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. > > > 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 >>
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]; 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). 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 > >>
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 >>>>
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]; }; /* Restore arguments before returning from trampoline to let original function
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(+)