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

Re: [PATCH v3 4/5] evtchn: convert domain event lock to an r/w one



Hi Jan,

On 23/11/2020 13:29, Jan Beulich wrote:
@@ -620,7 +620,7 @@ int evtchn_close(struct domain *d1, int
      long           rc = 0;
again:
-    spin_lock(&d1->event_lock);
+    write_lock(&d1->event_lock);
if ( !port_is_valid(d1, port1) )
      {
@@ -690,13 +690,11 @@ int evtchn_close(struct domain *d1, int
                  BUG();
if ( d1 < d2 )
-            {
-                spin_lock(&d2->event_lock);
-            }
+                read_lock(&d2->event_lock);

This change made me realized that I don't quite understand how the rwlock is meant to work for event_lock. I was actually expecting this to be a write_lock() given there are state changed in the d2 events.

Could you outline how a developper can find out whether he/she should use read_lock or write_lock?

[...]

--- a/xen/common/rwlock.c
+++ b/xen/common/rwlock.c
@@ -102,6 +102,14 @@ void queue_write_lock_slowpath(rwlock_t
      spin_unlock(&lock->lock);
  }
+void _rw_barrier(rwlock_t *lock)
+{
+    check_barrier(&lock->lock.debug);
+    smp_mb();
+    while ( _rw_is_locked(lock) )
+        arch_lock_relax();
+    smp_mb();
+}

As I pointed out when this implementation was first proposed (see [1]), there is a risk that the loop will never exit.

I think the following implementation would be better (although it is ugly):

write_lock();
/* do nothing */
write_unlock();

This will act as a barrier between lock held before and after the call.

As an aside, I think the introduction of rw_barrier() deserve to be a in separate patch to help the review.

Cheers,

--
Julien Grall



 


Rackspace

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