On Tue, Jan 21, 2025 at 6:43 PM D. Wythe <alibuda@xxxxxxxxxxxxxxxxx> wrote:
>
> 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?
yep, sgtm
>
> 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
> > >
