Re: SoC: linuxolator update: first patch

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.
- 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
	 */

- 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.
- 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.
- In linux_proc_init() you pass EMUL_LOCKED to em_find() in the CLONE_VM
  case when the lock isn't held.
- 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.
- 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.
- You should include the appropriate header to get the declaration of 'hz'
  rather than just adding an 'extern' directly in your code.
- In futex_get() you don't handle the race of two concurrent creates.
- In futex_sleep(), you probably should be using an interlocked sleep such
  as msleep() or cv_wait() to avoid lost wakeups.
- In futex_sleep(), you should probably loop rather than recurse to avoid
  blowing the stack.
- 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.
- You should stick the sx locks in a header and not litter 'externs' in
  source files.
- Should come up with a better name than 'schedtail' for the eventhandler
  in fork_exit().  Maybe 'process_fork_exit'?
- 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.
- 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.

-- 
John Baldwin