Message-ID: <bf26fcc9-a2b5-9d6f-a2ac-e39a0b14d838@fb.com>
Date:   Wed, 30 Dec 2020 10:21:09 -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 v2] btf: support ints larger than 128 bits



On 12/19/20 8:36 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 enabled 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>
> ---
> changes since v2:
>   - added tests as suggested by Yonghong Song
>   - added kernel pretty-printer
> 
>   Documentation/bpf/btf.rst                     |   4 +-
>   include/uapi/linux/btf.h                      |   2 +-
>   kernel/bpf/btf.c                              |  54 +-
>   tools/bpf/bpftool/btf_dumper.c                |  40 ++
>   tools/include/uapi/linux/btf.h                |   2 +-
>   tools/lib/bpf/btf.c                           |   2 +-
>   tools/testing/selftests/bpf/Makefile          |   3 +-
>   tools/testing/selftests/bpf/prog_tests/btf.c  |   3 +-
>   .../selftests/bpf/progs/test_btf_extint.c     |  50 ++
>   tools/testing/selftests/bpf/test_extint.py    | 535 ++++++++++++++++++

For easier review, maybe you can break this patch into a patch series 
like below?
   patch 1 (kernel related changes and doc)
       kernel/bpf/btf.c, include/uapi/linux/btf.h,
       tools/include/uapi/linux/btf.h
       Documentation/bpf/btf.rst
   patch 2 (libbpf support)
       tools/lib/bpf/btf.c
   patch 3 (bpftool support)
       tools/bpf/bpftool/btf_dumper.c
   patch 4 (testing)
       rest files

>   10 files changed, 679 insertions(+), 16 deletions(-)
>   create mode 100644 tools/testing/selftests/bpf/progs/test_btf_extint.c
>   create mode 100755 tools/testing/selftests/bpf/test_extint.py
> 
> 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/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index 8d6bdb4f4d61..44bc17207e9b 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -166,7 +166,8 @@
>    *
>    */
>   
> -#define BITS_PER_U128 (sizeof(u64) * BITS_PER_BYTE * 2)
> +#define BITS_PER_U128 128
> +#define BITS_PER_U512 512
>   #define BITS_PER_BYTE_MASK (BITS_PER_BYTE - 1)
>   #define BITS_PER_BYTE_MASKED(bits) ((bits) & BITS_PER_BYTE_MASK)
>   #define BITS_ROUNDDOWN_BYTES(bits) ((bits) >> 3)
> @@ -1907,9 +1908,9 @@ static int btf_int_check_member(struct btf_verifier_env *env,
>   	nr_copy_bits = BTF_INT_BITS(int_data) +
>   		BITS_PER_BYTE_MASKED(struct_bits_off);
>   
> -	if (nr_copy_bits > BITS_PER_U128) {
> +	if (nr_copy_bits > BITS_PER_U512) {
>   		btf_verifier_log_member(env, struct_type, member,
> -					"nr_copy_bits exceeds 128");
> +					"nr_copy_bits exceeds 512");
>   		return -EINVAL;
>   	}
>   
> @@ -1963,9 +1964,9 @@ static int btf_int_check_kflag_member(struct btf_verifier_env *env,
>   
>   	bytes_offset = BITS_ROUNDDOWN_BYTES(struct_bits_off);
>   	nr_copy_bits = nr_bits + BITS_PER_BYTE_MASKED(struct_bits_off);
> -	if (nr_copy_bits > BITS_PER_U128) {
> +	if (nr_copy_bits > BITS_PER_U512) {
>   		btf_verifier_log_member(env, struct_type, member,
> -					"nr_copy_bits exceeds 128");
> +					"nr_copy_bits exceeds 512");
>   		return -EINVAL;
>   	}
>   
> @@ -2012,9 +2013,9 @@ static s32 btf_int_check_meta(struct btf_verifier_env *env,
>   
>   	nr_bits = BTF_INT_BITS(int_data) + BTF_INT_OFFSET(int_data);
>   
> -	if (nr_bits > BITS_PER_U128) {
> -		btf_verifier_log_type(env, t, "nr_bits exceeds %zu",
> -				      BITS_PER_U128);
> +	if (nr_bits > BITS_PER_U512) {
> +		btf_verifier_log_type(env, t, "nr_bits exceeds %u",
> +				      BITS_PER_U512);
>   		return -EINVAL;
>   	}
>   
> @@ -2080,6 +2081,37 @@ static void btf_int128_print(struct btf_show *show, void *data)
>   				     lower_num);
>   }
>   
> +static void btf_bigint_print(struct btf_show *show, void *data, u16 nr_bits)
> +{
> +	/* data points to 256 or 512 bit int type */
> +	char buf[129];
> +	int last_u64 = nr_bits / 64 - 1;
> +	bool seen_nonzero = false;
> +	int i;
> +
> +	for (i = 0; i <= last_u64; i++) {
> +#ifdef __BIG_ENDIAN_BITFIELD
> +		u64 v = ((u64 *)data)[i];
> +#else
> +		u64 v = ((u64 *)data)[last_u64 - i];
> +#endif
> +		if (!seen_nonzero) {
> +			if (!v && i != last_u64)
> +				continue;
> +
> +			snprintf(buf, sizeof(buf), "%llx", v);
> +
> +			seen_nonzero = true;
> +		} else {
> +			size_t off = strlen(buf);
> +
> +			snprintf(buf + off, sizeof(buf) - off, "%016llx", v);
> +		}
> +	}
> +
> +	btf_show_type_value(show, "0x%s", buf);
> +}
> +
>   static void btf_int128_shift(u64 *print_num, u16 left_shift_bits,
>   			     u16 right_shift_bits)
>   {
> @@ -2172,7 +2204,7 @@ static void btf_int_show(const struct btf *btf, const struct btf_type *t,
>   	u32 int_data = btf_type_int(t);
>   	u8 encoding = BTF_INT_ENCODING(int_data);
>   	bool sign = encoding & BTF_INT_SIGNED;
> -	u8 nr_bits = BTF_INT_BITS(int_data);
> +	u16 nr_bits = BTF_INT_BITS(int_data);
>   	void *safe_data;
>   
>   	safe_data = btf_show_start_type(show, t, type_id, data);
> @@ -2186,6 +2218,10 @@ static void btf_int_show(const struct btf *btf, const struct btf_type *t,
>   	}
>   
>   	switch (nr_bits) {
> +	case 512:
> +	case 256:
> +		btf_bigint_print(show, safe_data, nr_bits);
> +		break;
>   	case 128:
>   		btf_int128_print(show, safe_data);
>   		break;

You shuold adjust for the following function as well.

/*
  * Regular int is not a bit field and it must be either
  * u8/u16/u32/u64 or __int128.
  */
static bool btf_type_int_is_regular(const struct btf_type *t)
{
         u8 nr_bits, nr_bytes;
         u32 int_data;

         int_data = btf_type_int(t);
         nr_bits = BTF_INT_BITS(int_data);
         nr_bytes = BITS_ROUNDUP_BYTES(nr_bits);
         if (BITS_PER_BYTE_MASKED(nr_bits) ||
             BTF_INT_OFFSET(int_data) ||
             (nr_bytes != sizeof(u8) && nr_bytes != sizeof(u16) &&
              nr_bytes != sizeof(u32) && nr_bytes != sizeof(u64) &&
              nr_bytes != (2 * sizeof(u64)))) {
                 return false;
         }

         return true;
}

This function is used to test struct/union int member type, array 
index/element int type.

> diff --git a/tools/bpf/bpftool/btf_dumper.c b/tools/bpf/bpftool/btf_dumper.c
> index 0e9310727281..8b5318ec5c26 100644
> --- a/tools/bpf/bpftool/btf_dumper.c
> +++ b/tools/bpf/bpftool/btf_dumper.c
> @@ -271,6 +271,41 @@ 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];
> +	int last_u64 = nr_bits / 64 - 1;
> +	bool seen_nonzero = false;
> +	int i;
> +
> +	for (i = 0; i <= last_u64; i++) {
> +#ifdef __BIG_ENDIAN_BITFIELD
> +		__u64 v = ((__u64 *)data)[i];
> +#else
> +		__u64 v = ((__u64 *)data)[last_u64 - i];
> +#endif
> +
> +		if (!seen_nonzero) {
> +			if (!v && i != last_u64)
> +				continue;
> +
> +			snprintf(buf, sizeof(buf), "%llx", v);
> +
> +			seen_nonzero = true;
> +		} 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 +408,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;
> diff --git a/tools/include/uapi/linux/btf.h b/tools/include/uapi/linux/btf.h
> index 5a667107ad2c..1696fd02b302 100644
> --- a/tools/include/uapi/linux/btf.h
> +++ b/tools/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/lib/bpf/btf.c b/tools/lib/bpf/btf.c
> index 3c3f2bc6c652..a676373f052b 100644
> --- a/tools/lib/bpf/btf.c
> +++ b/tools/lib/bpf/btf.c
> @@ -1722,7 +1722,7 @@ int btf__add_int(struct btf *btf, const char *name, size_t byte_sz, int encoding
>   	if (!name || !name[0])
>   		return -EINVAL;
>   	/* byte_sz must be power of 2 */
> -	if (!byte_sz || (byte_sz & (byte_sz - 1)) || byte_sz > 16)
> +	if (!byte_sz || (byte_sz & (byte_sz - 1)) || byte_sz > 64)
>   		return -EINVAL;
>   	if (encoding & ~(BTF_INT_SIGNED | BTF_INT_CHAR | BTF_INT_BOOL))
>   		return -EINVAL;
> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> index 8c33e999319a..436ad1aed3d9 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile
> @@ -70,7 +70,8 @@ TEST_PROGS := test_kmod.sh \
>   	test_bpftool_build.sh \
>   	test_bpftool.sh \
>   	test_bpftool_metadata.sh \
> -	test_xsk.sh
> +	test_xsk.sh \
> +	test_extint.py

Can we fold all tests into test_progs instead of test_extint.py?
People regularly run test_progs, but maybe not other shell scripts.
test_progs should be able to help test libbpf APIs and kernel APIs.
The existing prog_tests/btf.c is a good starting point. You can
add additional raw tests to test kernel btf with Extint. You can
add additional pretty print tests to test kernel bpffs map printout
with Extint. Putting tests in btf.c is also good as btf.c is the
place in selftests/bpf which tests basic btf types.

Also, you need to do a feature detection in btf.c. You can construct
a simple btf with Extint(256) type to see whether the kernel supports
it or not. If not, your newer tests should be skipped. There are
some test/skip examples in btf.c already.

The test_progs won't test bpftool change. in the past, people just
add some bpftool examples in the commit message to illustrate how
it is printed out and in general that is good enough.

>   
>   TEST_PROGS_EXTENDED := with_addr.sh \
>   	with_tunnels.sh \
> diff --git a/tools/testing/selftests/bpf/prog_tests/btf.c b/tools/testing/selftests/bpf/prog_tests/btf.c
> index 8ae97e2a4b9d..96a93502cf27 100644
> --- a/tools/testing/selftests/bpf/prog_tests/btf.c
> +++ b/tools/testing/selftests/bpf/prog_tests/btf.c
> @@ -4073,6 +4073,7 @@ struct btf_file_test {
>   static struct btf_file_test file_tests[] = {
>   	{ .file = "test_btf_haskv.o", },
>   	{ .file = "test_btf_newkv.o", },
> +	{ .file = "test_btf_extint.o", },
>   	{ .file = "test_btf_nokv.o", .btf_kv_notfound = true, },
>   };
>   
> @@ -4414,7 +4415,7 @@ static struct btf_raw_test pprint_test_template[] = {
>   	 * will have both int and enum types.
>   	 */
>   	.raw_types = {
> -		/* unsighed char */			/* [1] */
> +		/* unsigned char */			/* [1] */
>   		BTF_TYPE_INT_ENC(NAME_TBD, 0, 0, 8, 1),
>   		/* unsigned short */			/* [2] */
>   		BTF_TYPE_INT_ENC(NAME_TBD, 0, 0, 16, 2),
> diff --git a/tools/testing/selftests/bpf/progs/test_btf_extint.c b/tools/testing/selftests/bpf/progs/test_btf_extint.c
> new file mode 100644
> index 000000000000..b0fa9f130dda
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/test_btf_extint.c
> @@ -0,0 +1,50 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <linux/bpf.h>
> +#include <bpf/bpf_helpers.h>
> +#include "bpf_legacy.h"
> +
> +struct extint {
> +	_ExtInt(256) v256;
> +	_ExtInt(512) v512;
> +};
> +
> +struct bpf_map_def SEC("maps") btf_map = {
> +	.type = BPF_MAP_TYPE_ARRAY,
> +	.key_size = sizeof(int),
> +	.value_size = sizeof(struct extint),
> +	.max_entries = 1,
> +};
> +
> +BPF_ANNOTATE_KV_PAIR(btf_map, int, struct extint);

Please use newer .maps format instead of legacy one (the
above bpf_legacy.h). There are a lot of examples in
tools/testing/selftests/bpf/progs/ directory.

> +
> +__attribute__((noinline))
> +int test_long_fname_2(void)
> +{
> +	struct extint *bi;
> +	int key = 0;
> +
> +	bi = bpf_map_lookup_elem(&btf_map, &key);
> +	if (!bi)
> +		return 0;
> +
> +	bi->v256 <<= 64;
> +	bi->v256 += (_ExtInt(256))0xcafedead;
> +	bi->v512 <<= 128;
> +	bi->v512 += (_ExtInt(512))0xff00ff00ff00ffull;
> +
> +	return 0;
> +}
> +
> +__attribute__((noinline))
> +int test_long_fname_1(void)
> +{
> +	return test_long_fname_2();
> +}
> +
> +SEC("dummy_tracepoint")
> +int _dummy_tracepoint(void *arg)
> +{
> +	return test_long_fname_1();
> +}
> +
> +char _license[] SEC("license") = "GPL";
> diff --git a/tools/testing/selftests/bpf/test_extint.py b/tools/testing/selftests/bpf/test_extint.py
> new file mode 100755
> index 000000000000..86af815a0cf6
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/test_extint.py
> @@ -0,0 +1,535 @@
> +#!/usr/bin/python3
> +# SPDX-License-Identifier: GPL-2.0
> +
> +# Copyright (C) 2020 Sean Young <sean@...s.org>
> +# Copyright (C) 2017 Netronome Systems, Inc.
> +# Copyright (c) 2019 Mellanox Technologies. All rights reserved
> +#
> +# This software is licensed under the GNU General License Version 2,
> +# June 1991 as shown in the file COPYING in the top-level directory of this
> +# source tree.
> +#
> +# THE COPYRIGHT HOLDERS AND/OR OTHER PARTIES PROVIDE THE PROGRAM "AS IS"
> +# WITHOUT WARRANTY OF ANY KIND, EITHER EXPRESSED OR IMPLIED, INCLUDING,
> +# BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS
> +# FOR A PARTICULAR PURPOSE. THE ENTIRE RISK AS TO THE QUALITY AND PERFORMANCE
> +# OF THE PROGRAM IS WITH YOU. SHOULD THE PROGRAM PROVE DEFECTIVE, YOU ASSUME
> +# THE COST OF ALL NECESSARY SERVICING, REPAIR OR CORRECTION.
> +
> +from datetime import datetime
> +import argparse
> +import errno
> +import json
> +import os
> +import pprint
> +import random
> +import re
> +import stat
> +import string
> +import struct
> +import subprocess
> +import time
> +import traceback
> +
[...]

Powered by blists - more mailing lists