Re: [PATCH] btf: support ints larger than 128 bits

[Sean Young]

Hi Yonghong,

Thank you for the detailed review.

On Thu, Dec 17, 2020 at 06:12:11PM -0800, Yonghong Song wrote:
> On 12/17/20 7:01 AM, Sean Young wrote:
> > clang supports arbitrary length ints using the _ExtInt extension. This
> > can be useful to hold very large values, e.g. 256 bit or 512 bit types.
> > 
> > Larger types (e.g. 1024 bits) are possible but I am unaware of a use
> > case for these.
> > 
> > This requires the _ExtInt extension to enabled for BPF in clang, which
> > is under review.
> > 
> > Link: https://clang.llvm.org/docs/LanguageExtensions.html#extended-integer-types
> > Link: https://reviews.llvm.org/D93103
> > 
> > Signed-off-by: Sean Young <sean@xxxxxxxx>
> > ---
> >   Documentation/bpf/btf.rst      |  4 ++--
> >   include/uapi/linux/btf.h       |  2 +-
> >   tools/bpf/bpftool/btf_dumper.c | 39 ++++++++++++++++++++++++++++++++++
> >   tools/include/uapi/linux/btf.h |  2 +-
> >   4 files changed, 43 insertions(+), 4 deletions(-)
> 
> Thanks for the patch. But the change is not enough and no tests in the patch
> set.
> 
> For example, in kernel/bpf/btf.c, we BITS_PER_U128 to guard in various
> places where the number of integer bits must be <= 128 bits which is
> what we supported now. In function btf_type_int_is_regular(), # of int
> bits larger than 128 considered false. The extint like 256/512bits should be
> also regular int.

Right, thanks for spotting that. I'll give the next version some better
testing.

> extint permits non-power-of-2 bits (e.g., 192bits), to support them
> may not be necessary and this is not your use case. what do you think?

My feeling is that non-power-of-2 types are useful for llvm targets
which such registers. I'm not sure they have much use for our use case
or bpf in general. If anyone thinks otherwise I'm easily convinced.

> lib/bpf/btf.c btf__and_int() function also has the following check,
> 
>         /* byte_sz must be power of 2 */
>         if (!byte_sz || (byte_sz & (byte_sz - 1)) || byte_sz > 16)
>                 return -EINVAL;
> 
> So Extint 256 bits will fail here.

Indeed it will.

> Please do add some selftests tools/testing/selftests/bpf
> directories:
>    - to ensure btf with newly supported int types loaded successfully
>      in kernel
>    - to ensure bpftool map [pretty] print working fine with new types
>    - to ensure kernel map pretty print works fine
>      (tests at tools/testing/selftests/bpf/prog_tests/btf.c)
>    - to ensure btf manipulation APIs works with new types.

Absolutely. I'll send out a v2 when ready.

Thanks again for the great review.

Sean

> 
> > 
> > diff --git a/Documentation/bpf/btf.rst b/Documentation/bpf/btf.rst
> > index 44dc789de2b4..784f1743dbc7 100644
> > --- a/Documentation/bpf/btf.rst
> > +++ b/Documentation/bpf/btf.rst
> > @@ -132,7 +132,7 @@ The following sections detail encoding of each kind.
> >     #define BTF_INT_ENCODING(VAL)   (((VAL) & 0x0f000000) >> 24)
> >     #define BTF_INT_OFFSET(VAL)     (((VAL) & 0x00ff0000) >> 16)
> > -  #define BTF_INT_BITS(VAL)       ((VAL)  & 0x000000ff)
> > +  #define BTF_INT_BITS(VAL)       ((VAL)  & 0x000003ff)
> >   The ``BTF_INT_ENCODING`` has the following attributes::
> > @@ -147,7 +147,7 @@ pretty print. At most one encoding can be specified for the int type.
> >   The ``BTF_INT_BITS()`` specifies the number of actual bits held by this int
> >   type. For example, a 4-bit bitfield encodes ``BTF_INT_BITS()`` equals to 4.
> >   The ``btf_type.size * 8`` must be equal to or greater than ``BTF_INT_BITS()``
> > -for the type. The maximum value of ``BTF_INT_BITS()`` is 128.
> > +for the type. The maximum value of ``BTF_INT_BITS()`` is 512.
> >   The ``BTF_INT_OFFSET()`` specifies the starting bit offset to calculate values
> >   for this int. For example, a bitfield struct member has:
> > diff --git a/include/uapi/linux/btf.h b/include/uapi/linux/btf.h
> > index 5a667107ad2c..1696fd02b302 100644
> > --- a/include/uapi/linux/btf.h
> > +++ b/include/uapi/linux/btf.h
> > @@ -84,7 +84,7 @@ struct btf_type {
> >    */
> >   #define BTF_INT_ENCODING(VAL)	(((VAL) & 0x0f000000) >> 24)
> >   #define BTF_INT_OFFSET(VAL)	(((VAL) & 0x00ff0000) >> 16)
> > -#define BTF_INT_BITS(VAL)	((VAL)  & 0x000000ff)
> > +#define BTF_INT_BITS(VAL)	((VAL)  & 0x000003ff)
> >   /* Attributes stored in the BTF_INT_ENCODING */
> >   #define BTF_INT_SIGNED	(1 << 0)
> > diff --git a/tools/bpf/bpftool/btf_dumper.c b/tools/bpf/bpftool/btf_dumper.c
> > index 0e9310727281..45ed45ea9962 100644
> > --- a/tools/bpf/bpftool/btf_dumper.c
> > +++ b/tools/bpf/bpftool/btf_dumper.c
> > @@ -271,6 +271,40 @@ static void btf_int128_print(json_writer_t *jw, const void *data,
> >   	}
> >   }
> > +static void btf_bigint_print(json_writer_t *jw, const void *data, int nr_bits,
> > +			     bool is_plain_text)
> > +{
> > +	char buf[nr_bits / 4 + 1];
> > +	bool first = true;
> > +	int i;
> > +
> > +#ifdef __BIG_ENDIAN_BITFIELD
> > +	for (i = 0; i < nr_bits / 64; i++) {
> > +#else
> > +	for (i = nr_bits / 64 - 1; i >= 0; i++) {
> > +#endif
> > +		__u64 v = ((__u64 *)data)[i];
> > +
> > +		if (first) {
> > +			if (!v)
> > +				continue;
> > +
> > +			snprintf(buf, sizeof(buf), "%llx", v);
> > +
> > +			first = false;
> > +		} else {
> > +			size_t off = strlen(buf);
> > +
> > +			snprintf(buf + off, sizeof(buf) - off, "%016llx", v);
> > +		}
> > +	}
> > +
> > +	if (is_plain_text)
> > +		jsonw_printf(jw, "0x%s", buf);
> > +	else
> > +		jsonw_printf(jw, "\"0x%s\"", buf);
> > +}
> > +
> >   static void btf_int128_shift(__u64 *print_num, __u16 left_shift_bits,
> >   			     __u16 right_shift_bits)
> >   {
> > @@ -373,6 +407,11 @@ static int btf_dumper_int(const struct btf_type *t, __u8 bit_offset,
> >   		return 0;
> >   	}
> > +	if (nr_bits > 128) {
> > +		btf_bigint_print(jw, data, nr_bits, is_plain_text);
> > +		return 0;
> > +	}
> > +
> >   	if (nr_bits == 128) {
> >   		btf_int128_print(jw, data, is_plain_text);
> >   		return 0;
> [...]