On 12/15/18 3:04 PM, Martin Lau wrote:
> 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 <ka...@fb.com>
>>>>>> Signed-off-by: Yonghong Song <y...@fb.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.
Sounds good. This is my plan to do as well.
