Re: [PATCH v11 01/34] ARM: vGIC: avoid rank lock when reading priority

[Julien Grall <julien.grall@arm.com>]

Hi Stefano,

On 06/14/2017 07:15 PM, Stefano Stabellini wrote:
> On Wed, 14 Jun 2017, Julien Grall wrote:
>>>>>> In any case, all those macros does not prevent re-ordering at the
>>>>>> processor
>>>>>> level nor read/write atomicity if the variable is misaligned.
>>>>>
>>>>> My understanding is that the unwritten assumption in Xen is that
>>>>> variables are always aligned. You are right about processor level
>>>>> reordering, in fact when needed we have to have barriers
>>>>>
>>>>> I have read Andre's well written README.atomic, and he ends the
>>>>> document stating the following:
>>>>>
>>>>>
>>>>>> This makes read and write accesses to ints and longs (and their
>>>>>> respective
>>>>>> unsigned counter parts) naturally atomic.
>>>>>> However it would be beneficial to use atomic primitives anyway to
>>>>>> annotate
>>>>>> the code as being concurrent and to prevent silent breakage when
>>>>>> changing
>>>>>> the code
>>>>>
>>>>> with which I completely agree
>>>>
>>>> Which means you are happy to use either ACCESS_ONCE or
>>>> read_atomic/write_atomic as they in-fine exactly the same on the compiler
>>>> we
>>>> support.
>>>
>>> I do understand that both of them will produce the same output,
>>> therefore, both work for this use-case.
>>>
>>> I don't understand why anybody would prefer ACCESS_ONCE over
>>> read/write_atomic, given that with ACCESS_ONCE as a contributor/reviewer
>>> you additionally need to remember to check whether the argument is a
>>> native data type. Basically, I see ACCESS_ONCE as "more work" for me.
>>> Why do you think that ACCESS_ONCE is "better"?
>>
>> Have you looked at the implementation of ACCESS_ONCE? You don't have to check
>> the data type when using ACCESS_ONCE. There are safety check to avoid misusing
>> it.
> 
> It checks for a scalar type, not for native data type. They are not
> always the same thing but I think they are on arm.
That's the goal of specification (such as AAPCS).

>> What I want to avoid is this split mind we currently have about atomic.
>> They are either all safe or not.
> 
> What split mind? Do you mean ACCESS_ONCE vs. read/write_atomic? So far,
> there are no instances of ACCESS_ONCE in xen/arch/arm.

No. I mean there are places with exact same construct but sometimes 
consider we consider safe, sometimes not. We should have a clear common 
answer rather than arguing differently every time.

>> As Andre suggested, we should probably import a lighter version of
>> WRITE_ONCE/READ_ONCE. They are first easier to understand than
>> read_atomic/write_atomic that could be confused with atomic_read/atomic_write
>> (IIRC Jan agreed here).
>>
>> The main goal is to avoid assembly code when it is deem not necessary.
> 
> All right, this is one reason. Sorry if I seem unnecessarily contrarian,
> but this is the first time I read a reason for this recent push for using
> ACCESS_ONCE. You wrote that you preferred the read/write_atomic
> functions yourself on Monday.

I preferred {read,write}_atomic because the prototype is nicer to use 
than ACCESS_ONCE. In the ideal we should introduce WRITE_ONCE/READ_ONCE 
improving the naming and also having a nice prototype.

-- 
Julien Grall

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