From: Alexander Bluhm Subject: Re: improve spinning in mtx_enter To: Mark Kettenis Cc: Martin Pieuchot , mjguzik@gmail.com, tech@openbsd.org Date: Mon, 25 Mar 2024 21:56:24 +0100 On Sun, Mar 24, 2024 at 11:53:14AM +0100, Mark Kettenis wrote: > > > If you zoom into kernel and search for mtx_enter, you see that > > > spinning time goes down from 13.1% to 10.7% > > I assume that's an amd64 machine? Can we test this on some other > architectures too before we commit it? I don't expect a regression, > but it would be interesting to see what the effect is on an > architectures that has better atomic operations than amd64. On powerpc64 this diff hangs as soon the machine is going to multiuser. READ_ONCE(mtx->mtx_owner) fixes it. The problem is that the volatile in the struct mutex definition is at the wrong place. mtx_owner variable must be volatile, not the object it is pointing to. time make -j4 bsd without diff: 4m33.70s real 10m17.07s user 6m37.56s system time make -j4 bsd with diff below: 4m32.17s real 10m12.17s user 6m39.56s system Measurement is going up and down a bit, so it is more or less the same. > One potential issue with this diff is that the current code checks > whether we've panicked on every iteration of the loop. But with this > diff that doesn't happen. So a CPU that is already spinning wil no > longer be set free when the kernel panics. I don't think that's a > problem as long as we don't have IPL_IPI mutexes. It might actually > be a good thing. But it is a change in behaviour to watch out for. I think you refer to ths check in mtx_enter_try(). if (panicstr || db_active) return (1); It has not been added to interrupt mtx_enter() when another CPU panics. After a panic "ddb> boot reboot" should not grab any lock. This still works. New diff with correct volatile in struct mutex. ok? bluhm Index: kern/kern_lock.c =================================================================== RCS file: /data/mirror/openbsd/cvs/src/sys/kern/kern_lock.c,v diff -u -p -r1.72 kern_lock.c --- kern/kern_lock.c 26 Apr 2022 15:31:14 -0000 1.72 +++ kern/kern_lock.c 25 Mar 2024 20:35:05 -0000 @@ -264,15 +264,17 @@ mtx_enter(struct mutex *mtx) spc->spc_spinning++; while (mtx_enter_try(mtx) == 0) { - CPU_BUSY_CYCLE(); - + do { + CPU_BUSY_CYCLE(); #ifdef MP_LOCKDEBUG - if (--nticks == 0) { - db_printf("%s: %p lock spun out\n", __func__, mtx); - db_enter(); - nticks = __mp_lock_spinout; - } + if (--nticks == 0) { + db_printf("%s: %p lock spun out\n", + __func__, mtx); + db_enter(); + nticks = __mp_lock_spinout; + } #endif + } while (mtx->mtx_owner != NULL); } spc->spc_spinning--; } Index: sys/mutex.h =================================================================== RCS file: /data/mirror/openbsd/cvs/src/sys/sys/mutex.h,v diff -u -p -r1.20 mutex.h --- sys/mutex.h 3 Feb 2024 22:50:09 -0000 1.20 +++ sys/mutex.h 25 Mar 2024 20:35:25 -0000 @@ -40,7 +40,7 @@ #include struct mutex { - volatile void *mtx_owner; + void *volatile mtx_owner; int mtx_wantipl; int mtx_oldipl; #ifdef WITNESS