New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use process-local futex operations #58

Open
wants to merge 1 commit into
base: tbb_2018
from

Conversation

Projects
None yet
2 participants
@Lastique

Lastique commented May 15, 2018

The FUTEX_WAIT and FUTEX_WAKE operations are supposed to be applied to futexes that can be shared between processes. Using process-local equivalents can be more efficient.

Also add linux/futex.h include, which should nowdays be provided by all relevant Linux distributions.

@Lastique

This comment has been minimized.

Lastique commented May 15, 2018

This fixes #57.

@@ -34,15 +34,19 @@
#define __TBB_USE_FUTEX 1
#include <limits.h>
#include <errno.h>
// Unfortunately, some versions of Linux do not have a header that defines FUTEX_WAIT and FUTEX_WAKE.
#include <linux/futex.h>

This comment has been minimized.

@alexey-katranov

alexey-katranov Jun 1, 2018

By historical reason, machine/linux_common.h is included for *BSD systems in machine.h. These systems may not have the linux/futex.h header file (e.g. they may provide sys/futex.h). I would recommend to include linux/futex.h only when __linux__ is defined (in other words, to guard it with if __linux__ ... endif).

This comment has been minimized.

@Lastique

Lastique Jun 1, 2018

I was under impression that futexes are Linux-specific and expected SYS_futex to be not defined on other systems. However, it looks like OpenBSD supports them, but it is not covered by the conditions in tbb_machine.h.

I can add conditions for Linux and OpenBSD, but I will also have to disable the futex-related functions below, which presumably should not be defined when futexes are not available.

@Lastique Lastique force-pushed the Lastique:patch-1 branch from 2b2d466 to 9a666ed Jun 1, 2018

@Lastique

This comment has been minimized.

Lastique commented Jun 1, 2018

I've updated the PR.

@@ -227,7 +227,7 @@ template<> struct atomic_selector<8> {
#include "machine/linux_intel64.h"
#endif
#elif __linux__ || __FreeBSD__ || __NetBSD__
#elif __linux__ || __FreeBSD__ || __NetBSD__ || __OpenBSD__

This comment has been minimized.

@alexey-katranov

alexey-katranov Jun 2, 2018

Is || __OpenBSD__ enough to build TBB on OpenBSD (and without this improvement the build fails)?

This comment has been minimized.

@Lastique

Lastique Jun 2, 2018

I did not test OpenBSD, I don't have access to the system. The reason I added this check is that OpenBSD supports futexes, and the other code in the guarded headers doesn't look system-specific.

#endif
#endif
#if __TBB_USE_FUTEX

This comment has been minimized.

@alexey-katranov

alexey-katranov Jun 2, 2018

I suppose the logic with __TBB_USE_FUTEX is surplus because all this code is guarded with the SYS_futex macro (i.e. the futex syscall is supported). It is Ok if no one header file is included because the values of __TBB_FUTEX_WAIT and __TBB_FUTEX_WAKE are hardcoded to 0 and 1 respectively in that case.

This comment has been minimized.

@Lastique

Lastique Jun 2, 2018

I actually don't like that the macros are defined to some specific values in TBB because the values are obviously system-dependent. Including the header provides these system values.

This comment has been minimized.

@alexey-katranov

alexey-katranov Jun 2, 2018

I agree that using headers is the best approach and your patch is worth contributing. However, if TBB is used on the system where the introduced logic will not find the required header file, disabling hardcoded behavior will break backward compatibility.
My suggestion is to include header files where possible (where we know what to include) and not to disable the old behavior for the sake of backward compatibility.

This comment has been minimized.

@Lastique

Lastique Jun 2, 2018

Ok, will do.

Use process-local futex operations
The FUTEX_WAIT and FUTEX_WAKE operations are supposed to be applied to futexes
that can be shared between processes. Using process-local equivalents can be
more efficient.

Also add linux/futex.h include, which should nowdays be provided by all
relevant Linux distributions. Including the system header ensures that futex
operation macros are defined. Since linux_common.h is also included on various
BSD systems, include sys/futex.h where available and rely on our local
definitions where no system header is available.

@Lastique Lastique force-pushed the Lastique:patch-1 branch from 9a666ed to f288897 Jun 2, 2018

@Lastique

This comment has been minimized.

Lastique commented Jun 2, 2018

I've updated the patch to support systems with no futex.h.

@alexey-katranov

Thank you for the contribution. The patch will be merged into one of the future updates.

#define __TBB_USE_FUTEX 1
#if defined(__linux__)

This comment has been minimized.

@alexey-katranov

alexey-katranov Jun 5, 2018

We will slightly refactor this part of the code, à la:

#if defined(__has_include)
#define __TBB_has_include __has_include
#else
#define __TBB_has_include(x) 0
#endif

#if defined(__linux__) || __TBB_has_include(<linux/futex.h>)
#include <linux/futex.h>
#elif defined(__OpenBSD__) || __TBB_has_include(<sys/futex.h>)
#include <sys/futex.h>
#endif
@Lastique

This comment has been minimized.

Lastique commented Sep 17, 2018

Any news about this?

@alexey-katranov

This comment has been minimized.

alexey-katranov commented Sep 18, 2018

It turned out that this PR had low priority. However, we have not forgotten about it and want to merge the patch into one of the future updates. Sorry for the delay.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment