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(¤t_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
