investigation of Giant usage in kernel
kris at obsecurity.org
Tue Mar 27 03:58:01 UTC 2007
On Sun, Mar 04, 2007 at 03:11:36PM +0100, Divacky Roman wrote:
> I looked at where Giant is held in the kernel and I found these interesting things:
> 1) in fs/fifofs/fifo_vnops.c we lock Giant when calling sorecieve()/sosend()
> this is a bandaid for fixing a race that doesnt have to exist anymore. ie.
> it needs some testing and can be remvoed
I have removed this on my test machines and will see if the problem
recurs. It is probably not necessary.
> 2) in geom/geom_dev.c we lock Giant for:
> 2a) calling make_dev() which seems unecessary because make_dev protects
> itself with devmtx
> 2b) setting a flag in cdev which I think is unecessary as well
I removed this in CVS.
> 3) in kern/kern_descrip.c we lock Giant for the whole life of flock() I dont see
> a need for this protection becuase
> 3a) access to fp is protected with FILE_LOCK()ing
> 3b) otherwise it operates on local variables
> 3c) it also calls VOP_* functions but those dont require Giant, right?
I am not sure about this one. It might be tied in to 4).
> 4) in kern/kern_lockf.c we lock Giant for the whole life of lf_advlock() I dont
> think this is necessary because
> 4a) it operates on local variables
> 4b) it calls various lf_*lock() functions which seems to either protect
> themselves or not needed protection at all
There is no synchronization between e.g. the tailq manipulation from
different threads, so locking is needed. I have replaced this with a
global lockf_mtx in my p4 branch but a finer grained solution is
clearly desirable. This will probably involve rewriting or
restructuring the code though.
> 5) in kern/kern_time.c we lock Giant to protect
> 5a) tc_setclock() call - I dont know if this is necessary or not
> 5b) lease_updatetime call which is a function pointer that seems to be
> only assigned at one place (line 119 in kern_time.c) to a NOP function.
> can this be removed?
I guess you are protecting against two threads setting the clock at
once. This does not seem to be a big deal, I don't imagine this will
ever be in the critical path of anything.
> 6) in vm/device_pager.c we lock Giant to (also) protect cdevsw->d_mmap but the device mmap
> is either MPSAFE or assigned to giant_mmap (a wrapper that locks GIant and calls the original
> d_mmap) so this seems unecessary.
I'm not sure about this one either.
More information about the freebsd-hackers