Re: [PATCH v3 2/2] xen/evtchn: rework per event channel lock

[Jürgen Groß <jgross@xxxxxxxx>]

On 20.10.20 11:28, Jan Beulich wrote:
> On 16.10.2020 12:58, Juergen Gross wrote:
> > --- a/xen/arch/x86/pv/shim.c
> > +++ b/xen/arch/x86/pv/shim.c
> > @@ -660,11 +660,12 @@ void pv_shim_inject_evtchn(unsigned int port)
> >       if ( port_is_valid(guest, port) )
> >       {
> >           struct evtchn *chn = evtchn_from_port(guest, port);
> > -        unsigned long flags;
> >   
> > -        spin_lock_irqsave(&chn->lock, flags);
> > -        evtchn_port_set_pending(guest, chn->notify_vcpu_id, chn);
> > -        spin_unlock_irqrestore(&chn->lock, flags);
> > +        if ( evtchn_read_trylock(chn) )
> > +        {
> > +            evtchn_port_set_pending(guest, chn->notify_vcpu_id, chn);
> > +            evtchn_read_unlock(chn);
> > +        }
> Does this want some form of else, e.g. at least a printk_once()?
No, I don't think so.

This is just a race with the port_is_valid() test above where the
port is just being switched to invalid.

> > @@ -360,7 +352,7 @@ static long 
> > evtchn_bind_interdomain(evtchn_bind_interdomain_t *bind)
> >       if ( rc )
> >           goto out;
> >   
> > -    flags = double_evtchn_lock(lchn, rchn);
> > +    double_evtchn_lock(lchn, rchn);
> This introduces an unfortunate conflict with my conversion of
> the per-domain event lock to an rw one: It acquires rd's lock
> in read mode only, while the requirements here would not allow
> doing so. (Same in evtchn_close() then.)
Is it a problem to use write mode for those cases?

> > @@ -736,7 +723,8 @@ int evtchn_send(struct domain *ld, unsigned int lport)
> >   
> >       lchn = evtchn_from_port(ld, lport);
> >   
> > -    spin_lock_irqsave(&lchn->lock, flags);
> > +    if ( !evtchn_read_trylock(lchn) )
> > +        return 0;
> With this, the auxiliary call to xsm_evtchn_send() up from
> here should also go away again (possibly in a separate follow-
> on, which would then likely be a clean revert).
Yes.

> > @@ -798,9 +786,11 @@ void send_guest_vcpu_virq(struct vcpu *v, uint32_t virq)
> >   
> >       d = v->domain;
> >       chn = evtchn_from_port(d, port);
> > -    spin_lock(&chn->lock);
> > -    evtchn_port_set_pending(d, v->vcpu_id, chn);
> > -    spin_unlock(&chn->lock);
> > +    if ( evtchn_read_trylock(chn) )
> > +    {
> > +        evtchn_port_set_pending(d, v->vcpu_id, chn);
> > +        evtchn_read_unlock(chn);
> > +    }
> >   
> >    out:
> >       spin_unlock_irqrestore(&v->virq_lock, flags);
> > @@ -829,9 +819,11 @@ void send_guest_global_virq(struct domain *d, uint32_t 
> > virq)
> >           goto out;
> >   
> >       chn = evtchn_from_port(d, port);
> > -    spin_lock(&chn->lock);
> > -    evtchn_port_set_pending(d, chn->notify_vcpu_id, chn);
> > -    spin_unlock(&chn->lock);
> > +    if ( evtchn_read_trylock(chn) )
> > +    {
> > +        evtchn_port_set_pending(d, chn->notify_vcpu_id, chn);
> > +        evtchn_read_unlock(chn);
> > +    }
> >   
> >    out:
> >       spin_unlock_irqrestore(&v->virq_lock, flags);
> As said before, I think these lock uses can go away altogether.
> I shall put together a patch.
>
> And on the whole I'd really prefer if we first convinced ourselves
> that there's no way to simply get rid of the IRQ-safe locking
> forms instead, before taking a decision to go with this model with
> its extra constraints.
>
> > @@ -1060,15 +1053,16 @@ int evtchn_unmask(unsigned int port)
> >   {
> >       struct domain *d = current->domain;
> >       struct evtchn *evtchn;
> > -    unsigned long flags;
> >   
> >       if ( unlikely(!port_is_valid(d, port)) )
> >           return -EINVAL;
> >   
> >       evtchn = evtchn_from_port(d, port);
> > -    spin_lock_irqsave(&evtchn->lock, flags);
> > -    evtchn_port_unmask(d, evtchn);
> > -    spin_unlock_irqrestore(&evtchn->lock, flags);
> > +    if ( evtchn_read_trylock(evtchn) )
> > +    {
> > +        evtchn_port_unmask(d, evtchn);
> > +        evtchn_read_unlock(evtchn);
> > +    }
> I think this wants mentioning together with send / query in the
> description.
Okay.

> > --- a/xen/include/xen/event.h
> > +++ b/xen/include/xen/event.h
> > @@ -105,6 +105,60 @@ void notify_via_xen_event_channel(struct domain *ld, int 
> > lport);
> >   #define bucket_from_port(d, p) \
> >       ((group_from_port(d, p))[((p) % EVTCHNS_PER_GROUP) / EVTCHNS_PER_BUCKET])
> >   
> > +#define EVENT_WRITE_LOCK_INC    INT_MIN
> > +
> > +/*
> > + * Lock an event channel exclusively. This is allowed only with holding
> > + * d->event_lock AND when the channel is free or unbound either when taking
> > + * or when releasing the lock, as any concurrent operation on the event
> > + * channel using evtchn_read_trylock() will just assume the event channel is
> > + * free or unbound at the moment.
> ... when the evtchn_read_trylock() returns false.
Okay.

> > + */
> > +static inline void evtchn_write_lock(struct evtchn *evtchn)
> > +{
> > +    int val;
> > +
> > +    /*
> > +     * The lock can't be held by a writer already, as all writers need to
> > +     * hold d->event_lock.
> > +     */
> > +    ASSERT(atomic_read(&evtchn->lock) >= 0);
> > +
> > +    /* No barrier needed, atomic_add_return() is full barrier. */
> > +    for ( val = atomic_add_return(EVENT_WRITE_LOCK_INC, &evtchn->lock);
> > +          val != EVENT_WRITE_LOCK_INC;
> The _INC suffix is slightly odd for this 2nd use, but I guess
> the dual use will make it so for about any name you may pick.
I'll switch to a normal rwlock in V4.


Juergen