Message-ID: <20181215230358.dkbj756a4tiwipzy@kafai-mbp.dhcp.thefacebook.com>
Date:   Sat, 15 Dec 2018 23:04:07 +0000
From:   Martin Lau <kafai@...com>
To:     Yonghong Song <yhs@...com>
CC:     Alexei Starovoitov <alexei.starovoitov@...il.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Alexei Starovoitov <ast@...com>,
        Daniel Borkmann <daniel@...earbox.net>,
        Kernel Team <Kernel-team@...com>
Subject: Re: [PATCH bpf-next v2 2/8] bpf: btf: fix struct/union/fwd types with
 kind_flag

On Sat, Dec 15, 2018 at 02:26:44PM -0800, Yonghong Song wrote:
> 
> 
> On 12/15/18 2:10 PM, Martin Lau wrote:
> > On Sat, Dec 15, 2018 at 09:44:44AM -0800, Alexei Starovoitov wrote:
> >> On Sat, Dec 15, 2018 at 04:37:06PM +0000, Martin Lau wrote:
> >>> On Fri, Dec 14, 2018 at 03:34:27PM -0800, Yonghong Song wrote:
> >>>> This patch fixed two issues with BTF. One is related to
> >>>> struct/union bitfield encoding and the other is related to
> >>>> forward type.
> >>>>
> >>>> Issue #1 and solution:
> >>>> ======================
> >>>>
> >>>> Current btf encoding of bitfield follows what pahole generates.
> >>>> For each bitfield, pahole will duplicate the type chain and
> >>>> put the bitfield size at the final int or enum type.
> >>>> Since the BTF enum type cannot encode bit size,
> >>>> pahole workarounds the issue by generating
> >>>> an int type whenever the enum bit size is not 32.
> >>>>
> >>>> For example,
> >>>>    -bash-4.4$ cat t.c
> >>>>    typedef int ___int;
> >>>>    enum A { A1, A2, A3 };
> >>>>    struct t {
> >>>>      int a[5];
> >>>>      ___int b:4;
> >>>>      volatile enum A c:4;
> >>>>    } g;
> >>>>    -bash-4.4$ gcc -c -O2 -g t.c
> >>>> The current kernel supports the following BTF encoding:
> >>>>    $ pahole -JV t.o
> >>>>    [1] TYPEDEF ___int type_id=2
> >>>>    [2] INT int size=4 bit_offset=0 nr_bits=32 encoding=SIGNED
> >>>>    [3] ENUM A size=4 vlen=3
> >>>>          A1 val=0
> >>>>          A2 val=1
> >>>>          A3 val=2
> >>>>    [4] STRUCT t size=24 vlen=3
> >>>>          a type_id=5 bits_offset=0
> >>>>          b type_id=9 bits_offset=160
> >>>>          c type_id=11 bits_offset=164
> >>>>    [5] ARRAY (anon) type_id=2 index_type_id=2 nr_elems=5
> >>>>    [6] INT sizetype size=8 bit_offset=0 nr_bits=64 encoding=(none)
> >>>>    [7] VOLATILE (anon) type_id=3
> >>>>    [8] INT int size=1 bit_offset=0 nr_bits=4 encoding=(none)
> >>>>    [9] TYPEDEF ___int type_id=8
> >>>>    [10] INT (anon) size=1 bit_offset=0 nr_bits=4 encoding=SIGNED
> >>>>    [11] VOLATILE (anon) type_id=10
> >>>>
> >>>> Two issues are in the above:
> >>>>    . by changing enum type to int, we lost the original
> >>>>      type information and this will not be ideal later
> >>>>      when we try to convert BTF to a header file.
> >>>>    . the type duplication for bitfields will cause
> >>>>      BTF bloat. Duplicated types cannot be deduplicated
> >>>>      later if the bitfield size is different.
> >>>>
> >>>> To fix this issue, this patch implemented a compatible
> >>>> change for BTF struct type encoding:
> >>>>    . the bit 31 of struct_type->info, previously reserved,
> >>>>      now is used to indicate whether bitfield_size is
> >>>>      encoded in btf_member or not.
> >>>>    . if bit 31 of struct_type->info is set,
> >>>>      btf_member->offset will encode like:
> >>>>        bit 0 - 23: bit offset
> >>>>        bit 24 - 31: bitfield size
> >>>>      if bit 31 is not set, the old behavior is preserved:
> >>>>        bit 0 - 31: bit offset
> >>>>
> >>>> So if the struct contains a bit field, the maximum bit offset
> >>>> will be reduced to (2^24 - 1) instead of MAX_UINT. The maximum
> >>>> bitfield size will be 256 which is enough for today as maximum
> >>>> bitfield in compiler can be 128 where int128 type is supported.
> >>>>
> >>>> This kernel patch intends to support the new BTF encoding:
> >>>>    $ pahole -JV t.o
> >>>>    [1] TYPEDEF ___int type_id=2
> >>>>    [2] INT int size=4 bit_offset=0 nr_bits=32 encoding=SIGNED
> >>>>    [3] ENUM A size=4 vlen=3
> >>>>          A1 val=0
> >>>>          A2 val=1
> >>>>          A3 val=2
> >>>>    [4] STRUCT t kind_flag=1 size=24 vlen=3
> >>>>          a type_id=5 bitfield_size=0 bits_offset=0
> >>>>          b type_id=1 bitfield_size=4 bits_offset=160
> >>>>          c type_id=7 bitfield_size=4 bits_offset=164
> >>>>    [5] ARRAY (anon) type_id=2 index_type_id=2 nr_elems=5
> >>>>    [6] INT sizetype size=8 bit_offset=0 nr_bits=64 encoding=(none)
> >>>>    [7] VOLATILE (anon) type_id=3
> >>>>
> >>>> Issue #2 and solution:
> >>>> ======================
> >>>>
> >>>> Current forward type in BTF does not specify whether the original
> >>>> type is struct or union. This will not work for type pretty print
> >>>> and BTF-to-header-file conversion as struct/union must be specified.
> >>>>    $ cat tt.c
> >>>>    struct t;
> >>>>    union u;
> >>>>    int foo(struct t *t, union u *u) { return 0; }
> >>>>    $ gcc -c -g -O2 tt.c
> >>>>    $ pahole -JV tt.o
> >>>>    [1] INT int size=4 bit_offset=0 nr_bits=32 encoding=SIGNED
> >>>>    [2] FWD t type_id=0
> >>>>    [3] PTR (anon) type_id=2
> >>>>    [4] FWD u type_id=0
> >>>>    [5] PTR (anon) type_id=4
> >>>>
> >>>> To fix this issue, similar to issue #1, type->info bit 31
> >>>> is used. If the bit is set, it is union type. Otherwise, it is
> >>>> a struct type.
> >>>>
> >>>>    $ pahole -JV tt.o
> >>>>    [1] INT int size=4 bit_offset=0 nr_bits=32 encoding=SIGNED
> >>>>    [2] FWD t kind_flag=0 type_id=0
> >>>>    [3] PTR (anon) kind_flag=0 type_id=2
> >>>>    [4] FWD u kind_flag=1 type_id=0
> >>>>    [5] PTR (anon) kind_flag=0 type_id=4
> >>>>
> >>>> Pahole/LLVM change:
> >>>> ===================
> >>>>
> >>>> The new kind_flag functionality has been implemented in pahole
> >>>> and llvm:
> >>>>    https://github.com/yonghong-song/pahole/tree/bitfield
> >>>>    https://github.com/yonghong-song/llvm/tree/bitfield
> >>>>
> >>>> Note that pahole hasn't implemented func/func_proto kind
> >>>> and .BTF.ext. So to print function signature with bpftool,
> >>>> the llvm compiler should be used.
> >>>>
> >>>> Fixes: 69b693f0aefa ("bpf: btf: Introduce BPF Type Format (BTF)")
> >>>> Signed-off-by: Martin KaFai Lau <kafai@...com>
> >>>> Signed-off-by: Yonghong Song <yhs@...com>
> >>>> ---
> >>>>   include/uapi/linux/btf.h |  15 ++-
> >>>>   kernel/bpf/btf.c         | 274 ++++++++++++++++++++++++++++++++++++---
> >>>>   2 files changed, 267 insertions(+), 22 deletions(-)
> >>>>
> >>>> diff --git a/include/uapi/linux/btf.h b/include/uapi/linux/btf.h
> >>>> index 14f66948fc95..34aba40ed926 100644
> >>>> --- a/include/uapi/linux/btf.h
> >>>> +++ b/include/uapi/linux/btf.h
> >>>> @@ -34,7 +34,9 @@ struct btf_type {
> >>>>   	 * bits  0-15: vlen (e.g. # of struct's members)
> >>>>   	 * bits 16-23: unused
> >>>>   	 * bits 24-27: kind (e.g. int, ptr, array...etc)
> >>>> -	 * bits 28-31: unused
> >>>> +	 * bits 28-30: unused
> >>>> +	 * bit     31: kind_flag, currently used by
> >>>> +	 *             struct, union and fwd
> >>>>   	 */
> >>>>   	__u32 info;
> >>>>   	/* "size" is used by INT, ENUM, STRUCT and UNION.
> >>>> @@ -52,6 +54,7 @@ struct btf_type {
> >>>>   
> >>>>   #define BTF_INFO_KIND(info)	(((info) >> 24) & 0x0f)
> >>>>   #define BTF_INFO_VLEN(info)	((info) & 0xffff)
> >>>> +#define BTF_INFO_KFLAG(info)	((info) >> 31)
> >>>>   
> >>>>   #define BTF_KIND_UNKN		0	/* Unknown	*/
> >>>>   #define BTF_KIND_INT		1	/* Integer	*/
> >>>> @@ -110,9 +113,17 @@ struct btf_array {
> >>>>   struct btf_member {
> >>>>   	__u32	name_off;
> >>>>   	__u32	type;
> >>>> -	__u32	offset;	/* offset in bits */
> >>>> +	__u32	offset;	/* [bitfield_size and] offset in bits */
> >>>>   };
> >>>>   
> >>>> +/* If the type info kind_flag set, the btf_member.offset
> >>>> + * contains both member bit offset and bitfield size, and
> >>>> + * bitfield size will set for struct/union bitfield members.
> >>>> + * Otherwise, it contains only bit offset.
> >>>> + */
> >>> nit. It may be better to move this comment to the btf_member.offset
> >>> above.
> >>>
> >>>> +#define BTF_MEMBER_BITFIELD_SIZE(val)	((val) >> 24)
> >>>> +#define BTF_MEMBER_BIT_OFFSET(val)	((val) & 0xffffff)
> >>> After re-thinking this setup again, I still think
> >>> having these macros in btf.h to also do the kflag checking
> >>> would be nice.
> >>>
> >>> Unlike BTF_INFO_KIND() and BTF_INT_ENCODING() which don't
> >>> depend on other facts,  the btf.h raw user must check kflag
> >>> anyway before calling BTF_MEMBER_BIT*().
> >>> Forcing a kflag check before the user can access these convenient
> >>> 0xfffff and >>24 conversions may enforce this kflag check to
> >>> some extend.
> >>>
> >>> Since it is in uapi, it will not be easy to change later.
> >>> The above concern could be overkill ;), just want to ensure
> >>> it has been thought through a bit more here.
> >>>
> >>> It could be as easy as moving the new btf_member_bit*() from
> >>> btf.c to here and remove these two macros (or move them back to btf.c).
> >>
> >> I think moving:
> >> +static u32 btf_member_bitfield_size(const struct btf_type *struct_type,
> >> +                                   const struct btf_member *member)
> >> +{
> >> +       return btf_type_kflag(struct_type) ? BTF_MEMBER_BITFIELD_SIZE(member->offset)
> >> +                                          : 0;
> >> +}
> >>
> >> into uapi/btf.h may or may not be useful for btf uapi users.
> >> What are the chances that these static inline helpers will be
> >> reused by BTF logic in libbpf or other libs?
> >> At this point we don't know.
> > 
> >> So I would keep btf.h minimal.
> > ok. Make sense
> > 
> >> I agree that BTF_MEMBER_BIT_OFFSET() shouldn't be reused blindly.
> >> The users have to do BTF_INFO_KFLAG() check first.
> >> But this is the case for pretty much all of BTF data structures.
> > Other similar situation in btf.h (i.e. a single u32 field can be
> > interpreted differently) has at least an union as an indication
> > (e.g. size and type in btf_type)
> > 
> > Here we cannot add the union (bitfield_offset:24 and bitfield_size:8)
> > and we cannot change the name "offset" also.  I am worry about
> > m->offset will directly be used without checking the BTF_INFO_KFLAG().
> > 
> > may be a "union { __u32 offset; __u32 bitsize_offset; };"......
> 
> The union with two __u32 is great idea. Maybe the
> bitsize_offset becomes "bitfield_size_offset" to reflect
> its real intention?
SGTM.  Probably also spell out when to use "offset"
and when to use "bitfield_size_offset" like the
union in "struct btf_type".  The BTF_MEMBER_BIT*() macro
may also need to adjust to access the bitfield_size_offset
instead to make the case clearer.

Powered by blists - more mailing lists