Re: [PATCH 11/12] evtchn: convert vIRQ lock to an r/w one

[Julien Grall <julien@xxxxxxx>]

Hi Jan,

On 29/09/2020 14:37, Jan Beulich wrote:
> On 29.09.2020 15:03, Julien Grall wrote:
> > On 28/09/2020 12:02, Jan Beulich wrote:
> > > There's no need to serialize all sending of vIRQ-s; all that's needed
> > > is serialization against the closing of the respective event channels
> > > (by means of a barrier). To facilitate the conversion, introduce a new
> > > rw_barrier().
> > Looking at the code below, all the spin_lock() have been replaced by a
> > read_lock_*(). This is a bit surprising,
> It is, I agree, but that's what it ends up being. It is my understanding
> that the lock really only exists for the purpose of the barrier in
> evtchn_close().
>
> > > --- a/xen/common/spinlock.c
> > > +++ b/xen/common/spinlock.c
> > > @@ -2,7 +2,7 @@
> > >    #include <xen/irq.h>
> > >    #include <xen/smp.h>
> > >    #include <xen/time.h>
> > > -#include <xen/spinlock.h>
> > > +#include <xen/rwlock.h>
> > I would prefer if keep including <xen/spinlock.h> as the fact
> > <xen/rwlock.h> include it is merely an implementation details.
> Can do, sure.
>
> > > @@ -334,6 +334,12 @@ void _spin_unlock_recursive(spinlock_t *
> > >        }
> > >    }
> > >    
> > > +void _rw_barrier(rwlock_t *lock)
> > > +{
> > > +    check_barrier(&lock->lock.debug);
> > > +    do { smp_mb(); } while ( _rw_is_locked(lock) );
> > > +}
> > Why do you need to call smp_mb() at each loop? Would not it be
> > sufficient to write something similar to spin_barrier(). I.e:
> >
> > smp_mb();
> > while ( _rw_is_locked(lock) )
> >     cpu_relax();
> > smp_mb();
> Yes, this looks to be possible. Both for this and the question
> below it may be relevant to know that this patch pre-dates the
> conversion to ticket locks by quite a bit. Hence the construct
> above resembles the _spin_barrier() back at the time.
>
> > But I wonder if there is a risk with either implementation for
> > _rw_is_locked() to always return true and therefore never end.
> >
> > Let say we receive an interrupt, by the time it is handled, the
> > read/lock may have been taken again.
> With non-ticket locks I would say there was the same issue. But
> yes, ...
Most likely yes.

> > spin_barrier() seems to handle this situation fine because it just wait
> > for the head to change. I don't think we can do the same here...
> >
> > I am thinking that it may be easier to hold the write lock when doing
> > the update.
> ... perhaps this is indeed better. I have to admit that I never
> fully understood the benefit of using spin_barrier() in this code
> (as opposed to the use in evtchn_destroy()).
I am not entirely sure either. It looks like it is an attempt to make 
v->virq_to_evtchn[X] visible without holding a lock.
Any holder of the lock after spin_barrier() has completed will read 0 
and not try to use the lock.
But the update of v->virq_to_evtchn[X] should have used either 
ACCESS_ONCE() or write_atomic().
Cheers,

--
Julien Grall