cvs commit: src/lib/libthr/thread thr_mutex.c
src/lib/libkse/thread thr_mutex.c src/include pthread.h
Kris Kennaway
kris at FreeBSD.org
Tue Oct 30 03:04:57 PDT 2007
David Xu wrote:
> Kris Kennaway wrote:
>> David Xu wrote:
>>
>>> Daniel Eischen wrote:
>>>
>>>> On Tue, 30 Oct 2007, David Xu wrote:
>>>>
>>>>> I am not sure PTHREAD_MUTEX_ADAPTIVE_NP is a correct solution, in fact
>>>>> I think this is Linux crap, shouldn't PTHREAD_PRIO_PROTECT and
>>>>> PTHREAD_PRIO_INHERIT mutex be adaptivly spinned ?
>>>>> also this commit does not change mutex_self_lock() to handle the
>>>>> PTHREAD_MUTEX_ADAPTIVE_NP, what is the PTHREAD_MUTEX_ADAPTIVE_NP
>>>>> definition when the mutex is already locked by the currect thread ?
>>>>> deadlock or return error code ?
>>>>
>>>>
>>>>
>>>> I tend to agree with the "Linux crap" comment, but I hesitate
>>>> to use those words considering the recent sensor framework
>>>> incident ;-)
>>>>
>>>
>>> Isn't this commit an incident too ? :-) if it is not, then
>>> we should retire from FreeBSD now, as two thread library
>>> maintainers were bypassed.
>>
>>
>> Hi David,
>>
>> I'm honestly a bit surprised to hear that you consider yourself to be
>> the maintainer of this code, because while you have certainly worked
>> heavily on it in the past, I have sent several mails to you over the
>> past year or so raising various problems and ideas I have encountered
>> in the libthr and related code while working on performance tuning,
>> and you have declined to reply to many of them, or to participate in
>> the ongoing optimization work.
> At least I am maintainer of libthr today unless I am replaced
> tommorrow.
Firstly, neither of you are listed in src/MAINTAINERS as registering an
interest in the respective thread libraries. Secondly, being a
maintainer doesn't just mean that you get to say "no" to code you don't
like, it means you have responsibilities to become actively involved in
discussions and work done by others on "your" code. As mentioned above,
I have had almost no response from you when I have previously attempted
to engage you in discussion about various problems and ideas I have
encountered with the libthr and umtx code.
Being a maintainer doesn't mean that you just work on patches in secret
and occasionally commit them while ignoring all other requests for help
and discussion, it means you are expected to involve yourself in the
work being done in the community and help to guide it.
Maybe you have just been unusually busy this last year, but it is still
not reasonable to suddenly step in and attempt to exert a maintainer
privilege when you have avoided participating in previous work. In
short, you have not been acting as the maintainer of this code in recent
times, so it did not occur to me that you might still consider yourself
as such.
My apologies if I hurt your feelings.
> In fact, I am surpised to that this mutex type was added
> without public discussion, or even discussed with Dan.
I think you are making a big deal over a small change with clear
real-world benefits. Please limit further replies to the following
technical points. So far you seem to have conceded some but avoided others.
1) Doing adaptive spinning "correctly" in a general purpose manner is
hard. It is something we should try to work on in the future.
2) Other performance optimizations to the libthr code and umtx are
surely possible (such as the patches you discuss). I am eager to work
with you on this (in fact I have attempted to raise some such problems
with you in the past), and I hope the reverse is also true.
3) There is a specific problem encountered in real applications, where
certain pthread mutexes become highly contended and are held for short
time periods.
4) These specific mutexes can be optimized by the algorithm as
committed, which spins briefly in userland before entering the kernel,
but only for specific mutexes requested by the application.
5) This optimization solves the performance problem observed, and almost
completely eliminates the associated 30% performance drop in mysql on 8
CPU systems.
6) glibc has already defined an interface that enables behaviour 4).
mysql (and other applications) already uses this interface if present.
7) With the addition of a couple of lines of code implementing interface
6), which by construction have zero impact on other applications, we
have solved a serious performance problem.
8) Other changes under discussion (e.g. your previous commit) either do
not solve problem 3), or are not suitable for general use because of
loss of performance in other workloads.
>> Jeff, Attilio and I have ideas about how we might be able to improve
>> the libthr and umtx code going forward, so we'd be delighted to have
>> your help in working on them.
>>
>> Kris
>>
>
> My last commit improves mysql select-smack benchmark on 4-core xeon from
> 48000 queries/s to 70000 queries/s, so my work is alternative way
No, that is an orthogonal issue that (after measurement) does not solve
the same problem that is addressed by this change. I'd be happy to
discuss it with you in more detail if you are interested. We could also
discuss the fact that super-smack is a questionable target to be
optimizing because the main performance problems seem to be from a very
poor benchmark design.
I claim that real-world applications do not commonly do I/O in units of
1 byte :)
> I think adding new mutex type should be publically discussed rather
> than rushing into the source tree
It's easy to point to commits you don't like and say "this should have
been discussed", but actually we don't discuss every minor change that
is made to the tree. I think this is really a small change (both in
scope and in size) and not one I expected any reasonable objections to.
> since it adding new interfaces which
> have to be supported in future.
Are you really saying that there will be a support burden from these few
lines of code, that don't introduce any functional changes to the
application apart from performance?
Kris
More information about the cvs-all
mailing list