Re: [PATCH v4.1 2/2] xen/evtchn: rework per event channel lock

[Jan Beulich <jbeulich@xxxxxxxx>]

On 04.11.2020 16:53, Jürgen Groß wrote:
> On 04.11.20 16:29, Jan Beulich wrote:
>>> @@ -738,7 +725,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;
>>
>> Isn't there a change in behavior here? While sends through
>> ECS_UNBOUND ports indeed get silently ignored, ECS_FREE ones ought
>> to be getting -EINVAL (as should ECS_UNBOUND ones if they're
>> Xen-consumer ones). With the failed trylock you don't know which
>> of the two the port is in the process of being transitioned
>> to/from. The same would apply for other operations distinguishing
>> the two states. Right now both evtchn_status() and
>> evtchn_bind_vcpu() only use the domain-wide lock, but the latter
>> is getting switched by "evtchn: convert domain event lock to an
>> r/w one" (granted there's an RFC remark there whether that
>> transformation is worthwhile). Anyway, the main point of my remark
>> is that there's another subtlety here which I don't think becomes
>> obvious from description or comments - where the two states are
>> mentioned, it gets to look as if they can be treated equally.
> 
> Hmm, evtchn_send() seems to be called always with interrupts enabled.
> We could just use a standard read_lock() here if you think the different
> states should be treated as today.

This would avoid the caveat in this specific case, but it would
remain elsewhere (at least as an abstract trap to fall into). I
suppose evtchn_status() could use a regular read_lock() too, for
the same reason (if it was to be switched), and evtchn_bind_vcpu()
may need a write lock anyway (which is forbidden in your model,
i.e. I'd likely need to drop the switch to the finer grained lock
there).

>>> --- a/xen/include/xen/event.h
>>> +++ b/xen/include/xen/event.h
>>> @@ -105,6 +105,39 @@ 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])
>>>   
>>> +/*
>>> + * Lock an event channel exclusively. This is allowed only 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.
>>> + */
>>> +static inline void evtchn_write_lock(struct evtchn *evtchn)
>>> +{
>>> +    write_lock(&evtchn->lock);
>>> +
>>> +    evtchn->old_state = evtchn->state;
>>> +}
>>> +
>>> +static inline void evtchn_write_unlock(struct evtchn *evtchn)
>>> +{
>>> +    /* Enforce lock discipline. */
>>> +    ASSERT(evtchn->old_state == ECS_FREE || evtchn->old_state == 
>>> ECS_UNBOUND ||
>>> +           evtchn->state == ECS_FREE || evtchn->state == ECS_UNBOUND);
>>> +
>>> +    write_unlock(&evtchn->lock);
>>> +}
>>
>> These two aren't needed outside of event_channel.c, are they? If
>> so, and if they ought to go in a header rather than directly into
>> the .c file (where I'd prefer the latter, for the sake of minimal
>> exposure), then it should be event_channel.h.
> 
> I wanted to have the locking functions in one place.
> 
> In case you prefer it otherwise (and you seem to do so) I'd rather move
> the write lock functions to the .c file.

Yes please.

Jan