Re: [RFC] kern: simple futex for gnumach (version 5)

[Richard Braun]

On Thu, Dec 26, 2013 at 02:30:10PM +0100, Marin Ramesa wrote:
> +     futex->address = address;
> +     futex->num_futexed_threads = 0;
> +     futex->next_futex = NULL;
> +     futex->prev_futex = &(table->futex_elements[prev_hash_val]);
> +     futex->prev_futex->next_futex = futex;
> +     *futex->map = current_thread()->task->map;

A futex shouldn't be a task-local object since it's used for inter
process synchronization. There should be per-thread objects (probably
allocated on the stack since a thread can only be doing one wait at
a time), and these per-thread objects should be listed in the global
futex object. Waking up the futex then simply walks the list and wakes
up threads.

> +     if ((vm_map_lookup(futex->map, (vm_offset_t)address, VM_PROT_READ, 
> futex->version, futex->object,
> +                             futex->offset, futex->out_prot, futex->wired) 
> != KERN_SUCCESS)) {
> +             unsigned int temp_hash_val = futex_hash(address);
> +             __builtin_free(&(table->futex_elements[temp_hash_val]));

Why __builtin_malloc and __builtin_free ??

> +                     if (futex->num_futexed_threads == 128)
> +                             return FUTEX_RESOURCE_SHORTAGE;

I assume this limit is temporary. If not, remove it, there is no reason
to add a constraint like this one.

> +     suspend:
> +             assert_wait(&current_thread()->state , FALSE);
> +             simple_unlock(&futex->futex_wait_lock);
> +             kern_return_t ret = thread_suspend(current_thread());

I really doubt assert_wait can be used with thread_suspend and
thread_resume... Use thread_block and thread_wakeup instead. Again, be
more careful.

> +struct futex {

As previously said, rework that structure.

> +     /* Next futex in queue. */
> +     futex_t next_futex;
> +
> +     /* Previous futex in queue. */
> +     futex_t prev_futex;

Do NOT reimplement generic doubly-linked lists...

> +     /* Number of futexed threads at the same address. */
> +     unsigned int num_futexed_threads;
> +
> +     /* Array of futexed threads at the same address. */
> +     //thread_t futexed_threads;
> +
> +     thread_t futexed_threads[128];

Ugh. No.

> +int futex_init(futex_t futex, int *address);
> +extern int futex_wait(int *address, int value);
> +extern int futex_wake(int *address, boolean_t wake_all);
> +void futex_cross_address_space_wake(futex_t futex, boolean_t wake_all);
> +void futex_wake_threads(futex_t futex, boolean_t wake_all);
> +int futex_hash_table_init(void);
> +unsigned int futex_hash(int *address);
> +futex_t futex_hash_table_lookup_address(int *address);
> +int futex_hash_table_add_address(int *address);
> +void futex_hash_table_delete_futex(futex_t futex);

I know this isn't the traditional way to do it in Mach, but please,
extensively document the API in the header, e.g. as it's done for the
slab allocator. I also suggest moving everything not public (such as
the hash table) in the .c file, and if there is anything private that
still needs to be in a header, move that to a _i.h file ("i" could mean
internal/implementation/inline/whatever), so that the main header only
documents the public API.

-- 
Richard Braun