Re: [PATCH] unify the use of FUTEX_PRIVATE

[Rich Felker <dalias@libc.org>]

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