Re: [PATCH bpf-next v4 1/3] selftests/bpf: Introduce task local data

[Tony Ambardar <tony.ambardar@gmail.com>]

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.

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.


root@qemu-armhf:/usr/libexec/kselftests-bpf# ./test_progs -w 0 -a task_local_data
test_task_local_data_basic:PASS:pthread_mutex_init 0 nsec
libbpf: prog 'task_init': BPF program load failed: -EACCES
libbpf: prog 'task_init': -- BEGIN PROG LOAD LOG --
arg#0 reference type('UNKNOWN ') size cannot be determined: -22
0: R1=ctx() R10=fp0
; task = bpf_get_current_task_btf(); @ test_task_local_data.c:31
0: (85) call bpf_get_current_task_btf#158     ; R0_w=trusted_ptr_task_struct()
1: (bf) r6 = r0                       ; R0_w=trusted_ptr_task_struct() R6_w=trusted_ptr_task_struct()
; tld_obj->data_map = bpf_task_storage_get(&tld_data_map, task, 0, 0); @ task_local_data.bpf.h:135
2: (18) r1 = 0xc25ddc00               ; R1_w=map_ptr(map=tld_data_map,ks=4,vs=16)
4: (bf) r2 = r6                       ; R2_w=trusted_ptr_task_struct() R6_w=trusted_ptr_task_struct()
5: (b7) r3 = 0                        ; R3_w=0
6: (b7) r4 = 0                        ; R4_w=0
7: (85) call bpf_task_storage_get#156         ; R0=map_value_or_null(id=1,map=tld_data_map,ks=4,vs=16)
8: (b4) w7 = 1                        ; R7_w=1
9: (7b) *(u64 *)(r10 -16) = r0        ; R0=map_value_or_null(id=1,map=tld_data_map,ks=4,vs=16) R10=fp0 fp-16_w=map_value_or_null(id=1,map=tld_data_map,ks=4,vs=16)
; if (!tld_obj->data_map) @ task_local_data.bpf.h:136
10: (15) if r0 == 0x0 goto pc+37      ; R0=map_value(map=tld_data_map,ks=4,vs=16)
; tld_obj->key_map = bpf_task_storage_get(&tld_key_map, task, 0, @ task_local_data.bpf.h:139
11: (18) r1 = 0xc3ade000              ; R1_w=map_ptr(map=tld_key_map,ks=4,vs=6)
13: (bf) r2 = r6                      ; R2_w=trusted_ptr_task_struct() R6=trusted_ptr_task_struct()
14: (b7) r3 = 0                       ; R3_w=0
15: (b7) r4 = 1                       ; R4_w=1
16: (85) call bpf_task_storage_get#156        ; R0=map_value_or_null(id=2,map=tld_key_map,ks=4,vs=6)
17: (bf) r6 = r0                      ; R0=map_value_or_null(id=2,map=tld_key_map,ks=4,vs=6) R6_w=map_value_or_null(id=2,map=tld_key_map,ks=4,vs=6)
18: (7b) *(u64 *)(r10 -8) = r6        ; R6_w=map_value_or_null(id=2,map=tld_key_map,ks=4,vs=6) R10=fp0 fp-8_w=map_value_or_null(id=2,map=tld_key_map,ks=4,vs=6)
;  @ task_local_data.bpf.h:0
19: (15) if r6 == 0x0 goto pc+28      ; R6_w=map_value(map=tld_key_map,ks=4,vs=6)
20: (bf) r1 = r10                     ; R1_w=fp0 R10=fp0
;  @ test_task_local_data.c:0
21: (07) r1 += -16                    ; R1_w=fp-16
; if (!tld_fetch_key(&tld_obj, "value1", value1)) @ test_task_local_data.c:36
22: (18) r2 = 0xc2ddddd0              ; R2_w=map_value(map=.rodata.str1.1,ks=4,vs=30)
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...

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;
> +};
> +

[...]