Changeset View
Standalone View
sys/kern/kern_obm.c
- This file was added.
/*- | |||||
* SPDX-License-Identifier: BSD-2-Clause-FreeBSD | |||||
* | |||||
* Copyright (c) 2020 The FreeBSD Foundation | |||||
* All rights reserved. | |||||
* | |||||
* This software was developed by Konstantin Belousov <kib@FreeBSD.org> | |||||
* under sponsorship from the FreeBSD Foundation. | |||||
* | |||||
* 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. Redistributions in binary form must reproduce the above copyright | |||||
* notice, this list of conditions and the following disclaimer in the | |||||
* documentation and/or other materials provided with the distribution. | |||||
* | |||||
* THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS ``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 OR CONTRIBUTORS 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/cdefs.h> | |||||
__FBSDID("$FreeBSD$"); | |||||
#include <sys/param.h> | |||||
#include <sys/kernel.h> | |||||
#include <sys/lock.h> | |||||
#include <sys/obm.h> | |||||
#include <sys/proc.h> | |||||
#include <sys/sysctl.h> | |||||
#include <sys/systm.h> | |||||
#include <sys/turnstile.h> | |||||
#include <machine/atomic.h> | |||||
#ifdef OBM_DEBUG | |||||
static SYSCTL_NODE(_debug, OID_AUTO, obm, CTLFLAG_RD | CTLFLAG_MPSAFE, 0, | |||||
""); | |||||
static u_long obm_slow_lock; | |||||
SYSCTL_LONG(_debug_obm, OID_AUTO, slow_lock, CTLFLAG_RD, | |||||
&obm_slow_lock, 0, | |||||
""); | |||||
static u_long obm_slow_unlock; | |||||
SYSCTL_LONG(_debug_obm, OID_AUTO, slow_unlock, CTLFLAG_RD, | |||||
&obm_slow_unlock, 0, | |||||
""); | |||||
#endif | |||||
void | |||||
obm_init_lo(struct lock_object *lo, const char *name) | |||||
{ | |||||
bzero(lo, sizeof(*lo)); | |||||
lo->lo_name = name; | |||||
} | |||||
void | |||||
obm_init(obm_lock_t *obm) | |||||
{ | |||||
obm->lk = OBM_UNLOCKED; | |||||
} | |||||
void | |||||
obm_lock_slow(obm_lock_t *obm, struct lock_object *lo) | |||||
{ | |||||
struct turnstile *ts; | |||||
struct lock_delay_arg lda; | |||||
uint8_t v; | |||||
markj: Do you plan to add WITNESS integration? | |||||
Done Inline ActionsI do not have strong plans now. The pv list locks are leaf, so I think they could go away with much simpler checks. But if the patch is going to be productuzed, then sure. kib: I do not have strong plans now. The pv list locks are leaf, so I think they could go away with… | |||||
#ifdef OBM_DEBUG | |||||
atomic_add_long(&obm_slow_lock, 1); | |||||
#endif | |||||
lock_delay_arg_init(&lda, &locks_delay); | |||||
lock_delay(&lda); | |||||
for (;;) { | |||||
v = atomic_load_8(&obm->lk); | |||||
if (v == OBM_UNLOCKED) { | |||||
Done Inline Actionsmjg will probably point out that loading the lock state before attempting the cmpset can result in unnecessary cache coherence operations. I'm not sure how much it matters in practice since we expect contention on these locks to be low. markj: mjg will probably point out that loading the lock state before attempting the cmpset can result… | |||||
Done Inline ActionsThis is slow path. More, we enter the loop after lock_delay(). So IMO it is better to re-read v then to operate on certainly stale value. It might make sense to try to optimize it for non-first loop iteration, but again this is contended path. kib: This is slow path. More, we enter the loop after lock_delay(). So IMO it is better to re-read… | |||||
Done Inline ActionsI understand, it is only relevant if the lock is contended. For PVLL this can perhaps be ignored, but it is not ideal in a general purpose primitive. markj: I understand, it is only relevant if the lock is contended. For PVLL this can perhaps be… | |||||
Done Inline ActionsSo the entry point blindly lock_delays even if the lock is contested or fcmpset spuriously failed. Other primitives pass the found value instead, then the main loop can test on it without reading. mjg: So the entry point blindly lock_delays even if the lock is contested or fcmpset spuriously… | |||||
if (atomic_fcmpset_acq_char(&obm->lk, &v, OBM_LOCKED) != 0) | |||||
Done Inline ActionsPresumably this is just for testing? markj: Presumably this is just for testing? | |||||
Done Inline ActionsIt probably should go under lock profile option, or something similar. BTW, right now after around dozen buildkernels on the machine, I see debug.obm.slow_unlock: 22920 debug.obm.slow_lock: 111444 kib: It probably should go under lock profile option, or something similar. BTW, right now after… | |||||
break; | |||||
lock_delay(&lda); | |||||
continue; | |||||
} | |||||
ts = turnstile_trywait(lo); | |||||
v = atomic_load_8(&obm->lk); | |||||
if (v == OBM_UNLOCKED) { | |||||
turnstile_cancel(ts); | |||||
if (atomic_fcmpset_acq_8(&obm->lk, &v, OBM_LOCKED) != 0) | |||||
break; | |||||
lock_delay(&lda); | |||||
continue; | |||||
Done Inline ActionsIs it right to spin in lock_delay() in this case? Shouldn't we try to acquire the lock immediately? markj: Is it right to spin in lock_delay() in this case? Shouldn't we try to acquire the lock… | |||||
Done Inline ActionsI reworked the slow loop, putting lock_delay() just in places where it is expected to occur instead of relying on the start of the loop. kib: I reworked the slow loop, putting lock_delay() just in places where it is expected to occur… | |||||
} | |||||
if ((v & OBM_CONTESTED) == 0 && | |||||
atomic_fcmpset_8(&obm->lk, &v, v | OBM_CONTESTED) == 0) { | |||||
turnstile_cancel(ts); | |||||
continue; | |||||
} | |||||
Done Inline ActionsIf this fails you may the lock is taken by someone else which adds an avoidable turnstile trip. Other primitives have a goto label. mjg: If this fails you may the lock is taken by someone else which adds an avoidable turnstile trip. | |||||
turnstile_wait(ts, NULL, TS_SHARED_QUEUE); | |||||
} | |||||
TD_LOCKS_INC(curthread); | |||||
} | |||||
void | |||||
obm_unlock_slow(obm_lock_t *obm, struct lock_object *lo) | |||||
{ | |||||
struct turnstile *ts; | |||||
#ifdef OBM_DEBUG | |||||
Done Inline ActionsIt is kind of strange to pass v here just for the assertion. Why not read the lock state after acquiring the turnstile lock, and assert on that? markj: It is kind of strange to pass `v` here just for the assertion. Why not read the lock state… | |||||
Done Inline ActionsI asserted in obm_unlock() instead. Same for obm_lock_slow(). kib: I asserted in obm_unlock() instead. Same for obm_lock_slow(). | |||||
atomic_add_long(&obm_slow_unlock, 1); | |||||
#endif | |||||
turnstile_chain_lock(lo); | |||||
atomic_store_rel_8(&obm->lk, OBM_UNLOCKED); | |||||
ts = turnstile_lookup(lo); | |||||
if (ts != NULL) { | |||||
turnstile_broadcast(ts, TS_SHARED_QUEUE); | |||||
turnstile_unpend(ts); | |||||
} | |||||
turnstile_chain_unlock(lo); | |||||
TD_LOCKS_DEC(curthread); | |||||
} | |||||
Do you plan to add WITNESS integration?