On Thu, Nov 18, 2021 at 7:17 PM Alexei Starovoitov
<alexei.starovoitov@xxxxxxxxx> wrote:
>
> On Tue, Nov 16, 2021 at 04:38:18PM -0800, Andrii Nakryiko wrote:
> > >
> > > -static inline u32 btf_member_bit_offset(const struct btf_type *struct_type,
> > > - const struct btf_member *member)
> > > +static inline u32 __btf_member_bit_offset(const struct btf_type *struct_type,
> > > + const struct btf_member *member)
> >
> > a bit surprised you didn't choose to just remove them, given you had
> > to touch all 24 places in the kernel that call this helper
> > > - if (btf_member_bitfield_size(t, member)) {
> > > + if (__btf_member_bitfield_size(t, member)) {
> >
> > like in this case it would be btf_member_bitfield_size(t, j)
>
> In this and few other cases it's indeed possible, but not in net/ipv4/bpf_tcp_ca.c
> It has two callbacks:
> struct bpf_struct_ops {
> const struct bpf_verifier_ops *verifier_ops;
> int (*init)(struct btf *btf);
> int (*check_member)(const struct btf_type *t,
> const struct btf_member *member);
> int (*init_member)(const struct btf_type *t,
> const struct btf_member *member,
> so they cannot be changed without massive refactoring.
member_idx = member - btf_type_member(t);
But as I said, not a big deal.
> Also member pointer vs index is arguably a better api.
Not so sure about that, as it allows accidentally passing a completely
irrelevant member pointer from a different type. With member_idx you
can do some sanity checking and make sure you are not accessing
members out of bounds. But I don't really care, just wanted to point
out the possibility of eliminating a somewhat redundant helper.
> I'm not sure compiler can optimize index into pointer in case like below
> and won't introduce redundant loads.
>
> > > for_each_member(i, t, member) {
> > > - moff = btf_member_bit_offset(t, member) / 8;
> > > + moff = __btf_member_bit_offset(t, member) / 8;
> >
> > same here, seema like in all the cases we already have member_idx (i
> > in this case)
