atomic: declare atomic_load_*() operations to take constants

[ehem_freebsd_m5p.com]

Summary

atomic_load_*() functions should NOT be modifying their arguments.
Their arguments could instead be modified externally.

[pauamma_gundo.com]

share/man/man9/atomic.9

66

This doesn't seem consistent with changes to the source code, which all add "const" in 2 spots (before "volatile" and before "p"), not just before "volatile" as here. Am I missing something?

[ehem_freebsd_m5p.com]

share/man/man9/atomic.9

66

That is true, but this doesn't effect writing software. Declaring the local variable constant hints to the compiler even less needs to be saved. Whereas indicating the address passed in won't be written to does effect writing software (this came up elsewhere).
I'm unsure whether this is worthy of note and whether this needs mentioning.

sys/i386/include/atomic.h

407–408

I'm pretty sure this is only atomic on a single-processor system and fails the guarantee the function is supposed to provide. Though multiprocessor systems with anything before i586 are very rare (rare enough the Xen code likely shouldn't worry).

497–503

Here there is a problem I need some help with, since GCC's extended inline assembly language is troublesome. If I'm reading things right this does provide the atomic load functionality. Problem is Clang's interpretation of GCC's inline doesn't like how I've altered this.

I'm pretty sure argument 1 needs to be marked as input, but I'm not good enough with this GCC construct to be confident of getting the correct result.

[ehem_freebsd_m5p.com]

sys/i386/include/atomic.h

497–503

I think I've finally figured out how this works and how this needs to be adjusted...

I think the "+m" (*p) : simply needs to be turned into : "m" (*p). This changes this from an output to an input. I'm guessing this might be a copy and paste issue? (as originally uploaded, this breaks the i386 kernel) Someone else's opinion is needed as I'm out of my comfort zone.

[kib]

Can you demonstrate why this is useful? For me, volatile const combination of type qualifiers is nonsensical. It might have very limited sense for the implementation of the functions, but practically, due to complete opaqueness of the inline asm, it is not. The m/+m issue is also a sign.

sys/i386/include/atomic.h

407–408

This is why this function is suffixed with _i386.

Even if you can find alive MP 1.4 spec compliant 486-based machine, no efforts will be accepted to support it.

497–503

Replacing "+m" with "m" cannot change 'output' to 'input'. Output operands are listed after the first colon, and input operands are listed after the second. The + modifier means that the operand is both read and written by the assembler inline, which obviously contradicts your attempt to spread consts.

In fact I believe that the exiting constraint is the right thing to do, due to the following fine detail in the IA32 SDM: CMPXCHG always writes to the memory location, regardless of the result of the comparison. I actually saw this problem when implemented first version of userspace vdso_gettc().

[ehem_freebsd_m5p.com]

If you consider the definitions of volatile and const carefully, they do not conflict.

const means the current thread of execution/function will not alter the data at the address.

volatile means the data at the address is subject to alteration by external operations, another thread or hardware.

As such const volatile * is appropriate for the atomic_load_*() functions as it is indicating the function will not alter what is being pointed to, but what is being pointed to could change due to outside actions.

sys/i386/include/atomic.h

497–503

Take a second glance at what was entered. The one error is the comma would also need to be removed.

Having finally figured out what is occurring, it does indeed appear this is simply a copy and paste issue. Though there will be a write operation on the memory bus, memory won't actually be altered. As such to me it looks like argument 1 can simply be remarked as input.

True, this violates the strictest definition of const, but does appear it should provide the expected result. The function will return the 64-bit value at address p and will not alter data at that address (though x86 uses a memory write operation to accomplish this).

[kib]

In D34701#786517, @ehem_freebsd_m5p.com wrote:

If you consider the definitions of volatile and const carefully, they do not conflict.

const means the current thread of execution/function will not alter the data at the address.

volatile means the data at the address is subject to alteration by external operations, another thread or hardware.

As such const volatile * is appropriate for the atomic_load_*() functions as it is indicating the function will not alter what is being pointed to, but what is being pointed to could change due to outside actions.

I am not saying they conflict (that would be disallowed in std after all), but that the combination is nonsensical for atomic_load() context.

sys/i386/include/atomic.h

497–503

const does not work there because it writes, even if the same value. As I mentioned, I discovered it while working on vdso gettc(), where the data page is mapped ro, and atomic_load_acq_64() resulted in page fault on write.

[ehem_freebsd_m5p.com]

In D34701#786575, @kib wrote:

I am not saying they conflict (that would be disallowed in std after all), but that the combination is nonsensical for atomic_load() context.

Why is that? This situation looks like atomic_load*_64() simply cannot be properly implemented for i386 and shouldn't be present. I note from the existing man page:

The type “64” is currently not implemented for some of the atomic operations on the arm, i386, and powerpc architectures.

I'm actually surprised any of the atomic_*_64() functions are available for i386 (though clearly the cmpset family can have proper implementations).

sys/i386/include/atomic.h

497–503

Pondering what to do here...

This seems to suggest i386 simply shouldn't have atomic_load*_64() with this situation.

[ehem_freebsd_m5p.com]

Admit atomic_load_64() cannot be const on i386. 64-bit architectures do conform to this though.

I would prefer to replace all uses which need to function on i386 with atomic_fcmpset_64(), but appears right now the best option is allowing non-const on i386.

[pauamma_gundo.com]

share/man/man9/atomic.9

66

That is true, but this doesn't effect writing software. Declaring the local variable constant hints to the compiler even less needs to be saved. Whereas indicating the address passed in won't be written to does effect writing software (this came up elsewhere).
I'm unsure whether this is worthy of note and whether this needs mentioning.

Not currently a developer myself, but experience tells me that documentation and source code being inconsistent can cause uncertainty and confusion. (Which is wrong? Does it matter for the bug I'm seeing, which I may or may not fully understand? What if the inconsistency means *both* are wrong, as the saying goes?)

[ehem_freebsd_m5p.com]

In D34701#786575, @kib wrote:

I am not saying they conflict (that would be disallowed in std after all), but that the combination is nonsensical for atomic_load() context.

Why do you suggest that? A value in a memory region which can only be read by the current VM, but can be written by another VM would need this. This also allows passing pointers around as constants and the compiler will alerts you if a function doesn't provide that guarantee.

The atomic_load*() functions shouldn't be writing to the memory address or memory-mapped register. Just the implementation for i586 64-bit requires this oddity (since i586 has cmpxchg8b, but not other 64-bit loads).

share/man/man9/atomic.9

66

In this case I'm trying to state the function will conform to atomic_load(const volatile *), but telling the compiler it also conforms to atomic_load(const volatile *const) (this is stricter than the former). Anywhere the former is acceptable, the latter will also be acceptable; just I only want to guarantee the former.