Re: SoC: linuxolator update: first patch

On Tue, Aug 15, 2006 at 09:10:50AM -0400, John Baldwin wrote:
> On Monday 14 August 2006 13:04, Divacky Roman wrote:
> > Hi,
> > 
> > I am a student doing SoC linuxolator update. The work involved updating
> > linuxolator to be able to work with 2.6.x (2.6.16 in my case) kernel emulation.
> > 
> > To be able to run 2.6.x emulation I had to implement NPTL, which means NPTL, futexes
> > and thread area, pid mangling + various hacks to make it work.
> > 
> > This is the first patch meant for public revision/testing:
> > 
> > 	www.stud.fit.vutbr.cz/~xdivac02/linuxolator26-diff.patch
> 
> Some comments:
> 
> - You shouldn't add new nested includes (such as in
>   amd64/linux32/linux.h), especially sys/lock.h, sys/mutex.h, and
>   sys/sx.h.  (If you need to examine mutex internals you would include
>   sys/_mutex.h in a header, but for normal API usage you need to
>   include sys/mutex.h and sys/lock.h in the appropriate C files.
>   sys/param.h is far too large of a header to be used in a nested
>   include.

1) fixed. thnx!

> - Check style(9).  For example, block comments should look like this:
> 
> 	/*
> 	 * This is a really long comment that takes up
> 	 * more than one line.
> 	 */
> 
>   rather than this:
> 
> 	/* this is a really long comment that takes up
> 	 * more than one line
> 	 */

2) fixed. thnx!

> - You use the EMUL_LOCK in em_find() to look at p_emuldata but don't
>   hold it when writing to p_emuldata in linux_proc_init().  There you
>   only hold the proc lock.

3) fixed. thnx!

> - You should probably hold the proc lock until after the wakeup()
>   in linux_proc_init() since the pfind() / proc_unlock() is what holds
>   a reference to keep the child from going away.

4) fixed. thnx!

> - In linux_proc_init() you pass EMUL_LOCKED to em_find() in the CLONE_VM
>   case when the lock isn't held.

5) fixed by fix #5, thnx!

> - Personal style: please use malloc() and free() rather than MALLOC() and
>   FREE().  We haven't inlined malloc() in a long, long time and the macros
>   are really there for older code.

6) fixed. thnx!

> - You tsleep() in linux_schedtail() while holding one lock and do the
>   wakeup() in linux_proc_init() while holding no locks.  You've got lost
>   wakeup races that way that you work around by having to use a goto and
>   a sleep with a timeout.

7) I dont understand this. can you explain more? thnx

> - You should include the appropriate header to get the declaration of 'hz'
>   rather than just adding an 'extern' directly in your code.

8) I didnt find such header. 

> - In futex_get() you don't handle the race of two concurrent creates.

9) hopefully fixed.

> - In futex_sleep(), you probably should be using an interlocked sleep such
>   as msleep() or cv_wait() to avoid lost wakeups.

10) open issue, this is currently under investigation because we in fact happen
to have problems with futexes

> - In futex_sleep(), you should probably loop rather than recurse to avoid
>   blowing the stack.

11) I am not sure about the logic but I think the recursion is done exactly once
I'll coordinate with manu_at_netbsd

> - In futex_atomic_op(), if you take a page fault on one of your operations
>   it's going to blow up because of the page fault during a critical section.
>   Your futex ops should be atomic (perhaps you can do them all using
>   casuptr()?) and you shouldn't use a critical section here.

12) the operations should be atomic (the operation on shared data is atomic)
so I just removed the crit section.

> - You should stick the sx locks in a header and not litter 'externs' in
>   source files.

13) fixed. thnx!

> - Should come up with a better name than 'schedtail' for the eventhandler
>   in fork_exit().  Maybe 'process_fork_exit'?

14) linux uses this name. I might change it but I think its ok to be in sync
with linux.

> - You should probably ask Peter to review the link_elf_obj change.  An
>   alternative is to use linker_lookup_set() in your module load routine to
>   lookup your linker set details.

15) yes... when I start working on amd64 I will definitely do something about it

> - Regarding the locking, I'm not sure why you have the two separate sx locks,
>   and why you don't just merge them into a single lock instead.

16) it makes the locking easier and they cover different things. I like it this way :)

thnx for exhausting review! I commited some fixes to p4, you might want to check them

roman