Re: [PATCH v3] debug: test for more required cacellation points (BZ# 29274)

[Carlos O'Donell <carlos@redhat.com>]

On 7/26/22 15:10, Adhemerval Zanella Netto wrote:
> 
> 
> On 21/07/22 18:23, Carlos O'Donell wrote:
>> On 7/18/22 12:46, Adhemerval Zanella wrote:
>>> +
>>> +  /* This test is the early cancel version of the first test, and the intent
>>> +     is to have the cancellation happen at one of two regions:
>>> +
>>> +     1. Before the cancellable syscall registers cancellation.
>>> +     2. After the cancellable syscall registers cancellation.
>>> +
>>> +     This test will exercise if syscall cancellation registration is an atomic
>>> +     operation or not since the transition from the regions is designed to be
>>> +     atomic.
>>> +
>>> +     We can not control when the cancellation happens, but it will happen in
>>> +     one of the two regions.  The first test (the !only_early version)
>>> +     attempts to test the second region, while this test attempts to test the
>>> +     first regiont and the transition with some probability.  */
>>> +  for (int i = 0; i < array_length (tests); i++)
>>> +    {
>>> +      xpthread_barrier_init (&barrier, NULL, 2);
>>> +      /* Reset the counter for the cleanup handler.  */
>>> +      cl_called = 0;
>>> +
>>> +      /* After this wait the cancellation handler is in place.  */
>>> +      pthread_t thr = xpthread_create (0, tests[i].tf, NULL);
>>> +
>>> +      xpthread_cancel (thr);
>>> +      xpthread_barrier_wait (&barrier);
>>
>> Shouldn't this be:
>>
>> xpthread_barrier_wait (&barrier);
>> xpthread_cancel (thr);
>>
>> You want to:
>>
>> (a) Make sure a cancellation handler is registered.
>> (b) Deliver the signal *before* you reach the syscall __pthread_enable_asyncancel.
>> (c) Observe the cancelled bit early and act upon it.
>>
>> The test is effectively very similar to the first test but with no wait to allow
>> the thread to get to and block on the syscall.
>>
> In fact the ordering is correct because in this case there is no signal 
> involved, pthread_cancel will just mask the thread canceled 
> (CANCELING_BITMASK | CANCELED_BITMASK) since asynchronous mode is not set. 
> 
> Otherwise, pthread_cancel might be called when __pthread_enable_asyncancel
> is already called (changing the mode to asynchronous), and the the signal
> handler it will act, not __pthread_enable_asyncancel.
 
You are correct.

The pthread_barrier_wait call has no calls to cancelable futex APIs.

The pthread_cleanup_push call is not itself a cancellation point either.

Calling pthread_cancel first ensures the bit mask is set.

Calling pthread_barrier_wait after ensures that we will always enter the
cancellation region with the cancel bits set and verify that the thread
sees the values set and cancels without delivery of a signal.

In which case we need to rewrite the comments to match that.

The comments for the early-test need to describe this behaviour.

-- 
Cheers,
Carlos.