svn commit: r313996 - in head/sys: kern sys

[Mateusz Guzik]

On Wed, Feb 22, 2017 at 03:09:24PM -0800, Gleb Smirnoff wrote:
>   Mateusz,
> 
>   why do you __predict_false() the recursion scenario? I'm afraid
> that performance loss for mispredictions could outweight the
> gain due to predictions. AFAIK, mutex recursion is still a pretty
> common event in the kernel.
> 

Do you mean spin mutexes or blocking ones sa well?

For blocking mutexes, recursion almost never happens and I'm confident
in the prediction to not do it. It may be there is a lock somewere which
recurses a lot and its singlethreaded operation is harmed by the
prediction. For such locks a separate wrappers shouldl be introduced
which expect recursion to happen.

For spin mutexes the above is almost correct. Most calls do the slow
path to spin.

There is an important lock which sometimes recurses (sched lock) but it
is unclear if it got hurt by the change.

Note that lock clean up is a work in progress. For spin mutexes I'm
pondering revamping them a little - the inline path is extremelly long
and already constains a function call to spinlock_enter/exit. So the
plan would be to create a primitive with an inlined spinlock code to
keep one call and reduce inline code.

Finally, while mfcability of the work prevents riping out recursion
support from the code, an additional set of primitives which expect
recursion to happen can be introduced and used in places which do
recurse.

TL;DR I think the changes are a net win to spinlocks even if they
recurse and will be a bigger win with coming cleanups.

On the other hand if you profiled a regression with the above, I'm happy
to unpredict the change.


> On Mon, Feb 20, 2017 at 07:08:36PM +0000, Mateusz Guzik wrote:
> M> Author: mjg
> M> Date: Mon Feb 20 19:08:36 2017
> M> New Revision: 313996
> M> URL: https://svnweb.freebsd.org/changeset/base/313996
> M> 
> M> Log:
> M>   mtx: fix spin mutexes interaction with failed fcmpset
> M>   
> M>   While doing so move recursion support down to the fallback routine.
> M> 
> M> Modified:
> M>   head/sys/kern/kern_mutex.c
> M>   head/sys/sys/mutex.h
> M> 
> M> Modified: head/sys/kern/kern_mutex.c
> M> ==============================================================================
> M> --- head/sys/kern/kern_mutex.c	Mon Feb 20 17:33:25 2017	(r313995)
> M> +++ head/sys/kern/kern_mutex.c	Mon Feb 20 19:08:36 2017	(r313996)
> M> @@ -696,6 +696,14 @@ _mtx_lock_spin_cookie(volatile uintptr_t
> M>  	lock_delay_arg_init(&lda, &mtx_spin_delay);
> M>  	m = mtxlock2mtx(c);
> M>  
> M> +	if (__predict_false(v == MTX_UNOWNED))
> M> +		v = MTX_READ_VALUE(m);
> M> +
> M> +	if (__predict_false(v == tid)) {
> M> +		m->mtx_recurse++;
> M> +		return;
> M> +	}
> M> +
> M>  	if (LOCK_LOG_TEST(&m->lock_object, opts))
> M>  		CTR1(KTR_LOCK, "_mtx_lock_spin: %p spinning", m);
> M>  	KTR_STATE1(KTR_SCHED, "thread", sched_tdname((struct thread *)tid),
> M> 
> M> Modified: head/sys/sys/mutex.h
> M> ==============================================================================
> M> --- head/sys/sys/mutex.h	Mon Feb 20 17:33:25 2017	(r313995)
> M> +++ head/sys/sys/mutex.h	Mon Feb 20 19:08:36 2017	(r313996)
> M> @@ -223,12 +223,9 @@ void	thread_lock_flags_(struct thread *,
> M>  	uintptr_t _v = MTX_UNOWNED;					\
> M>  									\
> M>  	spinlock_enter();						\
> M> -	if (!_mtx_obtain_lock_fetch((mp), &_v, _tid)) {			\
> M> -		if (_v == _tid)						\
> M> -			(mp)->mtx_recurse++;				\
> M> -		else							\
> M> -			_mtx_lock_spin((mp), _v, _tid, (opts), (file), (line));\
> M> -	} else 								\
> M> +	if (!_mtx_obtain_lock_fetch((mp), &_v, _tid)) 			\
> M> +		_mtx_lock_spin((mp), _v, _tid, (opts), (file), (line)); \
> M> +	else 								\
> M>  		LOCKSTAT_PROFILE_OBTAIN_LOCK_SUCCESS(spin__acquire,	\
> M>  		    mp, 0, 0, file, line);				\
> M>  } while (0)
> M> _______________________________________________
> M> svn-src-all at freebsd.org mailing list
> M> https://lists.freebsd.org/mailman/listinfo/svn-src-all
> M> To unsubscribe, send any mail to "svn-src-all-unsubscribe at freebsd.org"
> 
> -- 
> Totus tuus, Glebius.
> _______________________________________________
> svn-src-all at freebsd.org mailing list
> https://lists.freebsd.org/mailman/listinfo/svn-src-all
> To unsubscribe, send any mail to "svn-src-all-unsubscribe at freebsd.org"

-- 
Mateusz Guzik <mjguzik gmail.com>