[patch 0/6] lightweight robust futexes: -V4

[patch 0/6] lightweight robust futexes: -V4

[Ingo Molnar]

This is release -V4 of the "lightweight robust futexes" patchset. The 
patchset can also be downloaded from:

  http://redhat.com/~mingo/lightweight-robust-futexes/

no big changes - docs updates and the tidying of futex_atomic_cmpxchg() 
semantics.

Changes since -V3:

 - added Documentation/robust-futex-ABI.txt (Paul Jackson)

 - fixed two mistakes in Documentation/robust-futexes.txt.
   (found by Paul Jackson)

 - added an explicit access_ok() check to the futex_atomic_cmpxchg()
   function. This is not needed in the place it's currently used (there 
   we have already validated the access_ok() range validity of the 
   userspace pointer), but it's good to do it nevertheless, just in case 
   the function gets used elsewhere in the futex code.

	Ingo

Re: [patch 0/6] lightweight robust futexes: -V4

[Jakub Jelinek]

On Tue, Feb 21, 2006 at 09:46:31AM +0100, Ingo Molnar wrote:
> 
> This is release -V4 of the "lightweight robust futexes" patchset. The 
> patchset can also be downloaded from:
> 
>   http://redhat.com/~mingo/lightweight-robust-futexes/
> 
> no big changes - docs updates and the tidying of futex_atomic_cmpxchg() 
> semantics.

To me the registering of thread's robust_list_head address is very
similar to registering thread's tid address and will be needed at the same
time (i.e. during program startup, when thread library is being initialized,
and upon thread creation), so I'd say we should register it the same
way as tid address and save a syscall per thread creation and a syscall
per process start.

TID address is registered through:
pid_t set_tid_address (int *tidptr)
syscall, so IMHO we should add a new syscall
pid_t set_tid_robust_addresses (int *tidptr, struct robust_list_head *robustptr)
which could register both tid and robust addresses.

For thread creation, we can just add CLONE_CHILD_SETROBUST clone flag
and if that flag is set, pass struct robust_list_head * as additional
argument.

The `len' argument (or really revision of the structure if really needed)
can be encoded in the structure, as in:
struct robust_list_head {
       struct robust_list list;
       short robust_list_head_len; /* or robust_list_head_version ? */
       short futex_offset;
       struct robust_list __user *list_op_pending;
};
or with long futex_offset, but using say upper 8 bits of the field as
version or length.

What do you think?

	Jakub

Re: [patch 0/6] lightweight robust futexes: -V4

[Ulrich Drepper]

On 2/21/06, Jakub Jelinek <jakub@redhat.com> wrote:
> TID address is registered through:
> pid_t set_tid_address (int *tidptr)
> syscall, so IMHO we should add a new syscall
> pid_t set_tid_robust_addresses (int *tidptr, struct robust_list_head *robustptr)
> which could register both tid and robust addresses.

The new syscall what certainly be used like this.  In fact, the two
syscalls happen exactly one after the other in my sources.  So I would
be in favor of making a change along these lines.  But instead of
fixing the interface in this way it should be extendable.  Pass a
structure and a flag value.  The latter specifies which elements of
the structure are used.  The structure could even grow over time.


> For thread creation, we can just add CLONE_CHILD_SETROBUST clone flag
> and if that flag is set, pass struct robust_list_head * as additional
> argument.

This is not necessary.  Especially because we already reached the
limit of parameters to clone.  A dedicated syscall to set up various
things like the TID pointer and the robust list is fine.


> The `len' argument (or really revision of the structure if really needed)
> can be encoded in the structure, as in:
> struct robust_list_head {
>        struct robust_list list;
>        short robust_list_head_len; /* or robust_list_head_version ? */
>        short futex_offset;
>        struct robust_list __user *list_op_pending;
> };
> or with long futex_offset, but using say upper 8 bits of the field as
> version or length.

I know you want to save SPARC but this kind of overloading I don't
really like.  If you need special treatment of the futex value make
this explicit and arch-dependent.

Re: [patch 0/6] lightweight robust futexes: -V4

[Jakub Jelinek]

On Tue, Feb 21, 2006 at 08:26:05AM -0800, Ulrich Drepper wrote:
> > The `len' argument (or really revision of the structure if really needed)
> > can be encoded in the structure, as in:
> > struct robust_list_head {
> >        struct robust_list list;
> >        short robust_list_head_len; /* or robust_list_head_version ? */
> >        short futex_offset;
> >        struct robust_list __user *list_op_pending;
> > };
> > or with long futex_offset, but using say upper 8 bits of the field as
> > version or length.
> 
> I know you want to save SPARC but this kind of overloading I don't
> really like.  If you need special treatment of the futex value make
> this explicit and arch-dependent.

This had nothing to do with SPARC actually, I only wanted to avoid
passing two extra arguments to clone rather than one.  But if you think
CLONE_CHILD_SETROBUST is unnecessary, so be it and the combined
set_tid_robust_address call can have tidptr, robustptr and robustlen
arguments.

	Jakub

Re: [patch 0/6] lightweight robust futexes: -V4

[Eric W. Biederman]

Jakub Jelinek <jakub@redhat.com> writes:

> On Tue, Feb 21, 2006 at 08:26:05AM -0800, Ulrich Drepper wrote:
>> > The `len' argument (or really revision of the structure if really needed)
>> > can be encoded in the structure, as in:
>> > struct robust_list_head {
>> >        struct robust_list list;
>> >        short robust_list_head_len; /* or robust_list_head_version ? */
>> >        short futex_offset;
>> >        struct robust_list __user *list_op_pending;
>> > };
>> > or with long futex_offset, but using say upper 8 bits of the field as
>> > version or length.
>> 
>> I know you want to save SPARC but this kind of overloading I don't
>> really like.  If you need special treatment of the futex value make
>> this explicit and arch-dependent.
>
> This had nothing to do with SPARC actually, I only wanted to avoid
> passing two extra arguments to clone rather than one.  But if you think
> CLONE_CHILD_SETROBUST is unnecessary, so be it and the combined
> set_tid_robust_address call can have tidptr, robustptr and robustlen
> arguments.

Not to be dense.  But can you actually measure the syscall overhead
you are trying to optimize out?

Especially where Ulrich was starting to ask for something that reminded
me of posix_spawn, I get a little nervous.  My gut feel is that 2
simple cheap syscalls, are comparable to one very configurable syscall.

I don't count cycles regularly so I don't know for certain, but lets
at least look before we leap.

Eric