GNOME Bugzilla – Bug 741442
threads: use FUTEX_WAIT_PRIVATE and FUTEX_WAKE_PRIVATE if possible
Last modified: 2015-03-13 01:04:28 UTC
Created attachment 292610 [details] [review] threads: use FUTEX_WAIT_PRIVATE and FUTEX_WAKE_PRIVATE if possible This avoids some expensive code paths in the kernel, see http://lwn.net/Articles/229668/
Review of attachment 292610 [details] [review]: The linked article is a bit rambly, but if I get the gist, the idea is that this is a futex that only works in the current process? If so, then I'm definitely OK with this. This patch seems sane with only one minor point. See below. ::: glib/gthread-posix.c @@ +1241,3 @@ +#ifndef FUTEX_WAIT_PRIVATE +#define FUTEX_WAIT_PRIVATE FUTEX_WAIT I assume that because this is defined using a flag it will be backcompat to all kernel versions that have ever had futexes.... It might be nice to be more explicit about how these types are defined, and/or add a comment to that effect.
> The linked article is a bit rambly, but if I get the gist, the idea is that > this is a futex that only works in the current process? If so, then I'm > definitely OK with this. Yes, that is also my understanding, that the _PRIVATE part signals that this is only for the current process, which allows the kernel to skip some expensive code paths. > This patch seems sane with only one minor point. See below. > > ::: glib/gthread-posix.c > @@ +1241,3 @@ > > +#ifndef FUTEX_WAIT_PRIVATE > +#define FUTEX_WAIT_PRIVATE FUTEX_WAIT > > I assume that because this is defined using a flag it will be backcompat to all > kernel versions that have ever had futexes.... > > It might be nice to be more explicit about how these types are defined, and/or > add a comment to that effect. The idea here was that in the unlikely case that someone compiles against ancient kernel headers where FUTEX_WAIT_PRIVATE doesn't exist, we just fall back to the plain old FUTEX_WAIT. If we compile against kernel headers that define _WAIT_PRIVATE, this should still work with an older kernel which would hopefully just ignore the additional bit. You want me to add that as a comment above the #ifndef block?
Actually, looking at older kernel sources [*], I'm not sure it's true that this is backwards binary compatible, i.e. that older kernels will ignore the additional flag. But does it actually matter? Is anyone going to compile against newer kernel headers and then use a super-ancient kernel? Is that supposed to work at all anyway? [*] https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/kernel/futex.c?id=refs/tags/v2.6.12
(In reply to comment #3) > Actually, looking at older kernel sources [*], I'm not sure it's true that this > is backwards binary compatible, i.e. that older kernels will ignore the > additional flag. But does it actually matter? Is anyone going to compile > against newer kernel headers and then use a super-ancient kernel? Is that > supposed to work at all anyway? Ubuntu builds are chroots of new userspace often installed on machines with older kernels (typically from last LTS or even the one before). So yes -- people do indeed do that. I'm not sure we can use this if it's not backward compatible. Perhaps we could detect if it's supposed on the currently-running kernel and save it into some variable. We only ever hit the futex() calls on the slow path anyway, so it's not really going to hurt us... In fact, we could assume that it is supposed and trap the error (EINVAL?) we receive if it's not, marking it as unsupported for future attempts.
I think you'd have to go back all the way to dapper (6.06) to find a kernel that didn't support the extra flag. Are people really going to run the latest glib version on more than 7.5-year old kernels after compiling it against different kernel headers? It seems like a bit of a stretch to me. Furthermore, I wonder if glib would even build or work properly with those old kernels anyway, given that certain embedded vendors have claimed in the past that they can't upgrade GLib because it doesn't work with their oldish (but still new-enough for us) kernels for some reason (don't ask me why). Don't know if run-time detection is worth it. If I remember correctly from some benchmarking done a while back, the _WAIT_PRIVATE is slightly (measurably) faster for contended mutexes for small numbers of N (threads).
build.gnome.org is RHEL6.6 and uses chroots in which glib git master is run for supporting tools (g-compile-resources etc). The test suite however is run on a more modern kernel in a VM, for this exact reason, among many others. It looks like _WAIT_PRIVATE is in the kernel headers there though. I have a strong interest in ensuring glib can continue to function on RHEL6 for quite some time to come, and am willing to invest time to ensure it does. One option besides the runtime detection is a build-time option for minimal kernel version. That's what glibc does to define a baseline.
I didn't realise that we were talking 2007ware here. According to the LWN article this was going in circa 2.6.21-rc5-mm4. It looks like they made it in for 2.6.22. RHEL 6 has a 2.6.32 kernel. I think it's probably relatively safe to unconditionally depend on this stuff. RHEL 5 might be in trouble (kernel 2.6.18) but do we care?
(In reply to comment #7) > I think it's probably relatively safe to unconditionally depend on this stuff. > RHEL 5 might be in trouble (kernel 2.6.18) but do we care? I have no driver for modern glib on RHEL5 era systems myself.
So we don't care? The 1.5 people who will ever attempt to compile the latest glib (plus anything else required) on such an ancient system, will likely compile it against the old kernel headers (or against whatever kernel their using on that machine anyway), in which case everything will still work just fine.
Attachment 292610 [details] pushed as 627a145 - threads: use FUTEX_WAIT_PRIVATE and FUTEX_WAKE_PRIVATE if possible
Thanks for the poke, Tim :)