Hi Stefano,
On 06/14/2017 06:32 PM, Stefano Stabellini wrote:
On Wed, 14 Jun 2017, Julien Grall wrote:
I don't understand your explanation. There are no PV protocols under
xen/, they are implemented in other repositories. I grepped for ACCESS
under xen/include/public, in case you referred to the PV protocol
headers, but couldn't find anything interesting.
Have a look at the pl011 emulation from Bhupinder. It will use plain '=' for
updating the PV drivers. So can you explain why it is fine there and not here?
Bhupinder's series is the first PV driver in the Xen codebase. It is
easy to forget that coding should/might have to be different compared to
Linux, which is the codebase I usually work with for PV drivers.
It is true that updating the indexes should be done atomically,
otherwise the other end might end up reading a wrong index value.
Furthermore implementation of
atomic_read/atomic_write in Linux (both ARM and x86) is based on
WRITE_ONCE/READ_ONCE, on Xen it is a simple assignment.
I don't follow why you are referring to Linux constructs in this
discussion about Xen atomic functions.
My point here is Xen and Linux are very similar. Actually a lot of the atomic
code is been taken from Linux (have a look to our atomic_read/atomic_write).
As the atomic code was added the code by you (likely from Linux), I don't
understand why you don't complain about the atomic implementation but
ACCESS_ONCE.
Linux and Xen are free to make different assumptions regarding
compilers.
It is not possible to say that Xen may have different assumptions when
you import code from Linux without deep review (yes most of the code
taken from Linux is as it is).
FWIW I am not really complaining about either atomics or ACCESS_ONCE, I
just don't see the advantage of using ACCESS_ONCE over read/write_atomic
(see below).
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.
What I want to avoid is this split mind we currently have about atomic.
They are either all safe or not.
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.
Regarding the "compiler with support": do we state clearly in any docs
or website what are the compilers we support? I think this would be the
right opportunity to do it.
That's a discussion to have on the README.atomics patch.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel