| Message ID | 20210928025228.88673-3-houtao1@huawei.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | introduce dummy BPF STRUCT_OPS | expand |
| Context | Check | Description |
|---|---|---|
| netdev/apply | fail | Patch does not apply to bpf-next |
| netdev/tree_selection | success | Clearly marked for bpf-next |
| bpf/vmtest-bpf-next-PR | fail | merge-conflict |
On Tue, Sep 28, 2021 at 10:52:25AM +0800, Hou Tao wrote: > Factor out a helper bpf_prepare_st_ops_prog() to prepare trampoline > for BPF_PROG_TYPE_STRUCT_OPS prog. It will be used by .test_run > callback in following patch. Thanks for the patches. This preparation change should be the first patch instead. > diff --git a/include/linux/bpf.h b/include/linux/bpf.h > index 155dfcfb8923..002bbb2c8bc7 100644 > --- a/include/linux/bpf.h > +++ b/include/linux/bpf.h > @@ -2224,4 +2224,9 @@ int bpf_bprintf_prepare(char *fmt, u32 fmt_size, const u64 *raw_args, > u32 **bin_buf, u32 num_args); > void bpf_bprintf_cleanup(void); > > +int bpf_prepare_st_ops_prog(struct bpf_tramp_progs *tprogs, > + struct bpf_prog *prog, > + const struct btf_func_model *model, > + void *image, void *image_end); Move it under where other bpf_struct_ops_.*() resides in bpf.h. > + > #endif /* _LINUX_BPF_H */ > diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c > index 9abcc33f02cf..ec3c25174923 100644 > --- a/kernel/bpf/bpf_struct_ops.c > +++ b/kernel/bpf/bpf_struct_ops.c > @@ -312,6 +312,20 @@ static int check_zero_holes(const struct btf_type *t, void *data) > return 0; > } > > +int bpf_prepare_st_ops_prog(struct bpf_tramp_progs *tprogs, > + struct bpf_prog *prog, > + const struct btf_func_model *model, > + void *image, void *image_end) The existing struct_ops functions in the kernel now have naming like bpf_struct_ops_.*(). How about renaming it to bpf_struct_ops_prepare_trampoline()? > +{ > + u32 flags; > + > + tprogs[BPF_TRAMP_FENTRY].progs[0] = prog; > + tprogs[BPF_TRAMP_FENTRY].nr_progs = 1; > + flags = model->ret_size > 0 ? BPF_TRAMP_F_RET_FENTRY_RET : 0; > + return arch_prepare_bpf_trampoline(NULL, image, image_end, > + model, flags, tprogs, NULL); > +} > + > static int bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key, > void *value, u64 flags) > { > @@ -368,7 +382,6 @@ static int bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key, > const struct btf_type *mtype, *ptype; > struct bpf_prog *prog; > u32 moff; > - u32 flags; > > moff = btf_member_bit_offset(t, member) / 8; > ptype = btf_type_resolve_ptr(btf_vmlinux, member->type, NULL); > @@ -430,14 +443,9 @@ static int bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key, > goto reset_unlock; > } > > - tprogs[BPF_TRAMP_FENTRY].progs[0] = prog; > - tprogs[BPF_TRAMP_FENTRY].nr_progs = 1; > - flags = st_ops->func_models[i].ret_size > 0 ? > - BPF_TRAMP_F_RET_FENTRY_RET : 0; This change can't apply to bpf-next now because commit 356ed64991c6 ("bpf: Handle return value of BPF_PROG_TYPE_STRUCT_OPS prog") is not pulled into bpf-next yet. Please mention the dependency in the cover letter if it is still the case in v2. > - err = arch_prepare_bpf_trampoline(NULL, image, > - st_map->image + PAGE_SIZE, > - &st_ops->func_models[i], > - flags, tprogs, NULL); > + err = bpf_prepare_st_ops_prog(tprogs, prog, > + &st_ops->func_models[i], > + image, st_map->image + PAGE_SIZE); > if (err < 0) > goto reset_unlock; > > -- > 2.29.2 >
Hi On 9/30/2021 1:56 AM, Martin KaFai Lau wrote: > On Tue, Sep 28, 2021 at 10:52:25AM +0800, Hou Tao wrote: >> Factor out a helper bpf_prepare_st_ops_prog() to prepare trampoline >> for BPF_PROG_TYPE_STRUCT_OPS prog. It will be used by .test_run >> callback in following patch. > Thanks for the patches. Thanks for you review. > > This preparation change should be the first patch instead. Will do. > >> diff --git a/include/linux/bpf.h b/include/linux/bpf.h >> index 155dfcfb8923..002bbb2c8bc7 100644 >> --- a/include/linux/bpf.h >> +++ b/include/linux/bpf.h >> @@ -2224,4 +2224,9 @@ int bpf_bprintf_prepare(char *fmt, u32 fmt_size, const u64 *raw_args, >> u32 **bin_buf, u32 num_args); >> void bpf_bprintf_cleanup(void); >> >> +int bpf_prepare_st_ops_prog(struct bpf_tramp_progs *tprogs, >> + struct bpf_prog *prog, >> + const struct btf_func_model *model, >> + void *image, void *image_end); > Move it under where other bpf_struct_ops_.*() resides in bpf.h. > >> + >> #endif /* _LINUX_BPF_H */ >> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c >> index 9abcc33f02cf..ec3c25174923 100644 >> --- a/kernel/bpf/bpf_struct_ops.c >> +++ b/kernel/bpf/bpf_struct_ops.c >> @@ -312,6 +312,20 @@ static int check_zero_holes(const struct btf_type *t, void *data) >> return 0; >> } >> >> +int bpf_prepare_st_ops_prog(struct bpf_tramp_progs *tprogs, >> + struct bpf_prog *prog, >> + const struct btf_func_model *model, >> + void *image, void *image_end) > The existing struct_ops functions in the kernel now have naming like > bpf_struct_ops_.*(). How about renaming it to > bpf_struct_ops_prepare_trampoline()? bpf_struct_ops_prepare_trampoline() may be a little long, and it will make the indentations of its parameters look ugly, so how about bpf_struct_ops_prep_prog() ? > >> +{ >> + u32 flags; >> + >> + tprogs[BPF_TRAMP_FENTRY].progs[0] = prog; >> + tprogs[BPF_TRAMP_FENTRY].nr_progs = 1; >> + flags = model->ret_size > 0 ? BPF_TRAMP_F_RET_FENTRY_RET : 0; >> + return arch_prepare_bpf_trampoline(NULL, image, image_end, >> + model, flags, tprogs, NULL); >> +} >> + >> static int bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key, >> void *value, u64 flags) >> { >> @@ -368,7 +382,6 @@ static int bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key, >> const struct btf_type *mtype, *ptype; >> struct bpf_prog *prog; >> u32 moff; >> - u32 flags; >> >> moff = btf_member_bit_offset(t, member) / 8; >> ptype = btf_type_resolve_ptr(btf_vmlinux, member->type, NULL); >> @@ -430,14 +443,9 @@ static int bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key, >> goto reset_unlock; >> } >> >> - tprogs[BPF_TRAMP_FENTRY].progs[0] = prog; >> - tprogs[BPF_TRAMP_FENTRY].nr_progs = 1; >> - flags = st_ops->func_models[i].ret_size > 0 ? >> - BPF_TRAMP_F_RET_FENTRY_RET : 0; > This change can't apply to bpf-next now because > commit 356ed64991c6 ("bpf: Handle return value of BPF_PROG_TYPE_STRUCT_OPS prog") > is not pulled into bpf-next yet. Please mention the dependency > in the cover letter if it is still the case in v2. Thanks for the reminder. Will do. > >> - err = arch_prepare_bpf_trampoline(NULL, image, >> - st_map->image + PAGE_SIZE, >> - &st_ops->func_models[i], >> - flags, tprogs, NULL); >> + err = bpf_prepare_st_ops_prog(tprogs, prog, >> + &st_ops->func_models[i], >> + image, st_map->image + PAGE_SIZE); >> if (err < 0) >> goto reset_unlock; >> >> -- >> 2.29.2 >> > .
On Thu, Sep 30, 2021 at 06:17:33PM +0800, Hou Tao wrote: > >> +int bpf_prepare_st_ops_prog(struct bpf_tramp_progs *tprogs, > >> + struct bpf_prog *prog, > >> + const struct btf_func_model *model, > >> + void *image, void *image_end) > > The existing struct_ops functions in the kernel now have naming like > > bpf_struct_ops_.*(). How about renaming it to > > bpf_struct_ops_prepare_trampoline()? > bpf_struct_ops_prepare_trampoline() may be a little long, and it will make > the indentations of its parameters look ugly, so how about > bpf_struct_ops_prep_prog() ? hmm... naming is hard... but it is preparing the trampoline instead of preparing the prog, and most other bpf funcs are using 'prepare' instead of 'prep'. My preference is a better naming on what the func does and a consistent naming with others. The indentation looks fine also. It is not too bad ;) bpf_struct_ops_prepare_prog() arch_prepare_bpf_trampoline() bpf_struct_ops_prepare_trampoline() The params indentation looks fine and within 80 cols: int bpf_struct_ops_prepare_trampoline(struct bpf_tramp_progs *tprogs, struct bpf_prog *prog, const struct btf_func_model *model, void *image, void *image_end0 { }
diff --git a/include/linux/bpf.h b/include/linux/bpf.h index 155dfcfb8923..002bbb2c8bc7 100644 --- a/include/linux/bpf.h +++ b/include/linux/bpf.h @@ -2224,4 +2224,9 @@ int bpf_bprintf_prepare(char *fmt, u32 fmt_size, const u64 *raw_args, u32 **bin_buf, u32 num_args); void bpf_bprintf_cleanup(void); +int bpf_prepare_st_ops_prog(struct bpf_tramp_progs *tprogs, + struct bpf_prog *prog, + const struct btf_func_model *model, + void *image, void *image_end); + #endif /* _LINUX_BPF_H */ diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c index 9abcc33f02cf..ec3c25174923 100644 --- a/kernel/bpf/bpf_struct_ops.c +++ b/kernel/bpf/bpf_struct_ops.c @@ -312,6 +312,20 @@ static int check_zero_holes(const struct btf_type *t, void *data) return 0; } +int bpf_prepare_st_ops_prog(struct bpf_tramp_progs *tprogs, + struct bpf_prog *prog, + const struct btf_func_model *model, + void *image, void *image_end) +{ + u32 flags; + + tprogs[BPF_TRAMP_FENTRY].progs[0] = prog; + tprogs[BPF_TRAMP_FENTRY].nr_progs = 1; + flags = model->ret_size > 0 ? BPF_TRAMP_F_RET_FENTRY_RET : 0; + return arch_prepare_bpf_trampoline(NULL, image, image_end, + model, flags, tprogs, NULL); +} + static int bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key, void *value, u64 flags) { @@ -368,7 +382,6 @@ static int bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key, const struct btf_type *mtype, *ptype; struct bpf_prog *prog; u32 moff; - u32 flags; moff = btf_member_bit_offset(t, member) / 8; ptype = btf_type_resolve_ptr(btf_vmlinux, member->type, NULL); @@ -430,14 +443,9 @@ static int bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key, goto reset_unlock; } - tprogs[BPF_TRAMP_FENTRY].progs[0] = prog; - tprogs[BPF_TRAMP_FENTRY].nr_progs = 1; - flags = st_ops->func_models[i].ret_size > 0 ? - BPF_TRAMP_F_RET_FENTRY_RET : 0; - err = arch_prepare_bpf_trampoline(NULL, image, - st_map->image + PAGE_SIZE, - &st_ops->func_models[i], - flags, tprogs, NULL); + err = bpf_prepare_st_ops_prog(tprogs, prog, + &st_ops->func_models[i], + image, st_map->image + PAGE_SIZE); if (err < 0) goto reset_unlock;
Factor out a helper bpf_prepare_st_ops_prog() to prepare trampoline for BPF_PROG_TYPE_STRUCT_OPS prog. It will be used by .test_run callback in following patch. Signed-off-by: Hou Tao <houtao1@huawei.com> --- include/linux/bpf.h | 5 +++++ kernel/bpf/bpf_struct_ops.c | 26 +++++++++++++++++--------- 2 files changed, 22 insertions(+), 9 deletions(-)