Re: [Xen-devel] [PATCH 3/3] evtchn/fifo: don't spin indefinitely when setting LINK

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 04.11.13 at 17:30, David Vrabel <david.vrabel@xxxxxxxxxx> wrote:
> On 04/11/13 14:57, Jan Beulich wrote:
>>>>> On 04.11.13 at 15:52, David Vrabel <david.vrabel@xxxxxxxxxx> wrote:
>>> On 04/11/13 14:39, Jan Beulich wrote:
>>>>>>> On 31.10.13 at 16:03, David Vrabel <david.vrabel@xxxxxxxxxx> wrote:
>>>>>          event_word_t *tail_word;
>>>>>          bool_t linked = 0;
>>>>>  
>>>>> +        evtchn->q = q;
>>>>> +
>>>>>          spin_lock_irqsave(&q->lock, flags);
>>>>>  
>>>>>          /*
>>>>
>>>> I fail to see how this change is related to the rest of the patch.
>>>
>>> This is needed so the correct queue lock is used in evtchn_fifo_unmask().
>> 
>> I guessed that, but I wasn't able to immediately see why that would
>> not have been a requirement before.
> 
> An event now has two queues associated with it.
> 
> 1. The queue it is supposed to be on.  This is found with
> evtchn->notify_vcpu_id and evtchn->priority when an event is being linked.
> 
> 2. The queue the event is on right now (the new evtchn->q), until this
> patch we didn't care which queue an event was on, only that it was on
> some queue.
> 
> These are not necessarily the same since an event may be moved between
> VCPUs or have its priority changed while it is already on a queue.
> 
> Some additional notes on the locking:
> 
> The locking in the FIFO-based ABI is designed on the assumption that
> every hypervisor operation on an event channel (set pending, unmask) is
> done while holding a per-event channel spin lock.
> 
> Currently, the per-event locking is done with the coarser per-domain
> event_lock.
> 
> The queue lock serializes adding events and clearing MASKED on tail events.
> 
> The event lock also protects evtchn->q and the evtchn->q->tail ==
> evtchn->port test.

But aren't you setting evtchn->q too late then?
evtchn_fifo_unmask() racing evtchn_fifo_set_pending() could
then lead to the latter seeing ->q still clear, the former setting it
and doing its linking work while in parallel the latter might clear
MASKED at any time.

Plus - shouldn't you clear ->q at some point again?

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel