Re: [PATCH v3 4/6] nptl: pthread_rwlock: Move timeout validation into _full functions

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

On 07/06/2019 14:01, Mike Crowe wrote:
> On Thursday 06 June 2019 at 16:18:05 -0300, Adhemerval Zanella wrote:
>>
>>
>> On 27/05/2019 17:03, Mike Crowe wrote:
>>> As recommended by the comments in the implementations of
>>> pthread_rwlock_timedrdlock and pthread_rwlock_timedwrlock, let's move the
>>> timeout validity checks into the corresponding pthread_rwlock_rdlock_full
>>> and pthread_rwlock_wrlock_full functions. Since these functions may be
>>> called with abstime == NULL, an extra check for that is necessary too.
>>
>> Is this really a gain here? It just moves some code around and add
>> an extra non required check. Wouldn't be better just to remove the
>> comments?
> 
> It looks like the comment was added by Torvald Riegel in
> cc25c8b4c1196a8c29e9a45b1e096b99a87b7f8c back in 2017.
> 
> From what I can tell from the x86_64 disassembly, the compiler throws away the extra check because
> these functions are all inline and it knows that abstime will be NULL.
> 
> However, I'm perfectly happy not to make this change, but it means that
> I'll need to add the same checks to pthread_rwlock_clockwrlock and
> pthread_wlock_clockrdlock.
> 
> I think that you've suggested consolidating these checks into a single
> function in the past, which would make that easier. How about adding
> is_valid_timespec or similar? Where would it be declared?

It seems we have multiple check that might be redundant, although it would
be likely removed from compiler.  On __pthread_rwlock_rdlock_full we have
the abstime check and futex_abstimed_wait, also present on futex_abstimed_wait.

However I think this should be ok and if we eventually make 
__pthread_rwlock_wrlock_full we can rearrange the checks to avoid
useless one.