Re: [PATCH v2 0/4] nptl: Add pthread_clockjoin_np

[Adhemerval Zanella <adhemerval dot zanella at linaro dot org>]

On 31/10/2019 16:38, Mike Crowe wrote:
> On Thursday 31 October 2019 at 11:31:30 -0300, Adhemerval Zanella wrote:
>> On 18/09/2019 09:30, Mike Crowe wrote:
>>> The aim of this series is to add pthread_clockjoin_np, which works
>>> like pthread_timedjoin_np except it accepts an additional parameter to
>>> indicate which clock the timeout should be measured against.
>>>
>>> The naming matches the pthread_cond_clockwait,
>>> pthread_mutex_clocklock, sem_clockwait, etc. functions added in glibc
>>> v2.30 and documented at http://austingroupbugs.net/view.php?id=1216.
>>>
>>> The series also includes some minimal documentation for
>>> pthread_tryjoin_np and pthread_timedjoin_np, along with some new
>>> tests.
>>>
>>> Thanks to everyone who provided feedback for the first version[1] of
>>> this series.
>>>
>>> [1] https://sourceware.org/ml/libc-alpha/2019-06/msg00911.html
>>
>> Hi Mike,
>>
>> I think your patchset it ready to push upstream.  I have fixed the small
>> nits I brought on the review along with some more specific changes below:
>>
>>   - Move the NEWS entry for pthread_clockjoin_np to its own bullet on
>>     2.31.
>>
>>   - Moved __pthread_clockjoin_ex to a hidden definition and removed the
>>     hidden_{proto,def}.
>>
>>   - Added some missing one line file description.
>>
>> I also pushed the set on a personal branch [1]. If you are ok with this 
>> set, I would like to push it upstream.
>>
>> [1] https://sourceware.org/git/?p=glibc.git;a=shortlog;h=refs/heads/azanella/pthread_clockjoin_np
> 
> Hi Adhemerval,
> 
> Thanks for doing all that. I thought that I'd addressed the nits, but I
> must have missed or lost some. :(
> 
> Your branch looks great, except it appears to be missing this hunk from the
> "nptl: Add pthread_clockjoin_np" change:
> 
> diff --git a/nptl/tst-join3.c b/nptl/tst-join3.c
> index c06e65a247..faf7683eaa 100644
> --- a/nptl/tst-join3.c
> +++ b/nptl/tst-join3.c
> @@ -56,7 +56,12 @@ do_test_clock (clockid_t clockid)
>    struct timespec timeout = timespec_add (xclock_now (clockid_for_get),
>                                            make_timespec (0, 200000000));
> 
> -  int val = pthread_timedjoin_np (th, &status, &timeout);
> +  int val;
> +  if (clockid == CLOCK_USE_TIMEDJOIN)
> +    val = pthread_timedjoin_np (th, &status, &timeout);
> +  else
> +    val = pthread_clockjoin_np (th, &status, clockid, &timeout);
> +
>    TEST_COMPARE (val, ETIMEDOUT);
> 
>    xpthread_mutex_unlock (&lock);
> 
> 
> I suspect that it's not causing the test to fail because the existing call
> to pthread_timedjoin_np will just time out immediately if passed the
> relatively small absolute CLOCK_MONOTONIC time. I shall see if I can
> improve this test to catch that in the future.
> 
> Thanks again.
> 
> Mike.

Right, I have updated my personal branch with the missing change, thanks
for spotting it.