Re: [PATCH v6 1/3] evtchn: slightly defer lock acquire where possible

[Julien Grall <julien@xen.org>]

Hi Jan,

On 27/05/2021 12:28, Jan Beulich wrote:
> port_is_valid() and evtchn_from_port() are fine to use without holding
> any locks. Accordingly acquire the per-domain lock slightly later in
> evtchn_close() and evtchn_bind_vcpu().

So I agree that port_is_valid() and evtchn_from_port() are fine to use 
without holding any locks in evtchn_bind_vcpu(). However, this is 
misleading to say there is no problem with evtchn_close().

evtchn_close() can be called with current != d and therefore, there is a 
risk that port_is_valid() may be valid and then invalid because 
d->valid_evtchns is decremented in evtchn_destroy().

Thankfully the memory is still there. So the current code is okayish and 
I could reluctantly accept this behavior to be spread. However, I don't 
think this should be left uncommented in both the code (maybe on top of 
port_is_valid()?) and the commit message.

> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> v6: Re-base for re-ordering / shrinking of series.
> v4: New.
> 
> --- a/xen/common/event_channel.c
> +++ b/xen/common/event_channel.c
> @@ -606,17 +606,14 @@ int evtchn_close(struct domain *d1, int
>       int            port2;
>       long           rc = 0;
>   
> - again:
> -    spin_lock(&d1->event_lock);
> -
>       if ( !port_is_valid(d1, port1) )
> -    {
> -        rc = -EINVAL;
> -        goto out;
> -    }
> +        return -EINVAL;
>   
>       chn1 = evtchn_from_port(d1, port1);
>   
> + again:
> +    spin_lock(&d1->event_lock);
> +
>       /* Guest cannot close a Xen-attached event channel. */
>       if ( unlikely(consumer_is_xen(chn1)) && guest )
>       {
> @@ -1041,16 +1038,13 @@ long evtchn_bind_vcpu(unsigned int port,
>       if ( (v = domain_vcpu(d, vcpu_id)) == NULL )
>           return -ENOENT;
>   
> -    spin_lock(&d->event_lock);
> -
>       if ( !port_is_valid(d, port) )
> -    {
> -        rc = -EINVAL;
> -        goto out;
> -    }
> +        return -EINVAL;
>   
>       chn = evtchn_from_port(d, port);
>   
> +    spin_lock(&d->event_lock);
> +
>       /* Guest cannot re-bind a Xen-attached event channel. */
>       if ( unlikely(consumer_is_xen(chn)) )
>       {
> 

Cheers,

-- 
Julien Grall