On Thu, Jun 26 2025 at 14:11, André Almeida wrote:
> +
> +int set_robust_list(struct robust_list_head *head, size_t len)
This function and the get() counterpart are global because they can?
> +{
> + return syscall(SYS_set_robust_list, head, len);
> +}
> +/*
> + * Basic lock struct, contains just the futex word and the robust list
> element
> + * Real implementations have also a *prev to easily walk in the list
> + */
> +struct lock_struct {
> + _Atomic(unsigned int) futex;
> + struct robust_list list;
tabular arrangement please.
> + pthread_barrier_wait(&barrier);
> +
> + /*
> + * There's a race here: the parent thread needs to be inside
> + * futex_wait() before the child thread dies, otherwise it will miss the
> + * wakeup from handle_futex_death() that this child will emit. We wait a
> + * little bit just to make sure that this happens.
> + */
> + sleep(1);
One second is quite a little bit. :)
> + /*
> + * futex_wait() should return 0 and the futex word should be marked with
> + * FUTEX_OWNER_DIED
> + */
> + ASSERT_EQ(ret, 0);
> + if (ret != 0)
> + printf("futex wait returned %d", errno);
What's the purpose of the extra printf() after the assert here? This
code is not even reached when ret != 0, no?
> + ASSERT_TRUE(*futex | FUTEX_OWNER_DIED);
That's always true no matter what the content of the futex variable is, no?
> +/*
> + * The only valid value for len is sizeof(*head)
> + */
> +static void test_set_robust_list_invalid_size(void)
> +{
> + struct robust_list_head head;
> + size_t head_size = sizeof(struct robust_list_head);
Groan. You already define the robust_list_head variable ahead of
head_size and violate the reverse fir tree ordering, so why don't you
use the obvious and actually robust 'sizeof(head)'?
> +/*
> + * Test get_robust_list with pid = 0, getting the list of the running thread
> + */
> +static void test_get_robust_list_self(void)
> +{
> + struct robust_list_head head, head2, *get_head;
> + size_t head_size = sizeof(struct robust_list_head), len_ptr;
Ditto.
> +static int child_list(void *arg)
> +{
> + struct robust_list_head *head = (struct robust_list_head *) arg;
void pointers really don't require type casts
> + int ret;
> +
> + ret = set_robust_list(head, sizeof(struct robust_list_head));
sizeof(*head)
> + if (ret)
> + ksft_test_result_fail("set_robust_list error\n");
> +
> + pthread_barrier_wait(&barrier);
> + pthread_barrier_wait(&barrier2);
Lacks a comment what this waits for
> + return 0;
> +}
> +
> +/*
> + * Test get_robust_list from another thread. We use two barriers here to
> ensure
> + * that:
> + * 1) the child thread set the list before we try to get it from the
> + * parent
> + * 2) the child thread still alive when we try to get the list from it
> + */
> +static void test_get_robust_list_child(void)
> +{
> + pid_t tid;
> + int ret;
> + struct robust_list_head head, *get_head;
> + size_t len_ptr;
Reverse fir tree ordering please.
> + ret = pthread_barrier_init(&barrier, NULL, 2);
> + ret = pthread_barrier_init(&barrier2, NULL, 2);
> + ASSERT_EQ(ret, 0);
> +
> + tid = create_child(&child_list, &head);
> + ASSERT_NE(tid, -1);
> +
> + pthread_barrier_wait(&barrier);
> +
> + ret = get_robust_list(tid, &get_head, &len_ptr);
> + ASSERT_EQ(ret, 0);
> + ASSERT_EQ(&head, get_head);
> +
> + pthread_barrier_wait(&barrier2);
> +
> + wait(NULL);
> + pthread_barrier_destroy(&barrier);
> + pthread_barrier_destroy(&barrier2);
> +
> + ksft_test_result_pass("%s\n", __func__);
> +}
> +
> +static int child_fn_lock_with_error(void *arg)
> +{
> + struct lock_struct *lock = (struct lock_struct *) arg;
See above
> + struct robust_list_head head;
> + int ret;
> +
> + ret = set_list(&head);
> + if (ret)
> + ksft_test_result_fail("set_robust_list error\n");
So you fail the test and continue to produce more fails or what? Why
does this not use one of these ASSERT thingies or return?
> + ret = mutex_lock(lock, &head, true);
> + if (ret)
> + ksft_test_result_fail("mutex_lock error\n");
> +
> + pthread_barrier_wait(&barrier);
> +
> + sleep(1);
> +
> + return 0;
> +}
> +
> +/*
> + * Same as robustness test, but inject an error where the mutex_lock() exits
> + * earlier, just after setting list_op_pending and taking the lock, to test
> the
> + * list_op_pending mechanism
> + */
> +static void test_set_list_op_pending(void)
> +{
> + struct lock_struct lock = { .futex = 0 };
> + struct robust_list_head head;
> + _Atomic(unsigned int) *futex = &lock.futex;
> + int ret;
See above
> + ASSERT_EQ(ret, 0);
> + if (ret != 0)
> + printf("futex wait returned %d", errno);
The random insertion of completely pointless printf()'s is stunning.
> + ASSERT_TRUE(*futex | FUTEX_OWNER_DIED);
Yet another always true assert which is happily optimized out by the
compiler.
> + wait(NULL);
> + pthread_barrier_destroy(&barrier);
> +
> + ksft_test_result_pass("%s\n", __func__);
> +}
> +static int child_wait_lock(void *arg)
> +{
> + struct lock_struct *lock = (struct lock_struct *) arg;
> + struct robust_list_head head;
> + int ret;
> +
> + pthread_barrier_wait(&barrier2);
> + ret = mutex_lock(lock, &head, false);
> +
> + if (ret)
> + ksft_test_result_fail("mutex_lock error\n");
> +
> + if (!(lock->futex | FUTEX_OWNER_DIED))
> + ksft_test_result_fail("futex not marked with
> FUTEX_OWNER_DIED\n");
Now I kinda understand this insanity. The child emits a fail and
exits. Then the parent ...
> + for (i = 0; i < CHILD_NR; i++)
> + create_child(&child_wait_lock, &locks[i]);
> +
> + /* Wait for all children to return */
> + while (wait(NULL) > 0);
> +
> + pthread_barrier_destroy(&barrier);
> + pthread_barrier_destroy(&barrier2);
> +
> + ksft_test_result_pass("%s\n", __func__);
... happily claims that the test passed.
Seriously?
Thread functions have a return value for a reason and wait(2) has a
wstatus argument for the very same reason.
> +static int child_circular_list(void *arg)
> +{
> + static struct robust_list_head head;
> + struct lock_struct a, b, c;
> + int ret;
> +
> + ret = set_list(&head);
> + if (ret)
> + ksft_test_result_fail("set_list error\n");
Yet another instance of the same ....
Thanks,
tglx
