Date:   Thu, 17 Dec 2020 18:12:11 -0800
From:   Yonghong Song <yhs@...com>
To:     Sean Young <sean@...s.org>, Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>,
        Martin KaFai Lau <kafai@...com>,
        Song Liu <songliubraving@...com>,
        John Fastabend <john.fastabend@...il.com>,
        KP Singh <kpsingh@...nel.org>,
        Nathan Chancellor <natechancellor@...il.com>,
        Nick Desaulniers <ndesaulniers@...gle.com>,
        Quentin Monnet <quentin@...valent.com>,
        Toke Høiland-Jørgensen <toke@...hat.com>,
        <linux-doc@...r.kernel.org>, <netdev@...r.kernel.org>,
        <bpf@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <clang-built-linux@...glegroups.com>
Subject: Re: [PATCH] btf: support ints larger than 128 bits



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@...s.org>
> ---
>   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.

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?

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.

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.

> 
> 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;
[...]

Powered by blists - more mailing lists