investigation of Giant usage in kernel

[Kris Kennaway]

On Sun, Mar 04, 2007 at 03:11:36PM +0100, Divacky Roman wrote:
> hi
> 
> 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.

Kris