Message-ID: <20181215174442.yjd5seywjkoep66f@ast-mbp.dhcp.thefacebook.com>
Date:   Sat, 15 Dec 2018 09:44:44 -0800
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Martin Lau <kafai@...com>
Cc:     Yonghong Song <yhs@...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 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.
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.
In that sense BTF_MEMBER_BITFIELD_SIZE/BTF_MEMBER_BIT_OFFSET
serve as a documentation and fit the style of btf.h header.
imo having these two macros in uapi/btf.h is better than
replacing them with comment only.

After this set the whole BTF really needs to be documented.

Powered by blists - more mailing lists