Date: Tue, 30 Oct 2007 11:04:57 +0100 From: Kris Kennaway <kris@FreeBSD.org> To: David Xu <davidxu@FreeBSD.org> Cc: Daniel Eischen <deischen@FreeBSD.org>, cvs-all@FreeBSD.org Subject: Re: cvs commit: src/lib/libthr/thread thr_mutex.c src/lib/libkse/thread thr_mutex.c src/include pthread.h Message-ID: <472701C9.4020208@FreeBSD.org> In-Reply-To: <4726F3C7.2060506@freebsd.org> References: <200710292101.l9TL1mAE049561@repoman.freebsd.org> <47268F17.1000106@freebsd.org> <Pine.GSO.4.64.0710292207140.19572@sea.ntplx.net> <47269AD0.3080906@freebsd.org> <4726EEFB.8030309@FreeBSD.org> <4726F3C7.2060506@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?472701C9.4020208>