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

[Jürgen Groß <jgross@xxxxxxxx>]

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.

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.

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


Juergen