Re: [PATCH] evtchn/fifo: don't corrupt queues if an old tail is linked

[David Vrabel <david.vrabel@citrix.com>]

On 09/12/13 13:10, Jan Beulich wrote:
>>>> On 09.12.13 at 13:56, David Vrabel <david.vrabel@citrix.com> wrote:
>> On 09/12/13 12:21, Jan Beulich wrote:
>>>>>> On 09.12.13 at 12:49, David Vrabel <david.vrabel@citrix.com> wrote:
>>>> On 09/12/13 09:32, Jan Beulich wrote:
>>>>>>>> On 06.12.13 at 18:38, David Vrabel <david.vrabel@citrix.com> wrote:
>>>>>> --- a/xen/include/xen/sched.h
>>>>>> +++ b/xen/include/xen/sched.h
>>>>>> @@ -98,6 +98,8 @@ struct evtchn
>>>>>>      } u;
>>>>>>      u8 priority;
>>>>>>      u8 pending:1;
>>>>>> +    u16 last_vcpu_id;
>>>>>> +    u8 last_priority;
>>>>>
>>>>> Is it really correct for these two new fields to remain uninitialized
>>>>> until evtchn_fifo_set_pending() would get run the first time (and
>>>>> hence thinking there was a move this first time through)?
>>>>
>>>> They're initialized to zero and I think this is fine. The code as-is is
>>>> simpler than having to special case events that have never been on a queue.
>>>
>>> I'm not asking to add a special case, I'm only asking to initialize all
>>> fields correctly. Just like you ought to set up ->priority, you
>>> likely ought to set up the two new fields.
>>
>> It's not clear how you think they're not initialized.  They're
>> initialized to zero when the evtchn is allocated and then they must only
>> be set in evtchn_fifo_set_pending() when they move to a new queue.
> 
> My primary concern is with them being zero (and hence out of sync
> with the real values that things start out with), there may be subtle
> corruption later on. Secondary is that - as said - this would at least
> trigger one unnecessary move in evtchn_fifo_set_pending().

(0, 0) is still a valid queue and it is always safe to do:

  if ( old_q->tail == port )
     old_q->tail = 0

so I'm not seeing any risk of subtle corruption anywhere.

An unnecessary move once per port is hardly expensive so not something I
would introduce complexity in the common case to avoid.

>> Do you think they should be initialized when an event is (re)bound?
>> Because this would be broken as an unbound event might be an old tail.
> 
> But if you don't do this, then you _require_ a set-priority operation,
> yet that one's necessarily non-atomic with the bind. Newly created
> event channels should start out at the default priority irrespective
> of what the underlying tracking structure in the hypervisor was
> used for before.

Xen can only move an event between queues if that event isn't on a
queue.  It is also not notified when an event is removed from a queue.

The guest can ensure a predictable state by only unbinding events that
are not currently on a queue.  e.g.,

    /* prevent it becoming LINKED. */
   set_bit(word, MASKED)
   /* wait for interrupt handlers to drain event from its queue. */
   while (test_bit(word, LINKED))
      ;
   /* Unlinked and masked, safe to unbind. If this port is bound again
      it will becoming pending on the correct new queue. */
   unbind()

There doesn't need to be anything added to Xen to support this.

The guest may need to defer to wait and unbind to a work queue or similar.

David