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
