Re: [PATCH v4 02/10] evtchn: bind-interdomain doesn't need to hold both domains' event locks

[Julien Grall]

Hi Jan,

On 05/01/2021 13:09, Jan Beulich wrote:
The local domain's lock is needed for the port allocation, but for the
remote side the per-channel lock is sufficient. The per-channel locks
then need to be acquired slightly earlier, though.

I was expecting is little bit more information in the commit message 
because there are a few changes in behavior with this change:

 1) AFAICT, evtchn_allocate_port() rely on rchn->state to be protected 
by the rd->event_lock. Now that you dropped the rd->event_lock, 
rchn->state may be accessed while it is updated in 
evtchn_bind_interdomain(). The port cannot go back to ECS_FREE here, but 
I think the access needs to be switched to {read, write}_atomic() or 
ACCESS_ONCE.

  2) xsm_evtchn_interdomain() is now going to be called without the 
rd->event_lock. Can you confirm that the lock is not needed by XSM?

Signed-off-by: Jan Beulich <jbeul...@suse.com>
---
v4: New.

--- a/xen/common/event_channel.c
+++ b/xen/common/event_channel.c
@@ -355,18 +355,7 @@ static long evtchn_bind_interdomain(evtc
      if ( (rd = rcu_lock_domain_by_id(rdom)) == NULL )
          return -ESRCH;
  
-    /* Avoid deadlock by first acquiring lock of domain with smaller id. */
-    if ( ld < rd )
-    {
-        spin_lock(&ld->event_lock);
-        spin_lock(&rd->event_lock);
-    }
-    else
-    {
-        if ( ld != rd )
-            spin_lock(&rd->event_lock);
-        spin_lock(&ld->event_lock);
-    }
+    spin_lock(&ld->event_lock);
  
      if ( (lport = get_free_port(ld)) < 0 )
          ERROR_EXIT(lport);
@@ -375,15 +364,19 @@ static long evtchn_bind_interdomain(evtc
      if ( !port_is_valid(rd, rport) )
          ERROR_EXIT_DOM(-EINVAL, rd);
      rchn = evtchn_from_port(rd, rport);
+
+    double_evtchn_lock(lchn, rchn);
+
      if ( (rchn->state != ECS_UNBOUND) ||
           (rchn->u.unbound.remote_domid != ld->domain_id) )

You want to unlock the event channels here.

          ERROR_EXIT_DOM(-EINVAL, rd);
  
      rc = xsm_evtchn_interdomain(XSM_HOOK, ld, lchn, rd, rchn);
      if ( rc )
+    {
+        double_evtchn_unlock(lchn, rchn);
          goto out;
-
-    double_evtchn_lock(lchn, rchn);
+    }
  
      lchn->u.interdomain.remote_dom  = rd;
      lchn->u.interdomain.remote_port = rport;
@@ -407,8 +400,6 @@ static long evtchn_bind_interdomain(evtc
   out:
      check_free_port(ld, lport);
      spin_unlock(&ld->event_lock);
-    if ( ld != rd )
-        spin_unlock(&rd->event_lock);
      
      rcu_unlock_domain(rd);
  


Cheers,

--
Julien Grall