On Fri, Jan 17, 2025 at 10:36:50AM -0800, Andrii Nakryiko wrote:
> On Wed, Jan 15, 2025 at 11:45 PM D. Wythe <alibuda@xxxxxxxxxxxxxxxxx> wrote:
> >
> > When a struct_ops named xxx_ops was registered by a module, and
> > it will be used in both built-in modules and the module itself,
> > so that the btf_type of xxx_ops will be present in btf_vmlinux
> > instead of in btf_mod, which means that the btf_type of
> > bpf_struct_ops_xxx_ops and xxx_ops will not be in the same btf.
> >
> > Here are four possible case:
> >
> > +--------+---------------+-------------+---------------------------------+
> > | | st_ops_xxx_ops| xxx_ops | |
> > +--------+---------------+-------------+---------------------------------+
> > | case 0 | btf_vmlinux | bft_vmlinux | be used and reg only in vmlinux |
> > +--------+---------------+-------------+---------------------------------+
> > | case 1 | btf_vmlinux | bpf_mod | INVALID |
> > +--------+---------------+-------------+---------------------------------+
> > | case 2 | btf_mod | btf_vmlinux | reg in mod but be used both in |
> > | | | | vmlinux and mod. |
> > +--------+---------------+-------------+---------------------------------+
> > | case 3 | btf_mod | btf_mod | be used and reg only in mod |
> > +--------+---------------+-------------+---------------------------------+
> >
> > At present, cases 0, 1, and 3 can be correctly identified, because
> > + if (ret < 0 || ret >= sizeof(stname))
> > + return -ENAMETOOLONG;
>
> see preexisting snprintf() above, we don't really handle truncation
> errors explicitly, they are extremely unlikely and not expected at
> all, and worst case nothing will be found and user will get some
> -ENOENT or something like that eventually. I'd drop this extra error
> checking and keep it streamlines, similar to tname
>
Sounds reasonable to me. I will remove the explicit error checks in the
next version.
> > +
> > + /* Look for the corresponding "map_value" type that will be used
> > + * in map_update(BPF_MAP_TYPE_STRUCT_OPS) first, figure out the btf
> > + * and the mod_btf.
> > + * For example, find "struct bpf_struct_ops_tcp_congestion_ops".
> > + */
> > + kern_vtype_id = find_ksym_btf_id(obj, stname, BTF_KIND_STRUCT,
> > &btf, mod_btf);
>
> nit: if this fits under 100 characters, keep it single line
>
> > + if (kern_vtype_id < 0) {
> > + pr_warn("struct_ops init_kern: struct %s is not found in kernel BTF\n",
> > + stname);
>
> same nit about preserving single-line statements as much as possible,
> they are much easier to read
None of them exceed 100 lines. Usually, I would check patches with 85 lines limitations,
but since 100 lines is acceptable, we can modify it to a single line here for
better readability.
And thanks very much for your suggestion, I plan to fix these style
issues in next versions with you ack, is this okay for you?
Best wishes,
D. Wythe
>
> > + return kern_vtype_id;
> > + }
> > + kern_vtype = btf__type_by_id(btf, kern_vtype_id);
> > +
> > + kern_type_id = btf__find_by_name_kind(btf, tname, BTF_KIND_STRUCT);
> > if (kern_type_id < 0) {
> > pr_warn("struct_ops init_kern: struct %s is not found in kernel BTF\n",
> > tname);
> > @@ -1020,20 +1039,6 @@ find_struct_ops_kern_types(struct bpf_object *obj, const char *tname_raw,
> > }
> > kern_type = btf__type_by_id(btf, kern_type_id);
> >
> > - /* Find the corresponding "map_value" type that will be used
> > - * in map_update(BPF_MAP_TYPE_STRUCT_OPS). For example,
> > - * find "struct bpf_struct_ops_tcp_congestion_ops" from the
> > - * btf_vmlinux.
> > - */
> > - kern_vtype_id = find_btf_by_prefix_kind(btf, STRUCT_OPS_VALUE_PREFIX,
> > - tname, BTF_KIND_STRUCT);
> > - if (kern_vtype_id < 0) {
> > - pr_warn("struct_ops init_kern: struct %s%s is not found in kernel BTF\n",
> > - STRUCT_OPS_VALUE_PREFIX, tname);
> > - return kern_vtype_id;
> > - }
> > - kern_vtype = btf__type_by_id(btf, kern_vtype_id);
> > -
> > /* Find "struct tcp_congestion_ops" from
> > * struct bpf_struct_ops_tcp_congestion_ops {
> > * [ ... ]
> > @@ -1046,8 +1051,8 @@ find_struct_ops_kern_types(struct bpf_object *obj, const char *tname_raw,
> > break;
> > }
> > if (i == btf_vlen(kern_vtype)) {
> > - pr_warn("struct_ops init_kern: struct %s data is not found in struct %s%s\n",
> > - tname, STRUCT_OPS_VALUE_PREFIX, tname);
> > + pr_warn("struct_ops init_kern: struct %s data is not found in struct %s\n",
> > + tname, stname);
> > return -EINVAL;
> > }
> >
> > --
> > 2.45.0
> >
