PERFORCE change 144415 for review

[John Baldwin]

On Tuesday 01 July 2008 07:03:59 am Hans Petter Selasky wrote:
> http://perforce.freebsd.org/chv.cgi?CH=144415
> 
> Change 144415 by hselasky at hselasky_laptop001 on 2008/07/01 11:03:31
> 
> 	
> 	To allow USB drivers using the Giant mutex, like ukbd
> 	and the tty layer (ucom), condition variable functions
> 	like mtx_sleep() and cv_wait() needs to support the Giant
> 	mutex. Previously using the Giant mutex with these functions
> 	resulted in a panic due to an unlock race between the 
> 	GIANT_DROP macro and the internal mutex unlock in the
> 	condition variable function. This patch will try to
> 	resolve that race.

I'd rather that the sleep and condition variable code just explicitly handle 
this case rather than changing DROP_GIANT().

> Affected files ...
> 
> .. //depot/projects/usb/src/sys/kern/kern_condvar.c#7 edit
> .. //depot/projects/usb/src/sys/kern/kern_synch.c#9 edit
> .. //depot/projects/usb/src/sys/sys/mutex.h#8 edit
> 
> Differences ...
> 
> ==== //depot/projects/usb/src/sys/kern/kern_condvar.c#7 (text+ko) ====
> 
> @@ -123,7 +123,7 @@
>  	sleepq_lock(cvp);
>  
>  	cvp->cv_waiters++;
> -	DROP_GIANT();
> +	DROP_GIANT(lock);
>  
>  	sleepq_add(cvp, lock, cvp->cv_description, SLEEPQ_CONDVAR, 0);
>  	if (class->lc_flags & LC_SLEEPABLE)
> @@ -176,7 +176,7 @@
>  	sleepq_lock(cvp);
>  
>  	cvp->cv_waiters++;
> -	DROP_GIANT();
> +	DROP_GIANT(lock);
>  
>  	sleepq_add(cvp, lock, cvp->cv_description, SLEEPQ_CONDVAR, 0);
>  	if (class->lc_flags & LC_SLEEPABLE)
> @@ -233,7 +233,7 @@
>  	sleepq_lock(cvp);
>  
>  	cvp->cv_waiters++;
> -	DROP_GIANT();
> +	DROP_GIANT(lock);
>  
>  	sleepq_add(cvp, lock, cvp->cv_description, SLEEPQ_CONDVAR |
>  	    SLEEPQ_INTERRUPTIBLE, 0);
> @@ -293,7 +293,7 @@
>  	sleepq_lock(cvp);
>  
>  	cvp->cv_waiters++;
> -	DROP_GIANT();
> +	DROP_GIANT(lock);
>  
>  	sleepq_add(cvp, lock, cvp->cv_description, SLEEPQ_CONDVAR, 0);
>  	sleepq_set_timeout(cvp, timo);
> @@ -356,7 +356,7 @@
>  	sleepq_lock(cvp);
>  
>  	cvp->cv_waiters++;
> -	DROP_GIANT();
> +	DROP_GIANT(lock);
>  
>  	sleepq_add(cvp, lock, cvp->cv_description, SLEEPQ_CONDVAR |
>  	    SLEEPQ_INTERRUPTIBLE, 0);
> 
> ==== //depot/projects/usb/src/sys/kern/kern_synch.c#9 (text+ko) ====
> 
> @@ -181,7 +181,7 @@
>  	CTR5(KTR_PROC, "sleep: thread %ld (pid %ld, %s) on %s (%p)",
>  	    td->td_tid, p->p_pid, td->td_name, wmesg, ident);
>  
> -	DROP_GIANT();
> +	DROP_GIANT(lock);
>  	if (lock != NULL && !(class->lc_flags & LC_SLEEPABLE)) {
>  		WITNESS_SAVE(lock, lock_witness);
>  		lock_state = class->lc_unlock(lock);
> 
> ==== //depot/projects/usb/src/sys/sys/mutex.h#8 (text+ko) ====
> 
> @@ -366,17 +366,44 @@
>   *
>   * Note that DROP_GIANT*() needs to be paired with PICKUP_GIANT() 
>   * The #ifndef is to allow lint-like tools to redefine DROP_GIANT.
> + *
> + * Note that by default DROP_GIANT takes no argument. Optionally you
> + * can specify an argument which explicitly has the name "lock" and
> + * type "struct lock_object *". If this "lock" pointer is equal to
> + * "&Giant", the DROP_GIANT macro will not do the final drop on the
> + * Giant mutex, but expects the calling code to do so. This feature is
> + * used by condition variables to allow sleeping on Giant. The
> + * condition variable code will then do the final drop!
>   */
>  #ifndef DROP_GIANT
> -#define DROP_GIANT()							\
> +#define	DROP_GIANT(arg) DROP_GIANT_SUB_##arg(arg)
> +#define	DROP_GIANT_SUB_lock(arg) DROP_GIANT_SUB(arg) /* "lock" argument */
> +#define	DROP_GIANT_SUB_(arg) DROP_GIANT_SUB(NULL) /* no argument */
> +#define	DROP_GIANT_SUB(lock)						\
>  do {									\
> -	int _giantcnt = 0;						\
> +	unsigned int _giantcnt;						\
>  	WITNESS_SAVE_DECL(Giant);					\
>  									\
>  	if (mtx_owned(&Giant)) {					\
> -		WITNESS_SAVE(&Giant.lock_object, Giant);		\
> -		for (_giantcnt = 0; mtx_owned(&Giant); _giantcnt++)	\
> -			mtx_unlock(&Giant);				\
> +		unsigned int _giantn;					\
> +		if (((void *)(lock)) == ((void *)&Giant)) {		\
> +			/* special case */				\
> +			_giantn = Giant.mtx_recurse;			\
> +		} else {						\
> +			/* default case */				\
> +			_giantn = Giant.mtx_recurse + 1;		\
> +		}							\
> +		if (_giantn != 0) {					\
> +			WITNESS_SAVE(&Giant.lock_object, Giant);	\
> +			_giantcnt = _giantn;				\
> +			do {						\
> +				mtx_unlock(&Giant);			\
> +			} while (--_giantn);				\
> +		} else {						\
> +			_giantcnt = 0;					\
> +		}							\
> +	} else {							\
> +		_giantcnt = 0;						\
>  	}
>  
>  #define PICKUP_GIANT()							\
> 



-- 
John Baldwin