Message-ID: <CAMB2axO1K3-=oAxfOd4bBopiC6NR_BFf28_jx1y=d9bpenAAgw@mail.gmail.com>
Date: Thu, 22 May 2025 09:49:23 -0700
From: Amery Hung <ameryhung@...il.com>
To: Tony Ambardar <tony.ambardar@...il.com>
Cc: bpf@...r.kernel.org, netdev@...r.kernel.org, alexei.starovoitov@...il.com, 
	andrii@...nel.org, daniel@...earbox.net, tj@...nel.org, memxor@...il.com, 
	martin.lau@...nel.org, kernel-team@...a.com
Subject: Re: [PATCH bpf-next v4 1/3] selftests/bpf: Introduce task local data

On Thu, May 22, 2025 at 1:36 AM Tony Ambardar <tony.ambardar@...il.com> wrote:
>
> Hi Amery,
>
> I'm trying out your series in an arm32 JIT testing env I'm working on.
>
>
> On Thu, May 15, 2025 at 02:16:00PM -0700, Amery Hung wrote:
>
> [...]
>
> > diff --git a/tools/testing/selftests/bpf/progs/task_local_data.bpf.h b/tools/testing/selftests/bpf/progs/task_local_data.bpf.h
> > new file mode 100644
> > index 000000000000..5f48e408a5e5
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/progs/task_local_data.bpf.h
> > @@ -0,0 +1,220 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#ifndef __TASK_LOCAL_DATA_BPF_H
> > +#define __TASK_LOCAL_DATA_BPF_H
> > +
> > +/*
> > + * Task local data is a library that facilitates sharing per-task data
> > + * between user space and bpf programs.
> > + *
> > + *
> > + * PREREQUISITE
> > + *
> > + * A TLD, an entry of data in task local data, first needs to be created by the
> > + * user space. This is done by calling user space API, tld_create_key(), with
> > + * the name of the TLD and the size.
> > + *
> > + *     tld_key_t prio, in_cs;
> > + *
> > + *     prio = tld_create_key("priority", sizeof(int));
> > + *     in_cs = tld_create_key("in_critical_section", sizeof(bool));
> > + *
> > + * A key associated with the TLD, which has an opaque type tld_key_t, will be
> > + * returned. It can be used to get a pointer to the TLD in the user space by
> > + * calling tld_get_data().
> > + *
> > + *
> > + * USAGE
> > + *
> > + * Similar to user space, bpf programs locate a TLD using the same key.
> > + * tld_fetch_key() allows bpf programs to retrieve a key created in the user
> > + * space by name, which is specified in the second argument of tld_create_key().
> > + * tld_fetch_key() additionally will cache the key in a task local storage map,
> > + * tld_key_map, to avoid performing costly string comparisons every time when
> > + * accessing a TLD. This requires the developer to define the map value type of
> > + * tld_key_map, struct tld_keys. It only needs to contain keys used by bpf
> > + * programs in the compilation unit.
> > + *
> > + * struct tld_keys {
> > + *     tld_key_t prio;
> > + *     tld_key_t in_cs;
> > + * };
> > + *
> > + * Then, for every new task, a bpf program will fetch and cache keys once and
> > + * for all. This should be done ideally in a non-critical path (e.g., in
> > + * sched_ext_ops::init_task).
> > + *
> > + *     struct tld_object tld_obj;
> > + *
> > + *     err = tld_object_init(task, &tld);
> > + *     if (err)
> > + *         return 0;
> > + *
> > + *     tld_fetch_key(&tld_obj, "priority", prio);
> > + *     tld_fetch_key(&tld_obj, "in_critical_section", in_cs);
> > + *
> > + * Note that, the first argument of tld_fetch_key() is a pointer to tld_object.
> > + * It should be declared as a stack variable and initialized via tld_object_init().
> > + *
> > + * Finally, just like user space programs, bpf programs can get a pointer to a
> > + * TLD by calling tld_get_data(), with cached keys.
> > + *
> > + *     int *p;
> > + *
> > + *     p = tld_get_data(&tld_obj, prio, sizeof(int));
> > + *     if (p)
> > + *         // do something depending on *p
> > + */
> > +#include <errno.h>
> > +#include <bpf/bpf_helpers.h>
> > +
> > +#define TLD_DATA_SIZE __PAGE_SIZE
> > +#define TLD_DATA_CNT 63
> > +#define TLD_NAME_LEN 62
> > +
> > +typedef struct {
> > +     __s16 off;
> > +} tld_key_t;
> > +
> > +struct u_tld_data *dummy_data;
> > +struct u_tld_metadata *dummy_metadata;
>
> I suspect I've overlooked something, but what are these 2 "dummy" globals
> used for? The code builds OK without them, although I do see test errors
> as noted below.
>

Hi, sorry for the confusion. The forward declaration is to prevent
dummy_data/metadata tld_map_value to be fwd_kind. I will explain this
in the comment.

The BTF should look like this:

[9] STRUCT 'tld_map_value' size=16 vlen=2
        'data' type_id=10 bits_offset=0
        'metadata' type_id=11 bits_offset=64
[10] PTR '(anon)' type_id=74
[11] PTR '(anon)' type_id=73
[57] STRUCT 'u_tld_data' size=4096 vlen=1
        'data' type_id=58 bits_offset=0
[61] STRUCT 'u_tld_metadata' size=4096 vlen=3
        'cnt' type_id=62 bits_offset=0
        'padding' type_id=64 bits_offset=8
        'metadata' type_id=67 bits_offset=512
[73] TYPE_TAG 'uptr' type_id=61
[74] TYPE_TAG 'uptr' type_id=57

Without the forward declaration, the BTF will look like this:

[9] STRUCT 'tld_map_value' size=16 vlen=2
        'data' type_id=10 bits_offset=0
        'metadata' type_id=11 bits_offset=64
[10] PTR '(anon)' type_id=63
[11] PTR '(anon)' type_id=61
[60] FWD 'u_tld_metadata' fwd_kind=struct
[61] TYPE_TAG 'uptr' type_id=60
[62] FWD 'u_tld_data' fwd_kind=struct
[63] TYPE_TAG 'uptr' type_id=62

> I'll also mention the only reason I noticed these is that "bpftool gen
> skeleton" automatically maps these to user space, but results in an
> ASSERT() failure during build on 32-bit targets due to lack of support,
> so dropping them avoids that.

Can you provide more details of the error?

>
>
> 24: (85) call pc+25
> caller:
>  R6_w=map_value(map=tld_key_map,ks=4,vs=6) R7=1 R10=fp0 fp-8_w=map_value(map=tld_key_map,ks=4,vs=6) fp-16=map_value(map=tld_data_map,ks=4,vs=16)
> callee:
>  frame1: R1_w=fp[0]-16 R2_w=map_value(map=.rodata.str1.1,ks=4,vs=30) R10=fp0
> 50: frame1: R1_w=fp[0]-16 R2_w=map_value(map=.rodata.str1.1,ks=4,vs=30) R10=fp0
> ; static u16 __tld_fetch_key(struct tld_object *tld_obj, const char *name) @ task_local_data.bpf.h:163
> 50: (7b) *(u64 *)(r10 -16) = r2       ; frame1: R2_w=map_value(map=.rodata.str1.1,ks=4,vs=30) R10=fp0 fp-16_w=map_value(map=.rodata.str1.1,ks=4,vs=30)
> 51: (b4) w7 = 0                       ; frame1: R7_w=0
> ; if (!tld_obj->data_map || !tld_obj->data_map->metadata) @ task_local_data.bpf.h:169
> 52: (79) r1 = *(u64 *)(r1 +0)         ; frame1: R1=map_value(map=tld_data_map,ks=4,vs=16) fp-16=map_value(map=.rodata.str1.1,ks=4,vs=30)
> 53: (15) if r1 == 0x0 goto pc+36      ; frame1: R1=map_value(map=tld_data_map,ks=4,vs=16)
> 54: (79) r6 = *(u64 *)(r1 +8)         ; frame1: R1=map_value(map=tld_data_map,ks=4,vs=16) R6_w=scalar()
> 55: (15) if r6 == 0x0 goto pc+34      ; frame1: R6_w=scalar(umin=1)
> ; cnt = tld_obj->data_map->metadata->cnt; @ task_local_data.bpf.h:172
> 56: (71) r8 = *(u8 *)(r6 +0)
> R6 invalid mem access 'scalar'
> processed 29 insns (limit 1000000) max_states_per_insn 0 total_states 3 peak_states 3 mark_read 1
> -- END PROG LOAD LOG --
> libbpf: prog 'task_init': failed to load: -EACCES
> libbpf: failed to load object 'test_task_local_data'
> libbpf: failed to load BPF skeleton 'test_task_local_data': -EACCES
> test_task_local_data_basic:FAIL:skel_open_and_load unexpected error: -13
> #409/1   task_local_data/task_local_data_basic:FAIL
>
>
> I'm unsure if this verifier error is related to the dummy pointers, but
> it does seem there's a pointer issue...
>

The error is exactly caused by removing the dummy_xxx.

> Further thoughts or suggestions (from anyone) would be most welcome.
>
> Thanks,
> Tony
>
> > +
> > +struct tld_metadata {
> > +     char name[TLD_NAME_LEN];
> > +     __u16 size;
> > +};
> > +
> > +struct u_tld_metadata {
> > +     __u8 cnt;
> > +     __u8 padding[63];
> > +     struct tld_metadata metadata[TLD_DATA_CNT];
> > +};
> > +
> > +struct u_tld_data {
> > +     char data[TLD_DATA_SIZE];
> > +};
> > +
> > +struct tld_map_value {
> > +     struct u_tld_data __uptr *data;
> > +     struct u_tld_metadata __uptr *metadata;
> > +};
> > +
> > +struct tld_object {
> > +     struct tld_map_value *data_map;
> > +     struct tld_keys *key_map;
> > +};
> > +
>
> [...]

Powered by blists - more mailing lists