On Thu, Feb 20, 2020 at 01:48:54PM +0100, Jan Beulich wrote:
On 20.02.2020 13:02, Roger Pau Monne wrote:
I've done some testing and at least the CPU down case is fixed now.
Posting early in order to get feedback on the approach taken.
Looks good, thanks, just a question and two comments:
--- a/xen/include/xen/rwlock.h
+++ b/xen/include/xen/rwlock.h
@@ -20,21 +20,30 @@ typedef struct {
#define DEFINE_RWLOCK(l) rwlock_t l = RW_LOCK_UNLOCKED
#define rwlock_init(l) (*(l) = (rwlock_t)RW_LOCK_UNLOCKED)
-/*
- * Writer states & reader shift and bias.
- *
- * Writer field is 8 bit to allow for potential optimisation, see
- * _write_unlock().
- */
-#define _QW_WAITING 1 /* A writer is waiting */
-#define _QW_LOCKED 0xff /* A writer holds the lock */
-#define _QW_WMASK 0xff /* Writer mask.*/
-#define _QR_SHIFT 8 /* Reader count shift */
+/* Writer states & reader shift and bias. */
+#define _QW_WAITING 1 /* A writer is waiting */
+#define _QW_LOCKED 3 /* A writer holds the lock */
Aiui things would work equally well if 2 was used here?
I think so, I left it as 3 because previously LOCKED would also
include WAITING, and I didn't want to change it in case I've missed
some code path that was relying on that.
+#define _QW_WMASK 3 /* Writer mask */
+#define _QW_CPUSHIFT 2 /* Writer CPU shift */
+#define _QW_CPUMASK 0x3ffc /* Writer CPU mask */
At least on x86, the shift involved here is quite certainly
more expensive than using wider immediates on AND and CMP
resulting from the _QW_MASK-based comparisons. I'd therefore
like to suggest to put the CPU in the low 12 bits.
Hm right. The LOCKED and WAITING bits don't need shifting anyway.
Another option is to use the recurse_cpu field of the
associated spin lock: The field is used for recursive locks
only, and hence the only conflict would be with
_spin_is_locked(), which we don't (and in the future then
also shouldn't) use on this lock.
I looked into that also, but things get more complicated AFAICT, as it's
not possible to atomically fetch the state of the lock and the owner
CPU at the same time. Neither you could set the LOCKED bit and the CPU
at the same time.
@@ -166,7 +180,8 @@ static inline void _write_unlock(rwlock_t *lock)
* If the writer field is atomic, it can be cleared directly.
* Otherwise, an atomic subtraction will be used to clear it.
*/
- atomic_sub(_QW_LOCKED, &lock->cnts);
+ ASSERT(_is_write_locked_by_me(atomic_read(&lock->cnts)));
+ atomic_sub(_write_lock_val(), &lock->cnts);
I think this would be more efficient with atomic_and(), not
the least because of the then avoided smp_processor_id().
Whether to mask off just _QW_WMASK or also the CPU number of
the last write owner would need to be determined. But with
using subtraction, in case of problems it'll likely be
harder to understand what actually went on, from looking at
the resulting state of the lock (this is in part a pre-
existing problem, but gets worse with subtraction of CPU
numbers).
Right, a mask would be better. Right now both need to be cleared (the
LOCKED and the CPU fields) as there's code that relies on !lock->cnts
as a way to determine that the lock is not read or write locked. If we
left the CPU lying around those checks would need to be adjusted.
