[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v2 1/2] xen/events: access last_priority and last_vcpu_id together

On 13.10.2020 16:20, Jürgen Groß wrote:
> On 13.10.20 15:58, Jan Beulich wrote:
>> On 12.10.2020 11:27, Juergen Gross wrote:
>>> The queue for a fifo event is depending on the vcpu_id and the
>>> priority of the event. When sending an event it might happen the
>>> event needs to change queues and the old queue needs to be kept for
>>> keeping the links between queue elements intact. For this purpose
>>> the event channel contains last_priority and last_vcpu_id values
>>> elements for being able to identify the old queue.
>>> In order to avoid races always access last_priority and last_vcpu_id
>>> with a single atomic operation avoiding any inconsistencies.
>>> Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
>> I seem to vaguely recall that at the time this seemingly racy
>> access was done on purpose by David. Did you go look at the
>> old commits to understand whether there really is a race which
>> can't be tolerated within the spec?
> At least the comments in the code tell us that the race regarding
> the writing of priority (not last_priority) is acceptable.

Ah, then it was comments. I knew I read something to this effect
somewhere, recently.

> Especially Julien was rather worried by the current situation. In
> case you can convince him the current handling is fine, we can
> easily drop this patch.

Julien, in the light of the above - can you clarify the specific
concerns you (still) have?

>>> --- a/xen/include/xen/sched.h
>>> +++ b/xen/include/xen/sched.h
>>> @@ -114,8 +114,7 @@ struct evtchn
>>>           u16 virq;      /* state == ECS_VIRQ */
>>>       } u;
>>>       u8 priority;
>>> -    u8 last_priority;
>>> -    u16 last_vcpu_id;
>>> +    u32 fifo_lastq;    /* Data for fifo events identifying last queue. */
>> This grows struct evtchn's size on at least 32-bit Arm. I'd
>> like to suggest including "priority" in the union, and call the
>> new field simply "fifo" or some such.
> This will add quite some complexity as suddenly all writes to the
> union will need to be made through a cmpxchg() scheme.
> Regarding the growth: struct evtchn is aligned to 64 bytes. So there
> is no growth at all, as the size will not be larger than those 64
> bytes.

Oh, I didn't spot this attribute, which I consider at least
suspicious. Without XSM I'm getting the impression that on 32-bit
Arm the structure's size would be 32 bytes or less without it, so
it looks as if it shouldn't be there unconditionally.




Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.