On Tue, Oct 15, 2024 at 11:41 AM Jordan Rome <linux@xxxxxxxxxxxxxx> wrote:
>
> Previously test_task_tid was setting `linfo.task.tid`
> to `getpid()` which is the same as `gettid()` for the
> parent process. Instead create a new child thread
> and set `linfo.task.tid` to `gettid()` to make sure
> the tid filtering logic is working as expected.
>
> Signed-off-by: Jordan Rome <linux@xxxxxxxxxxxxxx>
> ---
> .../selftests/bpf/prog_tests/bpf_iter.c | 26 +++++++++++++++----
> 1 file changed, 21 insertions(+), 5 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_iter.c b/tools/testing/selftests/bpf/prog_tests/bpf_iter.c
> index 52e6f7570475..5b056eb5d166 100644
> --- a/tools/testing/selftests/bpf/prog_tests/bpf_iter.c
> +++ b/tools/testing/selftests/bpf/prog_tests/bpf_iter.c
> @@ -226,7 +226,7 @@ static void test_task_common_nocheck(struct bpf_iter_attach_opts *opts,
> ASSERT_OK(pthread_create(&thread_id, NULL, &do_nothing_wait, NULL),
> "pthread_create");
>
> - skel->bss->tid = getpid();
> + skel->bss->tid = gettid();
>
> do_dummy_read_opts(skel->progs.dump_task, opts);
>
> @@ -249,25 +249,41 @@ static void test_task_common(struct bpf_iter_attach_opts *opts, int num_unknown,
> ASSERT_EQ(num_known_tid, num_known, "check_num_known_tid");
> }
>
> -static void test_task_tid(void)
> +static void *run_test_task_tid(void *arg)
> {
> + ASSERT_NEQ(getpid(), gettid(), "check_new_thread_id");
this is variable declaration block, move assertion after it (and empty line)
> LIBBPF_OPTS(bpf_iter_attach_opts, opts);
> union bpf_iter_link_info linfo;
> int num_unknown_tid, num_known_tid;
>
here
> memset(&linfo, 0, sizeof(linfo));
> - linfo.task.tid = getpid();
> + linfo.task.tid = gettid();
> opts.link_info = &linfo;
> opts.link_info_len = sizeof(linfo);
> test_task_common(&opts, 0, 1);
>
> linfo.task.tid = 0;
> linfo.task.pid = getpid();
> - test_task_common(&opts, 1, 1);
> + // This includes the parent thread, this thread, and the do_nothing_wait thread
we don't use C++-style comments in C code base, please use /* */
> + test_task_common(&opts, 2, 1);
>
> test_task_common_nocheck(NULL, &num_unknown_tid, &num_known_tid);
> - ASSERT_GT(num_unknown_tid, 1, "check_num_unknown_tid");
> + ASSERT_GT(num_unknown_tid, 2, "check_num_unknown_tid");
> ASSERT_EQ(num_known_tid, 1, "check_num_known_tid");
> +
> + pthread_exit(arg);
nit: wouldn't `return arg;` do the same?
> +}
> +
> +static void test_task_tid(void)
> +{
> + pthread_t thread_id;
> + void *ret;
> +
> + // Create a new thread so pid and tid aren't the same
C++ comment
> + ASSERT_OK(pthread_create(&thread_id, NULL, &run_test_task_tid, NULL),
> + "pthread_create");
> + ASSERT_FALSE(pthread_join(thread_id, &ret) || ret != NULL,
it's best to avoid combining two check in single ASSERT_*(), so
ASSERT_OK(pthread_join(...), ...);
ASSERT_NULL(ret, ...);
is way easier to follow and debug, if something breaks
But also, why do we check ret? Do we ever return non-NULL?
pw-bot: cr
> + "pthread_join");
> }
>
> static void test_task_pid(void)
> --
> 2.43.5
>
