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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 02.11.2020 14:41, Jürgen Groß wrote:
> 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.

This may be such a race yes, but why do you think it _will_ be?
Any holding of the lock for writing (or in fact, any pending
acquire in write mode) will make this fail, which - if it's not
such a race - will mean an event which wasn't sent when it
should have been, with potentially fatal (to the guest)
consequences.

>>> @@ -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?

"Problem" can have a wide range of meanings - it's not going to
be the end of the world, but I view any use of a write lock as
a problem when a read lock would suffice. This can still harm
parallelism.

Jan