Re: [PATCH v1 bpf-next 2/4] bpf: Refactor btf_find_field with btf_field_info_search

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

 




On 10/30/23 12:31 PM, Yonghong Song wrote:

On 10/23/23 3:00 PM, Dave Marchevsky wrote:
btf_find_field takes (btf_type, special_field_types) and returns info
about the specific special fields in btf_type, in the form of an array
of struct btf_field info. The meat of this 'search for special fields'

struct btf_field_info.

process happens in btf_find_datasec_var and btf_find_struct_field
helpers: each member is examined and, if it's special, a struct
btf_field_info describing it is added to the return array. Indeed, any
function that might add to the output probably also looks at struct
members or datasec vars.

Most of the parameters passed around between helpers doing the search
can be grouped into two categories: "info about the output array" and
"info about which fields to search for". This patch joins those together
in struct btf_field_info_search, simplifying the signatures of most
helpers involved in the search, including array flattening helper that
later patches in the series will add.

The aforementioned array flattening logic will greatly increase the
number of btf_field_info's needed to describe some structs, so this
patch also turns the statically-sized struct btf_field_info
info_arr[BTF_FIELDS_MAX] into a growable array with a larger max size.

Since this patch is a 'refactoring' patch, let us delay this patch


sorry. typo. "this patch" -> "this change".


until the next one. This patch should be strictly a refactoring
patch so it becomes easier to verify changes.


Implementation notes:
   * BTF_FIELDS_MAX is now max size of growable btf_field_info *infos
     instead of initial (and max) size of static result array
     * Static array before had 10 elems (+1 tmp btf_field_info)
     * growable array starts with 16 and doubles every time it needs to
       grow, up to BTF_FIELDS_MAX of 256

   * __next_field_infos is used with next_cnt > 1 later in the series

   * btf_find_{datasec_var, struct_field} have special logic for an edge
     case where the result array is full but the field being examined
     gets BTF_FIELD_IGNORE return from btf_find_{struct, kptr,graph_root}
     * If result wasn't BTF_FIELD_IGNORE, a btf_field_info would have to
       be added to the array. Since it is we can look at next field.
     * Before this patch the logic handling this edge case was hard to
       follow and used a tmp btf_struct_info. This patch moves the
       add-if-not-ignore logic down into btf_find_{struct, kptr,
       graph_root}, removing the need to opportunistically grab a
       btf_field_info to populate before knowing if it's actually
       necessary. Now a new one is grabbed only if the field shouldn't
       be ignored.

This extra 'tmp' btf_field_info approach sounds okay to me
in the original code. The previous code has a static size
and there is no realloc. Now you introduced realloc, so
removing extra 'tmp' seems do make sense.



   * Within btf_find_{datasec_var, struct_field}, each member is
     currently examined in two phases: first btf_get_field_type checks
     the member type name, then btf_find_{struct,graph_root,kptr} do
     additional sanity checking and populate struct btf_field_info. Kptr
     fields don't have a specific type name, though, so
     btf_get_field_type assumes that - if we're looking for kptrs - any
     member that fails type name check could be a kptr field.
     * As a result btf_find_kptr effectively does all the pointer
       hopping, sanity checking, and info population. Instead of trying
       to fit kptr handling into this two-phase model, where it's
       unwieldy, handle it in a separate codepath when name matching
       fails.

Signed-off-by: Dave Marchevsky <davemarchevsky@xxxxxx>
---
  include/linux/bpf.h |   4 +-
  kernel/bpf/btf.c    | 331 +++++++++++++++++++++++++++++---------------
  2 files changed, 219 insertions(+), 116 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index b4825d3cdb29..e07cac5cc3cf 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -171,8 +171,8 @@ struct bpf_map_ops {
  };
    enum {
-    /* Support at most 10 fields in a BTF type */
-    BTF_FIELDS_MAX       = 10,
+    /* Support at most 256 fields in a BTF type */
+    BTF_FIELDS_MAX       = 256,
  };
    enum btf_field_type {
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 975ef8e73393..e999ba85c363 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -3257,25 +3257,94 @@ struct btf_field_info {
      };
  };
  +struct btf_field_info_search {
+    /* growable array. allocated in __next_field_infos
+     * free'd in btf_parse_fields
+     */
+    struct btf_field_info *infos;
+    /* size of infos */
+    int info_cnt;
+    /* index of next btf_field_info to populate */
+    int idx;
+
+    /* btf_field_types to search for */
+    u32 field_mask;
+    /* btf_field_types found earlier */
+    u32 seen_mask;
+};
+
+/* Reserve next_cnt contiguous btf_field_info's for caller to populate
+ * Returns ptr to first reserved btf_field_info
+ */
+static struct btf_field_info *__next_field_infos(struct btf_field_info_search *srch,
+                         u32 next_cnt)
+{
+    struct btf_field_info *new_infos, *ret;
+
+    if (!next_cnt)
+        return ERR_PTR(-EINVAL);

Looks next_cnt is never 0.

+
+    if (srch->idx + next_cnt < srch->info_cnt)
+        goto nogrow_out;
+
+    /* Need to grow */
+    if (srch->idx + next_cnt > BTF_FIELDS_MAX)
+        return ERR_PTR(-E2BIG);
+
+    while (srch->idx + next_cnt >= srch->info_cnt)
+        srch->info_cnt = srch->infos ? srch->info_cnt * 2 : 16;
+
+    new_infos = krealloc(srch->infos,
+                 srch->info_cnt * sizeof(struct btf_field_info),
+                 GFP_KERNEL | __GFP_NOWARN);
+    if (!new_infos)
+        return ERR_PTR(-ENOMEM);
+    srch->infos = new_infos;
+
+nogrow_out:
+    ret = &srch->infos[srch->idx];
+    srch->idx += next_cnt;
+    return ret;
+}
+
+/* Request srch's next free btf_field_info to populate, possibly growing
+ * srch->infos
+ */
+static struct btf_field_info *__next_field_info(struct btf_field_info_search *srch)
+{
+    return __next_field_infos(srch, 1);
+}
+
  static int btf_find_struct(const struct btf *btf, const struct btf_type *t,
                 u32 off, int sz, enum btf_field_type field_type,
-               struct btf_field_info *info)
+               struct btf_field_info_search *srch)
  {
+    struct btf_field_info *info;
+
      if (!__btf_type_is_struct(t))
          return BTF_FIELD_IGNORE;
      if (t->size != sz)
          return BTF_FIELD_IGNORE;
+
+    info = __next_field_info(srch);
+    if (IS_ERR_OR_NULL(info))
+        return PTR_ERR(info);

info cannot be NULL.

+
      info->type = field_type;
      info->off = off;
      return BTF_FIELD_FOUND;
  }
  -static int btf_find_kptr(const struct btf *btf, const struct btf_type *t,
-             u32 off, int sz, struct btf_field_info *info)
+static int btf_maybe_find_kptr(const struct btf *btf, const struct btf_type *t,
+                   u32 off, struct btf_field_info_search *srch)
  {
+    struct btf_field_info *info;
      enum btf_field_type type;
      u32 res_id;
  +    if (!(srch->field_mask & BPF_KPTR))
+        return BTF_FIELD_IGNORE;
+
      /* Permit modifiers on the pointer itself */
      if (btf_type_is_volatile(t))
          t = btf_type_by_id(btf, t->type);
@@ -3304,6 +3373,10 @@ static int btf_find_kptr(const struct btf *btf, const struct btf_type *t,
      if (!__btf_type_is_struct(t))
          return -EINVAL;
  +    info = __next_field_info(srch);
+    if (IS_ERR_OR_NULL(info))
+        return PTR_ERR(info);

info cannot be NULL.

+
      info->type = type;
      info->off = off;
      info->kptr.type_id = res_id;
@@ -3340,9 +3413,10 @@ const char *btf_find_decl_tag_value(const struct btf *btf, const struct btf_type
  static int
  btf_find_graph_root(const struct btf *btf, const struct btf_type *pt,
              const struct btf_type *t, int comp_idx, u32 off,
-            int sz, struct btf_field_info *info,
+            int sz, struct btf_field_info_search *srch,
              enum btf_field_type head_type)
  {
+    struct btf_field_info *info;
      const char *node_field_name;
      const char *value_type;
      s32 id;
@@ -3367,6 +3441,11 @@ btf_find_graph_root(const struct btf *btf, const struct btf_type *pt,
      node_field_name++;
      if (str_is_empty(node_field_name))
          return -EINVAL;
+
+    info = __next_field_info(srch);
+    if (IS_ERR_OR_NULL(info))
+        return PTR_ERR(info);
+

ditto.

      info->type = head_type;
      info->off = off;
      info->graph_root.value_btf_id = id;
@@ -3374,25 +3453,24 @@ btf_find_graph_root(const struct btf *btf, const struct btf_type *pt,
      return BTF_FIELD_FOUND;
  }
  -#define field_mask_test_name(field_type, field_type_str)        \
-    if (field_mask & field_type && !strcmp(name, field_type_str)) {    \
-        type = field_type;                    \
-        goto end;                        \
+#define field_mask_test_name(field_type, field_type_str)            \
+    if (srch->field_mask & field_type && !strcmp(name, field_type_str)) {    \
+        return field_type;                        \
      }
  -#define field_mask_test_name_check_seen(field_type, field_type_str)    \ -    if (field_mask & field_type && !strcmp(name, field_type_str)) {    \
-        if (*seen_mask & field_type)                \
-            return -E2BIG;                    \
-        *seen_mask |= field_type;                \
-        type = field_type;                    \
-        goto end;                        \
+#define field_mask_test_name_check_seen(field_type, field_type_str)        \ +    if (srch->field_mask & field_type && !strcmp(name, field_type_str)) {    \
+        if (srch->seen_mask & field_type) \
+            return -E2BIG;                        \
+        srch->seen_mask |= field_type;                    \
+        return field_type;                        \
      }
  -static int btf_get_field_type(const char *name, u32 field_mask, u32 *seen_mask,
-                  int *align, int *sz)

[...]






[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