Re: [COMMITTED PATCH] Split timed-wait functions out of nptl/lowlevellock.c.

[Torvald Riegel <triegel at redhat dot com>]

On Tue, 2015-06-09 at 17:13 -0700, Roland McGrath wrote:
> > It seems you made the split just to support the absolute timeout path
> > that NaCl futexes provide out of the box.  IMO, this should have been
> > addressed by fixing the glibc-internal futex API, not by adding a
> > NaCl-specific variant and lots of new dummy files just to get back to
> > the common implementation for all other archs.  Didn't we agree to have
> > as much commonality in our synchronization code as possible?
> > 
> > I'd appreciate if we could have discussion on such patches in the future
> > before they are committed.  (And I wouldn't mind at all if you'd revert
> > this patch.)
> 
> In fact, the NaCl-specific __lll_timedwait_tid change was the real
> motivation and the NaCl-specific __lll_timedlock_wait (which shouldn't
> need to be, as you note) was just nearby low-hanging fruit.
> 
> The actual change in this thread was purely splitting one file of
> multiple functions into more files of fewer functions each.  That is
> simply moving things in the direction that they traditionally always
> were and always should be in libc: separate functions in separate files.
> One of the reasons that's the good way to be is that it makes more,
> finer grains that can be overridden in sysdeps/ variants without
> duplicating the source code that doesn't need to vary.

In general, I agree.  Nonetheless, in the particular case of
synchronization code, I'd argue that we're likely doing something wrong
if we need to specialize, say, Linux pthread_cond_signal, while keeping
all other Linux pthread_cond_* functions unchanged.  Functions that
synchronize with each other are often much more dependent on each other
than in sequential code, so the more variations there are the more
complex it gets.

Your patch added arch-specific variants on Linux, which isn't a bug as
you correctly say, but it goes in the opposite direction of where we
want to be at, I believe.

Splitting into a Linux and a NaCl variant is a different story, of
course.

Another thing I have observed is that having variants of files available
seems to invite variant-specific changes that could actually have been
applied or fixed generically.  Maybe that's something we can counter by
paying more attention to this during review -- but not having separate
files in the first place is a bigger hurdle :)
(This is a general comment, of course, not specifically on your patch.)