Re: [PATCH v3 5/6] nptl: Add POSIX-proposed pthread_rwlock_clockrdlock & pthread_rwlock_clockwrlock

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

On 07/06/2019 14:14, Mike Crowe wrote:
> [snip review comments that I will address]
> 
> On Friday 07 June 2019 at 11:05:44 -0300, Adhemerval Zanella wrote:
>>> diff --git a/nptl/pthread_rwlock_clockwrlock.c b/nptl/pthread_rwlock_clockwrlock.c
>>> new file mode 100644
>>> index 0000000..38ba693
>>> --- /dev/null
>>> +++ b/nptl/pthread_rwlock_clockwrlock.c
>>> @@ -0,0 +1,29 @@
>>> +/* Implement pthread_rwlock_clockwrlock.
>>> +
>>> +   Copyright (C) 2019 Free Software Foundation, Inc.
>>> +   This file is part of the GNU C Library.
>>> +
>>> +   The GNU C Library is free software; you can redistribute it and/or
>>> +   modify it under the terms of the GNU Lesser General Public
>>> +   License as published by the Free Software Foundation; either
>>> +   version 2.1 of the License, or (at your option) any later version.
>>> +
>>> +   The GNU C Library is distributed in the hope that it will be useful,
>>> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.	 See the GNU
>>> +   Lesser General Public License for more details.
>>> +
>>> +   You should have received a copy of the GNU Lesser General Public
>>> +   License along with the GNU C Library; if not, see
>>> +   <http://www.gnu.org/licenses/>.  */
>>> +
>>> +#include "pthread_rwlock_common.c"
>>> +
>>> +/* See pthread_rwlock_common.c.  */
>>> +int
>>> +pthread_rwlock_clockwrlock (pthread_rwlock_t *rwlock,
>>> +    clockid_t clockid,
>>> +    const struct timespec *abstime)
>>> +{
>>> +  return __pthread_rwlock_wrlock_full (rwlock, clockid, abstime);
>>> +}
>>
>> Ok.
>>
>> As a side note, the '__pthread_rwlock_wrlock_full' implementation is currently
>> included on 6 implementations (pthread_rwlock_timedrdlock.c, 
>> pthread_rwlock_wrlock.c, pthread_rwlock_rdlock.c, pthread_rwlock_tryrdlock.c,
>> pthread_rwlock_unlock.c, and pthread_rwlock_timedwrlock.c).  I wonder if it
>> would be better to factored it out to be an internal symbol instead.
>>
>> It would incur in a slight better code size, but we will need to check how
>> it changes the performance. I would not expect much, since it would be tail
>> call to an internal symbol.
> 
> For x86_64, it looks like pthread_rwlock_timedrdlock is about 700 bytes, so
> we could gain approximately 3.5K by doing that.
> 
> I'm not sure that I'm knowledgeable enough in benchmarking real-world
> scenarios to be able to come up with a test I'd trust, and I can come up
> with theoretical arguments in both directions.

I think 3.5k is noticeable and if performance-wise it does not yield any
drawback I think it would be good refactor. I don't have neither performance
workload to validate this change, but I presume on most architecture it would
be a tail call which would just an extra jump without the need to handle 
potential extra code segments.

I am ccing Torvalds, who rewrote the pthread cond code. Torvalds, do you recall
why you set the code to be inline instead of create an internal common symbol? 
Do you think refactoring to share the code would yield any performance
regressions?

> 
>>> diff --git a/nptl/pthread_rwlock_common.c b/nptl/pthread_rwlock_common.c
>>> index 120b880..50a366e 100644
>>> --- a/nptl/pthread_rwlock_common.c
>>> +++ b/nptl/pthread_rwlock_common.c
>>> @@ -278,17 +278,19 @@ __pthread_rwlock_rdunlock (pthread_rwlock_t *rwlock)
>>>  
>>>  static __always_inline int
>>>  __pthread_rwlock_rdlock_full (pthread_rwlock_t *rwlock,
>>> +    clockid_t clockid,
>>>      const struct timespec *abstime)
>>>  {
>>>    unsigned int r;
>>>  
>>> -  /* Make sure any passed in timeout value is valid.  Note that the previous
>>> -     implementation assumed that this check *must* not be performed if there
>>> -     would in fact be no blocking; however, POSIX only requires that "the
>>> -     validity of the abstime parameter need not be checked if the lock can be
>>> -     immediately acquired" (i.e., we need not but may check it).  */
>>> -  if (abstime
>>> -      && __glibc_unlikely (abstime->tv_nsec >= 1000000000
>>> +  /* Make sure any passed in clockid and timeout value are valid. Note
>>> +     that the previous implementation assumed that this check *must*
>>> +     not be performed if there would in fact be no blocking; however,
>>> +     POSIX only requires that "the validity of the abstime parameter
>>> +     need not be checked if the lock can be immediately acquired"
>>> +     (i.e., we need not but may check it). */
>>> +  if (abstime && __glibc_unlikely (!futex_abstimed_supported_clockid (clockid)
>>> +      || abstime->tv_nsec >= 1000000000
>>>        || abstime->tv_nsec < 0))
>>>      return EINVAL;
>>
>> The futex_abstimed_wait already check for abstime and if its tv_sec < 0.  Would 
>> it be better to move the clock validity when it would be really used?
> 
> I think that it's better to consistently return an error if an invalid
> clock is supplied, even if it is not necessary to use the timeout. Doing
> otherwise risks bugs (in code using glibc) that aren't found until much
> later.

Right, but I am thinking that if we eventually refactor to make it a common
internal symbol compiler might not be able infer abstime is always null (even
if is called with a NULL value for abstime). I will check again if current
organization with all patchset applied is ok.