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