> Date: Mon, 18 Dec 2017 10:08:23 +0100 > From: Martin Pieuchot <[email protected]> > > On 14/12/17(Thu) 16:06, Martin Pieuchot wrote: > > Diff below moves amd64 and i386 mutex to the common C implementation. > > > > The differences are: > > - membar_enter_after_atomic(9) instead of membar_enter(9), and > > - membar_exit_before_atomic(9) instead of membar_exit(9) > > > > I'd appreciate any performance test to know if the performance > > degradation is acceptable with these barriers. > > Hrvoje Popovski confirmed there's not performance regression with this > implementation on his forwarding setup. > > So I'd like reviews and oks before improving stuff in tree.
The ultimate goal is still to make this MI-code isn't it? ok kettenis@ > > Index: amd64/amd64/mutex.c > > =================================================================== > > RCS file: amd64/amd64/mutex.c > > diff -N amd64/amd64/mutex.c > > --- /dev/null 1 Jan 1970 00:00:00 -0000 > > +++ amd64/amd64/mutex.c 14 Dec 2017 14:49:59 -0000 > > @@ -0,0 +1,151 @@ > > +/* $OpenBSD: mutex.c,v 1.19 2017/09/11 09:52:15 mpi Exp $ */ > > + > > +/* > > + * Copyright (c) 2004 Artur Grabowski <[email protected]> > > + * All rights reserved. > > + * > > + * Redistribution and use in source and binary forms, with or without > > + * modification, are permitted provided that the following conditions > > + * are met: > > + * > > + * 1. Redistributions of source code must retain the above copyright > > + * notice, this list of conditions and the following disclaimer. > > + * 2. The name of the author may not be used to endorse or promote products > > + * derived from this software without specific prior written > > permission. > > + * > > + * THIS SOFTWARE IS PROVIDED ``AS IS'' AND ANY EXPRESS OR IMPLIED > > WARRANTIES, > > + * INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY > > + * AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL > > + * THE AUTHOR BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, > > + * EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, > > + * PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR > > PROFITS; > > + * OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, > > + * WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR > > + * OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF > > + * ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. > > + */ > > + > > +#include <sys/param.h> > > +#include <sys/mutex.h> > > +#include <sys/systm.h> > > +#include <sys/atomic.h> > > + > > +#include <machine/intr.h> > > + > > +#include <ddb/db_output.h> > > + > > +void > > +__mtx_init(struct mutex *mtx, int wantipl) > > +{ > > + mtx->mtx_owner = NULL; > > + mtx->mtx_wantipl = wantipl; > > + mtx->mtx_oldipl = IPL_NONE; > > +} > > + > > +#ifdef MULTIPROCESSOR > > +#ifdef MP_LOCKDEBUG > > +#ifndef DDB > > +#error "MP_LOCKDEBUG requires DDB" > > +#endif > > + > > +/* CPU-dependent timing, needs this to be settable from ddb. */ > > +extern int __mp_lock_spinout; > > +#endif > > + > > +void > > +__mtx_enter(struct mutex *mtx) > > +{ > > +#ifdef MP_LOCKDEBUG > > + int nticks = __mp_lock_spinout; > > +#endif > > + > > + while (__mtx_enter_try(mtx) == 0) { > > + CPU_BUSY_CYCLE(); > > + > > +#ifdef MP_LOCKDEBUG > > + if (--nticks == 0) { > > + db_printf("%s: %p lock spun out", __func__, mtx); > > + db_enter(); > > + nticks = __mp_lock_spinout; > > + } > > +#endif > > + } > > +} > > + > > +int > > +__mtx_enter_try(struct mutex *mtx) > > +{ > > + struct cpu_info *owner, *ci = curcpu(); > > + int s; > > + > > + if (mtx->mtx_wantipl != IPL_NONE) > > + s = splraise(mtx->mtx_wantipl); > > + > > + owner = atomic_cas_ptr(&mtx->mtx_owner, NULL, ci); > > +#ifdef DIAGNOSTIC > > + if (__predict_false(owner == ci)) > > + panic("mtx %p: locking against myself", mtx); > > +#endif > > + if (owner == NULL) { > > + membar_enter_after_atomic(); > > + if (mtx->mtx_wantipl != IPL_NONE) > > + mtx->mtx_oldipl = s; > > +#ifdef DIAGNOSTIC > > + ci->ci_mutex_level++; > > +#endif > > + return (1); > > + } > > + > > + if (mtx->mtx_wantipl != IPL_NONE) > > + splx(s); > > + > > + return (0); > > +} > > +#else > > +void > > +__mtx_enter(struct mutex *mtx) > > +{ > > + struct cpu_info *ci = curcpu(); > > + > > +#ifdef DIAGNOSTIC > > + if (__predict_false(mtx->mtx_owner == ci)) > > + panic("mtx %p: locking against myself", mtx); > > +#endif > > + > > + if (mtx->mtx_wantipl != IPL_NONE) > > + mtx->mtx_oldipl = splraise(mtx->mtx_wantipl); > > + > > + mtx->mtx_owner = ci; > > + > > +#ifdef DIAGNOSTIC > > + ci->ci_mutex_level++; > > +#endif > > +} > > + > > +int > > +__mtx_enter_try(struct mutex *mtx) > > +{ > > + __mtx_enter(mtx); > > + return (1); > > +} > > +#endif > > + > > +void > > +__mtx_leave(struct mutex *mtx) > > +{ > > + int s; > > + > > + MUTEX_ASSERT_LOCKED(mtx); > > + > > +#ifdef DIAGNOSTIC > > + curcpu()->ci_mutex_level--; > > +#endif > > + > > + s = mtx->mtx_oldipl; > > +#ifdef MULTIPROCESSOR > > + membar_exit_before_atomic(); > > +#endif > > + mtx->mtx_owner = NULL; > > + if (mtx->mtx_wantipl != IPL_NONE) > > + splx(s); > > +} > > Index: amd64/conf/files.amd64 > > =================================================================== > > RCS file: /cvs/src/sys/arch/amd64/conf/files.amd64,v > > retrieving revision 1.91 > > diff -u -p -r1.91 files.amd64 > > --- amd64/conf/files.amd64 17 Oct 2017 14:25:35 -0000 1.91 > > +++ amd64/conf/files.amd64 14 Dec 2017 14:51:20 -0000 > > @@ -28,7 +28,7 @@ file arch/amd64/amd64/fpu.c > > file arch/amd64/amd64/softintr.c > > file arch/amd64/amd64/i8259.c > > file arch/amd64/amd64/cacheinfo.c > > -file arch/amd64/amd64/mutex.S > > +file arch/amd64/amd64/mutex.c > > file arch/amd64/amd64/vector.S > > file arch/amd64/amd64/copy.S > > file arch/amd64/amd64/spl.S > > Index: i386/conf/files.i386 > > =================================================================== > > RCS file: /cvs/src/sys/arch/i386/conf/files.i386,v > > retrieving revision 1.235 > > diff -u -p -r1.235 files.i386 > > --- i386/conf/files.i386 17 Oct 2017 14:25:35 -0000 1.235 > > +++ i386/conf/files.i386 14 Dec 2017 14:50:43 -0000 > > @@ -21,6 +21,7 @@ file arch/i386/i386/est.c !small_kernel > > file arch/i386/i386/gdt.c > > file arch/i386/i386/in_cksum.s > > file arch/i386/i386/machdep.c > > +file arch/i386/i386/mutex.c > > file arch/i386/i386/hibernate_machdep.c hibernate > > file arch/i386/i386/via.c > > file arch/i386/i386/locore.s > > Index: i386/i386/locore.s > > =================================================================== > > RCS file: /cvs/src/sys/arch/i386/i386/locore.s,v > > retrieving revision 1.180 > > diff -u -p -r1.180 locore.s > > --- i386/i386/locore.s 25 Aug 2017 19:28:48 -0000 1.180 > > +++ i386/i386/locore.s 14 Dec 2017 14:45:36 -0000 > > @@ -1319,8 +1319,6 @@ ENTRY(cpu_paenable) > > #include <i386/i386/apicvec.s> > > #endif > > > > -#include <i386/i386/mutex.S> > > - > > .section .rodata > > .globl _C_LABEL(_stac) > > _C_LABEL(_stac): > > Index: i386/i386/mutex.c > > =================================================================== > > RCS file: i386/i386/mutex.c > > diff -N i386/i386/mutex.c > > --- /dev/null 1 Jan 1970 00:00:00 -0000 > > +++ i386/i386/mutex.c 14 Dec 2017 14:47:29 -0000 > > @@ -0,0 +1,151 @@ > > +/* $OpenBSD: mutex.c,v 1.19 2017/09/11 09:52:15 mpi Exp $ */ > > + > > +/* > > + * Copyright (c) 2004 Artur Grabowski <[email protected]> > > + * All rights reserved. > > + * > > + * Redistribution and use in source and binary forms, with or without > > + * modification, are permitted provided that the following conditions > > + * are met: > > + * > > + * 1. Redistributions of source code must retain the above copyright > > + * notice, this list of conditions and the following disclaimer. > > + * 2. The name of the author may not be used to endorse or promote products > > + * derived from this software without specific prior written > > permission. > > + * > > + * THIS SOFTWARE IS PROVIDED ``AS IS'' AND ANY EXPRESS OR IMPLIED > > WARRANTIES, > > + * INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY > > + * AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL > > + * THE AUTHOR BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, > > + * EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, > > + * PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR > > PROFITS; > > + * OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, > > + * WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR > > + * OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF > > + * ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. > > + */ > > + > > +#include <sys/param.h> > > +#include <sys/mutex.h> > > +#include <sys/systm.h> > > +#include <sys/atomic.h> > > + > > +#include <machine/intr.h> > > + > > +#include <ddb/db_output.h> > > + > > +void > > +__mtx_init(struct mutex *mtx, int wantipl) > > +{ > > + mtx->mtx_owner = NULL; > > + mtx->mtx_wantipl = wantipl; > > + mtx->mtx_oldipl = IPL_NONE; > > +} > > + > > +#ifdef MULTIPROCESSOR > > +#ifdef MP_LOCKDEBUG > > +#ifndef DDB > > +#error "MP_LOCKDEBUG requires DDB" > > +#endif > > + > > +/* CPU-dependent timing, needs this to be settable from ddb. */ > > +extern int __mp_lock_spinout; > > +#endif > > + > > +void > > +__mtx_enter(struct mutex *mtx) > > +{ > > +#ifdef MP_LOCKDEBUG > > + int nticks = __mp_lock_spinout; > > +#endif > > + > > + while (__mtx_enter_try(mtx) == 0) { > > + CPU_BUSY_CYCLE(); > > + > > +#ifdef MP_LOCKDEBUG > > + if (--nticks == 0) { > > + db_printf("%s: %p lock spun out", __func__, mtx); > > + db_enter(); > > + nticks = __mp_lock_spinout; > > + } > > +#endif > > + } > > +} > > + > > +int > > +__mtx_enter_try(struct mutex *mtx) > > +{ > > + struct cpu_info *owner, *ci = curcpu(); > > + int s; > > + > > + if (mtx->mtx_wantipl != IPL_NONE) > > + s = splraise(mtx->mtx_wantipl); > > + > > + owner = atomic_cas_ptr(&mtx->mtx_owner, NULL, ci); > > +#ifdef DIAGNOSTIC > > + if (__predict_false(owner == ci)) > > + panic("mtx %p: locking against myself", mtx); > > +#endif > > + if (owner == NULL) { > > + membar_enter_after_atomic(); > > + if (mtx->mtx_wantipl != IPL_NONE) > > + mtx->mtx_oldipl = s; > > +#ifdef DIAGNOSTIC > > + ci->ci_mutex_level++; > > +#endif > > + return (1); > > + } > > + > > + if (mtx->mtx_wantipl != IPL_NONE) > > + splx(s); > > + > > + return (0); > > +} > > +#else > > +void > > +__mtx_enter(struct mutex *mtx) > > +{ > > + struct cpu_info *ci = curcpu(); > > + > > +#ifdef DIAGNOSTIC > > + if (__predict_false(mtx->mtx_owner == ci)) > > + panic("mtx %p: locking against myself", mtx); > > +#endif > > + > > + if (mtx->mtx_wantipl != IPL_NONE) > > + mtx->mtx_oldipl = splraise(mtx->mtx_wantipl); > > + > > + mtx->mtx_owner = ci; > > + > > +#ifdef DIAGNOSTIC > > + ci->ci_mutex_level++; > > +#endif > > +} > > + > > +int > > +__mtx_enter_try(struct mutex *mtx) > > +{ > > + __mtx_enter(mtx); > > + return (1); > > +} > > +#endif > > + > > +void > > +__mtx_leave(struct mutex *mtx) > > +{ > > + int s; > > + > > + MUTEX_ASSERT_LOCKED(mtx); > > + > > +#ifdef DIAGNOSTIC > > + curcpu()->ci_mutex_level--; > > +#endif > > + > > + s = mtx->mtx_oldipl; > > +#ifdef MULTIPROCESSOR > > + membar_exit_before_atomic(); > > +#endif > > + mtx->mtx_owner = NULL; > > + if (mtx->mtx_wantipl != IPL_NONE) > > + splx(s); > > +} > > > >
