Re: [PATCH 04/14] add gsync tests

[Samuel Thibault]

Luca Dariz, le jeu. 28 déc. 2023 20:42:51 +0100, a ecrit:
> +static void single_t2(void *arg)
> +{
> +  int err;
> +  msleep(100);
> +  err = gsync_wake(mach_task_self(), (vm_offset_t)&single_shared, 0, 0);
> +  ASSERT_RET(err, "gsync_wake t2");
> +
> +  err = thread_terminate(mach_thread_self());
> +  ASSERT_RET(err, "thread_terminate");

thread_terminate is not actually supposed to return, so that'd rather be
an assert(0)

> +}
> +
> +static void test_single()
> +{
> +  int err;
> +  single_shared = 0;
> +  err = gsync_wait(mach_task_self(), (vm_offset_t)&single_shared, 0, 0, 100, 
> GSYNC_TIMED);
> +  ASSERT(err == KERN_TIMEDOUT, "gsync_wait 1");
> +
> +  single_shared = 1;
> +  err = gsync_wait(mach_task_self(), (vm_offset_t)&single_shared, 0, 0, 100, 
> GSYNC_TIMED);
> +  ASSERT(err == KERN_INVALID_ARGUMENT, "gsync_wait 2");
> +  err = gsync_wait(mach_task_self(), (vm_offset_t)&single_shared, 1, 0, 100, 
> GSYNC_TIMED);
> +  ASSERT(err == KERN_TIMEDOUT, "gsync_wait 2b");
> +
> +  err = gsync_wake(mach_task_self(), (vm_offset_t)&single_shared, 0, 0);
> +  ASSERT_RET(err, "gsync_wake 1");
> +
> +  err = gsync_wake(mach_task_self(), (vm_offset_t)&single_shared, 0, 
> GSYNC_BROADCAST);
> +  ASSERT_RET(err, "gsync_wake 2");

Here, you want to try gsync_wait again, and check that they time out
again (i.e. the kernel did forget about the gsync_wake, as expected).

> +  err = gsync_wake(mach_task_self(), (vm_offset_t)&single_shared, 2, 
> GSYNC_MUTATE);
> +  ASSERT_RET(err, "gsync_wake 2a");
> +  ASSERT(single_shared == 2, "gsync_wake single_shared");
> +
> +  err = gsync_wake(mach_task_self(), (vm_offset_t)&single_shared, 0, 0);
> +  ASSERT_RET(err, "gsync_wake 3");
> +  err = gsync_wake(mach_task_self(), (vm_offset_t)&single_shared, 0, 0);
> +  ASSERT_RET(err, "gsync_wake 3a");

Why two such calls?

> +  single_shared = 1;
> +  test_thread_start(mach_task_self(), single_t2, 0);
> +  err = gsync_wait(mach_task_self(), (vm_offset_t)&single_shared, 1, 0, 0, 
> 0);
> +  ASSERT_RET(err, "gsync_wait 4");

This is racy since the thread may start and call gsync_wake before
gsync_wait gets to block, and thus gsync_wait would stay stuck. Of
course in practice this doesn't happen because of the msleep(100) in the
thread, but that still leaves it racy and prone to fail if the system is
e.g. very busy. You want to make single_t2 modify single_shared, so that
if test_single2 happens to call gsync_wait after that, it will just not
block and return KERN_INVALID_ARGUMENT instead, as expected.

> +static void test_single2()
> +{
> +  int err;
> +  test_thread_start(mach_task_self(), single_t2, 0);
> +  single_shared = 2;
> +  err = gsync_wait(mach_task_self(), (vm_offset_t)&single_shared, 2, 0, 0, 
> 0);
> +  ASSERT_RET(err, "gsync_wait 1");
> +  // should this fail?
> +  err = gsync_wake(mach_task_self(), (vm_offset_t)&single_shared, 0, 0);
> +  ASSERT_RET(err, "gsync_wake 2");
> +}

I don't understand the test. Why waking after waiting? There is nobody
to wake. You also want to set single_shared before starting the thread.

Samuel