SoC: linuxolator update: first patch
John Baldwin
jhb at freebsd.org
Tue Aug 15 13:14:57 UTC 2006
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
More information about the freebsd-current
mailing list