[robust-futex-3] futex: robust futex support

[robust-futex-3] futex: robust futex support

Post by david sing » Wed, 18 Jan 2006 11:40:07



-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to XXXX@XXXXX.COM
More majordomo info at http://www.yqcomputer.com/
Please read the FAQ at http://www.yqcomputer.com/





Ulrich,
I've fixed another memory leak in free_robust_list. The entries in
the slab caches
now look correct through the full test suite up to 7500 threads. Does
your glibc
work correctly with this patch?


diff -u linux-2.6.15/kernel/futex.c linux-2.6.15/kernel/futex.c
--- linux-2.6.15/kernel/futex.c
+++ linux-2.6.15/kernel/futex.c
@@ -917,6 +917,8 @@
up(&mapping->robust_head->robust_sem);

kmem_cache_free(file_futex_cachep, futex_head);
+ mapping->robust_head = NULL;
+
return;
}


The new full patch is attached, and at

http://www.yqcomputer.com/ ~dsingleton/robustutex-3

David
 
 
 

[robust-futex-3] futex: robust futex support

Post by Ulrich Dre » Thu, 19 Jan 2006 02:40:22


I'll see shortly.

But looking at the patch, I don't understand the use of
FUTEX_ATTR_SHARED. The EADDRNOTAVAIL error is something the kernel
has to return if the address is not that of an object in a shared
memory region. It's not information directly provided by the user of
futex_register.

So, I suggest removing the attr parameter from futex_register and
after get_futex_key, when you know where the futex is actually
located, return -EADDRNOTAVAIL if the futex is in private memory.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to XXXX@XXXXX.COM
More majordomo info at http://www.yqcomputer.com/
Please read the FAQ at http://www.yqcomputer.com/

 
 
 

[robust-futex-3] futex: robust futex support

Post by Ulrich Dre » Thu, 19 Jan 2006 03:01:24

And another thing: semaphores are on their way out. So, in
futex_deregister and in futex_head, shouldn't you use mutexes? I
don't see that you realy need semaphores.

In futex_register, you define mm and initialize it with current->mm.
That's OK. But why then are you using

+ down_read(¤t->mm->mmap_sem);

just a few lines below?

And finally (for now): in get_futex_key the VMA containing the futex
is determined. And yet, in futex_register you have an identical
find_extend_vma call. I don't know how expensive this function is.
But I would assume that at least the error handling in futex_register
can go away since the VMA cannot be torn down while mmap_sem is taken,
right? But perhaps this just points to more inconsistencies. Why is
the list/sem lookup in get_futex_key? Only to share the code with
futex_deregister. But is that really worth it? The majority of calls
to get_futex_key come from all the other call sites so the code you
added is only a cost without any gain. Especially since you could in
futex_register do the whole thing without any additional cost and
because most of the new tests in get_futex_key are again tested in
futex_register (to determined shared vs non-shared) and do not have to
be tested in futex_deregister (we know the futex is in shared memory).

I suggest that if find_extend_vma is sufficiently expensive, pass a
pointer to a variable of that type to get_futex_key. If it is cheap,
don't do anything. Pull the new code in get_futex_key into
futex_register and futex_deregister, optimize out unnecessary code,
and merge with the rest of the functions. It'll be much less
invasive.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to XXXX@XXXXX.COM
More majordomo info at http://www.yqcomputer.com/
Please read the FAQ at http://www.yqcomputer.com/