xen/x86: irq: Avoid a TOCTOU race in pirq_spin_lock_irq_desc()
diff mbox series

Message ID 20200722165300.22655-1-julien@xen.org
State New
Headers show
Series
  • xen/x86: irq: Avoid a TOCTOU race in pirq_spin_lock_irq_desc()
Related show

Commit Message

Julien Grall July 22, 2020, 4:53 p.m. UTC
From: Julien Grall <jgrall@amazon.com>

Even if we assigned pirq->arch.irq to a variable, a compile is still
allowed to read pirq->arch.irq multiple time. This means that the value
checked may be different from the value used to get the desc.

Force the compiler to only do one read access by using read_atomic().

Signed-off-by: Julien Grall <jgrall@amazon.com>
---
 xen/arch/x86/irq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Jan Beulich July 23, 2020, 11:23 a.m. UTC | #1
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).

And this is of course besides the fact that I think we have many
more instances where guaranteeing a single access would be
needed, if we're afraid of the described permitted compiler
behavior. Which then makes me wonder if this is really something
we should fix one by one, rather than by at least larger scope
audits (in order to not suggest "throughout the code base").

As a minor remark, unless you've observed problematic behavior,
would you mind adding "potential" or "theoretical" to the title?

Jan
Julien Grall July 23, 2020, 1:22 p.m. UTC | #2
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).

The existing implementation of ACCESS_ONCE() can only work on scalar 
type. The implementation is based on a Linux, although we have an extra 
check. Looking through the Linux history, it looks like it is not 
possible to make ACCESS_ONCE() work with non-scalar types:

     ACCESS_ONCE does not work reliably on non-scalar types. For
     example gcc 4.6 and 4.7 might remove the volatile tag for such
     accesses during the SRA (scalar replacement of aggregates) step
     https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58145)

I understand that our implementation of read_atomic(), write_atomic() 
would lead to less optimized code. So maybe we want to import 
READ_ONCE() and WRITE_ONCE() from Linux?

As a side note, I have seen suggestion only (see [1]) which suggest that 
they those helpers wouldn't be portable:

"One relatively unimportant misunderstanding is due to the fact that the 
standard only talks about accesses to volatile objects. It does not talk 
about accesses via volatile qualified pointers. Some programmers believe 
that using a pointer-to-volatile should be handled as though it pointed 
to a volatile object. That is not guaranteed by the standard and is 
therefore not portable. However, this is relatively unimportant because 
gcc does in fact treat a pointer-to-volatile as though it pointed to a 
volatile object."

I would assume that the use is OK on CLang and GCC given that Linux has 
been using it.


> And this is of course besides the fact that I think we have many
> more instances where guaranteeing a single access would be
> needed, if we're afraid of the described permitted compiler
> behavior. Which then makes me wonder if this is really something
> we should fix one by one, rather than by at least larger scope
> audits (in order to not suggest "throughout the code base").

It depends how much the contributor can invest on chasing the rest of 
the issues. The larger the scope is, the less likely you will find 
someone that has bandwith to allocate for fixing it completely.

If the scope is "a field", then I think it is a reasonable suggesting.

In this case, I had a look at arch.irq and wasn't able to spot other 
potential issue.

> As a minor remark, unless you've observed problematic behavior,
> would you mind adding "potential" or "theoretical" to the title?

I am not aware of any issues with compiler so far. So I can add 
"potential" in the title.

Cheers,

[1] https://www.airs.com/blog/archives/154
Jan Beulich July 23, 2020, 1:54 p.m. UTC | #3
On 23.07.2020 15:22, Julien Grall wrote:
> 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).

Ideally yes. I'm unconvinced though that either construct fits all
needs (for {read,write}_atomic() there may be reasons why the
compiler is allowed to produce multiple generated code instances
from a single source instance, while for *_ONCE() the compiler may
be allowed to split the access into pieces (as can be easily seen
for an access to a uint64_t variable on 32-bit x86 at least, and
by deduction I then can't see why it shouldn't be allowed to use
byte-wise accesses).

> The existing implementation of ACCESS_ONCE() can only work on scalar 
> type. The implementation is based on a Linux, although we have an extra 
> check. Looking through the Linux history, it looks like it is not 
> possible to make ACCESS_ONCE() work with non-scalar types:
> 
>      ACCESS_ONCE does not work reliably on non-scalar types. For
>      example gcc 4.6 and 4.7 might remove the volatile tag for such
>      accesses during the SRA (scalar replacement of aggregates) step
>      https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58145)
> 
> I understand that our implementation of read_atomic(), write_atomic() 
> would lead to less optimized code.

I.e. you see ways for the compiler to be more clever than using
a single "move" instruction for a single move? Or are you referring
to insn scheduling by the compiler (which my gut feeling would say
is impacted as much by an asm volatile() as by accessing a volatile
object).

> So maybe we want to import 
> READ_ONCE() and WRITE_ONCE() from Linux?

So far I was under the impression that our ACCESS_ONCE() is the
result of folding (older) Linux'es READ_ONCE() and WRITE_ONCE()
into a single construct.

> As a side note, I have seen suggestion only (see [1]) which suggest that 
> they those helpers wouldn't be portable:
> 
> "One relatively unimportant misunderstanding is due to the fact that the 
> standard only talks about accesses to volatile objects. It does not talk 
> about accesses via volatile qualified pointers. Some programmers believe 
> that using a pointer-to-volatile should be handled as though it pointed 
> to a volatile object. That is not guaranteed by the standard and is 
> therefore not portable. However, this is relatively unimportant because 
> gcc does in fact treat a pointer-to-volatile as though it pointed to a 
> volatile object."
> 
> I would assume that the use is OK on CLang and GCC given that Linux has 
> been using it.

Then again your change here is exactly to drop such assumptions of
ours on compiler behavior.

>> And this is of course besides the fact that I think we have many
>> more instances where guaranteeing a single access would be
>> needed, if we're afraid of the described permitted compiler
>> behavior. Which then makes me wonder if this is really something
>> we should fix one by one, rather than by at least larger scope
>> audits (in order to not suggest "throughout the code base").
> 
> It depends how much the contributor can invest on chasing the rest of 
> the issues. The larger the scope is, the less likely you will find 
> someone that has bandwith to allocate for fixing it completely.

I certainly understand that.

> If the scope is "a field", then I think it is a reasonable suggesting.
> 
> In this case, I had a look at arch.irq and wasn't able to spot other 
> potential issue.

That's good to know, and may be worth mentioning - if not in the
description, then maybe in a post-commit-message remark?

Jan
Andrew Cooper July 23, 2020, 1:59 p.m. UTC | #4
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.

>
> The existing implementation of ACCESS_ONCE() can only work on scalar
> type. The implementation is based on a Linux, although we have an
> extra check. Looking through the Linux history, it looks like it is
> not possible to make ACCESS_ONCE() work with non-scalar types:
>
>     ACCESS_ONCE does not work reliably on non-scalar types. For
>     example gcc 4.6 and 4.7 might remove the volatile tag for such
>     accesses during the SRA (scalar replacement of aggregates) step
>     https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58145)
>
> I understand that our implementation of read_atomic(), write_atomic()
> would lead to less optimized code.

There are cases where read_atomic()/write_atomic() prevent optimisations
which ACCESS_ONCE() would allow, but it is only for code of the form:

ACCESS_ONCE(ptr) |= val;

Which a sufficiently clever compiler could convert to a single `or $val,
ptr` instruction on x86, while read_atomic()/write_atomic() would force
it to be `mov ptr, %reg; or $val, %reg; mov %reg, ptr`.

That said - your note about GCC treating the pointed-to object as
volatile probably means it won't make the above optimisation, even
though it would be appropriate to do so.

> So maybe we want to import READ_ONCE() and WRITE_ONCE() from Linux?

There is no point.  Linux has taken a massive detour through wildly
different READ/WRITE_ONCE() functions (to fix the above GCC bugs), and
are now back to something very similar to ACCESS_ONCE().

~Andrew
Julien Grall Aug. 14, 2020, 7:25 p.m. UTC | #5
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.

Cheers,
Roger Pau Monné Aug. 17, 2020, 12:46 p.m. UTC | #6
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)?

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.

Roger.
Julien Grall Aug. 17, 2020, 1:14 p.m. UTC | #7
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.

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.

May I ask why we would want to expose the difference to the user?

Cheers,
Roger Pau Monné Aug. 17, 2020, 2:01 p.m. UTC | #8
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?

> 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.

Thanks, Roger.
Julien Grall Aug. 17, 2020, 2:39 p.m. UTC | #9
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.

> 
>> 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?

Cheers,
Roger Pau Monné Aug. 17, 2020, 3:03 p.m. UTC | #10
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.

> > 
> > > 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

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.

Thanks, Roger.
Julien Grall Aug. 17, 2020, 3:53 p.m. UTC | #11
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,
Roger Pau Monné Aug. 17, 2020, 5:33 p.m. UTC | #12
On Mon, Aug 17, 2020 at 04:53:51PM +0100, Julien Grall wrote:
> 
> 
> 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?

Or about the existing ones switching behavior, which is again quite
unlikely I would like to assume.

> 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(...) |= ...

I'm not sure how will that behave when used with a compile known
value that's smaller than the size of the destination. Could the
compiler optimize this as a partial read/write if only the lower byte
is modified for example?

> 
> > 
> > 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 guess we could go that route, and properly document what each helper
is supposed to do, and that {READ/WRITE}_ONCE guarantee atomicity
while ACCESS_ONCE requires special condition for us to guarantee the
operation will be atomic.

> 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.

I'm fine if what's expected from each helper is documented, it just
seems IMO more confusing that using more differentiated naming for the
helpers, because the fact that ACCESS_ONCE is atomic is a compiler
defined behavior, but not something that could be guaranteed from the
code itself.

Thanks, Roger.
Julien Grall Aug. 17, 2020, 5:56 p.m. UTC | #13
On 17/08/2020 18:33, Roger Pau Monné wrote:
> On Mon, Aug 17, 2020 at 04:53:51PM +0100, Julien Grall wrote:
>>
>>
>> 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?
> 
> Or about the existing ones switching behavior, which is again quite
> unlikely I would like to assume.

The main goal of the macro is to mark place which require the variable 
to be accessed once. So, in the unlikely event this may happen, it would 
be easy to modify the implementation.

> 
>> 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(...) |= ...
> 
> I'm not sure how will that behave when used with a compile known
> value that's smaller than the size of the destination. Could the
> compiler optimize this as a partial read/write if only the lower byte
> is modified for example?

Here what Andrew wrote in a previous answer:

"Which a sufficiently clever compiler could convert to a single `or 
$val, ptr` instruction on x86, while read_atomic()/write_atomic() would 
force it to be `mov ptr, %reg; or $val, %reg; mov %reg, ptr`."

On Arm, a RwM operation will still not be atomic as it would require 3 
instructions.

> 
>>
>>>
>>> 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 guess we could go that route, and properly document what each helper
> is supposed to do, and that {READ/WRITE}_ONCE guarantee atomicity
> while ACCESS_ONCE requires special condition for us to guarantee the
> operation will be atomic.
> 
>> 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.
> 
> I'm fine if what's expected from each helper is documented, it just
> seems IMO more confusing that using more differentiated naming for the
> helpers, because the fact that ACCESS_ONCE is atomic is a compiler
> defined behavior, but not something that could be guaranteed from the
> code itself.

I am happy to try to document the behavior of each helpers. Are you 
happy if I attempt to do the renaming in a follow-up patch?

Cheers,
Roger Pau Monné Aug. 18, 2020, 8:35 a.m. UTC | #14
On Mon, Aug 17, 2020 at 06:56:24PM +0100, Julien Grall wrote:
> 
> 
> On 17/08/2020 18:33, Roger Pau Monné wrote:
> > On Mon, Aug 17, 2020 at 04:53:51PM +0100, Julien Grall wrote:
> > > 
> > > 
> > > 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?
> > 
> > Or about the existing ones switching behavior, which is again quite
> > unlikely I would like to assume.
> 
> The main goal of the macro is to mark place which require the variable to be
> accessed once. So, in the unlikely event this may happen, it would be easy
> to modify the implementation.
> 
> > 
> > > 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(...) |= ...
> > 
> > I'm not sure how will that behave when used with a compile known
> > value that's smaller than the size of the destination. Could the
> > compiler optimize this as a partial read/write if only the lower byte
> > is modified for example?
> 
> Here what Andrew wrote in a previous answer:
> 
> "Which a sufficiently clever compiler could convert to a single `or $val,
> ptr` instruction on x86, while read_atomic()/write_atomic() would force it
> to be `mov ptr, %reg; or $val, %reg; mov %reg, ptr`."
> 
> On Arm, a RwM operation will still not be atomic as it would require 3
> instructions.

I don't think we should rely on this behavior of ACCESS_ONCE (OR being
translated into a single instruction), as it seems to even be more
fragile than relying on ACCESS_ONCE performing reads and writes
accesses as single instructions.

Once question I through about given the example is how are we going to
name an atomic OR operation if we ever require one? OR_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 guess we could go that route, and properly document what each helper
> > is supposed to do, and that {READ/WRITE}_ONCE guarantee atomicity
> > while ACCESS_ONCE requires special condition for us to guarantee the
> > operation will be atomic.
> > 
> > > 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.
> > 
> > I'm fine if what's expected from each helper is documented, it just
> > seems IMO more confusing that using more differentiated naming for the
> > helpers, because the fact that ACCESS_ONCE is atomic is a compiler
> > defined behavior, but not something that could be guaranteed from the
> > code itself.
> 
> I am happy to try to document the behavior of each helpers. Are you happy if
> I attempt to do the renaming in a follow-up patch?

Sure, TBH this has diverged so much that should have had it's own
thread.

The patch itself looks fine to me, regardless of whether READ_ONCE or
read_atomic is used.

Thanks, Roger.
Roger Pau Monné Aug. 18, 2020, 8:36 a.m. UTC | #15
On Wed, Jul 22, 2020 at 05:53:00PM +0100, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> Even if we assigned pirq->arch.irq to a variable, a compile is still
> allowed to read pirq->arch.irq multiple time. This means that the value
> checked may be different from the value used to get the desc.
> 
> Force the compiler to only do one read access by using read_atomic().
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

The whole discussion about renaming {write/read}_atomic is orthogonal
to this patch IMO.

Thanks, Roger.
Jan Beulich Aug. 18, 2020, 8:39 a.m. UTC | #16
On 14.08.2020 21:25, 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()?

Wouldn't this lead to confusion with Linux'es macros of the same names?

> I would also suggest to move them implementation in a new header asm/lib.h.

Probably.

Jan
Julien Grall Aug. 18, 2020, 8:53 a.m. UTC | #17
Hi Jan,

On 18/08/2020 09:39, Jan Beulich wrote:
> On 14.08.2020 21:25, 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()?
> 
> Wouldn't this lead to confusion with Linux'es macros of the same names?

 From my understanding, the purpose of READ_ONCE()/WRITE_ONCE() in Linux 
is the same as our read_atomic()/write_atomic().

So I think it would be fine to rename them. An alternative would be port 
the Linux version in Xen and drop ours.

Cheers,
Jan Beulich Aug. 18, 2020, 8:57 a.m. UTC | #18
On 18.08.2020 10:53, Julien Grall wrote:
> Hi Jan,
> 
> On 18/08/2020 09:39, Jan Beulich wrote:
>> On 14.08.2020 21:25, 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()?
>>
>> Wouldn't this lead to confusion with Linux'es macros of the same names?
> 
> From my understanding, the purpose of READ_ONCE()/WRITE_ONCE() in Linux is the same as our read_atomic()/write_atomic().
> 
> So I think it would be fine to rename them. An alternative would be port the Linux version in Xen and drop ours.

The port of Linux'es {READ,WRITE}_ONCE() is our ACCESS_ONCE(). As pointed
out before, ACCESS_ONCE() and {read,write}_atomic() serve slightly
different purposes, and so far it looks like all of us are lacking ideas
on how to construct something that catches all cases by one single approach.

Jan
Julien Grall Aug. 18, 2020, 9:12 a.m. UTC | #19
Hi Jan,

On 18/08/2020 09:57, Jan Beulich wrote:
> On 18.08.2020 10:53, Julien Grall wrote:
>> Hi Jan,
>>
>> On 18/08/2020 09:39, Jan Beulich wrote:
>>> On 14.08.2020 21:25, 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()?
>>>
>>> Wouldn't this lead to confusion with Linux'es macros of the same names?
>>
>>  From my understanding, the purpose of READ_ONCE()/WRITE_ONCE() in Linux is the same as our read_atomic()/write_atomic().
>>
>> So I think it would be fine to rename them. An alternative would be port the Linux version in Xen and drop ours.
> 
> The port of Linux'es {READ,WRITE}_ONCE() is our ACCESS_ONCE().

Not really... Our ACCESS_ONCE() only supports scalar type. {READ, 
WRITE}_ONCE() are able to support non-scalar type as well.

> As pointed
> out before, ACCESS_ONCE() and {read,write}_atomic() serve slightly
> different purposes, and so far it looks like all of us are lacking ideas
> on how to construct something that catches all cases by one single approach.

I am guessing you are referring to [1], right?

If you read the documentation of READ_ONCE()/WRITE_ONCE(), they are 
meant to be atomic in some cases. The cases are exactly the same as 
{read, write}_atomic().

I will ask the same thing as I asked to Roger. If Linux can rely on it, 
why can't we?

Although, I agree that the implementation is not portable to another 
compiler. But that's why they are implemented in compiler.h.

Cheers,

[1] <d3ba0dad-63db-06ad-ff3f-f90fe8649845@suse.com>

> 


> Jan
>
Roger Pau Monné Aug. 18, 2020, 10:13 a.m. UTC | #20
On Thu, Jul 23, 2020 at 02:59:57PM +0100, 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.

I've been thinking about this a bit, and maybe the problem is the
other way around. Having an 'atomic' type doesn't make much sense IMO,
because atomicity is not (solely) based on the type but rather on how
the accesses are performed.

Maybe it would make more sense to rename atomic_t to counter_t: a
counter that guarantees atomic accesses. Then the atomic namespace
could be used to implement the low level helpers that actually
guarantee atomicity and support a wider set of types than what
atomic_t currently is (ie: an int).

Roger.
Jan Beulich Aug. 18, 2020, 11:27 a.m. UTC | #21
On 18.08.2020 11:12, Julien Grall wrote:
> Hi Jan,
> 
> On 18/08/2020 09:57, Jan Beulich wrote:
>> On 18.08.2020 10:53, Julien Grall wrote:
>>> Hi Jan,
>>>
>>> On 18/08/2020 09:39, Jan Beulich wrote:
>>>> On 14.08.2020 21:25, 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()?
>>>>
>>>> Wouldn't this lead to confusion with Linux'es macros of the same names?
>>>
>>>  From my understanding, the purpose of READ_ONCE()/WRITE_ONCE() in Linux is the same as our read_atomic()/write_atomic().
>>>
>>> So I think it would be fine to rename them. An alternative would be port the Linux version in Xen and drop ours.
>>
>> The port of Linux'es {READ,WRITE}_ONCE() is our ACCESS_ONCE().
> 
> Not really... Our ACCESS_ONCE() only supports scalar type. {READ, WRITE}_ONCE() are able to support non-scalar type as well.

I guess that's merely because ours was derived from an earlier version of
Linux.

>> As pointed
>> out before, ACCESS_ONCE() and {read,write}_atomic() serve slightly
>> different purposes, and so far it looks like all of us are lacking ideas
>> on how to construct something that catches all cases by one single approach.
> 
> I am guessing you are referring to [1], right?
> 
> If you read the documentation of READ_ONCE()/WRITE_ONCE(), they are meant to be atomic in some cases. The cases are exactly the same as {read, write}_atomic().
> 
> I will ask the same thing as I asked to Roger. If Linux can rely on it, why can't we?

That's not the way I'd like to see arguments go here: If Linux has
something suitable, I'm fine with us using it. But we ought to be
permitted to question whether what we inherit is indeed fit for
purpose, and afaict there is at least one gap to be filled. In no
case should we blindly pull in things from Linux and then assume
that simply by doing so all is well.

> Although, I agree that the implementation is not portable to another compiler. But that's why they are implemented in compiler.h.

Aiui items in compiler.h are meant to be suitable for all compilers,
whereas truly gcc-specific things (for example) live in
compiler-gcc.h (where Linux has two further ones they support).

Jan
Julien Grall Aug. 18, 2020, 11:52 a.m. UTC | #22
On 18/08/2020 12:27, Jan Beulich wrote:
> On 18.08.2020 11:12, Julien Grall wrote:
>>> As pointed
>>> out before, ACCESS_ONCE() and {read,write}_atomic() serve slightly
>>> different purposes, and so far it looks like all of us are lacking ideas
>>> on how to construct something that catches all cases by one single approach.
>>
>> I am guessing you are referring to [1], right?
>>
>> If you read the documentation of READ_ONCE()/WRITE_ONCE(), they are meant to be atomic in some cases. The cases are exactly the same as {read, write}_atomic().
>>
>> I will ask the same thing as I asked to Roger. If Linux can rely on it, why can't we?
> 
> That's not the way I'd like to see arguments go here: If Linux has
> something suitable, I'm fine with us using it. But we ought to be
> permitted to question whether what we inherit is indeed fit for
> purpose, and afaict there is at least one gap to be filled. In no
> case should we blindly pull in things from Linux and then assume
> that simply by doing so all is well.

I don't think any of us here are compilers guru, so I would tend to rely 
on Linux memory work. After all their code received much more attention. 
But sure we can question everything they have been doing.

To me the expected semantics (/!\ I am not referring to the 
implementation) for all the helpers are the same. But you seem to 
disagree on that.

I read the thread again and I couldn't find any explanation how a 
developper could chose between ACCESS_ONCE() and {read, write}_atomic().

Can you outline how one would decide?

Cheers,
Jan Beulich Aug. 18, 2020, 4:06 p.m. UTC | #23
On 18.08.2020 13:52, Julien Grall wrote:
> 
> 
> On 18/08/2020 12:27, Jan Beulich wrote:
>> On 18.08.2020 11:12, Julien Grall wrote:
>>>> As pointed
>>>> out before, ACCESS_ONCE() and {read,write}_atomic() serve slightly
>>>> different purposes, and so far it looks like all of us are lacking ideas
>>>> on how to construct something that catches all cases by one single approach.
>>>
>>> I am guessing you are referring to [1], right?
>>>
>>> If you read the documentation of READ_ONCE()/WRITE_ONCE(), they are meant to be atomic in some cases. The cases are exactly the same as {read, write}_atomic().
>>>
>>> I will ask the same thing as I asked to Roger. If Linux can rely on it, why can't we?
>>
>> That's not the way I'd like to see arguments go here: If Linux has
>> something suitable, I'm fine with us using it. But we ought to be
>> permitted to question whether what we inherit is indeed fit for
>> purpose, and afaict there is at least one gap to be filled. In no
>> case should we blindly pull in things from Linux and then assume
>> that simply by doing so all is well.
> 
> I don't think any of us here are compilers guru, so I would tend to rely on Linux memory work. After all their code received much more attention. But sure we can question everything they have been doing.
> 
> To me the expected semantics (/!\ I am not referring to the implementation) for all the helpers are the same. But you seem to disagree on that.
> 
> I read the thread again and I couldn't find any explanation how a developper could chose between ACCESS_ONCE() and {read, write}_atomic().
> 
> Can you outline how one would decide?

As long as both have weaknesses I'm afraid it's really a case-by-case
decision. If you're strictly after only the property that one has and
the other doesn't, the case is clear. In all other cases it'll need
weighing of the risks. No clear rules, I'm afraid.

Jan
Julien Grall Aug. 20, 2020, 9:50 a.m. UTC | #24
Hi Roger,

On 18/08/2020 09:35, Roger Pau Monné wrote:
> On Mon, Aug 17, 2020 at 06:56:24PM +0100, Julien Grall wrote:
>>
>>
>> On 17/08/2020 18:33, Roger Pau Monné wrote:
>>> On Mon, Aug 17, 2020 at 04:53:51PM +0100, Julien Grall wrote:
>>>>
>>>>
>>>> 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?
>>>
>>> Or about the existing ones switching behavior, which is again quite
>>> unlikely I would like to assume.
>>
>> The main goal of the macro is to mark place which require the variable to be
>> accessed once. So, in the unlikely event this may happen, it would be easy
>> to modify the implementation.
>>
>>>
>>>> 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(...) |= ...
>>>
>>> I'm not sure how will that behave when used with a compile known
>>> value that's smaller than the size of the destination. Could the
>>> compiler optimize this as a partial read/write if only the lower byte
>>> is modified for example?
>>
>> Here what Andrew wrote in a previous answer:
>>
>> "Which a sufficiently clever compiler could convert to a single `or $val,
>> ptr` instruction on x86, while read_atomic()/write_atomic() would force it
>> to be `mov ptr, %reg; or $val, %reg; mov %reg, ptr`."
>>
>> On Arm, a RwM operation will still not be atomic as it would require 3
>> instructions.
> 
> I don't think we should rely on this behavior of ACCESS_ONCE (OR being
> translated into a single instruction), as it seems to even be more
> fragile than relying on ACCESS_ONCE performing reads and writes
> accesses as single instructions.

Agree.

> 
> Once question I through about given the example is how are we going to
> name an atomic OR operation if we ever require one? OR_ONCE?

Good question.

I looked again a staging and couldn't find any ACCESS_ONCE(...) |=. So I 
would suggest to do nothing until there is such instance.

> 
>>>
>>>>
>>>>>
>>>>> 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 guess we could go that route, and properly document what each helper
>>> is supposed to do, and that {READ/WRITE}_ONCE guarantee atomicity
>>> while ACCESS_ONCE requires special condition for us to guarantee the
>>> operation will be atomic.
>>>
>>>> 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.
>>>
>>> I'm fine if what's expected from each helper is documented, it just
>>> seems IMO more confusing that using more differentiated naming for the
>>> helpers, because the fact that ACCESS_ONCE is atomic is a compiler
>>> defined behavior, but not something that could be guaranteed from the
>>> code itself.
>>
>> I am happy to try to document the behavior of each helpers. Are you happy if
>> I attempt to do the renaming in a follow-up patch?
> 
> Sure, TBH this has diverged so much that should have had it's own
> thread.

I will start a more generic thread to see if we can adopt Linux memory 
barriers document. This would make our life easier when discussing 
memory issues in Xen (I expect a lot more to come).

> 
> The patch itself looks fine to me, regardless of whether READ_ONCE or
> read_atomic is used.

Thanks!

Cheers,

Patch
diff mbox series

diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
index a69937c840b9..25f2eb611692 100644
--- 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);
 
         if ( irq <= 0 )
             return NULL;