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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 09.12.13 at 13:56, David Vrabel <david.vrabel@xxxxxxxxxx> wrote:
> On 09/12/13 12:21, Jan Beulich wrote:
>>>>> On 09.12.13 at 12:49, David Vrabel <david.vrabel@xxxxxxxxxx> wrote:
>>> On 09/12/13 09:32, Jan Beulich wrote:
>>>>>>> On 06.12.13 at 18:38, David Vrabel <david.vrabel@xxxxxxxxxx> 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().

> 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.

If that causes an issue with other state, then this needs addressing
(and should not serve as an excuse to leave things in an unpredictable
- from the guest's perspective - state).

Jan


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