Date: Thu, 1 May 2008 10:13:34 +0200 From: Roman Divacky <rdivacky@freebsd.org> To: Alexander Kabaev <kabaev@gmail.com> Cc: emulation@freebsd.org Subject: Re: [PATCH]: robust futexes Message-ID: <20080501081334.GA54624@freebsd.org> In-Reply-To: <20080430121513.33f9452b@kan.dnsalias.net> References: <20080430081806.GA81772@freebsd.org> <20080430121513.33f9452b@kan.dnsalias.net>
next in thread | previous in thread | raw e-mail | index | archive | help
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@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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20080501081334.GA54624>