Re: [PATCH v8 3/3] xen/events: do some cleanups in evtchn_fifo_set_pending()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 27.11.2020 16:17, Julien Grall wrote:
> 
> 
> On 27/11/2020 14:48, Jan Beulich wrote:
>> On 27.11.2020 15:39, Jürgen Groß wrote:
>>> On 27.11.20 15:23, Julien Grall wrote:
>>>> On 25/11/2020 10:51, Juergen Gross wrote:
>>>>> --- a/xen/common/event_fifo.c
>>>>> +++ b/xen/common/event_fifo.c
>>>>> @@ -175,6 +175,18 @@ static void evtchn_fifo_set_pending(struct vcpu
>>>>> *v, struct evtchn *evtchn)
>>>>>            return;
>>>>>        }
>>>>> +    /*
>>>>> +     * Control block not mapped.  The guest must not unmask an
>>>>> +     * event until the control block is initialized, so we can
>>>>> +     * just drop the event.
>>>>> +     */
>>>>> +    if ( unlikely(!v->evtchn_fifo->control_block) )
>>>>
>>>> Sort of unrelated, AFAICT, v->evtchn_fifo->control_block can be set
>>>> concurrently to this access.
>>>>
>>>> Thankfully, once the control block is mapped, it can't be unmapped.
>>>> However, there is still a possibility that you may see half of the update.
>>>>
>>>> Shouldn't the field access with ACCESS_ONCE()?
>>>
>>> Shouldn't this be another patch? Especially as the writing side needs
>>> the same treatment.
>>
>> Indeed. As said on several different occasions - our code base is
>> full of places where we chance torn accesses, if there really was
>> a compiler to let us down on this.
> 
> I am quite amazed you that you managed to test all the version of 
> GCC/Clang that were built and confirm this is unlikely to happen :).

It's (obviously) not that I tested all of them, but that I know
at least a little bit of how gcc generates code, that I'm unaware
of reports to the contrary, and that it would seem odd for a
compiler to split accesses when they can be done by one insn. Of
course one could build a compiler doing only byte accesses ...

>> This recurring pattern
>> shouldn't lead to unrelated patches getting bloated, unless _all_
>> affected sites get touched anyway.
> 
> You probably missed the point where I say "sort of unrelated". This 
> wasn't not a suggestion to fix it here (I should have been clearer 
> though) but instead point out issue as I see them.

Point taken.

Jan