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

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

On 13.10.20 17:28, Jan Beulich wrote:
> On 12.10.2020 11:27, Juergen Gross wrote:
> > @@ -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_tryread_lock(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_tryread_lock(chn) )
> > +    {
> > +        evtchn_port_set_pending(d, v->vcpu_id, chn);
> Is this simply a copy-and-paste mistake (re-using the code from
> send_guest_vcpu_virq()), or is there a reason you switch from
> where to obtain the vCPU to send to (in which case this ought
> to be mentioned in the description, and in which case you could
> use literal zero)?
Thanks for spotting! Its a copy-and-paste mistake.

> > --- a/xen/include/xen/event.h
> > +++ b/xen/include/xen/event.h
> > @@ -105,6 +105,45 @@ 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    MAX_VIRT_CPUS
> Isn't the ceiling on simultaneous readers the number of pCPU-s,
> and the value here then needs to be NR_CPUS + 1 to accommodate
> the maximum number of readers? Furthermore, with you dropping
> the disabling of interrupts, one pCPU can acquire a read lock
> now more than once, when interrupting a locked region.
Yes, I think you are right.

So at least 2 * (NR-CPUS + 1), or even 3 * (NR_CPUS + 1) for covering
NMIs, too?

> > +static inline void evtchn_write_lock(struct evtchn *evtchn)
> > +{
> > +    int val;
> > +
> > +    /* 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;
> > +          val = atomic_read(&evtchn->lock) )
> > +        cpu_relax();
> > +}
> > +
> > +static inline void evtchn_write_unlock(struct evtchn *evtchn)
> > +{
> > +    arch_lock_release_barrier();
> > +
> > +    atomic_sub(EVENT_WRITE_LOCK_INC, &evtchn->lock);
> > +}
> > +
> > +static inline bool evtchn_tryread_lock(struct evtchn *evtchn)
> The corresponding "generic" function is read_trylock() - I'd
> suggest to use the same base name, with the evtchn_ prefix.
Okay.

> > @@ -274,12 +312,12 @@ static inline int evtchn_port_poll(struct domain *d, 
> > evtchn_port_t port)
> >       if ( port_is_valid(d, port) )
> >       {
> >           struct evtchn *evtchn = evtchn_from_port(d, port);
> > -        unsigned long flags;
> >   
> > -        spin_lock_irqsave(&evtchn->lock, flags);
> > -        if ( evtchn_usable(evtchn) )
> > +        if ( evtchn_tryread_lock(evtchn) && evtchn_usable(evtchn) )
> > +        {
> >               rc = evtchn_is_pending(d, evtchn);
> > -        spin_unlock_irqrestore(&evtchn->lock, flags);
> > +            evtchn_read_unlock(evtchn);
> > +        }
> >       }
> This needs to be two nested if()-s, as you need to drop the lock
> even when evtchn_usable() returns false.
Oh, yes.


Juergen