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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On 8/8/22 5:02 PM, Andrii Nakryiko wrote:
On Tue, Jul 26, 2022 at 10:11 AM Yonghong Song <yhs@xxxxxx> 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@xxxxxx>
---
  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




[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux Actions SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]


  Powered by Linux