[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCHv1 4/4] spinlock: add fair read-write locks



Hi Jan,

Thank you for your comments.  Sorry for the slow response - xmas and all...
Responses below...


On 22/12/15 13:54, Jan Beulich wrote:
+/**
+ * rspin_until_writer_unlock - inc reader count & spin until writer is gone
Stale comment - the function doesn't increment anything.

Also throughout the file, with Linux coding style converted to Xen
style, comment style should be made Xen-like too.

Oh yes, missed that - will fix.


+    /*
+     * Readers come here when they cannot get the lock without waiting
+     */
+    if ( unlikely(in_irq()) )
+    {
+        /*
+         * Readers in interrupt context will spin until the lock is
+         * available without waiting in the queue.
+         */
+        smp_rmb();
+        cnts = atomic_read(&lock->cnts);
+        rspin_until_writer_unlock(lock, cnts);
+        return;
+    }
I can't immediately see the reason for this re-introduction of
unfairness - can you say a word on this, or perhaps extend the
comment?

We haven't found a reason this was introduced to Linux, but assume this was to reduce interrupt latency. I had thought to leave it there, in case we want to use it in the future, but now feel it would probably better to remove it, and deal with any irq related issues, if and when we use rw locks from irq handlers.


-        x = lock->lock & ~RW_WRITE_FLAG;
-    }
-    preempt_disable();
-}
-
-void _write_lock_irq(rwlock_t *lock)
-{
-    uint32_t x;
+        cnts = atomic_read(&lock->cnts);
+        if ( !(cnts & _QW_WMASK) &&
+             (atomic_cmpxchg(&lock->cnts, cnts,
+                             cnts | _QW_WAITING) == cnts) )
+            break;
Considering that (at least on x86) cmpxchg is relatively expensive,
and that atomic OR would be carried out by some cmpxchg-like
mechanism on most other arches anyway, can't this be an atomic
OR, followed by a read to check for another active writer?

I wanted to port know proven code. We have now run this code in xen for quite a while, and while I think you may well be correct, I'd rather get this version in, and then consider further optimisation testing in future patches.

+unlock:
+    spin_unlock(&lock->lock);
Labels indented by at least one space please.

Also - are you using any of the static functions in spinlock.c? If not,
putting the rwlock code in a new rwlock.c would help review quite a
bit, since code removal and code addition would then be separate
rather than intermixed.

Great idea. I think the reasons for keeping them together must just be historical. I'll try and split that out.


+/*
+ * Writer states & reader shift and bias
+ */
+#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      */
+#define    _QR_BIAS     (1U << _QR_SHIFT)
Is there a particular reason for the 8-bit writer mask (a 2-bit one
would seem to suffice)?

You picked up on optimisation in your later comment - I should add a comment here.

+static inline int _write_trylock(rwlock_t *lock)
+{
+    u32 cnts;
+
+    cnts = atomic_read(&lock->cnts);
+    if ( unlikely(cnts) )
+        return 0;
+
+    return likely(atomic_cmpxchg(&lock->cnts,
+                                 cnts, cnts | _QW_LOCKED) == cnts);
The | is pointless here considering that cnts is zero.

I theorised that this was like this to aid the readability of the code, although I don't know if it does. I'd happily change it over, and replace the cnts with 0s.

+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);
+}
Ah, I guess the comment here is the explanation for the 8-bit
write mask.

Yes. A comment on the declaration would no doubt help too.

+static inline int _rw_is_write_locked(rwlock_t *lock)
+{
+    return atomic_read(&lock->cnts) & _QW_WMASK;
+}
This returns true for write-locked or writer-waiting - is this intended?

It was, but having thought about it a bit more, it would only have been "more useful" like this in unsafe (or at lest ugly) code, and so for that reason I should change it. I don't think this would have affected the safe cases.
Jan

Cheers

-jenny


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.