Message-ID: <aGJz2waYK8mhvB49@krava>
Date: Mon, 30 Jun 2025 13:24:11 +0200
From: Jiri Olsa <olsajiri@...il.com>
To: Amery Hung <ameryhung@...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 v5 2/3] selftests/bpf: Test basic task local
 data operations

On Fri, Jun 27, 2025 at 04:39:56PM -0700, Amery Hung wrote:
> Test basic operations of task local data with valid and invalid
> tld_create_key().
> 
> For invalid calls, make sure they return the right error code and check
> that the TLDs are not inserted by running tld_get_data("
> value_not_exists") on the bpf side. The call should a null pointer.
> 
> For valid calls, first make sure the TLDs are created by calling
> tld_get_data() on the bpf side. The call should return a valid pointer.
> 
> Finally, verify that the TLDs are indeed task-specific (i.e., their
> addresses do not overlap) with multiple user threads. This done by
> writing values unique to each thread, reading them from both user space
> and bpf, and checking if the value read back matches the value written.
> 
> Signed-off-by: Amery Hung <ameryhung@...il.com>
> ---
>  .../bpf/prog_tests/test_task_local_data.c     | 191 ++++++++++++++++++
>  .../bpf/progs/test_task_local_data.c          |  65 ++++++
>  2 files changed, 256 insertions(+)
>  create mode 100644 tools/testing/selftests/bpf/prog_tests/test_task_local_data.c
>  create mode 100644 tools/testing/selftests/bpf/progs/test_task_local_data.c
> 
> diff --git a/tools/testing/selftests/bpf/prog_tests/test_task_local_data.c b/tools/testing/selftests/bpf/prog_tests/test_task_local_data.c
> new file mode 100644
> index 000000000000..53cdb8466f8e
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/test_task_local_data.c
> @@ -0,0 +1,191 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <pthread.h>
> +#include <bpf/btf.h>
> +#include <test_progs.h>
> +
> +struct test_struct {
> +	__u64 a;
> +	__u64 b;
> +	__u64 c;
> +	__u64 d;
> +};

hi,
I can't compile this on my config, bacause of the KGDB_TESTS config
that defines struct test_struct

progs/test_task_local_data.c:16:8: error: redefinition of 'test_struct'
   16 | struct test_struct {
      |        ^
/home/jolsa/kernel/linux-qemu-1/tools/testing/selftests/bpf/tools/include/vmlinux.h:141747:8: note: previous definition is here
 141747 | struct test_struct {


also I have these tests passing localy, but it's failing CI:
  https://github.com/kernel-patches/bpf/actions/runs/15939264078/job/44964987935

thanks,
jirka


> +
> +#define TLD_FREE_DATA_ON_THREAD_EXIT
> +#define TLD_DYN_DATA_SIZE 4096
> +#include "task_local_data.h"
> +
> +#include "test_task_local_data.skel.h"
> +
> +TLD_DEFINE_KEY(value0_key, "value0", sizeof(int));
> +
> +/*
> + * Reset task local data between subtests by clearing metadata. This is safe
> + * as subtests run sequentially. Users of task local data libraries
> + * should not do this.
> + */
> +static void reset_tld(void)
> +{
> +	if (TLD_READ_ONCE(tld_metadata_p)) {
> +		/* Remove TLDs created by tld_create_key() */
> +		tld_metadata_p->cnt = 1;
> +		tld_metadata_p->size = TLD_DYN_DATA_SIZE;
> +		memset(&tld_metadata_p->metadata[1], 0,
> +		       (TLD_MAX_DATA_CNT - 1) * sizeof(struct tld_metadata));
> +	}
> +}
> +
> +/* Serialize access to bpf program's global variables */
> +static pthread_mutex_t global_mutex;
> +
> +static tld_key_t *tld_keys;
> +
> +#define TEST_BASIC_THREAD_NUM TLD_MAX_DATA_CNT
> +
> +void *test_task_local_data_basic_thread(void *arg)
> +{
> +	LIBBPF_OPTS(bpf_test_run_opts, opts);
> +	struct test_task_local_data *skel = (struct test_task_local_data *)arg;
> +	int fd, err, tid, *value0, *value1;
> +	struct test_struct *value2;
> +
> +	fd = bpf_map__fd(skel->maps.tld_data_map);
> +
> +	value0 = tld_get_data(fd, value0_key);
> +	if (!ASSERT_OK_PTR(value0, "tld_get_data"))
> +		goto out;
> +
> +	value1 = tld_get_data(fd, tld_keys[0]);
> +	if (!ASSERT_OK_PTR(value1, "tld_get_data"))
> +		goto out;
> +
> +	value2 = tld_get_data(fd, tld_keys[1]);
> +	if (!ASSERT_OK_PTR(value2, "tld_get_data"))
> +		goto out;
> +
> +	tid = gettid();
> +
> +	*value0 = tid + 0;
> +	*value1 = tid + 1;
> +	value2->a = tid + 2;
> +	value2->b = tid + 3;
> +	value2->c = tid + 4;
> +	value2->d = tid + 5;
> +
> +	pthread_mutex_lock(&global_mutex);
> +	/* Run task_main that read task local data and save to global variables */
> +	err = bpf_prog_test_run_opts(bpf_program__fd(skel->progs.task_main), &opts);
> +	ASSERT_OK(err, "run task_main");
> +	ASSERT_OK(opts.retval, "task_main retval");
> +
> +	ASSERT_EQ(skel->bss->test_value0, tid + 0, "tld_get_data value0");
> +	ASSERT_EQ(skel->bss->test_value1, tid + 1, "tld_get_data value1");
> +	ASSERT_EQ(skel->bss->test_value2.a, tid + 2, "tld_get_data value2.a");
> +	ASSERT_EQ(skel->bss->test_value2.b, tid + 3, "tld_get_data value2.b");
> +	ASSERT_EQ(skel->bss->test_value2.c, tid + 4, "tld_get_data value2.c");
> +	ASSERT_EQ(skel->bss->test_value2.d, tid + 5, "tld_get_data value2.d");
> +	pthread_mutex_unlock(&global_mutex);
> +
> +	/* Make sure valueX are indeed local to threads */
> +	ASSERT_EQ(*value0, tid + 0, "value0");
> +	ASSERT_EQ(*value1, tid + 1, "value1");
> +	ASSERT_EQ(value2->a, tid + 2, "value2.a");
> +	ASSERT_EQ(value2->b, tid + 3, "value2.b");
> +	ASSERT_EQ(value2->c, tid + 4, "value2.c");
> +	ASSERT_EQ(value2->d, tid + 5, "value2.d");
> +
> +	*value0 = tid + 5;
> +	*value1 = tid + 4;
> +	value2->a = tid + 3;
> +	value2->b = tid + 2;
> +	value2->c = tid + 1;
> +	value2->d = tid + 0;
> +
> +	/* Run task_main again */
> +	pthread_mutex_lock(&global_mutex);
> +	err = bpf_prog_test_run_opts(bpf_program__fd(skel->progs.task_main), &opts);
> +	ASSERT_OK(err, "run task_main");
> +	ASSERT_OK(opts.retval, "task_main retval");
> +
> +	ASSERT_EQ(skel->bss->test_value0, tid + 5, "tld_get_data value0");
> +	ASSERT_EQ(skel->bss->test_value1, tid + 4, "tld_get_data value1");
> +	ASSERT_EQ(skel->bss->test_value2.a, tid + 3, "tld_get_data value2.a");
> +	ASSERT_EQ(skel->bss->test_value2.b, tid + 2, "tld_get_data value2.b");
> +	ASSERT_EQ(skel->bss->test_value2.c, tid + 1, "tld_get_data value2.c");
> +	ASSERT_EQ(skel->bss->test_value2.d, tid + 0, "tld_get_data value2.d");
> +	pthread_mutex_unlock(&global_mutex);
> +
> +out:
> +	pthread_exit(NULL);
> +}
> +
> +static void test_task_local_data_basic(void)
> +{
> +	struct test_task_local_data *skel;
> +	pthread_t thread[TEST_BASIC_THREAD_NUM];
> +	char dummy_key_name[TLD_NAME_LEN];
> +	tld_key_t key;
> +	int i, err;
> +
> +	reset_tld();
> +
> +	ASSERT_OK(pthread_mutex_init(&global_mutex, NULL), "pthread_mutex_init");
> +
> +	skel = test_task_local_data__open_and_load();
> +	if (!ASSERT_OK_PTR(skel, "skel_open_and_load"))
> +		return;
> +
> +	tld_keys = calloc(TEST_BASIC_THREAD_NUM, sizeof(tld_key_t));
> +	if (!ASSERT_OK_PTR(tld_keys, "calloc tld_keys"))
> +		goto out;
> +
> +	ASSERT_FALSE(tld_key_is_err(value0_key), "TLD_DEFINE_KEY");
> +	tld_keys[0] = tld_create_key("value1", sizeof(int));
> +	ASSERT_FALSE(tld_key_is_err(tld_keys[0]), "tld_create_key");
> +	tld_keys[1] = tld_create_key("value2", sizeof(struct test_struct));
> +	ASSERT_FALSE(tld_key_is_err(tld_keys[1]), "tld_create_key");
> +
> +	/*
> +	 * Shouldn't be able to store data exceed a page. Create a TLD just big
> +	 * enough to exceed a page. TLDs already created are int value0, int
> +	 * value1, and struct test_struct value2.
> +	 */
> +	key = tld_create_key("value_not_exist",
> +			     TLD_PAGE_SIZE - 2 * sizeof(int) - sizeof(struct test_struct) + 1);
> +	ASSERT_EQ(tld_key_err_or_zero(key), -E2BIG, "tld_create_key");
> +
> +	key = tld_create_key("value2", sizeof(struct test_struct));
> +	ASSERT_EQ(tld_key_err_or_zero(key), -EEXIST, "tld_create_key");
> +
> +	/* Shouldn't be able to create the (TLD_MAX_DATA_CNT+1)-th TLD */
> +	for (i = 3; i < TLD_MAX_DATA_CNT; i++) {
> +		snprintf(dummy_key_name, TLD_NAME_LEN, "dummy_value%d", i);
> +		tld_keys[i] = tld_create_key(dummy_key_name, sizeof(int));
> +		ASSERT_FALSE(tld_key_is_err(tld_keys[i]), "tld_create_key");
> +	}
> +	key = tld_create_key("value_not_exist", sizeof(struct test_struct));
> +	ASSERT_EQ(tld_key_err_or_zero(key), -ENOSPC, "tld_create_key");
> +
> +	/* Access TLDs from multiple threads and check if they are thread-specific */
> +	for (i = 0; i < TEST_BASIC_THREAD_NUM; i++) {
> +		err = pthread_create(&thread[i], NULL, test_task_local_data_basic_thread, skel);
> +		if (!ASSERT_OK(err, "pthread_create"))
> +			goto out;
> +	}
> +
> +out:
> +	for (i = 0; i < TEST_BASIC_THREAD_NUM; i++)
> +		pthread_join(thread[i], NULL);
> +
> +	if (tld_keys) {
> +		free(tld_keys);
> +		tld_keys = NULL;
> +	}
> +	tld_free();
> +	test_task_local_data__destroy(skel);
> +}
> +
> +void test_task_local_data(void)
> +{
> +	if (test__start_subtest("task_local_data_basic"))
> +		test_task_local_data_basic();
> +}
> diff --git a/tools/testing/selftests/bpf/progs/test_task_local_data.c b/tools/testing/selftests/bpf/progs/test_task_local_data.c
> new file mode 100644
> index 000000000000..94d1745dd8d4
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/test_task_local_data.c
> @@ -0,0 +1,65 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <vmlinux.h>
> +#include <errno.h>
> +#include <bpf/bpf_helpers.h>
> +
> +#include "task_local_data.bpf.h"
> +
> +struct tld_keys {
> +	tld_key_t value0;
> +	tld_key_t value1;
> +	tld_key_t value2;
> +	tld_key_t value_not_exist;
> +};
> +
> +struct test_struct {
> +	unsigned long a;
> +	unsigned long b;
> +	unsigned long c;
> +	unsigned long d;
> +};
> +
> +int test_value0;
> +int test_value1;
> +struct test_struct test_value2;
> +
> +SEC("syscall")
> +int task_main(void *ctx)
> +{
> +	struct tld_object tld_obj;
> +	struct test_struct *struct_p;
> +	struct task_struct *task;
> +	int err, *int_p;
> +
> +	task = bpf_get_current_task_btf();
> +	err = tld_object_init(task, &tld_obj);
> +	if (err)
> +		return 1;
> +
> +	int_p = tld_get_data(&tld_obj, value0, "value0", sizeof(int));
> +	if (int_p)
> +		test_value0 = *int_p;
> +	else
> +		return 2;
> +
> +	int_p = tld_get_data(&tld_obj, value1, "value1", sizeof(int));
> +	if (int_p)
> +		test_value1 = *int_p;
> +	else
> +		return 3;
> +
> +	struct_p = tld_get_data(&tld_obj, value2, "value2", sizeof(struct test_struct));
> +	if (struct_p)
> +		test_value2 = *struct_p;
> +	else
> +		return 4;
> +
> +	int_p = tld_get_data(&tld_obj, value_not_exist, "value_not_exist", sizeof(int));
> +	if (int_p)
> +		return 5;
> +
> +	return 0;
> +}
> +
> +char _license[] SEC("license") = "GPL";
> -- 
> 2.47.1
> 
> 

Powered by blists - more mailing lists