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 <jgr...@suse.com> >> >> 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. Jan
