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

[Torvald Riegel <triegel at redhat dot com>]

On Tue, 2019-06-11 at 09:19 -0300, Adhemerval Zanella wrote:
> 
> 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?

I wasn't aware that there was that much concern about code size, and I
wanted to give as much optimization opportunity to the compiler as
possible.  There is not that much information (like abstime==NULL) that is
passed in into the full algorithm, so maybe it doesn't matter as much in
the end.

> Do you think refactoring to share the code would yield any performance
> regressions?

Not sure.  It might only be noticable matter on the fast paths, so maybe
check those (eg, single-threaded pthread_rwlock_tryrdlock).