(x | y) - y why can't it simply be x or even `x | 0`

The code is found in _spin_lock_contested, which is called from _spin_lock_quick when someone else is attempting to obtain the lock :

count = atomic_fetchadd_int(&spin->counta, 1);
if (__predict_false(count != 0)) {
    _spin_lock_contested(spin, ident, count);
}

If there's no contest, then count (the previous value) should be 0, but it isn't. This count value is passed as parameter to _spin_lock_contested as the value parameter. This value is then checked with the if from the OP :

/*
 * WARNING! Caller has already incremented the lock.  We must
 *      increment the count value (from the inline's fetch-add)
 *      to match.
 *
 * Handle the degenerate case where the spinlock is flagged SHARED
 * with only our reference.  We can convert it to EXCLUSIVE.
 */
if (value == (SPINLOCK_SHARED | 1) - 1) {
    if (atomic_cmpset_int(&spin->counta, SPINLOCK_SHARED | 1, 1))
        return;
}

Keeping in mind that value is the previous value of spin->counta, and the latter has already been incremented by 1, we expect spin->counta to equal value + 1 (unless something has changed in the meantime).

So, checking if spin->counta == SPINLOCK_SHARED | 1 (the precondition of the atomic_cmpset_int) corresponds to checking if value + 1 == SPINLOCK_SHARED | 1, which can be rewritten as value == (SPINLOCK_SHARED | 1) - 1 (again, if nothing has changed in the meantime).

While value == (SPINLOCK_SHARED | 1) - 1 could be rewritten as value == SPINLOCK_SHARED, it's left as is, to clarify the intent of the comparison (ie. to compare the incremented previous value with the test value).

Or iow. the answer appears to be : for clarity and code consistency.

I think the goal is probably to ignore the lowest significant bit:

• If SPINLOCK_SHARED expressed in binary is xxx0 -> result is xxx0
• If SPINLOCK_SHARED = xxx1 -> result is also xxx0

would have been perhaps clearer to use a bit mask expression ?

The effect of

(SPINLOCK_SHARED | 1) - 1

is to ensure that the low-order bit of the result is cleared prior to the comparison with value. I agree that it seems rather pointless but apparently the low-order bit has a particular usage or meaning which is not apparent in this code, and I think we have to assume that the devs had a good reason for doing this. An interesting question would be - is this same pattern (| 1) -1) used throughout the codebase you're looking at?