On 17/08/2020 16:03, Roger Pau Monné wrote:
On Mon, Aug 17, 2020 at 03:39:52PM +0100, Julien Grall wrote:
On 17/08/2020 15:01, Roger Pau Monné wrote:
On Mon, Aug 17, 2020 at 02:14:01PM +0100, Julien Grall wrote:
Hi,
On 17/08/2020 13:46, Roger Pau Monné wrote:
On Fri, Aug 14, 2020 at 08:25:28PM +0100, Julien Grall wrote:
Hi Andrew,
Sorry for the late answer.
On 23/07/2020 14:59, Andrew Cooper wrote:
On 23/07/2020 14:22, Julien Grall wrote:
Hi Jan,
On 23/07/2020 12:23, Jan Beulich wrote:
On 22.07.2020 18:53, Julien Grall wrote:
--- a/xen/arch/x86/irq.c
+++ b/xen/arch/x86/irq.c
@@ -1187,7 +1187,7 @@ struct irq_desc *pirq_spin_lock_irq_desc(
for ( ; ; )
{
- int irq = pirq->arch.irq;
+ int irq = read_atomic(&pirq->arch.irq);
There we go - I'd be fine this way, but I'm pretty sure Andrew
would want this to be ACCESS_ONCE(). So I guess now is the time
to settle which one to prefer in new code (or which criteria
there are to prefer one over the other).
I would prefer if we have a single way to force the compiler to do a
single access (read/write).
Unlikely to happen, I'd expect.
But I would really like to get rid of (or at least rename)
read_atomic()/write_atomic() specifically because they've got nothing to
do with atomic_t's and the set of functionality who's namespace they share.
Would you be happy if I rename both to READ_ONCE() and WRITE_ONCE()? I would
also suggest to move them implementation in a new header asm/lib.h.
Maybe {READ/WRITE}_SINGLE (to note those should be implemented using a
single instruction)?
The asm volatile statement contains only one instruction, but this doesn't
mean the helper will generate a single instruction.
Well, the access should be done using a single instruction, which is
what we care about when using this helpers.
You may have other instructions to get the registers ready for the access.
ACCESS_ONCE (which also has the _ONCE suffix) IIRC could be
implemented using several instructions, and hence doesn't seem right
that they all have the _ONCE suffix.
The goal here is the same, we want to access the variable *only* once.
Right, but this is not guaranteed by the current implementation of
ACCESS_ONCE AFAICT, as the compiler *might* split the access into two
(or more) instructions, and hence won't be an atomic access anymore?
From my understanding, at least on GCC/Clang, ACCESS_ONCE() should be atomic
if you are using aligned address and the size smaller than a register size.
Yes, any sane compiler shouldn't split such access, but this is not
guaranteed by the current code in ACCESS_ONCE.
To be sure, your concern here is not about GCC/Clang but other
compilers. Am I correct?
We already have a collection of compiler specific macros in compiler.h.
So how about we classify this macro as a compiler specific one? (See
more below).
May I ask why we would want to expose the difference to the user?
I'm not saying we should, but naming them using the _ONCE suffix seems
misleading IMO, as they have different guarantees than what
ACCESS_ONCE currently provides.
Lets leave aside how ACCESS_ONCE() is implemented for a moment.
If ACCESS_ONCE() doesn't guarantee atomicy, then it means you may read a mix
of the old and new value. This would most likely break quite a few of the
users because the result wouldn't be coherent.
Do you have place in mind where the non-atomicity would be useful?
Not that I'm aware, I think they could all be safely switched to use
the atomic variants
There is concern that read_atomic(), write_atomic() prevent the compiler
to do certain optimization. Andrew gave the example of:
ACCESS_ONCE(...) |= ...
In fact I wouldn't be surprised if users of ACCESS_ONCE break if the
access was split into multiple instructions.
My comment was to notice that just renaming the atomic read/write
helpers to use the _ONCE prefix is IMO weird as they offer different
properties than ACCESS_ONCE, and hence might confuse users.Just
looking at READ_ONCE users could assume all _ONCE helpers would
guarantee atomicity, which is not the case.
Our implementation of ACCESS_ONCE() is very similar to what Linux used
to have. There READ_ONCE()/WRITE_ONCE() are also using the same principles.
From my understanding, you can safely assume the access will be atomic
if the following conditions are met:
- The address is correctly size
- The size is smaller than the word machine size
I would agree this may not be correct on all the existing compilers. But
this macro could easily be re-implemented if we add support for a
compiler with different guarantee.
Therefore, I fail to see why we can't use the same guarantee in Xen.
Cheers,
--
Julien Grall
