Re: [PATCH 04/14] add gsync tests

[Luca Dariz]

Il 29/12/23 14:56, Samuel Thibault ha scritto:
> 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)

right, I think I added the FAILURE() macro later than this and I forgot 
to update

> > +}
> > +
> > +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).

ok added

> > +  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?

To be honest I don't remember, maybe a mistake. I'll try to add more 
descriptive messages on assertions.

> > +  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.

ok

> > +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.

Looking at it again, it really seems the same case as above, I don't 
remember why I added the additional wake. I'll make this a 
test_single_from_thread() or similar, merge your suggestion above and 
remove the duplicate test from test_single(), I guess the exact value 
used in the synchronization is not relevant, as long as it's the 
expected one.


Luca