Created attachment 7437 [details] Use SYSENTER for pthread_cond_broadcast/signal(). On x86, pthread_cond_broadcast/signal() enter the kernel using |int $0x80|, apparently because there was a time in the distant past when 6-arg syscalls couldn't made through __kernel_vsyscall(): /* FIXME: Until Ingo fixes 4G/4G vDSO, 6 arg syscalls are broken for sysenter. ENTER_KERNEL */ This bug has been fixed, so we can update glibc. One motivation for landing this patch is that some applications may see a perf improvement from using the faster syscall interface. pthread_cond_broadcast/signal() are frequently used by some applications. (Another motivation for this patch is rather unsavory so I hesitate to mention it. Some naughty programs may want to hook __kernel_vsyscall() for their own purposes, and the |int $0x80| hack prevents these futex calls from being seen by those kind of hooks.) I apologize for not knowing the glibc patch protocol. Please let me know if I need to request review from someone.
Ideally this asm will be removed at some point; I think that would resolve this issue automatically. BTW, the idea of letting apps hook __kernel_vsyscall is a very bad one. In order to fix issue #12683, use of __kernel_vsyscall needs to be abandoned for cancellable syscalls. Treating such hooking as a supported usage would preclude fixing a serious open bug.
(In reply to Rich Felker from comment #1) > Ideally this asm will be removed at some point; I think that would resolve > this issue automatically. Do you happen to know if that's filed already? I'm curious what it would be replaced with. > > BTW, the idea of letting apps hook __kernel_vsyscall is a very bad one. In > order to fix issue #12683, use of __kernel_vsyscall needs to be abandoned > for cancellable syscalls. Treating such hooking as a supported usage would > preclude fixing a serious open bug. Oh, please don't let me give you the wrong idea; I'm not at all advocating for official support of hooking __kernel_vsyscall. What I do with __kernel_vsyscall stays in the privacy of my home ;). (Trust me, I don't want to be touching it either.) But please take this patch at face value.
On Tue, Feb 25, 2014 at 04:51:18AM +0000, cjones.bugs at gmail dot com wrote: > Do you happen to know if that's filed already? I'm curious what it would be > replaced with. There is portable C code for all of these functions already and a strong sentiment from some users and developers that the asm is unnecessary, error-prone, and lags behind the C in getting fixes and improvements. See the related thread on the libc-alpha mailing list: [RFC][BZ #16549, #16410] Remove pthread_(cond)wait assembly implementations? Basically I think if it could be demonstrated that the C performs just as well (or within a margin of difference that's not significant), the asm could be removed.
I see, thanks. How do we a arrive a decision on whether to take this trivial patch?
On Tue, Feb 25, 2014 at 05:47:16AM +0000, bugdal at aerifal dot cx wrote: > http://sourceware.org/bugzilla/show_bug.cgi?id=16630 > > --- Comment #3 from Rich Felker <bugdal at aerifal dot cx> --- > On Tue, Feb 25, 2014 at 04:51:18AM +0000, cjones.bugs at gmail dot com wrote: > > Do you happen to know if that's filed already? I'm curious what it would be > > replaced with. > > There is portable C code for all of these functions already and a > strong sentiment from some users and developers that the asm is > unnecessary, error-prone, and lags behind the C in getting fixes and > improvements. See the related thread on the libc-alpha mailing list: > > [RFC][BZ #16549, #16410] Remove pthread_(cond)wait assembly implementations? > > Basically I think if it could be demonstrated that the C performs just > as well (or within a margin of difference that's not significant), the > asm could be removed. > Actually c is around 5000 cycles faster. My guess is that its because assembly does extra syscall which has bigger impact than microoptimizations, I did not trace that yet.
If the maintainers of this code don't want to take this patch, please resolve this bug WONTFIX to get it off our plates.
Please be patient. The maintainers are in the process of evaluating whether the asm can just be removed, in which case the bug would be FIXED, not WONTFIX.
I don't mean that to sound impatient, I just want to emphasize that I'd prefer an explicit "no" through a WONTFIX than an implicit one through silence. I hate patches that die on the vine :).
I think it's likely that the patch submitted won't be needed (because the code will hopefully just be removed instead), but either way, it's not a "WONTFIX" and it would be inappropriate to mark it as such. It's just that we're waiting to see whether applying this patch (or some variant of it) or just removing the offending asm is the right solution.
The new condition variable does not use custom assembly code anymore but uses the generic syscall implementation (through that being used in the Linux futex wrappers), which should use sysenter if that's available. Thus, this should be fixed.