Re: [PATCH v3 4/5] evtchn: convert domain event lock to an r/w one

[Julien Grall <julien@xxxxxxx>]

Hi Jan,

On 11/12/2020 10:32, Jan Beulich wrote:
> On 09.12.2020 12:54, Julien Grall wrote:
> > On 23/11/2020 13:29, Jan Beulich wrote:
> > > @@ -620,7 +620,7 @@ int evtchn_close(struct domain *d1, int
> > >        long           rc = 0;
> > >    
> > >     again:
> > > -    spin_lock(&d1->event_lock);
> > > +    write_lock(&d1->event_lock);
> > >    
> > >        if ( !port_is_valid(d1, port1) )
> > >        {
> > > @@ -690,13 +690,11 @@ int evtchn_close(struct domain *d1, int
> > >                    BUG();
> > >    
> > >                if ( d1 < d2 )
> > > -            {
> > > -                spin_lock(&d2->event_lock);
> > > -            }
> > > +                read_lock(&d2->event_lock);
> > This change made me realized that I don't quite understand how the
> > rwlock is meant to work for event_lock. I was actually expecting this to
> > be a write_lock() given there are state changed in the d2 events.
> Well, the protection needs to be against racing changes, i.e.
> parallel invocations of this same function, or evtchn_close().
> It is debatable whether evtchn_status() and
> domain_dump_evtchn_info() would better also be locked out
> (other read_lock() uses aren't applicable to interdomain
> channels).
>
> > Could you outline how a developper can find out whether he/she should
> > use read_lock or write_lock?
> I could try to, but it would again be a port type dependent
> model, just like for the per-channel locks.
It is quite important to have clear locking strategy (in particular 
rwlock) so we can make correct decision when to use read_lock or write_lock.
> So I'd like it to
> be clarified first whether you aren't instead indirectly
> asking for these to become write_lock()
Well, I don't understand why this is a read_lock() (even with your 
previous explanation). I am not suggesting to switch to a write_lock(), 
but instead asking for the reasoning behind the decision.
> > > --- a/xen/common/rwlock.c
> > > +++ b/xen/common/rwlock.c
> > > @@ -102,6 +102,14 @@ void queue_write_lock_slowpath(rwlock_t
> > >        spin_unlock(&lock->lock);
> > >    }
> > >    
> > > +void _rw_barrier(rwlock_t *lock)
> > > +{
> > > +    check_barrier(&lock->lock.debug);
> > > +    smp_mb();
> > > +    while ( _rw_is_locked(lock) )
> > > +        arch_lock_relax();
> > > +    smp_mb();
> > > +}
> > As I pointed out when this implementation was first proposed (see [1]),
> > there is a risk that the loop will never exit.
> The [1] reference was missing, but I recall you saying so.
>
> > I think the following implementation would be better (although it is ugly):
> >
> > write_lock();
> > /* do nothing */
> > write_unlock();
> >
> > This will act as a barrier between lock held before and after the call.
> Right, and back then I indicated agreement. When getting to
> actually carry out the change, I realized though that then the less
> restrictive check_barrier() can't be used anymore (or to be precise,
> it could be used, but the stronger check_lock() would subsequently
> still come into play). This isn't a problem here, but would be for
> any IRQ-safe r/w lock that the barrier may want to be used on down
> the road.
>
> Thinking about it, a read_lock() / read_unlock() pair would suffice
> though. But this would then still have check_lock() involved.
>
> Given all of this, maybe it's better not to introduce the function
> at all and instead open-code the read_lock() / read_unlock() pair at
> the use site.
IIUC, the read_lock() would be sufficient because we only care about 
"write" side and not read. Is that correct?
> > As an aside, I think the introduction of rw_barrier() deserve to be a in
> > separate patch to help the review.
> I'm aware there are differing views on this - to me, putting this in
> a separate patch would be introduction of dead code. 
This is only dead code if we decide to not use rw_barrier() :).

The idea behind introducing rw_barrier() in its own patch is so you can 
explanation why it was implemented like that. Arguably, this explanation 
can be added in the same patch...
There are other added benefits such as making a hint to the reviewer 
that this part will require more careful review. I am sure one will say 
that reviewer should always be careful...
But, personally, my level of carefulness will depend on the author and 
the type of the patch.
Anyway, I am happy with the open-coded version with an explanation in 
the code/commit message.
Cheers,

--
Julien Grall