On Tue, 27 Jul 1999, Jason Thorpe wrote:

> Attached below are my thoughts for some MI APIs for handling SMP things.


> The following functions or macros will be exported by <machine/lock.h>:
> 
> 	void cpu_simple_lock_init(__volatile struct simplelock *alp);
> 
> 		MANDATORY
> 
> 		This function, inline function, or macro initializes
> 		the `lock_data' member of `struct simplelock' to the
> 		value 0.

Locks need to have an spl() associated with them.  Otherwise code could
grab a lock and be interrupted, and the interrupt handler could try to
grab that lock once again.

Now you could have these separately so you need to do an splfoo() before a
cpu_simple_lock(foo_lock), but it's simpler to simply add an spl field to
the lock structure itself and do:

void cpu_simple_lock_init(__volatile struct simplelock *alp, int spl);


Next thing:  Locks are owned by something, and what they're owned by must
be defined.  It could be a process, a thread, or a particular CPU, but it
must be defined or chaos will ensue.  

You should also keep track of the owner of the current lock if LOCKDEBUG
is defined to simplify debugging of deadlocks.  

Currently it seems that the lock owner is a particular CPU since
sys/lock.h has:

        unsigned long lock_holder;              /* CPU ID */

This is probably not good since it implies a particular CPU owns a lock.
If a process running on a CPU grabs a lock and is then suspended, the CPU
still owns the lock and another process that tries to grab the lock, well,
bad things happen any way you look at it.

OTOH, if the model is that locks are owned by processes, and a process
running on one CPU grabs a lock, but is then suspended and another process
scheduled that also tries to grab that lock, that process should not be
able to get the lock.  Likewise, if the first process it scheduled on
another CPU, it should keep that lock and be allowed to free that lock.

> 
> 	void cpu_simple_lock(__volatile struct simplelock *alp);
> 
> 		MANDATORY
> 
> 		This function, inline function, or macro acquires
> 		the specified simple lock by performing the following
> 		operations in an atomic manner:
> 
> 			while (alp->lock_data != 0) /* spin */;
> 			alp->lock_data = 1;

Are all locks spinlocks?  What about stealing the adaptive mutex concept
from Solaris?

> DATA STRUCTURES
> ---------------
> 
> The following data structures will be exported by <machine/cpu.h>:
> 
> 	struct cpu_info;
> 
> 		only if MULTIPROCESSOR
> 
> 		This structure will always export the following members:
> 
> 		- struct proc *ci_curproc: current process on this processor
> 
> 		- struct simplelock ci_slock: simple lock for cpu_info
> 		  structure.  Note that use of this lock must remain
> 		  untracked in the case of lock debugging.  While this
> 		  lock is held, all interrupts MUST be blocked, as this
> 		  lock may be asserted from an interrupt context.
> 
> 		- u_long ci_cpuid: CPU identifier of associated CPU
> 
> 		This structure will export the following members if
> 		the DIAGNOSTIC or LOCKDEBUG CPP symbols are defined:
> 
> 		- u_long ci_spin_locks: number of spin locks held by
> 		  this processor
> 
> 		- u_long ci_simple_locks: number of simple locks held
> 		  by this processor
> 
> 		This structure will export the following members if
> 		the LOCKDEBG CPP symbol is defined:
> 
> 		- struct spinlock_list ci_spin_lock_list: TAILQ of
> 		  spin locks held by this processor
> 
> 		- struct simplelock_list ci_simple_lock_list: TAILQ of
> 		  simple locks held by this processor
> 
> 	extern struct cpu_info cpu_info[];
> 
> 		only if MULTIPROCESSOR
> 
> 		This is an array of cpu_info structures declared and allocated
> 		in a machine-dependent source file.  It must be allocated
> 		and initialized before any machine-independent code is
> 		executed during the bootstrap process.  This array will
> 		be indexed by the value returned by the `cpu_number()'
> 		function.
> 
> 		cpu_info[] will be referenced in <sys/proc.h> by the
> 		following code block:
> 
> #if defined(MULTIPROCESSOR)
> #define	curproc		cpu_info[cpu_number()].ci_curproc
> #else
> extern struct proc	*curproc;
> #endif
> 

Why not just have a function that returns a pointer to the current
cpu_info structure?  Array indexing may require rather expensive
multiplication if the cpu_info structure is not a power of 2.

=========================================================================
Eduardo Horvath				eeh@one-o.com
	"I need to find a pithy new quote." -- me