[PATCH]: robust futexes

[Roman Divacky]

On Wed, Apr 30, 2008 at 12:15:13PM -0400, Alexander Kabaev wrote:
> On Wed, 30 Apr 2008 10:18:06 +0200
> Roman Divacky <rdivacky at freebsd.org> wrote:
> 
> > hi
> > 
> > I implemented robust futexes in linuxulator and I need to get it
> > reviewed/tested. The best way to test it is (according to linux
> > documnetation) to run yum and kill -9 it while it runs. 
> > 
> > The patch is here:
> > http://www.vlakno.cz/~rdivacky/linux_robust_futex.patch
> > 
> > the patch should be ok as I followed linux code very closely (most of
> > the code runs in userspace so kernel has very well defined work). I
> > tested it lightly on i386.
> > 
> > I'd like to commit this quite soon so please help.
> > 
> > thnx!
> > 
> > roman
> Hi,
> 
> some comments:
> 
> linux_emul.c:
> 
> @@ -86,6 +86,7 @@
>  		em = malloc(sizeof *em, M_LINUX, M_WAITOK | M_ZERO);
>  		em->pid = child;
>  		em->pdeath_signal = 0;
> +		em->robust_futexes = NULL;
> 
> M_ZERO is not quite zero enough? :)
 
I prefer it to be initialized so people can see immediately that its '0/NULL'
dont think it matters much :) if this penalizes fork() by more than 10% I'll
remove that :-D

> linux_futex.c in release_futexes:
> 
> +	head = em->robust_futexes;
> +
> +	if (fetch_robust_entry(&entry, &head->list.next, &pi))
> +		return;
> 
> Aren't you taking a fault in copyin unconditionally if
> em->robust_mutexes happens to be NULL? Why not check is for NULL first?

I think it can be done this way and it makes sense... I wonder why linux
does not check it, they usually optimize everything... 
 
> Also, is sched_relinguish really necessary after each each futex
> recovery _except_ from the 'pending' futex one?

linux calls cond_resched() in this place and I believe there's a reason for this.
I dont know much what the userspace part is doing but I believe it makes
sense to reschedule after each futex recovery so other threads can "do stuff".

anyway.. this is what linux does and I believe we should stick to it. Do you have
any particular reason why you mind this?
 
> i386/conf/GENERIC:
> 
> Does not belong in this patch, probably included in by mistake.

yes.. this is included by mistake..

thnx a lot for the review! an updated patch can be found at:

	www.vlakno.cz/~rdivacky/linux_robust_futex.2.patch

roman