: robust futexes

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

On Wed, 30 Apr 2008 10:18:06 +0200
Roman Divacky <rdivackyfreebsd.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? 

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?

Also, is sched_relinguish really necessary after each each
futex
recovery _except_ from the 'pending' futex one?

i386/conf/GENERIC:

Does not belong in this patch, probably included in by
mistake.



-- 
Alexander Kabaev	

On Wed, Apr 30, 2008 at 12:15:13PM -0400, Alexander Kabaev
wrote:
> On Wed, 30 Apr 2008 10:18:06 +0200
> Roman Divacky <rdivackyfreebsd.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
_______________________________________________
freebsd-emulationfreebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-emu
lation
To unsubscribe, send any mail to
"freebsd-emulation-unsubscribefreebsd.org"