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

[Julien Grall <julien@xxxxxxx>]

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 <jbeulich@xxxxxxxx>
> ---
> 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