[PATCH] unify the use of FUTEX_PRIVATE

[PATCH] unify the use of FUTEX_PRIVATE

[Jens Gustedt]

The flag 1<<7 is used in several places for different purposes that are
not always easy to distinguish. Mark those usages that correspond to the
flag that is used by the kernel for futexes.
---
 src/internal/pthread_impl.h         | 2 +-
 src/thread/__timedwait.c            | 2 +-
 src/thread/pthread_barrier_wait.c   | 2 +-
 src/thread/pthread_cond_timedwait.c | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/internal/pthread_impl.h b/src/internal/pthread_impl.h
index 757b86ad..5734a4e9 100644
--- a/src/internal/pthread_impl.h
+++ b/src/internal/pthread_impl.h
@@ -127,7 +127,7 @@ int __timedwait_cp(volatile int *, int, clockid_t, const struct timespec *, int)
 void __wait(volatile int *, volatile int *, int, int);
 static inline void __wake(volatile void *addr, int cnt, int priv)
 {
-	if (priv) priv = 128;
+	if (priv) priv = FUTEX_PRIVATE;
 	if (cnt<0) cnt = INT_MAX;
 	__syscall(SYS_futex, addr, FUTEX_WAKE|priv, cnt) != -ENOSYS ||
 	__syscall(SYS_futex, addr, FUTEX_WAKE, cnt);
diff --git a/src/thread/__timedwait.c b/src/thread/__timedwait.c
index 13d8465a..d2079c88 100644
--- a/src/thread/__timedwait.c
+++ b/src/thread/__timedwait.c
@@ -14,7 +14,7 @@ int __timedwait_cp(volatile int *addr, int val,
 	int r;
 	struct timespec to, *top=0;
 
-	if (priv) priv = 128;
+	if (priv) priv = FUTEX_PRIVATE;
 
 	if (at) {
 		if (at->tv_nsec >= 1000000000UL) return EINVAL;
diff --git a/src/thread/pthread_barrier_wait.c b/src/thread/pthread_barrier_wait.c
index 06b83db9..cc2a8bbf 100644
--- a/src/thread/pthread_barrier_wait.c
+++ b/src/thread/pthread_barrier_wait.c
@@ -84,7 +84,7 @@ int pthread_barrier_wait(pthread_barrier_t *b)
 			a_spin();
 		a_inc(&inst->finished);
 		while (inst->finished == 1)
-			__syscall(SYS_futex,&inst->finished,FUTEX_WAIT|128,1,0) != -ENOSYS
+			__syscall(SYS_futex,&inst->finished,FUTEX_WAIT|FUTEX_PRIVATE,1,0) != -ENOSYS
 			|| __syscall(SYS_futex,&inst->finished,FUTEX_WAIT,1,0);
 		return PTHREAD_BARRIER_SERIAL_THREAD;
 	}
diff --git a/src/thread/pthread_cond_timedwait.c b/src/thread/pthread_cond_timedwait.c
index 3526ecfb..ed8569c2 100644
--- a/src/thread/pthread_cond_timedwait.c
+++ b/src/thread/pthread_cond_timedwait.c
@@ -54,7 +54,7 @@ static inline void unlock_requeue(volatile int *l, volatile int *r, int w)
 {
 	a_store(l, 0);
 	if (w) __wake(l, 1, 1);
-	else __syscall(SYS_futex, l, FUTEX_REQUEUE|128, 0, 1, r) != -ENOSYS
+	else __syscall(SYS_futex, l, FUTEX_REQUEUE|FUTEX_PRIVATE, 0, 1, r) != -ENOSYS
 		|| __syscall(SYS_futex, l, FUTEX_REQUEUE, 0, 1, r);
 }
 
-- 
2.11.0

Re: [PATCH] unify the use of FUTEX_PRIVATE

[Szabolcs Nagy]

* Jens Gustedt <Jens.Gustedt@inria.fr> [2017-06-24 10:18:05 +0200]:
> The flag 1<<7 is used in several places for different purposes that are
> not always easy to distinguish. Mark those usages that correspond to the
> flag that is used by the kernel for futexes.
> ---
>  src/internal/pthread_impl.h         | 2 +-
>  src/thread/__timedwait.c            | 2 +-
>  src/thread/pthread_barrier_wait.c   | 2 +-
>  src/thread/pthread_cond_timedwait.c | 2 +-
>  4 files changed, 4 insertions(+), 4 deletions(-)
> 

i see further use of 128

src/thread/pthread_attr_get.c:  *pshared = a->__attr / 128U % 2;
src/thread/pthread_cond_timedwait.c:            unlock_requeue(&node.prev->barrier, &m->_m_lock, m->_m_type & 128);
src/thread/pthread_create.c:            int priv = (m->_m_type & 128) ^ 128;
src/thread/pthread_mutex_timedlock.c:   int r, t, priv = (m->_m_type & 128) ^ 128;
src/thread/pthread_mutex_trylock.c:     if (m->_m_type & 128) {
src/thread/pthread_mutex_unlock.c:      int priv = (m->_m_type & 128) ^ 128;
src/thread/pthread_mutexattr_setpshared.c:      a->__attr &= ~128U;
src/thread/pthread_rwlock_init.c:       if (a) rw->_rw_shared = a->__attr[0]*128;
src/thread/pthread_rwlock_timedrdlock.c:                r = __timedwait(&rw->_rw_lock, t, CLOCK_REALTIME, at, rw->_rw_shared^128);
src/thread/pthread_rwlock_timedwrlock.c:                r = __timedwait(&rw->_rw_lock, t, CLOCK_REALTIME, at, rw->_rw_shared^128);
src/thread/pthread_rwlock_unlock.c:     int val, cnt, waiters, new, priv = rw->_rw_shared^128;
src/thread/sem_init.c:  sem->__val[2] = pshared ? 0 : 128;

i think some of these are the same flag

Re: [PATCH] unify the use of FUTEX_PRIVATE

[Jens Gustedt]

Am 24. Juni 2017 18:40:26 MESZ schrieb Szabolcs Nagy <nsz@port70.net>:
>* Jens Gustedt <Jens.Gustedt@inria.fr> [2017-06-24 10:18:05 +0200]:
>> The flag 1<<7 is used in several places for different purposes that
>are
>> not always easy to distinguish. Mark those usages that correspond to
>the
>> flag that is used by the kernel for futexes.
>> ---
>>  src/internal/pthread_impl.h         | 2 +-
>>  src/thread/__timedwait.c            | 2 +-
>>  src/thread/pthread_barrier_wait.c   | 2 +-
>>  src/thread/pthread_cond_timedwait.c | 2 +-
>>  4 files changed, 4 insertions(+), 4 deletions(-)
>> 
>
>i see further use of 128
>
>src/thread/pthread_attr_get.c:  *pshared = a->__attr / 128U % 2;
>src/thread/pthread_cond_timedwait.c:           
>unlock_requeue(&node.prev->barrier, &m->_m_lock, m->_m_type & 128);
>src/thread/pthread_create.c:            int priv = (m->_m_type & 128) ^
>128;
>src/thread/pthread_mutex_timedlock.c:   int r, t, priv = (m->_m_type &
>128) ^ 128;
>src/thread/pthread_mutex_trylock.c:     if (m->_m_type & 128) {
>src/thread/pthread_mutex_unlock.c:      int priv = (m->_m_type & 128) ^
>128;
>src/thread/pthread_mutexattr_setpshared.c:      a->__attr &= ~128U;
>src/thread/pthread_rwlock_init.c:       if (a) rw->_rw_shared =
>a->__attr[0]*128;
>src/thread/pthread_rwlock_timedrdlock.c:                r =
>__timedwait(&rw->_rw_lock, t, CLOCK_REALTIME, at, rw->_rw_shared^128);
>src/thread/pthread_rwlock_timedwrlock.c:                r =
>__timedwait(&rw->_rw_lock, t, CLOCK_REALTIME, at, rw->_rw_shared^128);
>src/thread/pthread_rwlock_unlock.c:     int val, cnt, waiters, new,
>priv = rw->_rw_shared^128;
>src/thread/sem_init.c:  sem->__val[2] = pshared ? 0 : 128;
>
>i think some of these are the same flag

I don't think so. 
For mutexes the sense is "shared" so just the opposite meaning, and for rwlock,  too, it seems. 
For sem it is just a screwed Boolean..

I would merely tend to have different names for them, but that's a bit invasive.

Jens 


-- 
Jens Gustedt - INRIA & ICube, Strasbourg, France

Re: [PATCH] unify the use of FUTEX_PRIVATE

[Jens Gustedt]

[-- Attachment #1: Type: text/plain, Size: 1970 bytes --]

Hello,

On Sat, 24 Jun 2017 19:51:27 +0200 Jens Gustedt <jens.gustedt@inria.fr>
wrote:

> Am 24. Juni 2017 18:40:26 MESZ schrieb Szabolcs Nagy <nsz@port70.net>:
> >i think some of these are the same flag  
> 
> I don't think so. 
> For mutexes the sense is "shared" so just the opposite meaning, and
> for rwlock,  too, it seems. For sem it is just a screwed Boolean..
> 
> I would merely tend to have different names for them, but that's a
> bit invasive.

We could do something like

/* Shared and private flags for different control structures use the
same bit to favor optimization. */
/* for mutex, mutexattr and rwlock */
#define PSHARED FUTEX_PRIVATE
/* for sem */
#define PRIVATE FUTEX_PRIVATE

But then it could also be confusing that condattr uses bit 31 for the
same purpose, and pthread_cond_t uses a pointer type. This could lead
to

/* Some shared and private flags for different control structures use
the same bit to favor optimization. */
#define MTX_SHARED   FUTEX_PRIVATE
#define MATR_SHARED  FUTEX_PRIVATE
#define RWL_SHARED   FUTEX_PRIVATE
#define RWATR_SHARED 1
#define CND_SHARED   (void*)-1
#define CATR_SHARED  (1<<31)
/* Maybe replace by 1? */
#define SEM_PRIVATE  FUTEX_PRIVATE

To set the bit, in some places we use bit shift, in some
multiplication with 128 or 128U. To read the bit some do just masking
and then use the result as a "Boolean", some use bit shift and then
%2 to filter out the bit.

Probably I still overlooked some usages, and the naming could still be
improved.

I could try to produce a patch in that sense, if there is consensus to
go in that direction.

Thanks
Jens

-- 
:: INRIA Nancy Grand Est ::: Camus ::::::: ICube/ICPS :::
:: ::::::::::::::: office Strasbourg : +33 368854536   ::
:: :::::::::::::::::::::: gsm France : +33 651400183   ::
:: ::::::::::::::: gsm international : +49 15737185122 ::
:: http://icube-icps.unistra.fr/index.php/Jens_Gustedt ::

[-- Attachment #2: Digitale Signatur von OpenPGP --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH] unify the use of FUTEX_PRIVATE

[Rich Felker]

On Sat, Jun 24, 2017 at 11:31:04PM +0200, Jens Gustedt wrote:
> Hello,
> 
> On Sat, 24 Jun 2017 19:51:27 +0200 Jens Gustedt <jens.gustedt@inria.fr>
> wrote:
> 
> > Am 24. Juni 2017 18:40:26 MESZ schrieb Szabolcs Nagy <nsz@port70.net>:
> > >i think some of these are the same flag  
> > 
> > I don't think so. 
> > For mutexes the sense is "shared" so just the opposite meaning, and
> > for rwlock,  too, it seems. For sem it is just a screwed Boolean..
> > 
> > I would merely tend to have different names for them, but that's a
> > bit invasive.
> 
> We could do something like
> 
> /* Shared and private flags for different control structures use the
> same bit to favor optimization. */
> /* for mutex, mutexattr and rwlock */
> #define PSHARED FUTEX_PRIVATE
> /* for sem */
> #define PRIVATE FUTEX_PRIVATE
> 
> But then it could also be confusing that condattr uses bit 31 for the
> same purpose, and pthread_cond_t uses a pointer type. This could lead
> to
> 
> /* Some shared and private flags for different control structures use
> the same bit to favor optimization. */
> #define MTX_SHARED   FUTEX_PRIVATE
> #define MATR_SHARED  FUTEX_PRIVATE
> #define RWL_SHARED   FUTEX_PRIVATE
> #define RWATR_SHARED 1
> #define CND_SHARED   (void*)-1
> #define CATR_SHARED  (1<<31)
> /* Maybe replace by 1? */
> #define SEM_PRIVATE  FUTEX_PRIVATE
> 
> To set the bit, in some places we use bit shift, in some
> multiplication with 128 or 128U. To read the bit some do just masking
> and then use the result as a "Boolean", some use bit shift and then
> %2 to filter out the bit.
> 
> Probably I still overlooked some usages, and the naming could still be
> improved.
> 
> I could try to produce a patch in that sense, if there is consensus to
> go in that direction.

For now let's just use a macro for FUTEX_PRIVATE. My concern was
mainly about whether there are cases where it's assumed that the 128
or ^128 translates to FUTEX_PRIVATE by arithmetic ops. The __wait,
__timedwait, and __wake interfaces just treat their argument as a
boolean now, so I think the only places there might be remaining
assumptions of that sort is where a flag bit from the sync object is
passed directly into a futex __syscall -- places like the condvar
implementation. You've looked at all this code just now so you
probably have a better idea if there are any such places; if so I
think the assumption that the bits match up should just be replaced by
a conditional (e.g. flags & 128 ? 0 : FUTEX_PRIVATE or such).

Rich

Re: [PATCH] unify the use of FUTEX_PRIVATE

[Jens Gustedt]

[-- Attachment #1: Type: text/plain, Size: 1731 bytes --]

Hello Rich,

On Sat, 24 Jun 2017 18:16:30 -0400 Rich Felker <dalias@libc.org> wrote:

> For now let's just use a macro for FUTEX_PRIVATE.

Ok, was my guess :)

> My concern was
> mainly about whether there are cases where it's assumed that the 128
> or ^128 translates to FUTEX_PRIVATE by arithmetic ops. The __wait,
> __timedwait, and __wake interfaces just treat their argument as a
> boolean now, so I think the only places there might be remaining
> assumptions of that sort is where a flag bit from the sync object is
> passed directly into a futex __syscall -- places like the condvar
> implementation. You've looked at all this code just now so you
> probably have a better idea if there are any such places;

All the usages that I found go through a Boolean operation. The only
place where -maybe- the compiler might shortcut some of it is __wake
because it is inlined. In any case, by just using the macro the
compiler see's the same 128.

> if so I
> think the assumption that the bits match up should just be replaced by
> a conditional (e.g. flags & 128 ? 0 : FUTEX_PRIVATE or such).

There is none in the arch independent code, so you could go with my
patch as it is, I think.

I only found one other place, but which I don't understand (and I am
not sure that I want to :). This is in
src/thread/x32/syscall_cp_fixup.c
where there is a special casing for SYS_futex

Thanks
Jens

-- 
:: INRIA Nancy Grand Est ::: Camus ::::::: ICube/ICPS :::
:: ::::::::::::::: office Strasbourg : +33 368854536   ::
:: :::::::::::::::::::::: gsm France : +33 651400183   ::
:: ::::::::::::::: gsm international : +49 15737185122 ::
:: http://icube-icps.unistra.fr/index.php/Jens_Gustedt ::

[-- Attachment #2: Digitale Signatur von OpenPGP --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH] unify the use of FUTEX_PRIVATE

[Rich Felker]

On Sun, Jun 25, 2017 at 09:29:51AM +0200, Jens Gustedt wrote:
> Hello Rich,
> 
> On Sat, 24 Jun 2017 18:16:30 -0400 Rich Felker <dalias@libc.org> wrote:
> 
> > For now let's just use a macro for FUTEX_PRIVATE.
> 
> Ok, was my guess :)
> 
> > My concern was
> > mainly about whether there are cases where it's assumed that the 128
> > or ^128 translates to FUTEX_PRIVATE by arithmetic ops. The __wait,
> > __timedwait, and __wake interfaces just treat their argument as a
> > boolean now, so I think the only places there might be remaining
> > assumptions of that sort is where a flag bit from the sync object is
> > passed directly into a futex __syscall -- places like the condvar
> > implementation. You've looked at all this code just now so you
> > probably have a better idea if there are any such places;
> 
> All the usages that I found go through a Boolean operation. The only
> place where -maybe- the compiler might shortcut some of it is __wake
> because it is inlined. In any case, by just using the macro the
> compiler see's the same 128.
> 
> > if so I
> > think the assumption that the bits match up should just be replaced by
> > a conditional (e.g. flags & 128 ? 0 : FUTEX_PRIVATE or such).
> 
> There is none in the arch independent code, so you could go with my
> patch as it is, I think.

Yes, I agree. Applying it. Thanks!

> I only found one other place, but which I don't understand (and I am
> not sure that I want to :). This is in
> src/thread/x32/syscall_cp_fixup.c
> where there is a special casing for SYS_futex

This could also be changed, but being that it's arch-specific and not
something people would read when trying to understand the code, I
don't think it really matters. We can change it in another commit if
you really want to.

Rich