[Bug nptl/16630] New: Use SYSENTER for pthread_cond_broadcast/signal() (i.e. fix "FIXME: Ingo" issue)

[Bug nptl/16630] New: Use SYSENTER for pthread_cond_broadcast/signal() (i.e. fix "FIXME: Ingo" issue)

[cjones.bugs at gmail dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=16630

            Bug ID: 16630
           Summary: Use SYSENTER for pthread_cond_broadcast/signal() (i.e.
                    fix "FIXME: Ingo" issue)
           Product: glibc
           Version: unspecified
            Status: NEW
          Severity: normal
          Priority: P2
         Component: nptl
          Assignee: unassigned at sourceware dot org
          Reporter: cjones.bugs at gmail dot com
                CC: drepper.fsp at gmail dot com

Created attachment 7437
  --> https://sourceware.org/bugzilla/attachment.cgi?id=7437&action=edit
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.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug nptl/16630] Use SYSENTER for pthread_cond_broadcast/signal() (i.e. fix "FIXME: Ingo" issue)

[bugdal at aerifal dot cx]

https://sourceware.org/bugzilla/show_bug.cgi?id=16630

Rich Felker <bugdal at aerifal dot cx> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |bugdal at aerifal dot cx

--- Comment #1 from Rich Felker <bugdal at aerifal dot cx> ---
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.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug nptl/16630] Use SYSENTER for pthread_cond_broadcast/signal() (i.e. fix "FIXME: Ingo" issue)

[cjones.bugs at gmail dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=16630

--- Comment #2 from Chris Jones <cjones.bugs at gmail dot com> ---
(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.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug nptl/16630] Use SYSENTER for pthread_cond_broadcast/signal() (i.e. fix "FIXME: Ingo" issue)

[bugdal at aerifal dot cx]

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.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug nptl/16630] Use SYSENTER for pthread_cond_broadcast/signal() (i.e. fix "FIXME: Ingo" issue)

[cjones.bugs at gmail dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=16630

--- Comment #4 from Chris Jones <cjones.bugs at gmail dot com> ---
I see, thanks.

How do we a arrive a decision on whether to take this trivial patch?

-- 
You are receiving this mail because:
You are on the CC list for the bug.

Re: [Bug nptl/16630] Use SYSENTER for pthread_cond_broadcast/signal() (i.e. fix "FIXME: Ingo" issue)

[Ondřej Bílka]

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.

[Bug nptl/16630] Use SYSENTER for pthread_cond_broadcast/signal() (i.e. fix "FIXME: Ingo" issue)

[neleai at seznam dot cz]

http://sourceware.org/bugzilla/show_bug.cgi?id=16630

--- Comment #5 from Ondrej Bilka <neleai at seznam dot cz> ---
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.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug nptl/16630] Use SYSENTER for pthread_cond_broadcast/signal() (i.e. fix "FIXME: Ingo" issue)

[cjones.bugs at gmail dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=16630

--- Comment #6 from Chris Jones <cjones.bugs at gmail dot com> ---
If the maintainers of this code don't want to take this patch, please resolve
this bug WONTFIX to get it off our plates.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug nptl/16630] Use SYSENTER for pthread_cond_broadcast/signal() (i.e. fix "FIXME: Ingo" issue)

[bugdal at aerifal dot cx]

https://sourceware.org/bugzilla/show_bug.cgi?id=16630

--- Comment #7 from Rich Felker <bugdal at aerifal dot cx> ---
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.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug nptl/16630] Use SYSENTER for pthread_cond_broadcast/signal() (i.e. fix "FIXME: Ingo" issue)

[cjones.bugs at gmail dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=16630

--- Comment #8 from Chris Jones <cjones.bugs at gmail dot com> ---
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 :).

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug nptl/16630] Use SYSENTER for pthread_cond_broadcast/signal() (i.e. fix "FIXME: Ingo" issue)

[bugdal at aerifal dot cx]

https://sourceware.org/bugzilla/show_bug.cgi?id=16630

--- Comment #9 from Rich Felker <bugdal at aerifal dot cx> ---
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.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug nptl/16630] Use SYSENTER for pthread_cond_broadcast/signal() (i.e. fix "FIXME: Ingo" issue)

[fweimer at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=16630

Florian Weimer <fweimer at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
              Flags|                            |security-

-- 
You are receiving this mail because:
You are on the CC list for the bug.