BPF Archive on lore.kernel.org
 help / color / Atom feed
From: Andrii Nakryiko <andrii.nakryiko@gmail.com>
To: Ilya Leoshkevich <iii@linux.ibm.com>
Cc: Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Andrii Nakryiko <andrii@kernel.org>, Yonghong Song <yhs@fb.com>,
	Arnaldo Carvalho de Melo <acme@redhat.com>,
	John Fastabend <john.fastabend@gmail.com>,
	bpf <bpf@vger.kernel.org>,
	Heiko Carstens <heiko.carstens@de.ibm.com>,
	Vasily Gorbik <gor@linux.ibm.com>
Subject: Re: [PATCH v4 bpf-next 2/7] libbpf: Add BTF_KIND_FLOAT support
Date: Mon, 22 Feb 2021 23:03:56 -0800
Message-ID: <CAEf4Bzav=QQwOfjQsosYWYt6YLXUV19Zswy2pddRDYgZEXCgbg@mail.gmail.com> (raw)
In-Reply-To: <20210222214917.83629-3-iii@linux.ibm.com>

On Mon, Feb 22, 2021 at 1:50 PM Ilya Leoshkevich <iii@linux.ibm.com> wrote:
>
> The logic follows that of BTF_KIND_INT most of the time. Sanitization
> replaces BTF_KIND_FLOATs with BTF_KIND_CONSTs pointing to
> equally-sized BTF_KIND_ARRAYs on older kernels, for example, the
> following:
>
>     [4] FLOAT 'float' size=4
>
> becomes the following:
>
>     [4] CONST '(anon)' type_id=10
>     ...
>     [8] INT 'unsigned int' size=4 bits_offset=0 nr_bits=32 encoding=(none)
>     [9] INT 'unsigned char' size=1 bits_offset=0 nr_bits=8 encoding=(none)
>     [10] ARRAY '(anon)' type_id=9 index_type_id=8 nr_elems=4
>

I liked Yonghong's initial suggestion to replace it with PTR to VOID.
The only concern was that if this type was used from VAR, then
sizeof(void *) != sizeof(float) on 64-bit architectures, which might
theoretically mess up DATASEC layout. But is this a real concern? BPF
programs don't really support floats, so there is no point in
declaring float variables. I'd rather stick to a simple FLOAT -> PTR
substitution than extend generated BTF.

Alternatively, was FLOAT -> anonymous empty STRUCT with desired size
considered? Any problems with that?

> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>  tools/lib/bpf/btf.c             | 44 +++++++++++++++++
>  tools/lib/bpf/btf.h             |  8 ++++
>  tools/lib/bpf/btf_dump.c        |  4 ++
>  tools/lib/bpf/libbpf.c          | 84 +++++++++++++++++++++++++++++++--
>  tools/lib/bpf/libbpf.map        |  5 ++
>  tools/lib/bpf/libbpf_internal.h |  2 +
>  6 files changed, 142 insertions(+), 5 deletions(-)
>
> diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
> index fdfff544f59a..1ebfcc687dab 100644
> --- a/tools/lib/bpf/btf.c
> +++ b/tools/lib/bpf/btf.c
> @@ -292,6 +292,7 @@ static int btf_type_size(const struct btf_type *t)
>         case BTF_KIND_PTR:
>         case BTF_KIND_TYPEDEF:
>         case BTF_KIND_FUNC:
> +       case BTF_KIND_FLOAT:
>                 return base_size;
>         case BTF_KIND_INT:
>                 return base_size + sizeof(__u32);
> @@ -339,6 +340,7 @@ static int btf_bswap_type_rest(struct btf_type *t)
>         case BTF_KIND_PTR:
>         case BTF_KIND_TYPEDEF:
>         case BTF_KIND_FUNC:
> +       case BTF_KIND_FLOAT:
>                 return 0;
>         case BTF_KIND_INT:
>                 *(__u32 *)(t + 1) = bswap_32(*(__u32 *)(t + 1));
> @@ -579,6 +581,7 @@ __s64 btf__resolve_size(const struct btf *btf, __u32 type_id)
>                 case BTF_KIND_UNION:
>                 case BTF_KIND_ENUM:
>                 case BTF_KIND_DATASEC:
> +               case BTF_KIND_FLOAT:
>                         size = t->size;
>                         goto done;
>                 case BTF_KIND_PTR:
> @@ -622,6 +625,7 @@ int btf__align_of(const struct btf *btf, __u32 id)
>         switch (kind) {
>         case BTF_KIND_INT:
>         case BTF_KIND_ENUM:
> +       case BTF_KIND_FLOAT:
>                 return min(btf_ptr_sz(btf), (size_t)t->size);

well this won't work for 12-byte floats, would it?

>         case BTF_KIND_PTR:
>                 return btf_ptr_sz(btf);
> @@ -2400,6 +2404,42 @@ int btf__add_datasec(struct btf *btf, const char *name, __u32 byte_sz)
>         return btf_commit_type(btf, sz);
>  }
>
> +/*
> + * Append new BTF_KIND_FLOAT type with:
> + *   - *name* - non-empty, non-NULL type name;
> + *   - *sz* - size of the type, in bytes;
> + * Returns:
> + *   - >0, type ID of newly added BTF type;
> + *   - <0, on error.
> + */
> +int btf__add_float(struct btf *btf, const char *name, size_t byte_sz)
> +{
> +       struct btf_type *t;
> +       int sz, name_off;
> +
> +       /* non-empty name */
> +       if (!name || !name[0])
> +               return -EINVAL;
> +

check byte_sz here?


> +       if (btf_ensure_modifiable(btf))
> +               return -ENOMEM;
> +
> +       sz = sizeof(struct btf_type);
> +       t = btf_add_type_mem(btf, sz);
> +       if (!t)
> +               return -ENOMEM;
> +
> +       name_off = btf__add_str(btf, name);
> +       if (name_off < 0)
> +               return name_off;
> +
> +       t->name_off = name_off;
> +       t->info = btf_type_info(BTF_KIND_FLOAT, 0, 0);
> +       t->size = byte_sz;
> +
> +       return btf_commit_type(btf, sz);
> +}
> +
>  /*
>   * Append new data section variable information entry for current DATASEC type:
>   *   - *var_type_id* - type ID, describing type of the variable;
> @@ -3653,6 +3693,7 @@ static int btf_dedup_prep(struct btf_dedup *d)
>                 case BTF_KIND_FWD:
>                 case BTF_KIND_TYPEDEF:
>                 case BTF_KIND_FUNC:
> +               case BTF_KIND_FLOAT:
>                         h = btf_hash_common(t);
>                         break;
>                 case BTF_KIND_INT:
> @@ -3749,6 +3790,7 @@ static int btf_dedup_prim_type(struct btf_dedup *d, __u32 type_id)
>                 break;
>
>         case BTF_KIND_FWD:
> +       case BTF_KIND_FLOAT:
>                 h = btf_hash_common(t);
>                 for_each_dedup_cand(d, hash_entry, h) {
>                         cand_id = (__u32)(long)hash_entry->value;
> @@ -4010,6 +4052,7 @@ static int btf_dedup_is_equiv(struct btf_dedup *d, __u32 cand_id,
>                         return btf_compat_enum(cand_type, canon_type);
>
>         case BTF_KIND_FWD:
> +       case BTF_KIND_FLOAT:
>                 return btf_equal_common(cand_type, canon_type);
>
>         case BTF_KIND_CONST:
> @@ -4506,6 +4549,7 @@ static int btf_dedup_remap_type(struct btf_dedup *d, __u32 type_id)
>         switch (btf_kind(t)) {
>         case BTF_KIND_INT:
>         case BTF_KIND_ENUM:
> +       case BTF_KIND_FLOAT:
>                 break;
>
>         case BTF_KIND_FWD:
> diff --git a/tools/lib/bpf/btf.h b/tools/lib/bpf/btf.h
> index 1237bcd1dd17..c3b11bcebeda 100644
> --- a/tools/lib/bpf/btf.h
> +++ b/tools/lib/bpf/btf.h
> @@ -132,6 +132,9 @@ LIBBPF_API int btf__add_datasec(struct btf *btf, const char *name, __u32 byte_sz
>  LIBBPF_API int btf__add_datasec_var_info(struct btf *btf, int var_type_id,
>                                          __u32 offset, __u32 byte_sz);
>
> +/* float construction APIs */
> +LIBBPF_API int btf__add_float(struct btf *btf, const char *name, size_t byte_sz);

nit: can you please put it right after btf__add_int() and drop the comment?

> +
>  struct btf_dedup_opts {
>         unsigned int dedup_table_size;
>         bool dont_resolve_fwds;

[...]

>  static bool kernel_supports(enum kern_feature_id feat_id)
> @@ -4940,6 +5010,8 @@ static int bpf_core_fields_are_compat(const struct btf *local_btf,
>                 local_id = btf_array(local_type)->type;
>                 targ_id = btf_array(targ_type)->type;
>                 goto recur;
> +       case BTF_KIND_FLOAT:
> +               return local_type->size == targ_type->size;

we don't enforce this for INTs, so maybe let's not do this for FLOATs as well?

But really, FLOAT is not supported by BPF programs, so we should
probably just error out on FLOAT relo.

>         default:
>                 pr_warn("unexpected kind %d relocated, local [%d], target [%d]\n",
>                         btf_kind(local_type), local_id, targ_id);
> @@ -5122,6 +5194,8 @@ static int bpf_core_types_are_compat(const struct btf *local_btf, __u32 local_id
>                 skip_mods_and_typedefs(targ_btf, targ_type->type, &targ_id);
>                 goto recur;
>         }
> +       case BTF_KIND_FLOAT:
> +               return local_type->size == targ_type->size;

ditto


>         default:
>                 pr_warn("unexpected kind %s relocated, local [%d], target [%d]\n",
>                         btf_kind_str(local_type), local_id, targ_id);

[...]

  reply index

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-02-22 21:49 [PATCH v4 bpf-next 0/7] " Ilya Leoshkevich
2021-02-22 21:49 ` [PATCH v4 bpf-next 1/7] bpf: Add BTF_KIND_FLOAT to uapi Ilya Leoshkevich
2021-02-22 21:49 ` [PATCH v4 bpf-next 2/7] libbpf: Add BTF_KIND_FLOAT support Ilya Leoshkevich
2021-02-23  7:03   ` Andrii Nakryiko [this message]
2021-02-23  7:08     ` Andrii Nakryiko
2021-02-23 20:14     ` Ilya Leoshkevich
2021-02-22 21:49 ` [PATCH v4 bpf-next 3/7] tools/bpftool: " Ilya Leoshkevich
2021-02-22 21:49 ` [PATCH v4 bpf-next 4/7] selftests/bpf: Use 25th bit in "invalid BTF_INFO" test Ilya Leoshkevich
2021-02-22 21:49 ` [PATCH v4 bpf-next 5/7] bpf: Add BTF_KIND_FLOAT support Ilya Leoshkevich
2021-02-22 21:49 ` [PATCH v4 bpf-next 6/7] selftest/bpf: Add BTF_KIND_FLOAT tests Ilya Leoshkevich
2021-02-23  7:11   ` Andrii Nakryiko
2021-02-23 20:16     ` Ilya Leoshkevich
2021-02-23 21:35       ` Andrii Nakryiko
2021-02-22 21:49 ` [PATCH v4 bpf-next 7/7] bpf: Document BTF_KIND_FLOAT in btf.rst Ilya Leoshkevich

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAEf4Bzav=QQwOfjQsosYWYt6YLXUV19Zswy2pddRDYgZEXCgbg@mail.gmail.com' \
    --to=andrii.nakryiko@gmail.com \
    --cc=acme@redhat.com \
    --cc=andrii@kernel.org \
    --cc=ast@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=daniel@iogearbox.net \
    --cc=gor@linux.ibm.com \
    --cc=heiko.carstens@de.ibm.com \
    --cc=iii@linux.ibm.com \
    --cc=john.fastabend@gmail.com \
    --cc=yhs@fb.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

BPF Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/bpf/0 bpf/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 bpf bpf/ https://lore.kernel.org/bpf \
		bpf@vger.kernel.org
	public-inbox-index bpf

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.bpf


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git