Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <cover.1609855479.git.sean@mess.org> <13cfab3593e0ea960ca732c259bfa60bf3c16b3b.1609855479.git.sean@mess.org>
 <CAEf4BzZgPx7YZ_S6a142gu+0XqxOq5-0=iMnAr1-DDJqyNOQrg@mail.gmail.com>
In-Reply-To: <CAEf4BzZgPx7YZ_S6a142gu+0XqxOq5-0=iMnAr1-DDJqyNOQrg@mail.gmail.com>
From:   Andrii Nakryiko <andrii.nakryiko@gmail.com>
Date:   Tue, 5 Jan 2021 21:16:46 -0800
Message-ID: <CAEf4Bza-D-FtFkeEt3jEpR-9QNpXdRCnhx2Sekkj44xqRv+i-Q@mail.gmail.com>
Subject: Re: [PATCH v3 1/4] btf: add support for ints larger than 128 bits
To:     Sean Young <sean@mess.org>
Cc:     Yonghong Song <yhs@fb.com>, Alexei Starovoitov <ast@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        Andrii Nakryiko <andrii@kernel.org>,
        Martin KaFai Lau <kafai@fb.com>,
        Song Liu <songliubraving@fb.com>,
        John Fastabend <john.fastabend@gmail.com>,
        KP Singh <kpsingh@kernel.org>,
        Nathan Chancellor <natechancellor@gmail.com>,
        Nick Desaulniers <ndesaulniers@google.com>,
        Quentin Monnet <quentin@isovalent.com>,
        =?UTF-8?B?VG9rZSBIw7hpbGFuZC1Kw7hyZ2Vuc2Vu?= <toke@redhat.com>,
        linux-doc@vger.kernel.org, Networking <netdev@vger.kernel.org>,
        bpf <bpf@vger.kernel.org>,
        open list <linux-kernel@vger.kernel.org>,
        clang-built-linux <clang-built-linux@googlegroups.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, Jan 5, 2021 at 9:10 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Tue, Jan 5, 2021 at 6:45 AM Sean Young <sean@mess.org> 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@mess.org>
> > ---
> >  Documentation/bpf/btf.rst      |  4 +--
> >  include/uapi/linux/btf.h       |  2 +-
> >  kernel/bpf/btf.c               | 54 ++++++++++++++++++++++++++++------
> >  tools/include/uapi/linux/btf.h |  2 +-
> >  4 files changed, 49 insertions(+), 13 deletions(-)
> >
> > 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

to avoid duplicating this #ifdef with my suggestion, you can do something like

#ifdef __BIG_ENDIAN_BITFIELD
u64 *v = (u64 *)data;
int step = 1;
#else
u64 *v = (u64 *)data + last_u64;
int step = -1;
#endif

and then just `v += step;` everywhere

> > +               if (!seen_nonzero) {
> > +                       if (!v && i != last_u64)
> > +                               continue;
> > +
> > +                       snprintf(buf, sizeof(buf), "%llx", v);
> > +
> > +                       seen_nonzero = true;
> > +               } else {
> > +                       size_t off = strlen(buf);
>
> this is wasteful, snprintf() returns number of characters printed, so
> you can maintain offset properly
>
> > +
> > +                       snprintf(buf + off, sizeof(buf) - off, "%016llx", v);
> > +               }
> > +       }
> > +
> > +       btf_show_type_value(show, "0x%s", buf);
> > +}
>
> seen_nonzero is a bit convoluted, two simple loops might be more
> straightforward:
>
> u64 v;
> int off;
>
> /* find first non-zero u64 (or stop on the last one regardless) */
> for (i = 0; i < last_u64; i++) {
>   v = ...;
>   if (!v)
>     continue;
> }
> /* print non-zero or zero, but last u64 */
> off = snprintf(buf, sizeof(buf), "%llx", v);
> /* print the rest with zero padding */
> for (i++; i <= last_u64; i++) {
>   v = ...;
>   off += snprintf(buf + off, sizeof(buf) - off, "%016llx", v);
> }
>
> > +
> >  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);
>
> btf_bigint_print() supersedes btf_int128_print(), why maintain both?
>
> >                 break;
> > 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)
> > --
> > 2.29.2
> >